Skip to content

Commit

Permalink
Namespace resources are tracked by graph (#1320)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@bjee19
bjee19 authored Dec 15, 2023
1 parent 40e234b commit ae48c2b
Show file tree
Hide file tree
Showing 8 changed files with 596 additions and 263 deletions.
2 changes: 1 addition & 1 deletion internal/mode/static/state/change_processor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}),
Expand Down
169 changes: 141 additions & 28 deletions internal/mode/static/state/change_processor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
})
})
})

Expand Down
27 changes: 24 additions & 3 deletions internal/mode/static/state/graph/graph.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit ae48c2b

Please sign in to comment.