From f9ec7b6f221d74caa0c06c75f891fa6c0cc70c49 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 12 Jan 2023 15:32:34 -0600 Subject: [PATCH 1/9] Run webhook validation In case the webhook is not installed or not running validation properly, we still want NKG to ensure that the webhook validation is always performed and NKG rejects any invalid resource. Fixes https://github.com/nginxinc/nginx-kubernetes-gateway/issues/362 --- deploy/manifests/nginx-gateway.yaml | 7 + internal/manager/controllers.go | 12 +- internal/manager/controllers_test.go | 154 +++++++----------- internal/manager/manager.go | 13 +- internal/manager/validators.go | 27 +++ internal/manager/validators_test.go | 77 +++++++++ internal/reconciler/implementation.go | 80 +++++++-- internal/reconciler/implementation_test.go | 103 ++++++++---- .../reconcilerfakes/fake_event_recorder.go | 85 ++++++++++ internal/reconciler/recorder.go | 12 ++ internal/state/change_processor.go | 8 +- internal/state/change_processor_test.go | 13 +- 12 files changed, 440 insertions(+), 151 deletions(-) create mode 100644 internal/manager/validators.go create mode 100644 internal/manager/validators_test.go create mode 100644 internal/reconciler/reconcilerfakes/fake_event_recorder.go create mode 100644 internal/reconciler/recorder.go diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index b60811d94c..b75e9f26c7 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 0a2414ffa0..ddb6ad8b65 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 c4282d3ab6..75a6cecf67 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 b7cb1cffd6..30e61a1138 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,20 @@ func Start(cfg config.Config) error { objectType: &gatewayv1beta1.GatewayClass{}, options: []controllerOption{ withNamespacedNameFilter(filter.CreateFilterForGatewayClass(cfg.GatewayClassName)), + // not adding Webhook validator because there is no validation.ValidateGatewayClass }, }, { objectType: &gatewayv1beta1.Gateway{}, + options: []controllerOption{ + withWebhookValidator(createValidator(validation.ValidateGateway)), + }, }, { objectType: &gatewayv1beta1.HTTPRoute{}, + options: []controllerOption{ + withWebhookValidator(createValidator(validation.ValidateHTTPRoute)), + }, }, { objectType: &apiv1.Service{}, @@ -99,8 +107,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 0000000000..425bcefd2a --- /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 0000000000..3ee5ec454c --- /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 { + obj client.Object + errorList field.ErrorList + expectPanic bool + expectErr bool + name string + }{ + { + 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).To(Not(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 49783386dd..b010b8e14b 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,11 @@ 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" +) + // 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 +79,6 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( logger.Info("Reconciling the resource") - found := true obj := newObject(r.cfg.ObjectType) err := r.cfg.Getter.Get(ctx, req.NamespacedName, obj) if err != nil { @@ -74,7 +86,8 @@ 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 + obj = nil } if r.cfg.NamespacedNameFilter != nil { @@ -84,28 +97,59 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( } } - var e interface{} - var operation string - - if !found { - e = &events.DeleteEvent{ - Type: r.cfg.ObjectType, - NamespacedName: req.NamespacedName, - } - operation = "deleted" - } else { - e = &events.UpsertEvent{ - Resource: obj, - } - operation = "upserted" + var validationError error + if obj != nil && r.cfg.WebhookValidator != nil { + validationError = r.cfg.WebhookValidator(obj) } + e := generateEvent(r.cfg.ObjectType, req.NamespacedName, obj, validationError) + 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)) } + if validationError != nil { + logger.Error(validationError, webhookValidationErrorLogMsg) + r.cfg.EventRecorder.Eventf(obj, apiv1.EventTypeWarning, "Rejected", + webhookValidationErrorLogMsg+"; validation error: %v", validationError) + return reconcile.Result{}, nil + } + + op := "Upserted" + if _, deleted := e.(*events.DeleteEvent); deleted { + op = "Deleted" + } + + logger.Info(fmt.Sprintf("%s the resource", op)) + return reconcile.Result{}, nil } + +func generateEvent( + objType client.Object, + nsName types.NamespacedName, + obj client.Object, + validationErr error, +) interface{} { + if obj == nil { + return &events.DeleteEvent{ + Type: objType, + NamespacedName: nsName, + } + } + + if validationErr != nil { + // If the resource is invalid, we will delete it from the NGINX configuration. + return &events.DeleteEvent{ + Type: objType, + NamespacedName: nsName, + } + } + + return &events.UpsertEvent{ + Resource: obj, + } +} diff --git a/internal/reconciler/implementation_test.go b/internal/reconciler/implementation_test.go index a8e6d6940a..61dfc9592a 100644 --- a/internal/reconciler/implementation_test.go +++ b/internal/reconciler/implementation_test.go @@ -113,6 +113,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 +144,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 +171,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,6 +199,54 @@ 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: func(obj client.Object) error { + if client.ObjectKeyFromObject(obj) == hr2NsName { + return errors.New("test") + } + return nil + }, + 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() { diff --git a/internal/reconciler/reconcilerfakes/fake_event_recorder.go b/internal/reconciler/reconcilerfakes/fake_event_recorder.go new file mode 100644 index 0000000000..33f459eb85 --- /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 0000000000..954124a1eb --- /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 fdb4d3ec03..4f21af5661 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 d868670512..d4910098af 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) From fd79ca23a92ba32687a06650e17a6bc0ee08c6f2 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 19 Jan 2023 13:34:30 -0600 Subject: [PATCH 2/9] Fix alignment --- internal/manager/validators_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/manager/validators_test.go b/internal/manager/validators_test.go index 3ee5ec454c..8d96467fa8 100644 --- a/internal/manager/validators_test.go +++ b/internal/manager/validators_test.go @@ -11,11 +11,11 @@ import ( func TestCreateTypedValidator(t *testing.T) { tests := []struct { - obj client.Object errorList field.ErrorList + obj client.Object + name string expectPanic bool expectErr bool - name string }{ { obj: &v1beta1.HTTPRoute{}, From 95caaa9a2fa91285d764f40f8f8afe66a0ba8d12 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 19 Jan 2023 13:49:47 -0600 Subject: [PATCH 3/9] Fix alignment 2 --- internal/manager/validators_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/manager/validators_test.go b/internal/manager/validators_test.go index 8d96467fa8..6d2f6ed325 100644 --- a/internal/manager/validators_test.go +++ b/internal/manager/validators_test.go @@ -11,9 +11,9 @@ import ( func TestCreateTypedValidator(t *testing.T) { tests := []struct { - errorList field.ErrorList - obj client.Object name string + obj client.Object + errorList field.ErrorList expectPanic bool expectErr bool }{ From f130a9df26033385256a4270977e28cac8a50b65 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Fri, 20 Jan 2023 10:23:35 -0600 Subject: [PATCH 4/9] Improve recorder name --- internal/manager/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 30e61a1138..425293b888 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -107,7 +107,7 @@ func Start(cfg config.Config) error { ctx := ctlr.SetupSignalHandler() - recorderName := fmt.Sprintf("nginx-kubernetes-gateway-%s", cfg.GatewayClassName) + recorderName := fmt.Sprintf("nginx-kubernetes-gateway-with-gatewayclass-%s", cfg.GatewayClassName) recorder := mgr.GetEventRecorderFor(recorderName) for _, regCfg := range controllerRegCfgs { From 54b5de523d7cff0e6734007d388df166c2160936 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Fri, 20 Jan 2023 10:29:03 -0600 Subject: [PATCH 5/9] Reword a comment about not validating GatewayClass --- internal/manager/manager.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 425293b888..13b964bc1d 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -72,7 +72,8 @@ func Start(cfg config.Config) error { objectType: &gatewayv1beta1.GatewayClass{}, options: []controllerOption{ withNamespacedNameFilter(filter.CreateFilterForGatewayClass(cfg.GatewayClassName)), - // not adding Webhook validator because there is no validation.ValidateGatewayClass + // as of v0.6.0, the Gateway API Webhook doesn't include a validation function + // for the GatewayClass resource }, }, { From afcfcf248c88af5d5a69d453d2d2bd3fccd53486 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Fri, 20 Jan 2023 10:45:26 -0600 Subject: [PATCH 6/9] Improve reconcile --- internal/reconciler/implementation.go | 70 +++++++++++---------------- 1 file changed, 27 insertions(+), 43 deletions(-) diff --git a/internal/reconciler/implementation.go b/internal/reconciler/implementation.go index b010b8e14b..fd4a6bdab0 100644 --- a/internal/reconciler/implementation.go +++ b/internal/reconciler/implementation.go @@ -67,8 +67,9 @@ func newObject(objectType client.Object) 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" + 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. @@ -79,6 +80,13 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( logger.Info("Reconciling the resource") + 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 { @@ -86,23 +94,31 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( logger.Error(err, "Failed to get the resource") return reconcile.Result{}, err } - // The resource does not exist + // 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) } - e := generateEvent(r.cfg.ObjectType, req.NamespacedName, obj, validationError) + var e interface{} + var op string + + 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, + } + op = "Deleted" + } else { + e = &events.UpsertEvent{ + Resource: obj, + } + op = "Upserted" + } select { case <-ctx.Done(): @@ -115,41 +131,9 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( logger.Error(validationError, webhookValidationErrorLogMsg) r.cfg.EventRecorder.Eventf(obj, apiv1.EventTypeWarning, "Rejected", webhookValidationErrorLogMsg+"; validation error: %v", validationError) - return reconcile.Result{}, nil - } - - op := "Upserted" - if _, deleted := e.(*events.DeleteEvent); deleted { - op = "Deleted" } logger.Info(fmt.Sprintf("%s the resource", op)) return reconcile.Result{}, nil } - -func generateEvent( - objType client.Object, - nsName types.NamespacedName, - obj client.Object, - validationErr error, -) interface{} { - if obj == nil { - return &events.DeleteEvent{ - Type: objType, - NamespacedName: nsName, - } - } - - if validationErr != nil { - // If the resource is invalid, we will delete it from the NGINX configuration. - return &events.DeleteEvent{ - Type: objType, - NamespacedName: nsName, - } - } - - return &events.UpsertEvent{ - Resource: obj, - } -} From cdde5731b54f73dd96257389fb119f4ef5768473 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Fri, 20 Jan 2023 10:50:14 -0600 Subject: [PATCH 7/9] Shorten the assertion --- internal/manager/validators_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/manager/validators_test.go b/internal/manager/validators_test.go index 6d2f6ed325..f6423244fd 100644 --- a/internal/manager/validators_test.go +++ b/internal/manager/validators_test.go @@ -61,7 +61,7 @@ func TestCreateTypedValidator(t *testing.T) { result := v(test.obj) if test.expectErr { - g.Expect(result).To(Not(BeNil())) + g.Expect(result).ToNot(BeNil()) return } From 08a7dcbce9e2b2b65e82a0ee48073d1f35b7103d Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 24 Jan 2023 16:27:49 -0600 Subject: [PATCH 8/9] Move validation error handling --- internal/reconciler/implementation.go | 12 +++---- internal/reconciler/implementation_test.go | 42 +++++++++++++--------- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/internal/reconciler/implementation.go b/internal/reconciler/implementation.go index fd4a6bdab0..38a568e8fa 100644 --- a/internal/reconciler/implementation.go +++ b/internal/reconciler/implementation.go @@ -103,6 +103,12 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( 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 op string @@ -127,12 +133,6 @@ func (r *Implementation) Reconcile(ctx context.Context, req reconcile.Request) ( case r.cfg.EventCh <- e: } - if validationError != nil { - logger.Error(validationError, webhookValidationErrorLogMsg) - r.cfg.EventRecorder.Eventf(obj, apiv1.EventTypeWarning, "Rejected", - webhookValidationErrorLogMsg+"; validation error: %v", validationError) - } - 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 61dfc9592a..952c07e703 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 { @@ -207,16 +214,11 @@ var _ = Describe("Reconciler", func() { fakeRecorder = &reconcilerfakes.FakeEventRecorder{} rec = reconciler.NewImplementation(reconciler.Config{ - Getter: fakeGetter, - ObjectType: &v1beta1.HTTPRoute{}, - EventCh: eventCh, - WebhookValidator: func(obj client.Object) error { - if client.ObjectKeyFromObject(obj) == hr2NsName { - return errors.New("test") - } - return nil - }, - EventRecorder: fakeRecorder, + Getter: fakeGetter, + ObjectType: &v1beta1.HTTPRoute{}, + EventCh: eventCh, + WebhookValidator: hr2IsInvalidValidator, + EventRecorder: fakeRecorder, }) }) @@ -250,11 +252,17 @@ var _ = Describe("Reconciler", func() { }) 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, }) }) @@ -269,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()) @@ -279,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), ) }) }) From c96415fe299273d63a31451c027463e8e4690b3f Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 24 Jan 2023 16:43:13 -0600 Subject: [PATCH 9/9] Revert recorder name --- internal/manager/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 13b964bc1d..b7f1963800 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -108,7 +108,7 @@ func Start(cfg config.Config) error { ctx := ctlr.SetupSignalHandler() - recorderName := fmt.Sprintf("nginx-kubernetes-gateway-with-gatewayclass-%s", cfg.GatewayClassName) + recorderName := fmt.Sprintf("nginx-kubernetes-gateway-%s", cfg.GatewayClassName) recorder := mgr.GetEventRecorderFor(recorderName) for _, regCfg := range controllerRegCfgs {