diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index b60811d94..b75e9f26c 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -17,6 +17,13 @@ rules: verbs: - list - watch +- apiGroups: + - "" + resources: + - events + verbs: + - create + - patch - apiGroups: - discovery.k8s.io resources: diff --git a/internal/manager/controllers.go b/internal/manager/controllers.go index 0a2414ffa..ddb6ad8b6 100644 --- a/internal/manager/controllers.go +++ b/internal/manager/controllers.go @@ -26,6 +26,7 @@ type controllerConfig struct { k8sPredicate predicate.Predicate fieldIndices index.FieldIndices newReconciler newReconcilerFunc + webhookValidator reconciler.ValidatorFunc } type controllerOption func(*controllerConfig) @@ -55,6 +56,12 @@ func withNewReconciler(newReconciler newReconcilerFunc) controllerOption { } } +func withWebhookValidator(validator reconciler.ValidatorFunc) controllerOption { + return func(cfg *controllerConfig) { + cfg.webhookValidator = validator + } +} + func defaultControllerConfig() controllerConfig { return controllerConfig{ newReconciler: reconciler.NewImplementation, @@ -65,7 +72,8 @@ func registerController( ctx context.Context, objectType client.Object, mgr manager.Manager, - eventCh chan interface{}, + eventCh chan<- interface{}, + recorder reconciler.EventRecorder, options ...controllerOption, ) error { cfg := defaultControllerConfig() @@ -92,6 +100,8 @@ func registerController( ObjectType: objectType, EventCh: eventCh, NamespacedNameFilter: cfg.namespacedNameFilter, + WebhookValidator: cfg.webhookValidator, + EventRecorder: recorder, } err := builder.Complete(cfg.newReconciler(recCfg)) diff --git a/internal/manager/controllers_test.go b/internal/manager/controllers_test.go index c4282d3ab..75a6cecf6 100644 --- a/internal/manager/controllers_test.go +++ b/internal/manager/controllers_test.go @@ -6,8 +6,12 @@ import ( "reflect" "testing" + . "github.com/onsi/gomega" + "github.com/onsi/gomega/gcustom" + "github.com/onsi/gomega/types" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/gateway-api/apis/v1beta1" @@ -17,6 +21,7 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/managerfakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/manager/predicate" "github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler" + "github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler/reconcilerfakes" ) func TestRegisterController(t *testing.T) { @@ -81,114 +86,69 @@ func TestRegisterController(t *testing.T) { namespacedNameFilter := filter.CreateFilterForGatewayClass("test") fieldIndexes := index.CreateEndpointSliceFieldIndices() - eventCh := make(chan interface{}) + webhookValidator := createValidator(func(_ *v1beta1.HTTPRoute) field.ErrorList { + return nil + }) + + eventRecorder := &reconcilerfakes.FakeEventRecorder{} + + eventCh := make(chan<- interface{}) + + beSameFunctionPointer := func(expected interface{}) types.GomegaMatcher { + return gcustom.MakeMatcher(func(f interface{}) (bool, error) { + // comparing functions is not allowed in Go, so we're comparing the pointers + return reflect.ValueOf(expected).Pointer() == reflect.ValueOf(f).Pointer(), nil + }) + } for _, test := range tests { - newReconciler := func(c reconciler.Config) *reconciler.Implementation { - if c.Getter != test.fakes.mgr.GetClient() { - t.Errorf( - "regiterController() created a reconciler config with Getter %p but expected %p for case of %q", - c.Getter, - test.fakes.mgr.GetClient(), - test.msg, - ) - } - if c.ObjectType != objectType { - t.Errorf( - "registerController() created a reconciler config with ObjectType %T but expected %T for case of %q", - c.ObjectType, - objectType, - test.msg, - ) + t.Run(test.msg, func(t *testing.T) { + g := NewGomegaWithT(t) + + newReconciler := func(c reconciler.Config) *reconciler.Implementation { + g.Expect(c.Getter).To(BeIdenticalTo(test.fakes.mgr.GetClient())) + g.Expect(c.ObjectType).To(BeIdenticalTo(objectType)) + g.Expect(c.EventCh).To(BeIdenticalTo(eventCh)) + g.Expect(c.EventRecorder).To(BeIdenticalTo(eventRecorder)) + g.Expect(c.WebhookValidator).Should(beSameFunctionPointer(webhookValidator)) + g.Expect(c.NamespacedNameFilter).Should(beSameFunctionPointer(namespacedNameFilter)) + + return reconciler.NewImplementation(c) } - if c.EventCh != eventCh { - t.Errorf( - "registerController() created a reconciler config with EventCh %v but expected %v for case of %q", - c.EventCh, - eventCh, - test.msg, - ) - } - // comparing functions is not allowed in Go, so we're comparing the pointers - // nolint: lll - if reflect.ValueOf(c.NamespacedNameFilter).Pointer() != reflect.ValueOf(namespacedNameFilter).Pointer() { - t.Errorf( - "registerController() created a reconciler config with NamespacedNameFilter %p but expected %p for case of %q", - c.NamespacedNameFilter, - namespacedNameFilter, - test.msg, - ) + + err := registerController( + context.Background(), + objectType, + test.fakes.mgr, + eventCh, + eventRecorder, + withNamespacedNameFilter(namespacedNameFilter), + withK8sPredicate(predicate.ServicePortsChangedPredicate{}), + withFieldIndices(fieldIndexes), + withNewReconciler(newReconciler), + withWebhookValidator(webhookValidator), + ) + + if test.expectedErr == nil { + g.Expect(err).To(BeNil()) + } else { + g.Expect(err).To(MatchError(test.expectedErr)) } - return reconciler.NewImplementation(c) - } + indexCallCount := test.fakes.indexer.IndexFieldCallCount() - err := registerController( - context.Background(), - objectType, - test.fakes.mgr, - eventCh, - withNamespacedNameFilter(namespacedNameFilter), - withK8sPredicate(predicate.ServicePortsChangedPredicate{}), - withFieldIndices(fieldIndexes), - withNewReconciler(newReconciler), - ) - - if !errors.Is(err, test.expectedErr) { - t.Errorf( - "registerController() returned %q but expected %q for case of %q", - err, - test.expectedErr, - test.msg, - ) - } + g.Expect(indexCallCount).To(Equal(1)) - indexCallCount := test.fakes.indexer.IndexFieldCallCount() - if indexCallCount != 1 { - t.Errorf( - "registerController() called indexer.IndexField() %d times but expected 1 for case of %q", - indexCallCount, - test.msg, - ) - } else { _, objType, field, indexFunc := test.fakes.indexer.IndexFieldArgsForCall(0) - if objType != objectType { - t.Errorf( - "registerController() called indexer.IndexField() with object type %T but expected %T for case %q", - objType, - objectType, - test.msg, - ) - } - if field != index.KubernetesServiceNameIndexField { - t.Errorf("registerController() called indexer.IndexField() with field %q but expected %q for case %q", - field, - index.KubernetesServiceNameIndexField, - test.msg, - ) - } + g.Expect(objType).To(BeIdenticalTo(objectType)) + g.Expect(field).To(BeIdenticalTo(index.KubernetesServiceNameIndexField)) expectedIndexFunc := fieldIndexes[index.KubernetesServiceNameIndexField] - // comparing functions is not allowed in Go, so we're comparing the pointers - // nolint:lll - if reflect.ValueOf(indexFunc).Pointer() != reflect.ValueOf(expectedIndexFunc).Pointer() { - t.Errorf("registerController() called indexer.IndexField() with indexFunc %p but expected %p for case %q", - indexFunc, - expectedIndexFunc, - test.msg, - ) - } - } + g.Expect(indexFunc).To(beSameFunctionPointer(expectedIndexFunc)) - addCallCount := test.fakes.mgr.AddCallCount() - if addCallCount != test.expectedMgrAddCallCount { - t.Errorf( - "registerController() called mgr.Add() %d times but expected %d times for case %q", - addCallCount, - test.expectedMgrAddCallCount, - test.msg, - ) - } + addCallCount := test.fakes.mgr.AddCallCount() + g.Expect(addCallCount).To(Equal(test.expectedMgrAddCallCount)) + }) } } diff --git a/internal/manager/manager.go b/internal/manager/manager.go index b7cb1cffd..b7f196380 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -14,6 +14,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" k8spredicate "sigs.k8s.io/controller-runtime/pkg/predicate" gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" + "sigs.k8s.io/gateway-api/apis/v1beta1/validation" "github.com/nginxinc/nginx-kubernetes-gateway/internal/config" "github.com/nginxinc/nginx-kubernetes-gateway/internal/events" @@ -71,13 +72,21 @@ func Start(cfg config.Config) error { objectType: &gatewayv1beta1.GatewayClass{}, options: []controllerOption{ withNamespacedNameFilter(filter.CreateFilterForGatewayClass(cfg.GatewayClassName)), + // as of v0.6.0, the Gateway API Webhook doesn't include a validation function + // for the GatewayClass resource }, }, { objectType: &gatewayv1beta1.Gateway{}, + options: []controllerOption{ + withWebhookValidator(createValidator(validation.ValidateGateway)), + }, }, { objectType: &gatewayv1beta1.HTTPRoute{}, + options: []controllerOption{ + withWebhookValidator(createValidator(validation.ValidateHTTPRoute)), + }, }, { objectType: &apiv1.Service{}, @@ -99,8 +108,11 @@ func Start(cfg config.Config) error { ctx := ctlr.SetupSignalHandler() + recorderName := fmt.Sprintf("nginx-kubernetes-gateway-%s", cfg.GatewayClassName) + recorder := mgr.GetEventRecorderFor(recorderName) + for _, regCfg := range controllerRegCfgs { - err := registerController(ctx, regCfg.objectType, mgr, eventCh, regCfg.options...) + err := registerController(ctx, regCfg.objectType, mgr, eventCh, recorder, regCfg.options...) if err != nil { return fmt.Errorf("cannot register controller for %T: %w", regCfg.objectType, err) } diff --git a/internal/manager/validators.go b/internal/manager/validators.go new file mode 100644 index 000000000..425bcefd2 --- /dev/null +++ b/internal/manager/validators.go @@ -0,0 +1,27 @@ +package manager + +import ( + "errors" + "fmt" + + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler" +) + +// createValidator creates a reconciler.ValidatorFunc from a function that validates a resource of type R. +func createValidator[R client.Object](validate func(R) field.ErrorList) reconciler.ValidatorFunc { + return func(obj client.Object) error { + if obj == nil { + panic(errors.New("obj is nil")) + } + + r, ok := obj.(R) + if !ok { + panic(fmt.Errorf("obj type mismatch: got %T, expected %T", obj, r)) + } + + return validate(r).ToAggregate() + } +} diff --git a/internal/manager/validators_test.go b/internal/manager/validators_test.go new file mode 100644 index 000000000..f6423244f --- /dev/null +++ b/internal/manager/validators_test.go @@ -0,0 +1,77 @@ +package manager + +import ( + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/gateway-api/apis/v1beta1" +) + +func TestCreateTypedValidator(t *testing.T) { + tests := []struct { + name string + obj client.Object + errorList field.ErrorList + expectPanic bool + expectErr bool + }{ + { + obj: &v1beta1.HTTPRoute{}, + errorList: field.ErrorList{}, + expectPanic: false, + expectErr: false, + name: "no errors", + }, + { + obj: &v1beta1.HTTPRoute{}, + errorList: []*field.Error{{Detail: "test"}}, + expectPanic: false, + expectErr: true, + name: "one error", + }, + { + obj: nil, + errorList: field.ErrorList{}, + expectPanic: true, + expectErr: false, + name: "nil object", + }, + { + obj: &v1beta1.Gateway{}, + errorList: field.ErrorList{}, + expectPanic: true, + expectErr: false, + name: "wrong object type", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + + v := createValidator(createValidateHTTPRouteThatReturns(test.errorList)) + + if test.expectPanic { + g.Expect(func() { _ = v(test.obj) }).To(Panic()) + return + } + + result := v(test.obj) + + if test.expectErr { + g.Expect(result).ToNot(BeNil()) + return + } + + g.Expect(result).To(BeNil()) + }) + } +} + +func createValidateHTTPRouteThatReturns(errorList field.ErrorList) func(*v1beta1.HTTPRoute) field.ErrorList { + return func(*v1beta1.HTTPRoute) field.ErrorList { + return errorList + } +} diff --git a/internal/reconciler/implementation.go b/internal/reconciler/implementation.go index 49783386d..38a568e8f 100644 --- a/internal/reconciler/implementation.go +++ b/internal/reconciler/implementation.go @@ -5,6 +5,7 @@ import ( "fmt" "reflect" + apiv1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -18,6 +19,9 @@ import ( // If the function returns false, the reconciler will log the returned string. type NamespacedNameFilterFunc func(nsname types.NamespacedName) (bool, string) +// ValidatorFunc validates a Kubernetes resource. +type ValidatorFunc func(object client.Object) error + // Config contains the configuration for the Implementation. type Config struct { // Getter gets a resource from the k8s API. @@ -28,6 +32,10 @@ type Config struct { EventCh chan<- interface{} // NamespacedNameFilter filters resources the controller will process. Can be nil. NamespacedNameFilter NamespacedNameFilterFunc + // WebhookValidator validates a resource using the same rules as in the Gateway API Webhook. Can be nil. + WebhookValidator ValidatorFunc + // EventRecorder records event about resources. + EventRecorder EventRecorder } // Implementation is a reconciler for Kubernetes resources. @@ -58,6 +66,12 @@ func newObject(objectType client.Object) client.Object { return reflect.New(t).Interface().(client.Object) } +const ( + webhookValidationErrorLogMsg = "Rejected the resource because the Gateway API webhook failed to reject it with " + + "a validation error; make sure the webhook is installed and running correctly; " + + "NKG will delete any existing NGINX configuration that corresponds to the resource" +) + // Reconcile implements the reconcile.Reconciler Reconcile method. func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { logger := log.FromContext(ctx) @@ -66,7 +80,13 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( logger.Info("Reconciling the resource") - found := true + if r.cfg.NamespacedNameFilter != nil { + if allow, msg := r.cfg.NamespacedNameFilter(req.NamespacedName); !allow { + logger.Info(msg) + return reconcile.Result{}, nil + } + } + obj := newObject(r.cfg.ObjectType) err := r.cfg.Getter.Get(ctx, req.NamespacedName, obj) if err != nil { @@ -74,38 +94,46 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( logger.Error(err, "Failed to get the resource") return reconcile.Result{}, err } - found = false + // The resource does not exist (was deleted). + obj = nil } - if r.cfg.NamespacedNameFilter != nil { - if allow, msg := r.cfg.NamespacedNameFilter(req.NamespacedName); !allow { - logger.Info(msg) - return reconcile.Result{}, nil - } + var validationError error + if obj != nil && r.cfg.WebhookValidator != nil { + validationError = r.cfg.WebhookValidator(obj) + } + + if validationError != nil { + logger.Error(validationError, webhookValidationErrorLogMsg) + r.cfg.EventRecorder.Eventf(obj, apiv1.EventTypeWarning, "Rejected", + webhookValidationErrorLogMsg+"; validation error: %v", validationError) } var e interface{} - var operation string + var op string - if !found { + if obj == nil || validationError != nil { + // In case of a validation error, we handle the resource as if it was deleted. e = &events.DeleteEvent{ Type: r.cfg.ObjectType, NamespacedName: req.NamespacedName, } - operation = "deleted" + op = "Deleted" } else { e = &events.UpsertEvent{ Resource: obj, } - operation = "upserted" + op = "Upserted" } select { case <-ctx.Done(): - logger.Info(fmt.Sprintf("The resource was not %s because the context was canceled", operation)) + logger.Info("Did not process the resource because the context was canceled") + return reconcile.Result{}, nil case r.cfg.EventCh <- e: - logger.Info(fmt.Sprintf("The resource was %s", operation)) } + logger.Info(fmt.Sprintf("%s the resource", op)) + return reconcile.Result{}, nil } diff --git a/internal/reconciler/implementation_test.go b/internal/reconciler/implementation_test.go index a8e6d6940..952c07e70 100644 --- a/internal/reconciler/implementation_test.go +++ b/internal/reconciler/implementation_test.go @@ -56,6 +56,13 @@ var _ = Describe("Reconciler", func() { Name: hr2NsName.Name, }, } + + hr2IsInvalidValidator = func(obj client.Object) error { + if client.ObjectKeyFromObject(obj) == hr2NsName { + return errors.New("test") + } + return nil + } ) getReturnsHRForHR := func(hr *v1beta1.HTTPRoute) getFunc { @@ -113,6 +120,27 @@ var _ = Describe("Reconciler", func() { }) Describe("Normal cases", func() { + testUpsert := func(hr *v1beta1.HTTPRoute) { + fakeGetter.GetCalls(getReturnsHRForHR(hr)) + + resultCh := startReconciling(client.ObjectKeyFromObject(hr)) + + Eventually(eventCh).Should(Receive(Equal(&events.UpsertEvent{Resource: hr}))) + Eventually(resultCh).Should(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}}))) + } + + testDelete := func(hr *v1beta1.HTTPRoute) { + fakeGetter.GetCalls(getReturnsNotFoundErrorForHR(hr)) + + resultCh := startReconciling(client.ObjectKeyFromObject(hr)) + + Eventually(eventCh).Should(Receive(Equal(&events.DeleteEvent{ + NamespacedName: client.ObjectKeyFromObject(hr), + Type: &v1beta1.HTTPRoute{}, + }))) + Eventually(resultCh).Should(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}}))) + } + When("Reconciler doesn't have a filter", func() { BeforeEach(func() { rec = reconciler.NewImplementation(reconciler.Config{ @@ -123,24 +151,11 @@ var _ = Describe("Reconciler", func() { }) It("should upsert HTTPRoute", func() { - fakeGetter.GetCalls(getReturnsHRForHR(hr1)) - - resultCh := startReconciling(hr1NsName) - - Eventually(eventCh).Should(Receive(Equal(&events.UpsertEvent{Resource: hr1}))) - Eventually(resultCh).Should(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}}))) + testUpsert(hr1) }) It("should delete HTTPRoute", func() { - fakeGetter.GetCalls(getReturnsNotFoundErrorForHR(hr1)) - - resultCh := startReconciling(hr1NsName) - - Eventually(eventCh).Should(Receive(Equal(&events.DeleteEvent{ - NamespacedName: hr1NsName, - Type: &v1beta1.HTTPRoute{}, - }))) - Eventually(resultCh).Should(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}}))) + testDelete(hr1) }) }) @@ -163,24 +178,11 @@ var _ = Describe("Reconciler", func() { When("HTTPRoute is not ignored", func() { It("should upsert HTTPRoute", func() { - fakeGetter.GetCalls(getReturnsHRForHR(hr1)) - - resultCh := startReconciling(hr1NsName) - - Eventually(eventCh).Should(Receive(Equal(&events.UpsertEvent{Resource: hr1}))) - Eventually(resultCh).Should(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}}))) + testUpsert(hr1) }) It("should delete HTTPRoute", func() { - fakeGetter.GetCalls(getReturnsNotFoundErrorForHR(hr1)) - - resultCh := startReconciling(hr1NsName) - - Eventually(eventCh).Should(Receive(Equal(&events.DeleteEvent{ - NamespacedName: hr1NsName, - Type: &v1beta1.HTTPRoute{}, - }))) - Eventually(resultCh).Should(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}}))) + testDelete(hr1) }) }) @@ -204,14 +206,63 @@ var _ = Describe("Reconciler", func() { }) }) }) + + When("Reconciler includes a Webhook Validator", func() { + var fakeRecorder *reconcilerfakes.FakeEventRecorder + + BeforeEach(func() { + fakeRecorder = &reconcilerfakes.FakeEventRecorder{} + + rec = reconciler.NewImplementation(reconciler.Config{ + Getter: fakeGetter, + ObjectType: &v1beta1.HTTPRoute{}, + EventCh: eventCh, + WebhookValidator: hr2IsInvalidValidator, + EventRecorder: fakeRecorder, + }) + }) + + It("should upsert valid HTTPRoute", func() { + testUpsert(hr1) + Expect(fakeRecorder.EventfCallCount()).To(Equal(0)) + }) + + It("should reject invalid HTTPRoute", func() { + fakeGetter.GetCalls(getReturnsHRForHR(hr2)) + + resultCh := startReconciling(client.ObjectKeyFromObject(hr2)) + + Eventually(eventCh).Should(Receive(Equal(&events.DeleteEvent{ + NamespacedName: client.ObjectKeyFromObject(hr2), + Type: &v1beta1.HTTPRoute{}, + }))) + Eventually(resultCh).Should(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}}))) + + Expect(fakeRecorder.EventfCallCount()).To(Equal(1)) + obj, _, _, _, _ := fakeRecorder.EventfArgsForCall(0) + Expect(obj).To(Equal(hr2)) + }) + + It("should delete HTTPRoutes", func() { + testDelete(hr1) + testDelete(hr2) + Expect(fakeRecorder.EventfCallCount()).To(Equal(0)) + }) + }) }) Describe("Edge cases", func() { + var fakeRecorder *reconcilerfakes.FakeEventRecorder + BeforeEach(func() { + fakeRecorder = &reconcilerfakes.FakeEventRecorder{} + rec = reconciler.NewImplementation(reconciler.Config{ - Getter: fakeGetter, - ObjectType: &v1beta1.HTTPRoute{}, - EventCh: eventCh, + Getter: fakeGetter, + ObjectType: &v1beta1.HTTPRoute{}, + EventCh: eventCh, + WebhookValidator: hr2IsInvalidValidator, + EventRecorder: fakeRecorder, }) }) @@ -226,7 +277,7 @@ var _ = Describe("Reconciler", func() { }) DescribeTable("Reconciler should not block when ctx is done", - func(get getFunc, nsname types.NamespacedName) { + func(get getFunc, invalidResourceEventCount int, nsname types.NamespacedName) { fakeGetter.GetCalls(get) ctx, cancel := context.WithCancel(context.Background()) @@ -236,9 +287,11 @@ var _ = Describe("Reconciler", func() { Consistently(eventCh).ShouldNot(Receive()) Expect(resultCh).To(Receive(Equal(result{err: nil, reconcileResult: reconcile.Result{}}))) + Expect(fakeRecorder.EventfCallCount()).To(Equal(invalidResourceEventCount)) }, - Entry("Upserting HTTPRoute", getReturnsHRForHR(hr1), hr1NsName), - Entry("Deleting HTTPRoute", getReturnsNotFoundErrorForHR(hr1), hr1NsName), + Entry("Upserting valid HTTPRoute", getReturnsHRForHR(hr1), 0, hr1NsName), + Entry("Deleting valid HTTPRoute", getReturnsNotFoundErrorForHR(hr1), 0, hr1NsName), + Entry("Upserting invalid HTTPRoute", getReturnsHRForHR(hr2), 1, hr2NsName), ) }) }) diff --git a/internal/reconciler/reconcilerfakes/fake_event_recorder.go b/internal/reconciler/reconcilerfakes/fake_event_recorder.go new file mode 100644 index 000000000..33f459eb8 --- /dev/null +++ b/internal/reconciler/reconcilerfakes/fake_event_recorder.go @@ -0,0 +1,85 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package reconcilerfakes + +import ( + "sync" + + "github.com/nginxinc/nginx-kubernetes-gateway/internal/reconciler" + "k8s.io/apimachinery/pkg/runtime" +) + +type FakeEventRecorder struct { + EventfStub func(runtime.Object, string, string, string, ...interface{}) + eventfMutex sync.RWMutex + eventfArgsForCall []struct { + arg1 runtime.Object + arg2 string + arg3 string + arg4 string + arg5 []interface{} + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeEventRecorder) Eventf(arg1 runtime.Object, arg2 string, arg3 string, arg4 string, arg5 ...interface{}) { + fake.eventfMutex.Lock() + fake.eventfArgsForCall = append(fake.eventfArgsForCall, struct { + arg1 runtime.Object + arg2 string + arg3 string + arg4 string + arg5 []interface{} + }{arg1, arg2, arg3, arg4, arg5}) + stub := fake.EventfStub + fake.recordInvocation("Eventf", []interface{}{arg1, arg2, arg3, arg4, arg5}) + fake.eventfMutex.Unlock() + if stub != nil { + fake.EventfStub(arg1, arg2, arg3, arg4, arg5...) + } +} + +func (fake *FakeEventRecorder) EventfCallCount() int { + fake.eventfMutex.RLock() + defer fake.eventfMutex.RUnlock() + return len(fake.eventfArgsForCall) +} + +func (fake *FakeEventRecorder) EventfCalls(stub func(runtime.Object, string, string, string, ...interface{})) { + fake.eventfMutex.Lock() + defer fake.eventfMutex.Unlock() + fake.EventfStub = stub +} + +func (fake *FakeEventRecorder) EventfArgsForCall(i int) (runtime.Object, string, string, string, []interface{}) { + fake.eventfMutex.RLock() + defer fake.eventfMutex.RUnlock() + argsForCall := fake.eventfArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4, argsForCall.arg5 +} + +func (fake *FakeEventRecorder) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.eventfMutex.RLock() + defer fake.eventfMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeEventRecorder) 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 _ reconciler.EventRecorder = new(FakeEventRecorder) diff --git a/internal/reconciler/recorder.go b/internal/reconciler/recorder.go new file mode 100644 index 000000000..954124a1e --- /dev/null +++ b/internal/reconciler/recorder.go @@ -0,0 +1,12 @@ +package reconciler + +import "k8s.io/apimachinery/pkg/runtime" + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . EventRecorder + +// EventRecorder records events for a resource. +// It allows us to mock the record.EventRecorder.Eventf method. +type EventRecorder interface { + // Eventf is a method of k8s.io/client-go/tools/record.EventRecorder + Eventf(object runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) +} diff --git a/internal/state/change_processor.go b/internal/state/change_processor.go index fdb4d3ec0..4f21af566 100644 --- a/internal/state/change_processor.go +++ b/internal/state/change_processor.go @@ -113,14 +113,16 @@ func (c *ChangeProcessorImpl) CaptureDeleteChange(resourceType client.Object, ns if nsname.Name != c.cfg.GatewayClassName { panic(fmt.Errorf("gatewayclass resource must be %s, got %s", c.cfg.GatewayClassName, nsname.Name)) } + if c.store.gc != nil { + c.store.changed = true + } c.store.gc = nil - c.store.changed = true case *v1beta1.Gateway: + _, c.store.changed = c.store.gateways[nsname] delete(c.store.gateways, nsname) - c.store.changed = true case *v1beta1.HTTPRoute: + _, c.store.changed = c.store.httpRoutes[nsname] delete(c.store.httpRoutes, nsname) - c.store.changed = true case *v1.Service: delete(c.store.services, nsname) case *discoveryV1.EndpointSlice: diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index d86867051..d4910098a 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -1606,7 +1606,7 @@ var _ = Describe("ChangeProcessor", func() { hr1Updated = hr1.DeepCopy() hr1Updated.Generation++ - hr2NsName := types.NamespacedName{Namespace: "test", Name: "hr-2"} + hr2NsName = types.NamespacedName{Namespace: "test", Name: "hr-2"} hr2 = hr1.DeepCopy() hr2.Name = hr2NsName.Name @@ -1699,6 +1699,17 @@ var _ = Describe("ChangeProcessor", func() { Expect(changed).To(BeTrue()) }) }) + Describe("Deleting non-existing Gateway API resource", func() { + It("should not report changed after deleting non-existing", func() { + processor.CaptureDeleteChange(&v1beta1.GatewayClass{}, gcNsName) + processor.CaptureDeleteChange(&v1beta1.Gateway{}, gwNsName) + processor.CaptureDeleteChange(&v1beta1.HTTPRoute{}, hrNsName) + processor.CaptureDeleteChange(&v1beta1.HTTPRoute{}, hr2NsName) + + changed, _, _ := processor.Process(context.TODO()) + Expect(changed).To(BeFalse()) + }) + }) Describe("Multiple Kubernetes API resource changes", Ordered, func() { It("should report changed after multiple Upserts of related resources", func() { fakeRelationshipCapturer.ExistsReturns(true)