diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 84567cebd2..46272570c5 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -45,7 +45,6 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file" ngxruntime "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/relationship" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/telemetry" @@ -138,10 +137,9 @@ func StartManager(cfg config.Config) error { } processor := state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ - GatewayCtlrName: cfg.GatewayCtlrName, - GatewayClassName: cfg.GatewayClassName, - RelationshipCapturer: relationship.NewCapturerImpl(), - Logger: cfg.Logger.WithName("changeProcessor"), + GatewayCtlrName: cfg.GatewayCtlrName, + GatewayClassName: cfg.GatewayClassName, + Logger: cfg.Logger.WithName("changeProcessor"), Validators: validation.Validators{ HTTPFieldsValidator: ngxvalidation.HTTPValidator{}, }, diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index ee43d1c468..ff0034636d 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -22,7 +22,6 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/relationship" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -55,8 +54,6 @@ type ChangeProcessor interface { // ChangeProcessorConfig holds configuration parameters for ChangeProcessorImpl. type ChangeProcessorConfig struct { - // RelationshipCapturer captures relationships between Kubernetes API resources and Gateway API resources. - RelationshipCapturer relationship.Capturer // Validators validate resources according to data-plane specific rules. Validators validation.Validators // EventRecorder records events for Kubernetes resources. @@ -114,34 +111,32 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { clusterState: clusterStore, } - isReferenced := func(obj client.Object) bool { - nsname := types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()} + isReferenced := func(obj client.Object, nsname types.NamespacedName) bool { return processor.latestGraph != nil && processor.latestGraph.IsReferenced(obj, nsname) } trackingUpdater := newChangeTrackingUpdater( - cfg.RelationshipCapturer, extractGVK, []changeTrackingUpdaterObjectTypeCfg{ { gvk: extractGVK(&v1.GatewayClass{}), store: newObjectStoreMapAdapter(clusterStore.GatewayClasses), - predicate: alwaysProcess{}, + predicate: nil, }, { gvk: extractGVK(&v1.Gateway{}), store: newObjectStoreMapAdapter(clusterStore.Gateways), - predicate: alwaysProcess{}, + predicate: nil, }, { gvk: extractGVK(&v1.HTTPRoute{}), store: newObjectStoreMapAdapter(clusterStore.HTTPRoutes), - predicate: alwaysProcess{}, + predicate: nil, }, { gvk: extractGVK(&v1beta1.ReferenceGrant{}), store: newObjectStoreMapAdapter(clusterStore.ReferenceGrants), - predicate: alwaysProcess{}, + predicate: nil, }, { gvk: extractGVK(&apiv1.Namespace{}), @@ -151,12 +146,12 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { { gvk: extractGVK(&apiv1.Service{}), store: newObjectStoreMapAdapter(clusterStore.Services), - predicate: nil, + predicate: funcPredicate{stateChanged: isReferenced}, }, { gvk: extractGVK(&discoveryV1.EndpointSlice{}), store: nil, - predicate: nil, + predicate: funcPredicate{stateChanged: isReferenced}, }, { gvk: extractGVK(&apiv1.Secret{}), diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 24d64e254f..0376b43463 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -25,8 +25,6 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/relationship" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/relationship/relationshipfakes" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" ) @@ -272,12 +270,11 @@ var _ = Describe("ChangeProcessor", func() { BeforeEach(OncePerOrdered, func() { processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ - GatewayCtlrName: controllerName, - GatewayClassName: gcName, - RelationshipCapturer: relationship.NewCapturerImpl(), - Logger: zap.New(), - Validators: createAlwaysValidValidators(), - Scheme: createScheme(), + GatewayCtlrName: controllerName, + GatewayClassName: gcName, + Logger: zap.New(), + Validators: createAlwaysValidValidators(), + Scheme: createScheme(), }) }) @@ -430,7 +427,8 @@ var _ = Describe("ChangeProcessor", func() { { BackendRefs: []graph.BackendRef{ { - Weight: 1, + SvcNsName: types.NamespacedName{Namespace: "service-ns", Name: "service"}, + Weight: 1, }, }, ValidMatches: true, @@ -509,6 +507,12 @@ var _ = Describe("ChangeProcessor", func() { {Namespace: "test", Name: "hr-1"}: expRouteHR1, }, ReferencedSecrets: map[types.NamespacedName]*graph.Secret{}, + ReferencedServices: map[types.NamespacedName]struct{}{ + { + Namespace: "service-ns", + Name: "service", + }: {}, + }, } }) When("no upsert has occurred", func() { @@ -573,6 +577,9 @@ var _ = Describe("ChangeProcessor", func() { } expGraph.ReferencedSecrets = nil + expGraph.ReferencedServices = nil + + expRouteHR1.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -623,6 +630,9 @@ var _ = Describe("ChangeProcessor", func() { expGraph.Routes[hr1Name].ParentRefs[1].Attachment = expAttachment443 expGraph.ReferencedSecrets = nil + expGraph.ReferencedServices = nil + + expRouteHR1.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) @@ -643,6 +653,9 @@ var _ = Describe("ChangeProcessor", func() { Source: diffNsTLSSecret, } + expGraph.ReferencedServices = nil + expRouteHR1.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) @@ -849,6 +862,9 @@ var _ = Describe("ChangeProcessor", func() { Source: sameNsTLSSecret, } + expRouteHR1.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} + expGraph.ReferencedServices = nil + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) @@ -878,6 +894,9 @@ var _ = Describe("ChangeProcessor", func() { Source: sameNsTLSSecret, } + expRouteHR1.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} + expGraph.ReferencedServices = nil + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) @@ -898,6 +917,9 @@ var _ = Describe("ChangeProcessor", func() { expGraph.Routes = map[types.NamespacedName]*graph.Route{} expGraph.ReferencedSecrets = nil + expRouteHR1.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} + expGraph.ReferencedServices = nil + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) @@ -910,6 +932,9 @@ var _ = Describe("ChangeProcessor", func() { types.NamespacedName{Namespace: "test", Name: "gateway-2"}, ) + expRouteHR1.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} + expGraph.ReferencedServices = nil + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(&graph.Graph{}, graphCfg)).To(BeEmpty()) @@ -922,17 +947,22 @@ var _ = Describe("ChangeProcessor", func() { types.NamespacedName{Namespace: "test", Name: "hr-1"}, ) + expRouteHR1.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{} + expGraph.ReferencedServices = nil + changed, graphCfg := processor.Process() Expect(changed).To(BeTrue()) Expect(helpers.Diff(&graph.Graph{}, graphCfg)).To(BeEmpty()) }) }) }) + Describe("Process services and endpoints", Ordered, func() { var ( hr1, hr2, hr3, hrInvalidBackendRef, hrMultipleRules *v1.HTTPRoute hr1svc, sharedSvc, bazSvc1, bazSvc2, bazSvc3, invalidSvc, notRefSvc *apiv1.Service hr1slice1, hr1slice2, noRefSlice, missingSvcNameSlice *discoveryV1.EndpointSlice + gw *v1.Gateway ) createSvc := func(name string) *apiv1.Service { @@ -998,6 +1028,12 @@ var _ = Describe("ChangeProcessor", func() { hr1slice2 = createEndpointSlice("hr1-2", "foo-svc") noRefSlice = createEndpointSlice("no-ref", "no-ref") missingSvcNameSlice = createEndpointSlice("missing-svc-name", "") + + gw = createGateway("gw") + processor.CaptureUpsertChange(gc) + processor.CaptureUpsertChange(gw) + changed, _ := processor.Process() + Expect(changed).To(BeTrue()) }) testProcessChangedVal := func(expChanged bool) { @@ -1014,6 +1050,7 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureDeleteChange(obj, nsname) testProcessChangedVal(expChanged) } + When("hr1 is added", func() { It("should trigger a change", func() { testUpsertTriggersChange(hr1, true) @@ -1260,6 +1297,7 @@ var _ = Describe("ChangeProcessor", func() { }) }) }) + Describe("namespace changes", Ordered, func() { var ( ns, nsDifferentLabels, nsNoLabels *apiv1.Namespace @@ -1313,12 +1351,11 @@ var _ = Describe("ChangeProcessor", func() { }, } processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ - GatewayCtlrName: controllerName, - GatewayClassName: gcName, - RelationshipCapturer: relationship.NewCapturerImpl(), - Logger: zap.New(), - Validators: createAlwaysValidValidators(), - Scheme: createScheme(), + GatewayCtlrName: controllerName, + GatewayClassName: gcName, + Logger: zap.New(), + Validators: createAlwaysValidValidators(), + Scheme: createScheme(), }) processor.CaptureUpsertChange(gc) processor.CaptureUpsertChange(gw) @@ -1428,31 +1465,69 @@ var _ = Describe("ChangeProcessor", func() { // -- this is done in 'Normal cases of processing changes' var ( - processor *state.ChangeProcessorImpl - fakeRelationshipCapturer *relationshipfakes.FakeCapturer - gcNsName, gwNsName, hrNsName, hr2NsName, rgNsName types.NamespacedName - svcNsName, sliceNsName, secretNsName types.NamespacedName - gc, gcUpdated *v1.GatewayClass - gw1, gw1Updated, gw2 *v1.Gateway - hr1, hr1Updated, hr2 *v1.HTTPRoute - rg1, rg1Updated, rg2 *v1beta1.ReferenceGrant - svc *apiv1.Service - slice *discoveryV1.EndpointSlice - secret *apiv1.Secret + processor *state.ChangeProcessorImpl + gcNsName, gwNsName, hrNsName, hr2NsName, rgNsName, svcNsName, sliceNsName, secretNsName types.NamespacedName + gc, gcUpdated *v1.GatewayClass + gw1, gw1Updated, gw2 *v1.Gateway + hr1, hr1Updated, hr2 *v1.HTTPRoute + rg1, rg1Updated, rg2 *v1beta1.ReferenceGrant + svc, barSvc, unrelatedSvc *apiv1.Service + slice, barSlice, unrelatedSlice *discoveryV1.EndpointSlice + ns, unrelatedNS, testNs, barNs *apiv1.Namespace + secret, secretUpdated, unrelatedSecret, barSecret, barSecretUpdated *apiv1.Secret ) BeforeEach(OncePerOrdered, func() { - fakeRelationshipCapturer = &relationshipfakes.FakeCapturer{} - processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ - GatewayCtlrName: "test.controller", - GatewayClassName: "my-class", - RelationshipCapturer: fakeRelationshipCapturer, - Validators: createAlwaysValidValidators(), - Scheme: createScheme(), + GatewayCtlrName: "test.controller", + GatewayClassName: "test-class", + Validators: createAlwaysValidValidators(), + Scheme: createScheme(), }) - gcNsName = types.NamespacedName{Name: "my-class"} + secretNsName = types.NamespacedName{Namespace: "test", Name: "tls-secret"} + secret = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretNsName.Name, + Namespace: secretNsName.Namespace, + Generation: 1, + }, + Type: apiv1.SecretTypeTLS, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + } + secretUpdated = secret.DeepCopy() + secretUpdated.Generation++ + barSecret = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar-secret", + Namespace: "test", + Generation: 1, + }, + Type: apiv1.SecretTypeTLS, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + } + barSecretUpdated = barSecret.DeepCopy() + barSecretUpdated.Generation++ + unrelatedSecret = &apiv1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-tls-secret", + Namespace: "unrelated-ns", + Generation: 1, + }, + Type: apiv1.SecretTypeTLS, + Data: map[string][]byte{ + apiv1.TLSCertKey: cert, + apiv1.TLSPrivateKeyKey: key, + }, + } + + gcNsName = types.NamespacedName{Name: "test-class"} gc = &v1.GatewayClass{ ObjectMeta: metav1.ObjectMeta{ @@ -1470,8 +1545,62 @@ var _ = Describe("ChangeProcessor", func() { gw1 = &v1.Gateway{ ObjectMeta: metav1.ObjectMeta{ - Namespace: gwNsName.Namespace, - Name: gwNsName.Name, + Name: "gw-1", + Namespace: "test", + Generation: 1, + }, + Spec: v1.GatewaySpec{ + GatewayClassName: gcName, + Listeners: []v1.Listener{ + { + Name: "listener-80-1", + Hostname: nil, + Port: 80, + Protocol: v1.HTTPProtocolType, + AllowedRoutes: &v1.AllowedRoutes{ + Namespaces: &v1.RouteNamespaces{ + From: helpers.GetPointer(v1.NamespacesFromSelector), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "test": "namespace", + }, + }, + }, + }, + }, + { + Name: "listener-443-1", + Hostname: nil, + Port: 443, + Protocol: v1.HTTPSProtocolType, + TLS: &v1.GatewayTLSConfig{ + Mode: helpers.GetPointer(v1.TLSModeTerminate), + CertificateRefs: []v1.SecretObjectReference{ + { + Kind: (*v1.Kind)(helpers.GetPointer("Secret")), + Name: v1.ObjectName(secret.Name), + Namespace: (*v1.Namespace)(&secret.Namespace), + }, + }, + }, + }, + { + Name: "listener-500-1", + Hostname: nil, + Port: 500, + Protocol: v1.HTTPSProtocolType, + TLS: &v1.GatewayTLSConfig{ + Mode: helpers.GetPointer(v1.TLSModeTerminate), + CertificateRefs: []v1.SecretObjectReference{ + { + Kind: (*v1.Kind)(helpers.GetPointer("Secret")), + Name: v1.ObjectName(barSecret.Name), + Namespace: (*v1.Namespace)(&barSecret.Namespace), + }, + }, + }, + }, + }, }, } @@ -1481,14 +1610,14 @@ var _ = Describe("ChangeProcessor", func() { gw2 = gw1.DeepCopy() gw2.Name = "gw-2" + testNamespace := v1.Namespace("test") + kindService := v1.Kind("Service") + fooRef := createBackendRef(&kindService, "foo-svc", &testNamespace) + barRef := createBackendRef(&kindService, "bar-svc", &testNamespace) + hrNsName = types.NamespacedName{Namespace: "test", Name: "hr-1"} - hr1 = &v1.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: hrNsName.Namespace, - Name: hrNsName.Name, - }, - } + hr1 = createRoute("hr-1", "gw-1", "foo.example.com", fooRef, barRef) hr1Updated = hr1.DeepCopy() hr1Updated.Generation++ @@ -1498,21 +1627,79 @@ var _ = Describe("ChangeProcessor", func() { hr2 = hr1.DeepCopy() hr2.Name = hr2NsName.Name - svcNsName = types.NamespacedName{Namespace: "test", Name: "svc"} - + svcNsName = types.NamespacedName{Namespace: "test", Name: "foo-svc"} svc = &apiv1.Service{ ObjectMeta: metav1.ObjectMeta{ Namespace: svcNsName.Namespace, Name: svcNsName.Name, }, } + barSvc = &apiv1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "bar-svc", + }, + } + unrelatedSvc = &apiv1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "unrelated-svc", + }, + } sliceNsName = types.NamespacedName{Namespace: "test", Name: "slice"} - slice = &discoveryV1.EndpointSlice{ ObjectMeta: metav1.ObjectMeta{ Namespace: sliceNsName.Namespace, Name: sliceNsName.Name, + Labels: map[string]string{index.KubernetesServiceNameLabel: svc.Name}, + }, + } + barSlice = &discoveryV1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "bar-slice", + Labels: map[string]string{index.KubernetesServiceNameLabel: "bar-svc"}, + }, + } + unrelatedSlice = &discoveryV1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "unrelated-slice", + Labels: map[string]string{index.KubernetesServiceNameLabel: "unrelated-svc"}, + }, + } + + testNs = &apiv1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Labels: map[string]string{ + "test": "namespace", + }, + }, + } + ns = &apiv1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ns", + Labels: map[string]string{ + "test": "namespace", + }, + }, + } + barNs = &apiv1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar-ns", + Labels: map[string]string{ + "test": "namespace", + }, + }, + } + unrelatedNS = &apiv1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-ns", + Labels: map[string]string{ + "oranges": "bananas", + }, }, } @@ -1530,15 +1717,6 @@ var _ = Describe("ChangeProcessor", func() { rg2 = rg1.DeepCopy() rg2.Name = "rg-2" - - secretNsName = types.NamespacedName{Namespace: "test", Name: "test-secret"} - - secret = &apiv1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: secretNsName.Namespace, - Name: secretNsName.Name, - }, - } }) // Changing change - a change that makes processor.Process() report changed // Non-changing change - a change that doesn't do that @@ -1551,6 +1729,7 @@ var _ = Describe("ChangeProcessor", func() { It("should report changed after multiple Upserts", func() { processor.CaptureUpsertChange(gc) processor.CaptureUpsertChange(gw1) + processor.CaptureUpsertChange(testNs) processor.CaptureUpsertChange(hr1) processor.CaptureUpsertChange(rg1) @@ -1621,23 +1800,31 @@ var _ = Describe("ChangeProcessor", func() { }) }) Describe("Multiple Kubernetes API resource changes", Ordered, func() { - // Note: because secret resource is not used by the real relationship.Capturer, it is not used - // in the same way as service and endpoint slice in the tests below. + BeforeAll(func() { + // Set up graph + processor.CaptureUpsertChange(gc) + processor.CaptureUpsertChange(gw1) + processor.CaptureUpsertChange(testNs) + processor.CaptureUpsertChange(hr1) + processor.CaptureUpsertChange(secret) + processor.CaptureUpsertChange(barSecret) + changed, _ := processor.Process() + Expect(changed).To(BeTrue()) + }) + It("should report changed after multiple Upserts of related resources", func() { - fakeRelationshipCapturer.ExistsReturns(true) processor.CaptureUpsertChange(svc) processor.CaptureUpsertChange(slice) - + processor.CaptureUpsertChange(ns) + processor.CaptureUpsertChange(secretUpdated) changed, _ := processor.Process() Expect(changed).To(BeTrue()) }) - It("should report not changed after multiple Upserts of unrelated resources", func() { - fakeRelationshipCapturer.ExistsReturns(false) - processor.CaptureUpsertChange(svc) - processor.CaptureUpsertChange(slice) - - processor.CaptureUpsertChange(secret) + processor.CaptureUpsertChange(unrelatedSvc) + processor.CaptureUpsertChange(unrelatedSlice) + processor.CaptureUpsertChange(unrelatedNS) + processor.CaptureUpsertChange(unrelatedSecret) changed, _ := processor.Process() Expect(changed).To(BeFalse()) @@ -1645,15 +1832,16 @@ var _ = Describe("ChangeProcessor", func() { When("upserts of related resources are followed by upserts of unrelated resources", func() { It("should report changed", func() { // these are changing changes - fakeRelationshipCapturer.ExistsReturns(true) - processor.CaptureUpsertChange(svc) - processor.CaptureUpsertChange(slice) + processor.CaptureUpsertChange(barSvc) + processor.CaptureUpsertChange(barSlice) + processor.CaptureUpsertChange(barNs) + processor.CaptureUpsertChange(barSecretUpdated) // there are non-changing changes - fakeRelationshipCapturer.ExistsReturns(false) - processor.CaptureUpsertChange(svc) - processor.CaptureUpsertChange(slice) - processor.CaptureUpsertChange(secret) + processor.CaptureUpsertChange(unrelatedSvc) + processor.CaptureUpsertChange(unrelatedSlice) + processor.CaptureUpsertChange(unrelatedNS) + processor.CaptureUpsertChange(unrelatedSecret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) @@ -1662,15 +1850,16 @@ var _ = Describe("ChangeProcessor", func() { When("deletes of related resources are followed by upserts of unrelated resources", func() { It("should report changed", func() { // these are changing changes - fakeRelationshipCapturer.ExistsReturns(true) processor.CaptureDeleteChange(&apiv1.Service{}, svcNsName) processor.CaptureDeleteChange(&discoveryV1.EndpointSlice{}, sliceNsName) + processor.CaptureDeleteChange(&apiv1.Namespace{}, types.NamespacedName{Name: "ns"}) + processor.CaptureDeleteChange(&apiv1.Secret{}, secretNsName) // these are non-changing changes - fakeRelationshipCapturer.ExistsReturns(false) - processor.CaptureUpsertChange(svc) - processor.CaptureUpsertChange(slice) - processor.CaptureUpsertChange(secret) + processor.CaptureUpsertChange(unrelatedSvc) + processor.CaptureUpsertChange(unrelatedSlice) + processor.CaptureUpsertChange(unrelatedNS) + processor.CaptureUpsertChange(unrelatedSecret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) @@ -1680,85 +1869,44 @@ var _ = Describe("ChangeProcessor", func() { Describe("Multiple Kubernetes API and Gateway API resource changes", Ordered, func() { It("should report changed after multiple Upserts of new and related resources", func() { // new Gateway API resources - fakeRelationshipCapturer.ExistsReturns(false) processor.CaptureUpsertChange(gc) processor.CaptureUpsertChange(gw1) + processor.CaptureUpsertChange(testNs) processor.CaptureUpsertChange(hr1) processor.CaptureUpsertChange(rg1) // related Kubernetes API resources - fakeRelationshipCapturer.ExistsReturns(true) processor.CaptureUpsertChange(svc) processor.CaptureUpsertChange(slice) + processor.CaptureUpsertChange(ns) + processor.CaptureUpsertChange(secret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) }) - It("should report not changed after multiple Upserts of unrelated resources", func() { // unrelated Kubernetes API resources - fakeRelationshipCapturer.ExistsReturns(false) - processor.CaptureUpsertChange(svc) - processor.CaptureUpsertChange(slice) - processor.CaptureUpsertChange(secret) + processor.CaptureUpsertChange(unrelatedSvc) + processor.CaptureUpsertChange(unrelatedSlice) + processor.CaptureUpsertChange(unrelatedNS) + processor.CaptureUpsertChange(unrelatedSecret) changed, _ := processor.Process() Expect(changed).To(BeFalse()) }) - - It("should report changed after upserting related resources followed by upserting unchanged resources", - func() { - // these are changing changes - fakeRelationshipCapturer.ExistsReturns(true) - processor.CaptureUpsertChange(svc) - processor.CaptureUpsertChange(slice) - - // these are non-changing changes - fakeRelationshipCapturer.ExistsReturns(false) - processor.CaptureUpsertChange(gc) - processor.CaptureUpsertChange(gw1) - processor.CaptureUpsertChange(hr1) - processor.CaptureUpsertChange(rg1) - processor.CaptureUpsertChange(secret) - - changed, _ := processor.Process() - Expect(changed).To(BeTrue()) - }, - ) - It("should report changed after upserting changed resources followed by upserting unrelated resources", func() { // these are changing changes - fakeRelationshipCapturer.ExistsReturns(false) processor.CaptureUpsertChange(gcUpdated) processor.CaptureUpsertChange(gw1Updated) processor.CaptureUpsertChange(hr1Updated) processor.CaptureUpsertChange(rg1Updated) // these are non-changing changes - processor.CaptureUpsertChange(svc) - processor.CaptureUpsertChange(slice) - processor.CaptureUpsertChange(secret) - - changed, _ := processor.Process() - Expect(changed).To(BeTrue()) - }, - ) - It( - "should report changed after upserting related resources followed by upserting unchanged resources", - func() { - // these are changing changes - fakeRelationshipCapturer.ExistsReturns(true) - processor.CaptureUpsertChange(svc) - processor.CaptureUpsertChange(slice) - - // these are non-changing changes - fakeRelationshipCapturer.ExistsReturns(false) - processor.CaptureUpsertChange(gcUpdated) - processor.CaptureUpsertChange(gw1Updated) - processor.CaptureUpsertChange(hr1Updated) - processor.CaptureUpsertChange(rg1Updated) - processor.CaptureUpsertChange(secret) + processor.CaptureUpsertChange(unrelatedSvc) + processor.CaptureUpsertChange(unrelatedSlice) + processor.CaptureUpsertChange(unrelatedNS) + processor.CaptureUpsertChange(unrelatedSecret) changed, _ := processor.Process() Expect(changed).To(BeTrue()) @@ -1766,7 +1914,6 @@ var _ = Describe("ChangeProcessor", func() { ) }) }) - Describe("Webhook validation cases", Ordered, func() { var ( processor state.ChangeProcessor @@ -1782,13 +1929,12 @@ var _ = Describe("ChangeProcessor", func() { fakeEventRecorder = record.NewFakeRecorder(2 /* number of buffered events */) processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ - GatewayCtlrName: controllerName, - GatewayClassName: gcName, - RelationshipCapturer: relationship.NewCapturerImpl(), - Logger: zap.New(), - Validators: createAlwaysValidValidators(), - EventRecorder: fakeEventRecorder, - Scheme: createScheme(), + GatewayCtlrName: controllerName, + GatewayClassName: gcName, + Logger: zap.New(), + Validators: createAlwaysValidValidators(), + EventRecorder: fakeEventRecorder, + Scheme: createScheme(), }) gc = &v1.GatewayClass{ @@ -1947,21 +2093,22 @@ var _ = Describe("ChangeProcessor", func() { assertGwEvent() }) }) - Describe("Webhook assumptions", func() { - var processor state.ChangeProcessor + var ( + processor state.ChangeProcessor + fakeEventRecorder *record.FakeRecorder + ) BeforeEach(func() { fakeEventRecorder = record.NewFakeRecorder(1 /* number of buffered events */) processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ - GatewayCtlrName: controllerName, - GatewayClassName: gcName, - RelationshipCapturer: relationship.NewCapturerImpl(), - Logger: zap.New(), - Validators: createAlwaysValidValidators(), - EventRecorder: fakeEventRecorder, - Scheme: createScheme(), + GatewayCtlrName: controllerName, + GatewayClassName: gcName, + Logger: zap.New(), + Validators: createAlwaysValidValidators(), + EventRecorder: fakeEventRecorder, + Scheme: createScheme(), }) }) @@ -2078,22 +2225,15 @@ var _ = Describe("ChangeProcessor", func() { ) }) }) - Describe("Edge cases with panic", func() { - var ( - processor state.ChangeProcessor - fakeRelationshipCapturer *relationshipfakes.FakeCapturer - ) + var processor state.ChangeProcessor BeforeEach(func() { - fakeRelationshipCapturer = &relationshipfakes.FakeCapturer{} - processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ - GatewayCtlrName: "test.controller", - GatewayClassName: "my-class", - RelationshipCapturer: fakeRelationshipCapturer, - Validators: createAlwaysValidValidators(), - Scheme: createScheme(), + GatewayCtlrName: "test.controller", + GatewayClassName: "my-class", + Validators: createAlwaysValidValidators(), + Scheme: createScheme(), }) }) diff --git a/internal/mode/static/state/changed_predicate.go b/internal/mode/static/state/changed_predicate.go index 08fc3301b7..44b1ff71c9 100644 --- a/internal/mode/static/state/changed_predicate.go +++ b/internal/mode/static/state/changed_predicate.go @@ -1,35 +1,35 @@ package state -import "sigs.k8s.io/controller-runtime/pkg/client" +import ( + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) // stateChangedPredicate determines whether upsert and delete events constitute a change in state. type stateChangedPredicate interface { // upsert returns true if the newObject changes state. upsert(oldObject, newObject client.Object) bool // delete returns true if the deletion of the object changes state. - delete(object client.Object) bool + delete(object client.Object, nsname types.NamespacedName) bool } // funcPredicate applies the stateChanged function on upsert and delete. On upsert, the newObject is passed. // Implements stateChangedPredicate. type funcPredicate struct { - stateChanged func(object client.Object) bool + stateChanged func(object client.Object, nsname types.NamespacedName) bool } func (f funcPredicate) upsert(_, newObject client.Object) bool { - return f.stateChanged(newObject) -} + if newObject == nil { + panic("new object cannot be nil") + } -func (f funcPredicate) delete(object client.Object) bool { - return f.stateChanged(object) + return f.stateChanged(newObject, client.ObjectKeyFromObject(newObject)) } -// FIXME(kevin85421): We should remove this predicate and update changeTrackingUpdater once #1432 is merged. -type alwaysProcess struct{} - -func (alwaysProcess) delete(_ client.Object) bool { return true } - -func (alwaysProcess) upsert(_, _ client.Object) bool { return true } +func (f funcPredicate) delete(object client.Object, nsname types.NamespacedName) bool { + return f.stateChanged(object, nsname) +} // annotationChangedPredicate implements stateChangedPredicate based on the value of the annotation provided. // This predicate will return true on upsert if the annotation's value has changed. @@ -44,7 +44,7 @@ func (a annotationChangedPredicate) upsert(oldObject, newObject client.Object) b } if newObject == nil { - panic("Cannot determine if annotation has changed on upsert because new object is nil") + panic("cannot determine if annotation has changed on upsert because new object is nil") } oldAnnotation := oldObject.GetAnnotations()[a.annotation] @@ -53,4 +53,4 @@ func (a annotationChangedPredicate) upsert(oldObject, newObject client.Object) b return oldAnnotation != newAnnotation } -func (a annotationChangedPredicate) delete(_ client.Object) bool { return true } +func (a annotationChangedPredicate) delete(_ client.Object, _ types.NamespacedName) bool { return true } diff --git a/internal/mode/static/state/changed_predicate_test.go b/internal/mode/static/state/changed_predicate_test.go index 9902ca61a8..15af3f82d0 100644 --- a/internal/mode/static/state/changed_predicate_test.go +++ b/internal/mode/static/state/changed_predicate_test.go @@ -6,25 +6,40 @@ import ( . "github.com/onsi/gomega" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) func TestFuncPredicate(t *testing.T) { - alwaysTrueFunc := func(object client.Object) bool { return true } + alwaysTrueFunc := func(object client.Object, _ types.NamespacedName) bool { return true } + emptyObject := &v1.Pod{} p := funcPredicate{stateChanged: alwaysTrueFunc} g := NewWithT(t) - g.Expect(p.delete(nil)).To(BeTrue()) - g.Expect(p.upsert(nil, nil)).To(BeTrue()) + g.Expect(p.delete(nil, types.NamespacedName{})).To(BeTrue()) + g.Expect(p.upsert(nil, emptyObject)).To(BeTrue()) +} + +func TestFuncPredicate_Panic(t *testing.T) { + alwaysTrueFunc := func(object client.Object, _ types.NamespacedName) bool { return true } + + p := funcPredicate{stateChanged: alwaysTrueFunc} + + g := NewWithT(t) + + upsert := func() { + p.upsert(nil, nil) + } + g.Expect(upsert).Should(Panic()) } func TestAnnotationChangedPredicate_Delete(t *testing.T) { p := annotationChangedPredicate{} g := NewWithT(t) - g.Expect(p.delete(nil)).To(BeTrue()) + g.Expect(p.delete(nil, types.NamespacedName{})).To(BeTrue()) } func TestAnnotationChangedPredicate_Update(t *testing.T) { diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index 81ebb817c3..d48a304b32 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -402,7 +402,7 @@ func buildUpstreams( var errMsg string - eps, err := resolver.Resolve(ctx, br.Svc, br.Port) + eps, err := resolver.Resolve(ctx, br.SvcNsName, br.ServicePort) if err != nil { errMsg = err.Error() } diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index d4d8a3a7bf..cce6f0e749 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -82,16 +82,14 @@ func TestBuildConfiguration(t *testing.T) { Endpoints: fooEndpoints, } - fooSvc := &apiv1.Service{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}} - fakeResolver := &resolverfakes.FakeServiceResolver{} fakeResolver.ResolveReturns(fooEndpoints, nil) validBackendRef := graph.BackendRef{ - Svc: fooSvc, - Port: 80, - Valid: true, - Weight: 1, + SvcNsName: types.NamespacedName{Name: "foo", Namespace: "test"}, + ServicePort: apiv1.ServicePort{Port: 80}, + Valid: true, + Weight: 1, } expValidBackend := Backend{ @@ -1877,9 +1875,9 @@ func TestBuildUpstreams(t *testing.T) { var backends []graph.BackendRef for _, name := range serviceNames { backends = append(backends, graph.BackendRef{ - Svc: &apiv1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: name}}, - Port: 80, - Valid: name != "", + SvcNsName: types.NamespacedName{Namespace: "test", Name: name}, + ServicePort: apiv1.ServicePort{Port: 80}, + Valid: name != "", }) } return backends @@ -1995,8 +1993,12 @@ func TestBuildUpstreams(t *testing.T) { } fakeResolver := &resolverfakes.FakeServiceResolver{} - fakeResolver.ResolveCalls(func(ctx context.Context, svc *apiv1.Service, port int32) ([]resolver.Endpoint, error) { - switch svc.Name { + fakeResolver.ResolveCalls(func( + ctx context.Context, + svcNsName types.NamespacedName, + servicePort apiv1.ServicePort, + ) ([]resolver.Endpoint, error) { + switch svcNsName.Name { case "bar": return barEndpoints, nil case "baz": @@ -2012,7 +2014,7 @@ func TestBuildUpstreams(t *testing.T) { case "abc": return abcEndpoints, nil default: - return nil, fmt.Errorf("unexpected service %s", svc.Name) + return nil, fmt.Errorf("unexpected service %s", svcNsName.Name) } }) diff --git a/internal/mode/static/state/graph/backend_refs.go b/internal/mode/static/state/graph/backend_refs.go index 5bc1203ebd..f8eab82d80 100644 --- a/internal/mode/static/state/graph/backend_refs.go +++ b/internal/mode/static/state/graph/backend_refs.go @@ -14,22 +14,23 @@ import ( // BackendRef is an internal representation of a backendRef in an HTTPRoute. type BackendRef struct { - // Svc is the service referenced by the backendRef. - Svc *v1.Service - // Port is the port of the backendRef. - Port int32 + // SvcNsName is the NamespacedName of the Service referenced by the backendRef. + SvcNsName types.NamespacedName + // ServicePort is the ServicePort of the Service which is referenced by the backendRef. + ServicePort v1.ServicePort // Weight is the weight of the backendRef. Weight int32 // Valid indicates whether the backendRef is valid. + // No configuration should be generated for an invalid BackendRef. Valid bool } // ServicePortReference returns a string representation for the service and port that is referenced by the BackendRef. func (b BackendRef) ServicePortReference() string { - if b.Svc == nil { + if !b.Valid { return "" } - return fmt.Sprintf("%s_%s_%d", b.Svc.Namespace, b.Svc.Name, b.Port) + return fmt.Sprintf("%s_%s_%d", b.SvcNsName.Namespace, b.SvcNsName.Name, b.ServicePort.Port) } func addBackendRefsToRouteRules( @@ -116,11 +117,13 @@ func createBackendRef( return backendRef, &cond } - svc, port, err := getServiceAndPortFromRef(ref.BackendRef, sourceNamespace, services, refPath) + svcNsName, svcPort, err := getServiceAndPortFromRef(ref.BackendRef, sourceNamespace, services, refPath) if err != nil { backendRef = BackendRef{ - Weight: weight, - Valid: false, + SvcNsName: svcNsName, + ServicePort: svcPort, + Weight: weight, + Valid: false, } cond := staticConds.NewRouteBackendRefRefBackendNotFound(err.Error()) @@ -128,21 +131,25 @@ func createBackendRef( } backendRef = BackendRef{ - Svc: svc, - Port: port, - Valid: true, - Weight: weight, + SvcNsName: svcNsName, + ServicePort: svcPort, + Valid: true, + Weight: weight, } return backendRef, nil } +// getServiceAndPortFromRef extracts the NamespacedName of the Service and the port from a BackendRef. +// It can return an error and an empty v1.ServicePort in two cases: +// 1. The Service referenced from the BackendRef does not exist in the cluster/state. +// 2. The Port on the BackendRef does not match any of the ServicePorts on the Service. func getServiceAndPortFromRef( ref gatewayv1.BackendRef, routeNamespace string, services map[types.NamespacedName]*v1.Service, refPath *field.Path, -) (*v1.Service, int32, error) { +) (types.NamespacedName, v1.ServicePort, error) { ns := routeNamespace if ref.Namespace != nil { ns = string(*ref.Namespace) @@ -152,11 +159,16 @@ func getServiceAndPortFromRef( svc, ok := services[svcNsName] if !ok { - return nil, 0, field.NotFound(refPath.Child("name"), ref.Name) + return svcNsName, v1.ServicePort{}, field.NotFound(refPath.Child("name"), ref.Name) + } + + // safe to dereference port here because we already validated that the port is not nil in validateBackendRef. + svcPort, err := getServicePort(svc, int32(*ref.Port)) + if err != nil { + return svcNsName, v1.ServicePort{}, err } - // safe to dereference port here because we already validated that the port is not nil. - return svc, int32(*ref.Port), nil + return svcNsName, svcPort, nil } func validateHTTPBackendRef( @@ -233,3 +245,13 @@ func validateWeight(weight int32) error { return nil } + +func getServicePort(svc *v1.Service, port int32) (v1.ServicePort, error) { + for _, p := range svc.Spec.Ports { + if p.Port == port { + return p, nil + } + } + + return v1.ServicePort{}, fmt.Errorf("no matching port for Service %s and port %d", svc.Name, port) +} diff --git a/internal/mode/static/state/graph/backend_refs_test.go b/internal/mode/static/state/graph/backend_refs_test.go index d81cf386bc..d2205c752f 100644 --- a/internal/mode/static/state/graph/backend_refs_test.go +++ b/internal/mode/static/state/graph/backend_refs_test.go @@ -244,6 +244,17 @@ func TestGetServiceAndPortFromRef(t *testing.T) { Name: "service1", Namespace: "test", }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + }, + }, + }, + } + svc1NsName := types.NamespacedName{ + Namespace: "test", + Name: "service1", } svc2 := &v1.Service{ @@ -254,17 +265,17 @@ func TestGetServiceAndPortFromRef(t *testing.T) { } tests := []struct { - ref gatewayv1.BackendRef - expService *v1.Service - name string - expPort int32 - expErr bool + ref gatewayv1.BackendRef + expServiceNsName types.NamespacedName + name string + expServicePort v1.ServicePort + expErr bool }{ { - name: "normal case", - ref: getNormalRef(), - expService: svc1, - expPort: 80, + name: "normal case", + ref: getNormalRef(), + expServiceNsName: svc1NsName, + expServicePort: v1.ServicePort{Port: 80}, }, { name: "service does not exist", @@ -272,7 +283,19 @@ func TestGetServiceAndPortFromRef(t *testing.T) { backend.Name = "does-not-exist" return backend }), - expErr: true, + expErr: true, + expServiceNsName: types.NamespacedName{Name: "does-not-exist", Namespace: "test"}, + expServicePort: v1.ServicePort{}, + }, + { + name: "no matching port for service and port", + ref: getModifiedRef(func(backend gatewayv1.BackendRef) gatewayv1.BackendRef { + backend.Port = helpers.GetPointer[gatewayv1.PortNumber](504) + return backend + }), + expErr: true, + expServiceNsName: svc1NsName, + expServicePort: v1.ServicePort{}, }, } @@ -287,11 +310,11 @@ func TestGetServiceAndPortFromRef(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - svc, port, err := getServiceAndPortFromRef(test.ref, "test", services, refPath) + svcNsName, servicePort, err := getServiceAndPortFromRef(test.ref, "test", services, refPath) g.Expect(err != nil).To(Equal(test.expErr)) - g.Expect(svc).To(Equal(test.expService)) - g.Expect(port).To(Equal(test.expPort)) + g.Expect(svcNsName).To(Equal(test.expServiceNsName)) + g.Expect(servicePort).To(Equal(test.expServicePort)) }) } } @@ -374,7 +397,26 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { hrWithZeroBackendRefs := createRoute("hr4", "Service", 1, "svc1") hrWithZeroBackendRefs.Spec.Rules[0].BackendRefs = nil - svc1 := &v1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: "svc1"}} + svc1 := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "svc1", + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + }, + { + Port: 81, + }, + }, + }, + } + svc1NsName := types.NamespacedName{ + Namespace: "test", + Name: "svc1", + } services := map[types.NamespacedName]*v1.Service{ {Namespace: "test", Name: "svc1"}: svc1, @@ -395,10 +437,10 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { }, expectedBackendRefs: []BackendRef{ { - Svc: svc1, - Port: 80, - Valid: true, - Weight: 1, + SvcNsName: svc1NsName, + ServicePort: svc1.Spec.Ports[0], + Valid: true, + Weight: 1, }, }, expectedConditions: nil, @@ -413,16 +455,16 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { }, expectedBackendRefs: []BackendRef{ { - Svc: svc1, - Port: 80, - Valid: true, - Weight: 1, + SvcNsName: svc1NsName, + ServicePort: svc1.Spec.Ports[0], + Valid: true, + Weight: 1, }, { - Svc: svc1, - Port: 81, - Valid: true, - Weight: 5, + SvcNsName: svc1NsName, + ServicePort: svc1.Spec.Ports[1], + Valid: true, + Weight: 5, }, }, expectedConditions: nil, @@ -510,7 +552,23 @@ func TestAddBackendRefsToRulesTest(t *testing.T) { } func TestCreateBackend(t *testing.T) { - svc1 := &v1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: "service1"}} + svc1 := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "service1", + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + }, + }, + }, + } + svc1NamespacedName := types.NamespacedName{ + Namespace: "test", + Name: "service1", + } tests := []struct { expectedCondition *conditions.Condition @@ -524,10 +582,10 @@ func TestCreateBackend(t *testing.T) { BackendRef: getNormalRef(), }, expectedBackend: BackendRef{ - Svc: svc1, - Port: 80, - Weight: 5, - Valid: true, + SvcNsName: svc1NamespacedName, + ServicePort: svc1.Spec.Ports[0], + Weight: 5, + Valid: true, }, expectedServicePortReference: "test_service1_80", expectedCondition: nil, @@ -541,10 +599,10 @@ func TestCreateBackend(t *testing.T) { }), }, expectedBackend: BackendRef{ - Svc: svc1, - Port: 80, - Weight: 1, - Valid: true, + SvcNsName: svc1NamespacedName, + ServicePort: svc1.Spec.Ports[0], + Weight: 1, + Valid: true, }, expectedServicePortReference: "test_service1_80", expectedCondition: nil, @@ -558,10 +616,10 @@ func TestCreateBackend(t *testing.T) { }), }, expectedBackend: BackendRef{ - Svc: nil, - Port: 0, - Weight: 0, - Valid: false, + SvcNsName: types.NamespacedName{}, + ServicePort: v1.ServicePort{}, + Weight: 0, + Valid: false, }, expectedServicePortReference: "", expectedCondition: helpers.GetPointer( @@ -579,10 +637,10 @@ func TestCreateBackend(t *testing.T) { }), }, expectedBackend: BackendRef{ - Svc: nil, - Port: 0, - Weight: 5, - Valid: false, + SvcNsName: types.NamespacedName{}, + ServicePort: v1.ServicePort{}, + Weight: 5, + Valid: false, }, expectedServicePortReference: "", expectedCondition: helpers.GetPointer( @@ -600,10 +658,10 @@ func TestCreateBackend(t *testing.T) { }), }, expectedBackend: BackendRef{ - Svc: nil, - Port: 0, - Weight: 5, - Valid: false, + SvcNsName: types.NamespacedName{Name: "not-exist", Namespace: "test"}, + ServicePort: v1.ServicePort{}, + Weight: 5, + Valid: false, }, expectedServicePortReference: "", expectedCondition: helpers.GetPointer( @@ -635,3 +693,34 @@ func TestCreateBackend(t *testing.T) { }) } } + +func TestGetServicePort(t *testing.T) { + svc := &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + }, + { + Port: 81, + }, + { + Port: 82, + }, + }, + }, + } + + g := NewWithT(t) + // ports exist + for _, p := range []int32{80, 81, 82} { + port, err := getServicePort(svc, p) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(port.Port).To(Equal(p)) + } + + // port doesn't exist + port, err := getServicePort(svc, 83) + g.Expect(err).Should(HaveOccurred()) + g.Expect(port.Port).To(Equal(int32(0))) +} diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index c539886c48..556c38496b 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -2,12 +2,14 @@ package graph import ( v1 "k8s.io/api/core/v1" + discoveryV1 "k8s.io/api/discovery/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1beta1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -46,6 +48,9 @@ type Graph struct { ReferencedSecrets map[types.NamespacedName]*Secret // ReferencedNamespaces includes Namespaces with labels that match the Gateway Listener's label selector. ReferencedNamespaces map[types.NamespacedName]*v1.Namespace + // ReferencedServices includes the NamespacedNames of all the Services that are referenced by at least one HTTPRoute. + // Storing the whole resource is not necessary, compared to the similar maps above. + ReferencedServices map[types.NamespacedName]struct{} } // ProtectedPorts are the ports that may not be configured by a listener with a descriptive name of each port. @@ -53,11 +58,6 @@ 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(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 obj := resourceType.(type) { case *v1.Secret: _, exists := g.ReferencedSecrets[nsname] @@ -78,6 +78,17 @@ func (g *Graph) IsReferenced(resourceType client.Object, nsname types.Namespaced _, existed := g.ReferencedNamespaces[nsname] exists := isNamespaceReferenced(obj, g.Gateway) return existed || exists + // Service reference exists if at least one HTTPRoute references it. + case *v1.Service: + _, exists := g.ReferencedServices[nsname] + return exists + // EndpointSlice reference exists if its Service owner is referenced by at least one HTTPRoute. + case *discoveryV1.EndpointSlice: + svcName := index.GetServiceNameFromEndpointSlice(obj) + + // Service Namespace should be the same Namespace as the EndpointSlice + _, exists := g.ReferencedServices[types.NamespacedName{Namespace: nsname.Namespace, Name: svcName}] + return exists default: return false } @@ -112,6 +123,8 @@ func BuildGraph( referencedNamespaces := buildReferencedNamespaces(state.Namespaces, gw) + referencedServices := buildReferencedServices(routes) + g := &Graph{ GatewayClass: gc, Gateway: gw, @@ -120,6 +133,7 @@ func BuildGraph( IgnoredGateways: processedGws.Ignored, ReferencedSecrets: secretResolver.getResolvedSecrets(), ReferencedNamespaces: referencedNamespaces, + ReferencedServices: referencedServices, } return g diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index bc705d3d7d..ed10c01d51 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/gomega" v1 "k8s.io/api/core/v1" + discoveryV1 "k8s.io/api/discovery/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" @@ -13,6 +14,7 @@ import ( gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1beta1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" @@ -88,23 +90,21 @@ func TestBuildGraph(t *testing.T) { hr2 := createRoute("hr-2", "wrong-gateway", "listener-80-1") hr3 := createRoute("hr-3", "gateway-1", "listener-443-1") // https listener; should not conflict with hr1 - fooSvc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "service"}} - hr1Refs := []BackendRef{ { - Svc: fooSvc, - Port: 80, - Valid: true, - Weight: 1, + SvcNsName: types.NamespacedName{Namespace: "service", Name: "foo"}, + ServicePort: v1.ServicePort{Port: 80}, + Valid: true, + Weight: 1, }, } hr3Refs := []BackendRef{ { - Svc: fooSvc, - Port: 80, - Valid: true, - Weight: 1, + SvcNsName: types.NamespacedName{Namespace: "service", Name: "foo"}, + ServicePort: v1.ServicePort{Port: 80}, + Valid: true, + Weight: 1, }, } @@ -179,7 +179,18 @@ func TestBuildGraph(t *testing.T) { gw1 := createGateway("gateway-1") gw2 := createGateway("gateway-2") - svc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: "service", Name: "foo"}} + svc := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "service", Name: "foo", + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + }, + }, + }, + } rgSecret := &v1beta1.ReferenceGrant{ ObjectMeta: metav1.ObjectMeta{ @@ -336,6 +347,9 @@ func TestBuildGraph(t *testing.T) { ReferencedNamespaces: map[types.NamespacedName]*v1.Namespace{ client.ObjectKeyFromObject(ns): ns, }, + ReferencedServices: map[types.NamespacedName]struct{}{ + client.ObjectKeyFromObject(svc): {}, + }, } } @@ -427,6 +441,39 @@ func TestIsReferenced(t *testing.T) { }, } + serviceInGraph := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "serviceInGraph", + }, + } + serviceNotInGraph := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "serviceNotInGraph", + }, + } + serviceNotInGraphSameNameDifferentNS := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "not-default", + Name: "serviceInGraph", + }, + } + emptyService := &v1.Service{} + + createEndpointSlice := func(name string, svcName string) *discoveryV1.EndpointSlice { + return &discoveryV1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: name, + Labels: map[string]string{index.KubernetesServiceNameLabel: svcName}, + }, + } + } + endpointSliceInGraph := createEndpointSlice("endpointSliceInGraph", "serviceInGraph") + endpointSliceNotInGraph := createEndpointSlice("endpointSliceNotInGraph", "serviceNotInGraph") + emptyEndpointSlice := &discoveryV1.EndpointSlice{} + gw := &Gateway{ Listeners: []*Listener{ { @@ -457,6 +504,9 @@ func TestIsReferenced(t *testing.T) { ReferencedNamespaces: map[types.NamespacedName]*v1.Namespace{ client.ObjectKeyFromObject(nsInGraph): nsInGraph, }, + ReferencedServices: map[types.NamespacedName]struct{}{ + client.ObjectKeyFromObject(serviceInGraph): {}, + }, } tests := []struct { @@ -465,45 +515,97 @@ func TestIsReferenced(t *testing.T) { name string expected bool }{ + // Namespace tests { - name: "Namespace in graph's ReferencedNamespaces passes", + name: "Namespace in graph's ReferencedNamespaces is referenced", resource: nsInGraph, graph: graph, expected: true, }, { - name: "Namespace with a different name but same labels fails", + name: "Namespace with a different name but same labels is not referenced", resource: nsNotInGraph, graph: graph, expected: false, }, { - name: "Namespace not in ReferencedNamespaces but in Gateway Listener's AllowedRouteLabelSelector passes", + name: "Namespace not in ReferencedNamespaces but in Gateway Listener's AllowedRouteLabelSelector" + + " is referenced", resource: nsNotInGraphButInGateway, graph: graph, expected: true, }, + + // Secret tests { - name: "Secret in graph's ReferencedSecrets passes", + name: "Secret in graph's ReferencedSecrets is referenced", resource: baseSecret, graph: graph, expected: true, }, { - name: "Secret not in ReferencedSecrets with same Namespace and different Name fails", + name: "Secret not in ReferencedSecrets with same Namespace and different Name is not referenced", resource: sameNamespaceDifferentNameSecret, graph: graph, expected: false, }, { - name: "Secret not in ReferencedSecrets with different Namespace and same Name fails", + name: "Secret not in ReferencedSecrets with different Namespace and same Name is not referenced", resource: differentNamespaceSameNameSecret, graph: graph, expected: false, }, + + // Service tests + { + name: "Service is referenced", + resource: serviceInGraph, + graph: graph, + expected: true, + }, + { + name: "Service is not referenced", + resource: serviceNotInGraph, + graph: graph, + expected: false, + }, + { + name: "Service with same name but different namespace is not referenced", + resource: serviceNotInGraphSameNameDifferentNS, + graph: graph, + expected: false, + }, + { + name: "Empty Service", + resource: emptyService, + graph: graph, + expected: false, + }, + + // EndpointSlice tests + { + name: "EndpointSlice with Service owner in graph's ReferencedServices is referenced", + resource: endpointSliceInGraph, + graph: graph, + expected: true, + }, + { + name: "EndpointSlice with Service owner not in graph's ReferencedServices is not referenced", + resource: endpointSliceNotInGraph, + graph: graph, + expected: false, + }, + { + name: "Empty EndpointSlice", + resource: emptyEndpointSlice, + graph: graph, + expected: false, + }, + + // Edge cases { name: "Resource is not supported by IsReferenced", - resource: &v1.Service{}, + resource: &gatewayv1.HTTPRoute{}, graph: graph, expected: false, }, diff --git a/internal/mode/static/state/graph/service.go b/internal/mode/static/state/graph/service.go new file mode 100644 index 0000000000..9568b59df1 --- /dev/null +++ b/internal/mode/static/state/graph/service.go @@ -0,0 +1,47 @@ +package graph + +import ( + "k8s.io/apimachinery/pkg/types" +) + +func buildReferencedServices( + routes map[types.NamespacedName]*Route, +) map[types.NamespacedName]struct{} { + svcNames := make(map[types.NamespacedName]struct{}) + + // routes all have populated ParentRefs from when they were created. + // + // Get all the service names referenced from all the HTTPRoutes. + for _, route := range routes { + if !route.Valid { + continue + } + + // If none of the ParentRefs are attached to the Gateway, we want to skip the route. + attached := false + for _, ref := range route.ParentRefs { + if ref.Attachment.Attached { + attached = true + break + } + } + if !attached { + continue + } + + for _, rule := range route.Rules { + for _, ref := range rule.BackendRefs { + // Processes both valid and invalid BackendRefs as invalid ones still have referenced services + // we may want to track. + if ref.SvcNsName != (types.NamespacedName{}) { + svcNames[ref.SvcNsName] = struct{}{} + } + } + } + } + + if len(svcNames) == 0 { + return nil + } + return svcNames +} diff --git a/internal/mode/static/state/graph/service_test.go b/internal/mode/static/state/graph/service_test.go new file mode 100644 index 0000000000..82cf2960e0 --- /dev/null +++ b/internal/mode/static/state/graph/service_test.go @@ -0,0 +1,300 @@ +package graph + +import ( + "testing" + + . "github.com/onsi/gomega" + + "k8s.io/apimachinery/pkg/types" +) + +func TestBuildReferencedServices(t *testing.T) { + normalRoute := &Route{ + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + }, + }, + }, + Valid: true, + Rules: []Rule{ + { + BackendRefs: []BackendRef{ + { + SvcNsName: types.NamespacedName{Namespace: "banana-ns", Name: "service"}, + Weight: 1, + }, + }, + ValidMatches: true, + ValidFilters: true, + }, + }, + } + + validRouteTwoServicesOneRule := &Route{ + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + }, + }, + }, + Valid: true, + Rules: []Rule{ + { + BackendRefs: []BackendRef{ + { + SvcNsName: types.NamespacedName{Namespace: "service-ns", Name: "service"}, + Weight: 1, + }, + { + SvcNsName: types.NamespacedName{Namespace: "service-ns2", Name: "service2"}, + Weight: 1, + }, + }, + ValidMatches: true, + ValidFilters: true, + }, + }, + } + + validRouteTwoServicesTwoRules := &Route{ + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + }, + }, + }, + Valid: true, + Rules: []Rule{ + { + BackendRefs: []BackendRef{ + { + SvcNsName: types.NamespacedName{Namespace: "service-ns", Name: "service"}, + Weight: 1, + }, + }, + ValidMatches: true, + ValidFilters: true, + }, + { + BackendRefs: []BackendRef{ + { + SvcNsName: types.NamespacedName{Namespace: "service-ns2", Name: "service2"}, + Weight: 1, + }, + }, + ValidMatches: true, + ValidFilters: true, + }, + }, + } + + invalidRoute := &Route{ + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + }, + }, + }, + Valid: false, + Rules: []Rule{ + { + BackendRefs: []BackendRef{ + { + SvcNsName: types.NamespacedName{Namespace: "service-ns", Name: "service"}, + Weight: 1, + }, + }, + ValidMatches: true, + ValidFilters: true, + }, + }, + } + + unattachedRoute := &Route{ + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + Attached: false, + }, + }, + }, + Valid: true, + Rules: []Rule{ + { + BackendRefs: []BackendRef{ + { + SvcNsName: types.NamespacedName{Namespace: "service-ns", Name: "service"}, + Weight: 1, + }, + }, + ValidMatches: true, + ValidFilters: true, + }, + }, + } + + attachedRouteWithManyParentRefs := &Route{ + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + Attached: false, + }, + }, + { + Attachment: &ParentRefAttachmentStatus{ + Attached: false, + }, + }, + { + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + }, + }, + }, + Valid: true, + Rules: []Rule{ + { + BackendRefs: []BackendRef{ + { + SvcNsName: types.NamespacedName{Namespace: "service-ns", Name: "service"}, + Weight: 1, + }, + }, + ValidMatches: true, + ValidFilters: true, + }, + }, + } + validRouteNoServiceNsName := &Route{ + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + }, + }, + }, + Valid: true, + Rules: []Rule{ + { + BackendRefs: []BackendRef{ + { + Weight: 1, + }, + }, + ValidMatches: true, + ValidFilters: true, + }, + }, + } + + tests := []struct { + routes map[types.NamespacedName]*Route + exp map[types.NamespacedName]struct{} + name string + }{ + { + name: "normal route", + routes: map[types.NamespacedName]*Route{ + {Name: "normal-route"}: normalRoute, + }, + exp: map[types.NamespacedName]struct{}{ + {Namespace: "banana-ns", Name: "service"}: {}, + }, + }, + { + name: "route with two services in one Rule", + routes: map[types.NamespacedName]*Route{ + {Name: "two-svc-one-rule"}: validRouteTwoServicesOneRule, + }, + exp: map[types.NamespacedName]struct{}{ + {Namespace: "service-ns", Name: "service"}: {}, + {Namespace: "service-ns2", Name: "service2"}: {}, + }, + }, + { + name: "route with one service per rule", + routes: map[types.NamespacedName]*Route{ + {Name: "one-svc-per-rule"}: validRouteTwoServicesTwoRules, + }, + exp: map[types.NamespacedName]struct{}{ + {Namespace: "service-ns", Name: "service"}: {}, + {Namespace: "service-ns2", Name: "service2"}: {}, + }, + }, + { + name: "two valid routes with same services", + routes: map[types.NamespacedName]*Route{ + {Name: "one-svc-per-rule"}: validRouteTwoServicesTwoRules, + {Name: "two-svc-one-rule"}: validRouteTwoServicesOneRule, + }, + exp: map[types.NamespacedName]struct{}{ + {Namespace: "service-ns", Name: "service"}: {}, + {Namespace: "service-ns2", Name: "service2"}: {}, + }, + }, + { + name: "two valid routes with different services", + routes: map[types.NamespacedName]*Route{ + {Name: "one-svc-per-rule"}: validRouteTwoServicesTwoRules, + {Name: "normal-route"}: normalRoute, + }, + exp: map[types.NamespacedName]struct{}{ + {Namespace: "service-ns", Name: "service"}: {}, + {Namespace: "service-ns2", Name: "service2"}: {}, + {Namespace: "banana-ns", Name: "service"}: {}, + }, + }, + { + name: "invalid route", + routes: map[types.NamespacedName]*Route{ + {Name: "invalid-route"}: invalidRoute, + }, + exp: nil, + }, + { + name: "unattached route", + routes: map[types.NamespacedName]*Route{ + {Name: "unattached-route"}: unattachedRoute, + }, + exp: nil, + }, + { + name: "combination of valid and invalid routes", + routes: map[types.NamespacedName]*Route{ + {Name: "normal-route"}: normalRoute, + {Name: "invalid-route"}: invalidRoute, + }, + exp: map[types.NamespacedName]struct{}{ + {Namespace: "banana-ns", Name: "service"}: {}, + }, + }, + { + name: "route with many parentRefs and one is attached", + routes: map[types.NamespacedName]*Route{ + {Name: "multiple-parent-ref-route"}: attachedRouteWithManyParentRefs, + }, + exp: map[types.NamespacedName]struct{}{ + {Namespace: "service-ns", Name: "service"}: {}, + }, + }, + { + name: "valid route no service nsname", + routes: map[types.NamespacedName]*Route{ + {Name: "no-service-nsname"}: validRouteNoServiceNsName, + }, + exp: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(buildReferencedServices(test.routes)).To(Equal(test.exp)) + }) + } +} diff --git a/internal/mode/static/state/relationship/capturer.go b/internal/mode/static/state/relationship/capturer.go deleted file mode 100644 index c8af779ea9..0000000000 --- a/internal/mode/static/state/relationship/capturer.go +++ /dev/null @@ -1,155 +0,0 @@ -package relationship - -import ( - v1 "k8s.io/api/core/v1" - discoveryV1 "k8s.io/api/discovery/v1" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" - - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" -) - -//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Capturer - -// Capturer captures relationships between Kubernetes objects and can be queried for whether a relationship exists -// for a given object. -// -// The relationships between HTTPRoutes -> Services are many to 1, -// so these relationships are tracked using a counter. -// A Service relationship exists if at least one HTTPRoute references it. -// An EndpointSlice relationship exists if its Service owner is referenced by at least one HTTPRoute. -type Capturer interface { - Capture(obj client.Object) - Remove(resourceType client.Object, nsname types.NamespacedName) - Exists(resourceType client.Object, nsname types.NamespacedName) bool -} - -type ( - // routeToServicesMap maps HTTPRoute names to the set of Services it references. - routeToServicesMap map[types.NamespacedName]map[types.NamespacedName]struct{} - // serviceRefCountMap maps Service names to the number of HTTPRoutes that reference it. - serviceRefCountMap map[types.NamespacedName]int -) - -// CapturerImpl implements the Capturer interface. -type CapturerImpl struct { - routesToServices routeToServicesMap - serviceRefCount serviceRefCountMap - endpointSliceOwners map[types.NamespacedName]types.NamespacedName -} - -// NewCapturerImpl creates a new instance of CapturerImpl. -func NewCapturerImpl() *CapturerImpl { - return &CapturerImpl{ - routesToServices: make(routeToServicesMap), - serviceRefCount: make(serviceRefCountMap), - endpointSliceOwners: make(map[types.NamespacedName]types.NamespacedName), - } -} - -// Capture captures relationships for the given object. -func (c *CapturerImpl) Capture(obj client.Object) { - switch o := obj.(type) { - case *gatewayv1.HTTPRoute: - c.upsertForRoute(o) - case *discoveryV1.EndpointSlice: - svcName := index.GetServiceNameFromEndpointSlice(o) - if svcName != "" { - c.endpointSliceOwners[client.ObjectKeyFromObject(o)] = types.NamespacedName{ - Namespace: o.Namespace, - Name: svcName, - } - } - } -} - -// Remove removes the relationship for the given object from the CapturerImpl. -func (c *CapturerImpl) Remove(resourceType client.Object, nsname types.NamespacedName) { - switch resourceType.(type) { - case *gatewayv1.HTTPRoute: - c.deleteForRoute(nsname) - case *discoveryV1.EndpointSlice: - delete(c.endpointSliceOwners, nsname) - } -} - -// Exists returns true if the given object has a relationship with another object. -func (c *CapturerImpl) Exists(resourceType client.Object, nsname types.NamespacedName) bool { - switch resourceType.(type) { - case *v1.Service: - return c.serviceRefCount[nsname] > 0 - case *discoveryV1.EndpointSlice: - svcOwner, exists := c.endpointSliceOwners[nsname] - return exists && c.serviceRefCount[svcOwner] > 0 - } - - return false -} - -// GetRefCountForService is used for unit testing purposes. It is not exposed through the Capturer interface. -func (c *CapturerImpl) GetRefCountForService(svcName types.NamespacedName) int { - return c.serviceRefCount[svcName] -} - -func (c *CapturerImpl) upsertForRoute(route *gatewayv1.HTTPRoute) { - oldServices := c.routesToServices[client.ObjectKeyFromObject(route)] - newServices := getBackendServiceNamesFromRoute(route) - - for svc := range oldServices { - if _, exist := newServices[svc]; !exist { - c.decrementRefCount(svc) - } - } - - for svc := range newServices { - if _, exist := oldServices[svc]; !exist { - c.serviceRefCount[svc]++ - } - } - - c.routesToServices[client.ObjectKeyFromObject(route)] = newServices -} - -func (c *CapturerImpl) deleteForRoute(routeName types.NamespacedName) { - services := c.routesToServices[routeName] - - for svc := range services { - c.decrementRefCount(svc) - } - - delete(c.routesToServices, routeName) -} - -func (c *CapturerImpl) decrementRefCount(svcName types.NamespacedName) { - if count, exist := c.serviceRefCount[svcName]; exist { - if count == 1 { - delete(c.serviceRefCount, svcName) - - return - } - - c.serviceRefCount[svcName]-- - } -} - -func getBackendServiceNamesFromRoute(hr *gatewayv1.HTTPRoute) map[types.NamespacedName]struct{} { - svcNames := make(map[types.NamespacedName]struct{}) - - for _, rule := range hr.Spec.Rules { - for _, ref := range rule.BackendRefs { - if ref.Kind != nil && *ref.Kind != "Service" { - continue - } - - ns := hr.Namespace - if ref.Namespace != nil { - ns = string(*ref.Namespace) - } - - svcNames[types.NamespacedName{Namespace: ns, Name: string(ref.Name)}] = struct{}{} - } - } - - return svcNames -} diff --git a/internal/mode/static/state/relationship/capturer_suite_test.go b/internal/mode/static/state/relationship/capturer_suite_test.go deleted file mode 100644 index 917ec274fa..0000000000 --- a/internal/mode/static/state/relationship/capturer_suite_test.go +++ /dev/null @@ -1,13 +0,0 @@ -package relationship_test - -import ( - "testing" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -func TestRelationships(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Relationships Suite") -} diff --git a/internal/mode/static/state/relationship/capturer_test.go b/internal/mode/static/state/relationship/capturer_test.go deleted file mode 100644 index 38437d34c4..0000000000 --- a/internal/mode/static/state/relationship/capturer_test.go +++ /dev/null @@ -1,335 +0,0 @@ -package relationship_test - -import ( - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - v1 "k8s.io/api/core/v1" - discoveryV1 "k8s.io/api/discovery/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" - - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/relationship" -) - -func createBackendRefs(backendNames ...gatewayv1.ObjectName) []gatewayv1.HTTPBackendRef { - refs := make([]gatewayv1.HTTPBackendRef, 0, len(backendNames)) - for _, name := range backendNames { - refs = append(refs, gatewayv1.HTTPBackendRef{ - BackendRef: gatewayv1.BackendRef{ - BackendObjectReference: gatewayv1.BackendObjectReference{ - Kind: (*gatewayv1.Kind)(helpers.GetPointer("Service")), - Name: name, - Namespace: (*gatewayv1.Namespace)(helpers.GetPointer("test")), - }, - }, - }) - } - - return refs -} - -func createRules(backendRefs ...[]gatewayv1.HTTPBackendRef) []gatewayv1.HTTPRouteRule { - rules := make([]gatewayv1.HTTPRouteRule, 0, len(backendRefs)) - for _, refs := range backendRefs { - rules = append(rules, gatewayv1.HTTPRouteRule{BackendRefs: refs}) - } - - return rules -} - -func createRoute(name string, rules []gatewayv1.HTTPRouteRule) *gatewayv1.HTTPRoute { - return &gatewayv1.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: name}, - Spec: gatewayv1.HTTPRouteSpec{Rules: rules}, - } -} - -var _ = Describe("Capturer", func() { - var ( - capturer *relationship.CapturerImpl - - backendRef1 = createBackendRefs("svc1") - backendRef2 = createBackendRefs("svc2") - backendRef3 = createBackendRefs("svc3") - backendRef4 = createBackendRefs("svc4") - - hr1 = createRoute("hr1", createRules(backendRef1)) - hr2 = createRoute("hr2", createRules(backendRef2, backendRef3, backendRef4)) - - hrSvc1AndSvc2 = createRoute("hr-svc1-svc2", createRules(backendRef1, backendRef2)) - hrSvc1AndSvc3 = createRoute("hr-svc1-svc3", createRules(backendRef3, backendRef1)) - hrSvc1AndSvc4 = createRoute("hr-svc1-svc4", createRules(backendRef1, backendRef4)) - - hr1Name = types.NamespacedName{Namespace: hr1.Namespace, Name: hr1.Name} - hr2Name = types.NamespacedName{Namespace: hr2.Namespace, Name: hr2.Name} - hrSvc1AndSvc2Name = types.NamespacedName{Namespace: hrSvc1AndSvc2.Namespace, Name: hrSvc1AndSvc2.Name} - hrSvc1AndSvc3Name = types.NamespacedName{Namespace: hrSvc1AndSvc3.Namespace, Name: hrSvc1AndSvc3.Name} - hrSvc1AndSvc4Name = types.NamespacedName{Namespace: hrSvc1AndSvc4.Namespace, Name: hrSvc1AndSvc4.Name} - - svc1 = types.NamespacedName{Namespace: "test", Name: "svc1"} - svc2 = types.NamespacedName{Namespace: "test", Name: "svc2"} - svc3 = types.NamespacedName{Namespace: "test", Name: "svc3"} - svc4 = types.NamespacedName{Namespace: "test", Name: "svc4"} - ) - - Describe("Capture service relationships for routes", func() { - BeforeEach(OncePerOrdered, func() { - capturer = relationship.NewCapturerImpl() - }) - - assertServiceExists := func(svcName types.NamespacedName, exists bool, refCount int) { - ExpectWithOffset(1, capturer.Exists(&v1.Service{}, svcName)).To(Equal(exists)) - ExpectWithOffset(1, capturer.GetRefCountForService(svcName)).To(Equal(refCount)) - } - - Describe("Normal cases", Ordered, func() { - When("a route with a backend service is captured", func() { - It("reports a service relationship", func() { - capturer.Capture(hr1) - - assertServiceExists(svc1, true, 1) - }) - }) - When("a route with multiple backend services is captured", func() { - It("reports all service relationships for all captured routes", func() { - capturer.Capture(hr2) - - assertServiceExists(svc1, true, 1) - assertServiceExists(svc2, true, 1) - assertServiceExists(svc3, true, 1) - assertServiceExists(svc4, true, 1) - }) - }) - When("one backend service is removed from a captured route", func() { - It("removes the correct service relationship", func() { - hr2Updated := hr2.DeepCopy() - hr2Updated.Spec.Rules = hr2Updated.Spec.Rules[0:2] // remove the last rule - - capturer.Capture(hr2Updated) - - assertServiceExists(svc1, true, 1) - assertServiceExists(svc2, true, 1) - assertServiceExists(svc3, true, 1) - assertServiceExists(svc4, false, 0) - }) - }) - When("one backend service is added to a captured route", func() { - It("adds the correct service relationship", func() { - capturer.Capture(hr2) - - assertServiceExists(svc1, true, 1) - assertServiceExists(svc2, true, 1) - assertServiceExists(svc3, true, 1) - assertServiceExists(svc4, true, 1) - }) - }) - When("a route with multiple backend services is removed", func() { - It("removes all service relationships", func() { - capturer.Remove(&gatewayv1.HTTPRoute{}, hr2Name) - - assertServiceExists(svc2, false, 0) - assertServiceExists(svc3, false, 0) - assertServiceExists(svc4, false, 0) - - // Service referenced by hr1 still exists - assertServiceExists(svc1, true, 1) - }) - }) - When("a route is removed", func() { - It("removes service relationships", func() { - capturer.Remove(&gatewayv1.HTTPRoute{}, hr1Name) - - assertServiceExists(svc1, false, 0) - }) - }) - }) - Describe("Multiple routes that reference the same service", Ordered, func() { - When("multiple routes are captured that all reference the same service", func() { - It("reports all service relationships", func() { - capturer.Capture(hr1) - capturer.Capture(hrSvc1AndSvc2) - capturer.Capture(hrSvc1AndSvc3) - capturer.Capture(hrSvc1AndSvc4) - - assertServiceExists(svc1, true, 4) - assertServiceExists(svc2, true, 1) - assertServiceExists(svc3, true, 1) - assertServiceExists(svc4, true, 1) - }) - }) - When("one route is removed", func() { - It("reports remaining service relationships", func() { - capturer.Remove(&gatewayv1.HTTPRoute{}, hr1Name) - - // ref count for svc1 should decrease by one - assertServiceExists(svc1, true, 3) - - // all other ref counts stay the same - assertServiceExists(svc2, true, 1) - assertServiceExists(svc3, true, 1) - assertServiceExists(svc4, true, 1) - }) - }) - When("another route is removed", func() { - It("reports remaining service relationships", func() { - capturer.Remove(&gatewayv1.HTTPRoute{}, hrSvc1AndSvc2Name) - - // svc2 should no longer exist - assertServiceExists(svc2, false, 0) - - // ref count for svc1 should decrease by one - assertServiceExists(svc1, true, 2) - - // all other ref counts stay the same - assertServiceExists(svc3, true, 1) - assertServiceExists(svc4, true, 1) - }) - }) - When("another route is removed", func() { - It("reports remaining service relationships", func() { - capturer.Remove(&gatewayv1.HTTPRoute{}, hrSvc1AndSvc3Name) - - // svc3 should no longer exist - assertServiceExists(svc3, false, 0) - - // svc2 should still not exist - assertServiceExists(svc2, false, 0) - - // ref count for svc1 should decrease by one - assertServiceExists(svc1, true, 1) - - // svc4 ref count should stay the same - assertServiceExists(svc4, true, 1) - }) - When("final route is removed", func() { - It("removes all service relationships", func() { - capturer.Remove(&gatewayv1.HTTPRoute{}, hrSvc1AndSvc4Name) - - // no services should exist and all ref counts should be 0 - assertServiceExists(svc1, false, 0) - assertServiceExists(svc2, false, 0) - assertServiceExists(svc3, false, 0) - assertServiceExists(svc4, false, 0) - }) - }) - When("route is removed again", func() { - It("service ref counts remain at 0", func() { - capturer.Remove(&gatewayv1.HTTPRoute{}, hrSvc1AndSvc4Name) - - // no services should exist and all ref counts should still be 0 - assertServiceExists(svc1, false, 0) - assertServiceExists(svc2, false, 0) - assertServiceExists(svc3, false, 0) - assertServiceExists(svc4, false, 0) - }) - }) - }) - }) - Describe("Capture endpoint slice relationships", func() { - var ( - slice1 = &discoveryV1.EndpointSlice{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "es1", - Labels: map[string]string{index.KubernetesServiceNameLabel: "svc1"}, - }, - } - - slice2 = &discoveryV1.EndpointSlice{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "es2", - Labels: map[string]string{index.KubernetesServiceNameLabel: "svc1"}, - }, - } - - slice1Name = types.NamespacedName{Namespace: slice1.Namespace, Name: slice1.Name} - slice2Name = types.NamespacedName{Namespace: slice2.Namespace, Name: slice2.Name} - ) - - BeforeEach(OncePerOrdered, func() { - capturer = relationship.NewCapturerImpl() - }) - - Describe("Normal cases", Ordered, func() { - When("an endpoint slice is captured that has an unrelated service owner", func() { - It("does not report an endpoint slice relationship", func() { - capturer.Capture(slice1) - - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice1Name)).To(BeFalse()) - }) - }) - When("a relationship is captured for the service owner", func() { - It("adds an endpoint slice relationship", func() { - capturer.Capture(hr1) - - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice1Name)).To(BeTrue()) - }) - }) - When("another endpoint slice is captured with the same service owner", func() { - It("adds another endpoint slice relationship", func() { - capturer.Capture(slice2) - - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice1Name)).To(BeTrue()) - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice2Name)).To(BeTrue()) - }) - }) - When("an endpoint slice is removed", func() { - It("removes the endpoint slice relationship", func() { - capturer.Remove(&discoveryV1.EndpointSlice{}, slice2Name) - - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice2Name)).To(BeFalse()) - - // slice 1 relationship should still exist - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice1Name)).To(BeTrue()) - }) - }) - When("endpoint slice service owner changes to an unrelated service owner", func() { - It("removes the endpoint slice relationship", func() { - updatedSlice1 := slice1.DeepCopy() - updatedSlice1.Labels[index.KubernetesServiceNameLabel] = "unrelated-svc" - - capturer.Capture(updatedSlice1) - - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice1Name)).To(BeFalse()) - }) - }) - When("endpoint slice service owner changes to a related service owner", func() { - It("adds an endpoint slice relationship", func() { - capturer.Capture(slice1) - - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice1Name)).To(BeTrue()) - }) - }) - When("service relationship is removed", func() { - It("removes the endpoint slice relationship", func() { - capturer.Remove(&gatewayv1.HTTPRoute{}, hr1Name) - - Expect(capturer.Exists(&discoveryV1.EndpointSlice{}, slice1Name)).To(BeFalse()) - }) - }) - }) - }) - }) - Describe("Edge cases", func() { - BeforeEach(func() { - capturer = relationship.NewCapturerImpl() - }) - It("Capture does not panic when passed an unsupported resource type", func() { - Expect(func() { - capturer.Capture(&gatewayv1.GatewayClass{}) - }).ToNot(Panic()) - }) - It("Remove does not panic when passed an unsupported resource type", func() { - Expect(func() { - capturer.Remove(&gatewayv1.GatewayClass{}, types.NamespacedName{}) - }).ToNot(Panic()) - }) - It("Exist returns false if passed an unsupported resource type", func() { - Expect(capturer.Exists(&gatewayv1.GatewayClass{}, types.NamespacedName{})).To(BeFalse()) - }) - }) -}) diff --git a/internal/mode/static/state/relationship/relationshipfakes/fake_capturer.go b/internal/mode/static/state/relationship/relationshipfakes/fake_capturer.go deleted file mode 100644 index ea4f3b6783..0000000000 --- a/internal/mode/static/state/relationship/relationshipfakes/fake_capturer.go +++ /dev/null @@ -1,195 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package relationshipfakes - -import ( - "sync" - - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/relationship" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type FakeCapturer struct { - CaptureStub func(client.Object) - captureMutex sync.RWMutex - captureArgsForCall []struct { - arg1 client.Object - } - ExistsStub func(client.Object, types.NamespacedName) bool - existsMutex sync.RWMutex - existsArgsForCall []struct { - arg1 client.Object - arg2 types.NamespacedName - } - existsReturns struct { - result1 bool - } - existsReturnsOnCall map[int]struct { - result1 bool - } - RemoveStub func(client.Object, types.NamespacedName) - removeMutex sync.RWMutex - removeArgsForCall []struct { - arg1 client.Object - arg2 types.NamespacedName - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeCapturer) Capture(arg1 client.Object) { - fake.captureMutex.Lock() - fake.captureArgsForCall = append(fake.captureArgsForCall, struct { - arg1 client.Object - }{arg1}) - stub := fake.CaptureStub - fake.recordInvocation("Capture", []interface{}{arg1}) - fake.captureMutex.Unlock() - if stub != nil { - fake.CaptureStub(arg1) - } -} - -func (fake *FakeCapturer) CaptureCallCount() int { - fake.captureMutex.RLock() - defer fake.captureMutex.RUnlock() - return len(fake.captureArgsForCall) -} - -func (fake *FakeCapturer) CaptureCalls(stub func(client.Object)) { - fake.captureMutex.Lock() - defer fake.captureMutex.Unlock() - fake.CaptureStub = stub -} - -func (fake *FakeCapturer) CaptureArgsForCall(i int) client.Object { - fake.captureMutex.RLock() - defer fake.captureMutex.RUnlock() - argsForCall := fake.captureArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeCapturer) Exists(arg1 client.Object, arg2 types.NamespacedName) bool { - fake.existsMutex.Lock() - ret, specificReturn := fake.existsReturnsOnCall[len(fake.existsArgsForCall)] - fake.existsArgsForCall = append(fake.existsArgsForCall, struct { - arg1 client.Object - arg2 types.NamespacedName - }{arg1, arg2}) - stub := fake.ExistsStub - fakeReturns := fake.existsReturns - fake.recordInvocation("Exists", []interface{}{arg1, arg2}) - fake.existsMutex.Unlock() - if stub != nil { - return stub(arg1, arg2) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeCapturer) ExistsCallCount() int { - fake.existsMutex.RLock() - defer fake.existsMutex.RUnlock() - return len(fake.existsArgsForCall) -} - -func (fake *FakeCapturer) ExistsCalls(stub func(client.Object, types.NamespacedName) bool) { - fake.existsMutex.Lock() - defer fake.existsMutex.Unlock() - fake.ExistsStub = stub -} - -func (fake *FakeCapturer) ExistsArgsForCall(i int) (client.Object, types.NamespacedName) { - fake.existsMutex.RLock() - defer fake.existsMutex.RUnlock() - argsForCall := fake.existsArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 -} - -func (fake *FakeCapturer) ExistsReturns(result1 bool) { - fake.existsMutex.Lock() - defer fake.existsMutex.Unlock() - fake.ExistsStub = nil - fake.existsReturns = struct { - result1 bool - }{result1} -} - -func (fake *FakeCapturer) ExistsReturnsOnCall(i int, result1 bool) { - fake.existsMutex.Lock() - defer fake.existsMutex.Unlock() - fake.ExistsStub = nil - if fake.existsReturnsOnCall == nil { - fake.existsReturnsOnCall = make(map[int]struct { - result1 bool - }) - } - fake.existsReturnsOnCall[i] = struct { - result1 bool - }{result1} -} - -func (fake *FakeCapturer) Remove(arg1 client.Object, arg2 types.NamespacedName) { - fake.removeMutex.Lock() - fake.removeArgsForCall = append(fake.removeArgsForCall, struct { - arg1 client.Object - arg2 types.NamespacedName - }{arg1, arg2}) - stub := fake.RemoveStub - fake.recordInvocation("Remove", []interface{}{arg1, arg2}) - fake.removeMutex.Unlock() - if stub != nil { - fake.RemoveStub(arg1, arg2) - } -} - -func (fake *FakeCapturer) RemoveCallCount() int { - fake.removeMutex.RLock() - defer fake.removeMutex.RUnlock() - return len(fake.removeArgsForCall) -} - -func (fake *FakeCapturer) RemoveCalls(stub func(client.Object, types.NamespacedName)) { - fake.removeMutex.Lock() - defer fake.removeMutex.Unlock() - fake.RemoveStub = stub -} - -func (fake *FakeCapturer) RemoveArgsForCall(i int) (client.Object, types.NamespacedName) { - fake.removeMutex.RLock() - defer fake.removeMutex.RUnlock() - argsForCall := fake.removeArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 -} - -func (fake *FakeCapturer) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.captureMutex.RLock() - defer fake.captureMutex.RUnlock() - fake.existsMutex.RLock() - defer fake.existsMutex.RUnlock() - fake.removeMutex.RLock() - defer fake.removeMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeCapturer) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ relationship.Capturer = new(FakeCapturer) diff --git a/internal/mode/static/state/relationship/relationships_test.go b/internal/mode/static/state/relationship/relationships_test.go deleted file mode 100644 index fee64b5936..0000000000 --- a/internal/mode/static/state/relationship/relationships_test.go +++ /dev/null @@ -1,164 +0,0 @@ -package relationship - -import ( - "testing" - - . "github.com/onsi/gomega" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - v1 "sigs.k8s.io/gateway-api/apis/v1" - - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" -) - -func TestGetBackendServiceNamesFromRoute(t *testing.T) { - getNormalRefs := func(svcName v1.ObjectName) []v1.HTTPBackendRef { - return []v1.HTTPBackendRef{ - { - BackendRef: v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Kind: (*v1.Kind)(helpers.GetPointer("Service")), - Name: svcName, - Namespace: (*v1.Namespace)(helpers.GetPointer("test")), - Port: (*v1.PortNumber)(helpers.GetPointer[int32](80)), - }, - }, - }, - } - } - - getModifiedRefs := func( - svcName v1.ObjectName, - mod func([]v1.HTTPBackendRef) []v1.HTTPBackendRef, - ) []v1.HTTPBackendRef { - return mod(getNormalRefs(svcName)) - } - - hr := &v1.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{Namespace: "test"}, - Spec: v1.HTTPRouteSpec{ - Rules: []v1.HTTPRouteRule{ - { - BackendRefs: getNormalRefs("svc1"), - }, - { - BackendRefs: getNormalRefs("svc1"), // duplicate - }, - { - BackendRefs: getModifiedRefs( - "invalid-kind", - func(refs []v1.HTTPBackendRef) []v1.HTTPBackendRef { - refs[0].Kind = (*v1.Kind)(helpers.GetPointer("Invalid")) - return refs - }, - ), - }, - { - BackendRefs: getModifiedRefs( - "nil-namespace", - func(refs []v1.HTTPBackendRef) []v1.HTTPBackendRef { - refs[0].Namespace = nil - return refs - }, - ), - }, - { - BackendRefs: getModifiedRefs( - "diff-namespace", - func(refs []v1.HTTPBackendRef) []v1.HTTPBackendRef { - refs[0].Namespace = (*v1.Namespace)( - helpers.GetPointer("not-test"), - ) - return refs - }, - ), - }, - { - BackendRefs: nil, - }, - { - BackendRefs: getNormalRefs("svc2"), - }, - { - BackendRefs: getModifiedRefs( - "multiple-refs", - func(refs []v1.HTTPBackendRef) []v1.HTTPBackendRef { - return append(refs, v1.HTTPBackendRef{ - BackendRef: v1.BackendRef{ - BackendObjectReference: v1.BackendObjectReference{ - Kind: (*v1.Kind)( - helpers.GetPointer("Service"), - ), - Name: "multiple-refs2", - Namespace: (*v1.Namespace)( - helpers.GetPointer("test"), - ), - Port: (*v1.PortNumber)( - helpers.GetPointer[int32](80), - ), - }, - }, - }) - }), - }, - }, - }, - } - - expNames := map[types.NamespacedName]struct{}{ - {Namespace: "test", Name: "svc1"}: {}, - {Namespace: "test", Name: "nil-namespace"}: {}, - {Namespace: "not-test", Name: "diff-namespace"}: {}, - {Namespace: "test", Name: "svc2"}: {}, - {Namespace: "test", Name: "multiple-refs"}: {}, - {Namespace: "test", Name: "multiple-refs2"}: {}, - } - - g := NewWithT(t) - names := getBackendServiceNamesFromRoute(hr) - g.Expect(names).To(Equal(expNames)) -} - -func TestCapturerImpl_DecrementRouteCount(t *testing.T) { - testcases := []struct { - msg string - startingRefCount int - expectedRefCount int - exists bool - }{ - { - msg: "service does not exist in map", - startingRefCount: 0, - expectedRefCount: 0, - exists: false, - }, - { - msg: "service has ref count of 1", - startingRefCount: 1, - expectedRefCount: 0, - exists: false, - }, - { - msg: "service has ref count of 2", - startingRefCount: 2, - expectedRefCount: 1, - exists: true, - }, - } - - capturer := NewCapturerImpl() - svc := types.NamespacedName{Namespace: "test", Name: "svc"} - - for _, tc := range testcases { - g := NewWithT(t) - if tc.startingRefCount > 0 { - capturer.serviceRefCount[svc] = tc.startingRefCount - } - - capturer.decrementRefCount(svc) - - count, exists := capturer.serviceRefCount[svc] - g.Expect(exists).To(Equal(tc.exists)) - g.Expect(count).To(Equal(tc.expectedRefCount)) - } -} diff --git a/internal/mode/static/state/resolver/resolver.go b/internal/mode/static/state/resolver/resolver.go index 2dab45a392..06a14f8675 100644 --- a/internal/mode/static/state/resolver/resolver.go +++ b/internal/mode/static/state/resolver/resolver.go @@ -2,11 +2,11 @@ package resolver import ( "context" - "errors" "fmt" v1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -15,10 +15,10 @@ import ( //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . ServiceResolver -// ServiceResolver resolves a Service and Service Port to a list of Endpoints. +// ServiceResolver resolves a Service's NamespacedName and ServicePort to a list of Endpoints. // Returns an error if the Service or Service Port cannot be resolved. type ServiceResolver interface { - Resolve(ctx context.Context, svc *v1.Service, svcPort int32) ([]Endpoint, error) + Resolve(ctx context.Context, svcNsName types.NamespacedName, svcPort v1.ServicePort) ([]Endpoint, error) } // Endpoint is the internal representation of a Kubernetes endpoint. @@ -39,11 +39,16 @@ func NewServiceResolverImpl(client client.Client) *ServiceResolverImpl { return &ServiceResolverImpl{client: client} } -// Resolve resolves a Service and Port to a list of Endpoints. -// Returns an error if the Service or Port cannot be resolved. -func (e *ServiceResolverImpl) Resolve(ctx context.Context, svc *v1.Service, port int32) ([]Endpoint, error) { - if svc == nil { - return nil, errors.New("cannot resolve a nil Service") +// Resolve resolves a Service's NamespacedName and ServicePort to a list of Endpoints. +// Returns an error if the Service or ServicePort cannot be resolved. +func (e *ServiceResolverImpl) Resolve( + ctx context.Context, + svcNsName types.NamespacedName, + svcPort v1.ServicePort, +) ([]Endpoint, error) { + if svcPort.Port == 0 || svcNsName.Name == "" || svcNsName.Namespace == "" { + panic(fmt.Errorf("expected the following fields to be non-empty: name: %s, ns: %s, port: %d", + svcNsName.Name, svcNsName.Namespace, svcPort.Port)) } // We list EndpointSlices using the Service Name Index Field we added as an index to the EndpointSlice cache. @@ -52,15 +57,15 @@ func (e *ServiceResolverImpl) Resolve(ctx context.Context, svc *v1.Service, port err := e.client.List( ctx, &endpointSliceList, - client.MatchingFields{index.KubernetesServiceNameIndexField: svc.Name}, - client.InNamespace(svc.Namespace), + client.MatchingFields{index.KubernetesServiceNameIndexField: svcNsName.Name}, + client.InNamespace(svcNsName.Namespace), ) if err != nil || len(endpointSliceList.Items) == 0 { - return nil, fmt.Errorf("no endpoints found for Service %s", client.ObjectKeyFromObject(svc)) + return nil, fmt.Errorf("no endpoints found for Service %s", svcNsName) } - return resolveEndpoints(svc, port, endpointSliceList, initEndpointSetWithCalculatedSize) + return resolveEndpoints(svcNsName, svcPort, endpointSliceList, initEndpointSetWithCalculatedSize) } type initEndpointSetFunc func([]discoveryV1.EndpointSlice) map[Endpoint]struct{} @@ -88,21 +93,15 @@ func calculateReadyEndpoints(endpointSlices []discoveryV1.EndpointSlice) int { } func resolveEndpoints( - svc *v1.Service, - port int32, + svcNsName types.NamespacedName, + svcPort v1.ServicePort, endpointSliceList discoveryV1.EndpointSliceList, initEndpointsSet initEndpointSetFunc, ) ([]Endpoint, error) { - svcPort, err := getServicePort(svc, port) - if err != nil { - return nil, err - } - filteredSlices := filterEndpointSliceList(endpointSliceList, svcPort) if len(filteredSlices) == 0 { - svcNsName := client.ObjectKeyFromObject(svc) - return nil, fmt.Errorf("no valid endpoints found for Service %s and port %+v", svcNsName, svcPort) + return nil, fmt.Errorf("no valid endpoints found for Service %s and port %d", svcNsName, svcPort.Port) } // Endpoints may be duplicated across multiple EndpointSlices. @@ -135,16 +134,6 @@ func resolveEndpoints( return endpoints, nil } -func getServicePort(svc *v1.Service, port int32) (v1.ServicePort, error) { - for _, p := range svc.Spec.Ports { - if p.Port == port { - return p, nil - } - } - - return v1.ServicePort{}, fmt.Errorf("no matching port for Service %s and port %d", svc.Name, port) -} - // getDefaultPort returns the default port for a ServicePort. // This default port is used when the EndpointPort has a nil port which indicates all ports are valid. // If the ServicePort has a non-zero integer TargetPort, the TargetPort integer value is returned. diff --git a/internal/mode/static/state/resolver/resolver_test.go b/internal/mode/static/state/resolver/resolver_test.go index 411ca190e9..69c43e0240 100644 --- a/internal/mode/static/state/resolver/resolver_test.go +++ b/internal/mode/static/state/resolver/resolver_test.go @@ -7,6 +7,7 @@ import ( . "github.com/onsi/gomega" v1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" @@ -105,37 +106,6 @@ func TestFilterEndpointSliceList(t *testing.T) { g.Expect(filteredSliceList).To(Equal(expFilteredList)) } -func TestGetServicePort(t *testing.T) { - svc := &v1.Service{ - Spec: v1.ServiceSpec{ - Ports: []v1.ServicePort{ - { - Port: 80, - }, - { - Port: 81, - }, - { - Port: 82, - }, - }, - }, - } - - g := NewWithT(t) - // ports exist - for _, p := range []int32{80, 81, 82} { - port, err := getServicePort(svc, p) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(port.Port).To(Equal(p)) - } - - // port doesn't exist - port, err := getServicePort(svc, 83) - g.Expect(err).Should(HaveOccurred()) - g.Expect(port.Port).To(Equal(int32(0))) -} - func TestGetDefaultPort(t *testing.T) { testcases := []struct { msg string @@ -546,14 +516,9 @@ func BenchmarkResolve(b *testing.B) { 1000, } - svc := &v1.Service{ - Spec: v1.ServiceSpec{ - Ports: []v1.ServicePort{ - { - Port: 80, - }, - }, - }, + svcNsName := types.NamespacedName{ + Namespace: "default", + Name: "default-name", } initEndpointSet := func([]discoveryV1.EndpointSlice) map[Endpoint]struct{} { @@ -564,17 +529,19 @@ func BenchmarkResolve(b *testing.B) { list := generateEndpointSliceList(count) b.Run(fmt.Sprintf("%d endpoints", count), func(b *testing.B) { - bench(b, svc, list, initEndpointSet, count) + bench(b, svcNsName, list, initEndpointSet, count) }) b.Run(fmt.Sprintf("%d endpoints with optimization", count), func(b *testing.B) { - bench(b, svc, list, initEndpointSetWithCalculatedSize, count) + bench(b, svcNsName, list, initEndpointSetWithCalculatedSize, count) }) } } -func bench(b *testing.B, svc *v1.Service, list discoveryV1.EndpointSliceList, initSet initEndpointSetFunc, n int) { +func bench(b *testing.B, svcNsName types.NamespacedName, + list discoveryV1.EndpointSliceList, initSet initEndpointSetFunc, n int, +) { for i := 0; i < b.N; i++ { - res, err := resolveEndpoints(svc, 80, list, initSet) + res, err := resolveEndpoints(svcNsName, v1.ServicePort{Port: 80}, list, initSet) if len(res) != n { b.Fatalf("expected %d endpoints, got %d", n, len(res)) } diff --git a/internal/mode/static/state/resolver/resolverfakes/fake_service_resolver.go b/internal/mode/static/state/resolver/resolverfakes/fake_service_resolver.go index c1116462fb..8fa8a9fcdc 100644 --- a/internal/mode/static/state/resolver/resolverfakes/fake_service_resolver.go +++ b/internal/mode/static/state/resolver/resolverfakes/fake_service_resolver.go @@ -7,15 +7,16 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" ) type FakeServiceResolver struct { - ResolveStub func(context.Context, *v1.Service, int32) ([]resolver.Endpoint, error) + ResolveStub func(context.Context, types.NamespacedName, v1.ServicePort) ([]resolver.Endpoint, error) resolveMutex sync.RWMutex resolveArgsForCall []struct { arg1 context.Context - arg2 *v1.Service - arg3 int32 + arg2 types.NamespacedName + arg3 v1.ServicePort } resolveReturns struct { result1 []resolver.Endpoint @@ -29,13 +30,13 @@ type FakeServiceResolver struct { invocationsMutex sync.RWMutex } -func (fake *FakeServiceResolver) Resolve(arg1 context.Context, arg2 *v1.Service, arg3 int32) ([]resolver.Endpoint, error) { +func (fake *FakeServiceResolver) Resolve(arg1 context.Context, arg2 types.NamespacedName, arg3 v1.ServicePort) ([]resolver.Endpoint, error) { fake.resolveMutex.Lock() ret, specificReturn := fake.resolveReturnsOnCall[len(fake.resolveArgsForCall)] fake.resolveArgsForCall = append(fake.resolveArgsForCall, struct { arg1 context.Context - arg2 *v1.Service - arg3 int32 + arg2 types.NamespacedName + arg3 v1.ServicePort }{arg1, arg2, arg3}) stub := fake.ResolveStub fakeReturns := fake.resolveReturns @@ -56,13 +57,13 @@ func (fake *FakeServiceResolver) ResolveCallCount() int { return len(fake.resolveArgsForCall) } -func (fake *FakeServiceResolver) ResolveCalls(stub func(context.Context, *v1.Service, int32) ([]resolver.Endpoint, error)) { +func (fake *FakeServiceResolver) ResolveCalls(stub func(context.Context, types.NamespacedName, v1.ServicePort) ([]resolver.Endpoint, error)) { fake.resolveMutex.Lock() defer fake.resolveMutex.Unlock() fake.ResolveStub = stub } -func (fake *FakeServiceResolver) ResolveArgsForCall(i int) (context.Context, *v1.Service, int32) { +func (fake *FakeServiceResolver) ResolveArgsForCall(i int) (context.Context, types.NamespacedName, v1.ServicePort) { fake.resolveMutex.RLock() defer fake.resolveMutex.RUnlock() argsForCall := fake.resolveArgsForCall[i] diff --git a/internal/mode/static/state/resolver/service_resolver_test.go b/internal/mode/static/state/resolver/service_resolver_test.go index 412eb72fc1..ef605eb5ba 100644 --- a/internal/mode/static/state/resolver/service_resolver_test.go +++ b/internal/mode/static/state/resolver/service_resolver_test.go @@ -9,6 +9,7 @@ import ( discoveryV1 "k8s.io/api/discovery/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -87,7 +88,6 @@ func createFakeK8sClient(initObjs ...client.Object) (client.Client, error) { var _ = Describe("ServiceResolver", func() { httpPortName := "http-svc-port" - httpsPortName := "https-svc-port" var ( addresses1 = []string{"9.0.0.1", "9.0.0.2"} @@ -96,33 +96,19 @@ var _ = Describe("ServiceResolver", func() { diffPortAddresses = []string{"11.0.0.1", "11.0.0.2"} dupeAddresses = []string{"9.0.0.1", "12.0.0.1", "9.0.0.2"} - svc = &v1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test", - Name: "svc", - }, - Spec: v1.ServiceSpec{ - Ports: []v1.ServicePort{ - { - Name: httpPortName, - Port: 80, - TargetPort: intstr.IntOrString{ - Type: intstr.Int, - IntVal: 8080, - }, - Protocol: v1.ProtocolTCP, - }, - { - Name: httpsPortName, - Port: 443, - TargetPort: intstr.IntOrString{ - Type: intstr.String, - StrVal: "target-port", - }, - Protocol: v1.ProtocolTCP, - }, - }, + svcPort = v1.ServicePort{ + Name: httpPortName, + Port: 80, + TargetPort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 8080, }, + Protocol: v1.ProtocolTCP, + } + + svcNsName = types.NamespacedName{ + Namespace: "test", + Name: "svc", } slice1 = createSlice( @@ -212,22 +198,17 @@ var _ = Describe("ServiceResolver", func() { }, } - endpoints, err := serviceResolver.Resolve(context.TODO(), svc, 80) + endpoints, err := serviceResolver.Resolve(context.TODO(), svcNsName, svcPort) Expect(err).ToNot(HaveOccurred()) Expect(endpoints).To(ConsistOf(expectedEndpoints)) }) - It("returns an error if port does not exist in service", func() { - endpoints, err := serviceResolver.Resolve(context.TODO(), svc, 8080) // service port does not exist - Expect(err).To(HaveOccurred()) - Expect(endpoints).To(BeNil()) - }) It("returns an error if there are no valid endpoint slices for the service and port", func() { // delete valid endpoint slices Expect(fakeK8sClient.Delete(context.TODO(), slice1)).To(Succeed()) Expect(fakeK8sClient.Delete(context.TODO(), slice2)).To(Succeed()) Expect(fakeK8sClient.Delete(context.TODO(), dupeEndpointSlice)).To(Succeed()) - endpoints, err := serviceResolver.Resolve(context.TODO(), svc, 80) + endpoints, err := serviceResolver.Resolve(context.TODO(), svcNsName, svcPort) Expect(err).To(HaveOccurred()) Expect(endpoints).To(BeNil()) }) @@ -236,14 +217,21 @@ var _ = Describe("ServiceResolver", func() { Expect(fakeK8sClient.Delete(context.TODO(), sliceIPV6)).To(Succeed()) Expect(fakeK8sClient.Delete(context.TODO(), sliceNoMatchingPortName)).To(Succeed()) - endpoints, err := serviceResolver.Resolve(context.TODO(), svc, 80) + endpoints, err := serviceResolver.Resolve(context.TODO(), svcNsName, svcPort) Expect(err).To(HaveOccurred()) Expect(endpoints).To(BeNil()) }) - It("returns an error if the service is nil", func() { - endpoints, err := serviceResolver.Resolve(context.TODO(), nil, 80) - Expect(err).To(HaveOccurred()) - Expect(endpoints).To(BeNil()) + It("panics if the service NamespacedName is empty", func() { + resolve := func() { + _, _ = serviceResolver.Resolve(context.TODO(), types.NamespacedName{}, svcPort) + } + Expect(resolve).Should(Panic()) + }) + It("panics if the ServicePort is empty", func() { + resolve := func() { + _, _ = serviceResolver.Resolve(context.TODO(), types.NamespacedName{}, v1.ServicePort{}) + } + Expect(resolve).Should(Panic()) }) }) }) diff --git a/internal/mode/static/state/store.go b/internal/mode/static/state/store.go index d288e44cb2..5e58311402 100644 --- a/internal/mode/static/state/store.go +++ b/internal/mode/static/state/store.go @@ -8,8 +8,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/relationship" ) // Updater updates the cluster state. @@ -71,17 +69,20 @@ func (list gvkList) contains(gvk schema.GroupVersionKind) bool { } type multiObjectStore struct { - stores map[schema.GroupVersionKind]objectStore - extractGVK extractGVKFunc + stores map[schema.GroupVersionKind]objectStore + extractGVK extractGVKFunc + persistedGVKs gvkList } func newMultiObjectStore( stores map[schema.GroupVersionKind]objectStore, extractGVK extractGVKFunc, + persistedGVKs gvkList, ) *multiObjectStore { return &multiObjectStore{ - stores: stores, - extractGVK: extractGVK, + stores: stores, + extractGVK: extractGVK, + persistedGVKs: persistedGVKs, } } @@ -108,11 +109,15 @@ func (m *multiObjectStore) delete(objType client.Object, nsname types.Namespaced m.mustFindStoreForObj(objType).delete(nsname) } +func (m *multiObjectStore) persists(objTypeGVK schema.GroupVersionKind) bool { + return m.persistedGVKs.contains(objTypeGVK) +} + type changeTrackingUpdaterObjectTypeCfg struct { // store holds the objects of the gvk. If the store is nil, the objects of the gvk are not persisted. store objectStore - // predicate determines if upsert or delete event should trigger a change. - // If predicate is nil, then no upsert or delete event for this object will trigger a change. + // predicate determines how an upsert or delete event should trigger a change. + // If predicate is nil, then all upsert or delete events for this object will trigger a change. predicate stateChangedPredicate gvk schema.GroupVersionKind } @@ -121,24 +126,18 @@ type changeTrackingUpdaterObjectTypeCfg struct { // // It only works with objects with the GVKs registered in changeTrackingUpdaterObjectTypeCfg. Otherwise, it panics. // -// A change is tracked when: -// - An object with a GVK with a non-nil store and the stateChangedPredicate for that object returns true. -// - An object is upserted or deleted, and it is related to another object, -// based on the decision by the relationship capturer. +// A change is tracked when an object with a GVK has its stateChangedPredicate return true or if its predicate is nil. type changeTrackingUpdater struct { store *multiObjectStore - capturer relationship.Capturer stateChangedPredicates map[schema.GroupVersionKind]stateChangedPredicate extractGVK extractGVKFunc supportedGVKs gvkList - persistedGVKs gvkList changed bool } func newChangeTrackingUpdater( - capturer relationship.Capturer, extractGVK extractGVKFunc, objectTypeCfgs []changeTrackingUpdaterObjectTypeCfg, ) *changeTrackingUpdater { @@ -164,11 +163,9 @@ func newChangeTrackingUpdater( } return &changeTrackingUpdater{ - store: newMultiObjectStore(stores, extractGVK), + store: newMultiObjectStore(stores, extractGVK, persistedGVKs), extractGVK: extractGVK, supportedGVKs: supportedGVKs, - persistedGVKs: persistedGVKs, - capturer: capturer, stateChangedPredicates: stateChangedPredicates, } } @@ -182,17 +179,17 @@ func (s *changeTrackingUpdater) assertSupportedGVK(gvk schema.GroupVersionKind) func (s *changeTrackingUpdater) upsert(obj client.Object) (changed bool) { objTypeGVK := s.extractGVK(obj) - if !s.persistedGVKs.contains(objTypeGVK) { - return false - } + var oldObj client.Object - oldObj := s.store.get(obj, client.ObjectKeyFromObject(obj)) + if s.store.persists(objTypeGVK) { + oldObj = s.store.get(obj, client.ObjectKeyFromObject(obj)) - s.store.upsert(obj) + s.store.upsert(obj) + } stateChanged, ok := s.stateChangedPredicates[objTypeGVK] if !ok { - return false + return true } return stateChanged.upsert(oldObj, obj) @@ -202,40 +199,27 @@ func (s *changeTrackingUpdater) Upsert(obj client.Object) { s.assertSupportedGVK(s.extractGVK(obj)) changingUpsert := s.upsert(obj) - relationshipExisted := s.capturer.Exists(obj, client.ObjectKeyFromObject(obj)) - s.capturer.Capture(obj) - - relationshipExists := s.capturer.Exists(obj, client.ObjectKeyFromObject(obj)) - - // FIXME(pleshakov): Check generation in all cases to minimize the number of Graph regeneration. - // s.changed can be true even if the generation of the object did not change, because - // capturer and triggerStateChange don't take the generation into account. - // See https://github.com/nginxinc/nginx-gateway-fabric/issues/825 - - s.changed = s.changed || changingUpsert || relationshipExisted || relationshipExists + s.changed = s.changed || changingUpsert } func (s *changeTrackingUpdater) delete(objType client.Object, nsname types.NamespacedName) (changed bool) { objTypeGVK := s.extractGVK(objType) - if !s.persistedGVKs.contains(objTypeGVK) { - return false - } + if s.store.persists(objTypeGVK) { + if s.store.get(objType, nsname) == nil { + return false + } - obj := s.store.get(objType, nsname) - if obj == nil { - return false + s.store.delete(objType, nsname) } - s.store.delete(objType, nsname) - stateChanged, ok := s.stateChangedPredicates[objTypeGVK] if !ok { - return false + return true } - return stateChanged.delete(obj) + return stateChanged.delete(objType, nsname) } func (s *changeTrackingUpdater) Delete(objType client.Object, nsname types.NamespacedName) { @@ -243,9 +227,7 @@ func (s *changeTrackingUpdater) Delete(objType client.Object, nsname types.Names changingDelete := s.delete(objType, nsname) - s.changed = s.changed || changingDelete || s.capturer.Exists(objType, nsname) - - s.capturer.Remove(objType, nsname) + s.changed = s.changed || changingDelete } // getAndResetChangedStatus returns true if the previous updates (Upserts/Deletes) require an update of