Run webhook validation (#388)

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 #362

Author: pleshakov

deploy/manifests/nginx-gateway.yaml

@@ -17,6 +17,13 @@ rules:
   verbs:
   - list
   - watch
+- apiGroups:
+  - ""
+  resources:
+  - events
+  verbs:
+  - create
+  - patch
 - apiGroups:
   - discovery.k8s.io
   resources:

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))

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))
+		})
 	}
 }

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)
 		}

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()
+	}
+}

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
+	}
+}