Namespace resources are tracked by graph (#1320)

Refactor code such that Namespace resources are tracked by the graph, thus letting us remove dependency on using the Capturer to do so.

Problem: The Capturer is currently tracking relevant Namespaces and we would like to instead use the Graph.

Solution: Refactor codebase so Graph tracks referenced namespaces and remove dependency on the Capturer.

Author: bjee19

internal/mode/static/state/change_processor.go

@@ -146,7 +146,7 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl {
 			{
 				gvk:       extractGVK(&apiv1.Namespace{}),
 				store:     newObjectStoreMapAdapter(clusterStore.Namespaces),
-				predicate: nil,
+				predicate: funcPredicate{stateChanged: isReferenced},
 			},
 			{
 				gvk:       extractGVK(&apiv1.Service{}),

internal/mode/static/state/change_processor_test.go

@@ -1293,53 +1293,166 @@ var _ = Describe("ChangeProcessor", func() {
 				})
 			})
 		})
-		Describe("namespace changes", func() {
-			When("namespace is linked via label selectors", func() {
-				It("triggers an update when labels are removed", func() {
-					ns := &apiv1.Namespace{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: "ns",
-							Labels: map[string]string{
-								"app": "allowed",
-							},
+		Describe("namespace changes", Ordered, func() {
+			var (
+				ns, nsDifferentLabels, nsNoLabels *apiv1.Namespace
+				gw                                *v1.Gateway
+			)
+
+			BeforeAll(func() {
+				ns = &apiv1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "ns",
+						Labels: map[string]string{
+							"app": "allowed",
 						},
-					}
-					gw := &v1.Gateway{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: "gw",
+					},
+				}
+				nsDifferentLabels = &apiv1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "ns-different-labels",
+						Labels: map[string]string{
+							"oranges": "bananas",
 						},
-						Spec: v1.GatewaySpec{
-							Listeners: []v1.Listener{
-								{
-									AllowedRoutes: &v1.AllowedRoutes{
-										Namespaces: &v1.RouteNamespaces{
-											From: helpers.GetPointer(v1.NamespacesFromSelector),
-											Selector: &metav1.LabelSelector{
-												MatchLabels: map[string]string{
-													"app": "allowed",
-												},
+					},
+				}
+				nsNoLabels = &apiv1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "no-labels",
+					},
+				}
+				gw = &v1.Gateway{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "gw",
+					},
+					Spec: v1.GatewaySpec{
+						GatewayClassName: gcName,
+						Listeners: []v1.Listener{
+							{
+								Port:     80,
+								Protocol: v1.HTTPProtocolType,
+								AllowedRoutes: &v1.AllowedRoutes{
+									Namespaces: &v1.RouteNamespaces{
+										From: helpers.GetPointer(v1.NamespacesFromSelector),
+										Selector: &metav1.LabelSelector{
+											MatchLabels: map[string]string{
+												"app": "allowed",
 											},
 										},
 									},
 								},
 							},
 						},
-					}
+					},
+				}
+				processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{
+					GatewayCtlrName:      controllerName,
+					GatewayClassName:     gcName,
+					RelationshipCapturer: relationship.NewCapturerImpl(),
+					Logger:               zap.New(),
+					Validators:           createAlwaysValidValidators(),
+					Scheme:               createScheme(),
+				})
+				processor.CaptureUpsertChange(gc)
+				processor.CaptureUpsertChange(gw)
+				processor.Process()
+			})
 
-					processor.CaptureUpsertChange(gw)
+			When("a namespace is created that is not linked to a listener", func() {
+				It("does not trigger an update", func() {
+					processor.CaptureUpsertChange(nsNoLabels)
+					changed, _ := processor.Process()
+					Expect(changed).To(BeFalse())
+				})
+			})
+			When("a namespace is created that is linked to a listener", func() {
+				It("triggers an update", func() {
 					processor.CaptureUpsertChange(ns)
+					changed, _ := processor.Process()
+					Expect(changed).To(BeTrue())
+				})
+			})
+			When("a namespace is deleted that is not linked to a listener", func() {
+				It("does not trigger an update", func() {
+					processor.CaptureDeleteChange(nsNoLabels, types.NamespacedName{Name: "no-labels"})
+					changed, _ := processor.Process()
+					Expect(changed).To(BeFalse())
+				})
+			})
+			When("a namespace is deleted that is linked to a listener", func() {
+				It("triggers an update", func() {
+					processor.CaptureDeleteChange(ns, types.NamespacedName{Name: "ns"})
+					changed, _ := processor.Process()
+					Expect(changed).To(BeTrue())
+				})
+			})
+			When("a namespace that is not linked to a listener has its labels changed to match a listener", func() {
+				It("triggers an update", func() {
+					processor.CaptureUpsertChange(nsDifferentLabels)
+					changed, _ := processor.Process()
+					Expect(changed).To(BeFalse())
 
+					nsDifferentLabels.Labels = map[string]string{
+						"app": "allowed",
+					}
+					processor.CaptureUpsertChange(nsDifferentLabels)
+					changed, _ = processor.Process()
+					Expect(changed).To(BeTrue())
+				})
+			})
+			When("a namespace that is linked to a listener has its labels changed to no longer match a listener", func() {
+				It("triggers an update", func() {
+					nsDifferentLabels.Labels = map[string]string{
+						"oranges": "bananas",
+					}
+					processor.CaptureUpsertChange(nsDifferentLabels)
+					changed, _ := processor.Process()
+					Expect(changed).To(BeTrue())
+				})
+			})
+			When("a gateway changes its listener's labels", func() {
+				It("triggers an update when a namespace that matches the new labels is created", func() {
+					gwChangedLabel := gw.DeepCopy()
+					gwChangedLabel.Spec.Listeners[0].AllowedRoutes.Namespaces.Selector.MatchLabels = map[string]string{
+						"oranges": "bananas",
+					}
+					gwChangedLabel.Generation++
+					processor.CaptureUpsertChange(gwChangedLabel)
 					changed, _ := processor.Process()
 					Expect(changed).To(BeTrue())
 
-					newNS := ns.DeepCopy()
-					newNS.Labels = nil
-					processor.CaptureUpsertChange(newNS)
+					// After changing the gateway's labels and generation, the processor should be marked to update
+					// the nginx configuration and build a new graph. When processor.Process() gets called,
+					// the nginx configuration gets updated and a new graph is built with an updated
+					// referencedNamespaces. Thus, when the namespace "ns" is upserted with labels that no longer match
+					// the new labels on the gateway, it would not trigger a change as the namespace would no longer
+					// be in the updated referencedNamespaces and the labels no longer match the new labels on the
+					// gateway.
+					processor.CaptureUpsertChange(ns)
+					changed, _ = processor.Process()
+					Expect(changed).To(BeFalse())
 
+					processor.CaptureUpsertChange(nsDifferentLabels)
 					changed, _ = processor.Process()
 					Expect(changed).To(BeTrue())
 				})
 			})
+			When("a namespace that is not linked to a listener has its labels removed", func() {
+				It("does not trigger an update", func() {
+					ns.Labels = nil
+					processor.CaptureUpsertChange(ns)
+					changed, _ := processor.Process()
+					Expect(changed).To(BeFalse())
+				})
+			})
+			When("a namespace that is linked to a listener has its labels removed", func() {
+				It("triggers an update when labels are removed", func() {
+					nsDifferentLabels.Labels = nil
+					processor.CaptureUpsertChange(nsDifferentLabels)
+					changed, _ := processor.Process()
+					Expect(changed).To(BeTrue())
+				})
+			})
 		})
 	})
 

internal/mode/static/state/graph/graph.go

@@ -44,22 +44,40 @@ type Graph struct {
 	// in the cluster. We need such entries so that we can query the Graph to determine if a Secret is referenced
 	// by the Gateway, including the case when the Secret is newly created.
 	ReferencedSecrets map[types.NamespacedName]*Secret
+	// ReferencedNamespaces includes Namespaces with labels that match the Gateway Listener's label selector.
+	ReferencedNamespaces map[types.NamespacedName]*v1.Namespace
 }
 
 // ProtectedPorts are the ports that may not be configured by a listener with a descriptive name of each port.
 type ProtectedPorts map[int32]string
 
 // IsReferenced returns true if the Graph references the resource.
 func (g *Graph) IsReferenced(resourceType client.Object, nsname types.NamespacedName) bool {
-	// FIMXE(pleshakov): For now, only works with Secrets.
-	// Support EndpointSlices and Namespaces so that we can remove relationship.Capturer and use the Graph
+	// FIMXE(bjee19): For now, only works with Secrets and Namespaces.
+	// Support EndpointSlices so that we can remove relationship.Capturer and use the Graph
 	// as source to determine the relationships.
 	// See https://github.com/nginxinc/nginx-gateway-fabric/issues/824
 
-	switch resourceType.(type) {
+	switch obj := resourceType.(type) {
 	case *v1.Secret:
 		_, exists := g.ReferencedSecrets[nsname]
 		return exists
+	case *v1.Namespace:
+		// `existed` is needed as it checks the graph's ReferencedNamespaces which stores all the namespaces that
+		// match the Gateway listener's label selector when the graph was created. This covers the case when
+		// a Namespace changes its label so it no longer matches a Gateway listener's label selector, but because
+		// it was in the graph's ReferencedNamespaces we know that the Graph did reference the Namespace.
+		//
+		// However, if there is a Namespace which changes its label (previously it did not match) to match a Gateway
+		// listener's label selector, it will not be in the current graph's ReferencedNamespaces until it is rebuilt
+		// and thus not be caught in `existed`. Therefore, we need `exists` to check the graph's Gateway and see if the
+		// new Namespace actually matches any of the Gateway listener's label selector.
+		//
+		// `exists` does not cover the case highlighted above by `existed` and vice versa so both are needed.
+
+		_, existed := g.ReferencedNamespaces[nsname]
+		exists := isNamespaceReferenced(obj, g.Gateway)
+		return existed || exists
 	default:
 		return false
 	}
@@ -92,13 +110,16 @@ func BuildGraph(
 	bindRoutesToListeners(routes, gw, state.Namespaces)
 	addBackendRefsToRouteRules(routes, refGrantResolver, state.Services)
 
+	referencedNamespaces := buildReferencedNamespaces(state.Namespaces, gw)
+
 	g := &Graph{
 		GatewayClass:          gc,
 		Gateway:               gw,
 		Routes:                routes,
 		IgnoredGatewayClasses: processedGwClasses.Ignored,
 		IgnoredGateways:       processedGws.Ignored,
 		ReferencedSecrets:     secretResolver.getResolvedSecrets(),
+		ReferencedNamespaces:  referencedNamespaces,
 	}
 
 	return g