From b3cbf26dd53a29247b391b6c1671a8cb10053d0b Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Thu, 15 Feb 2024 18:21:45 +0000 Subject: [PATCH 1/3] Remove webhook validation code --- design/resource-validation.md | 40 +-- .../mode/static/state/change_processor.go | 37 +-- .../static/state/change_processor_test.go | 312 ------------------ .../mode/static/state/graph/backend_refs.go | 3 +- .../static/state/graph/backend_refs_test.go | 11 + .../static/state/graph/gateway_listener.go | 13 +- .../state/graph/gateway_listener_test.go | 34 ++ internal/mode/static/state/graph/httproute.go | 48 +-- .../mode/static/state/graph/httproute_test.go | 177 +++++++--- .../mode/static/state/graph/validation.go | 9 - internal/mode/static/state/store.go | 46 --- 11 files changed, 214 insertions(+), 516 deletions(-) diff --git a/design/resource-validation.md b/design/resource-validation.md index bd774158aa..de3dd054bc 100644 --- a/design/resource-validation.md +++ b/design/resource-validation.md @@ -91,36 +91,9 @@ Design a validation mechanism for Gateway API resources. ## Design -We will introduce two validation methods to be run by NGF control plane: - -1. Re-run of the Gateway API webhook validation -2. NGF-specific field validation - -### Re-run of Webhook Validation - -Before processing a resource, NGF will validate it using the functions from -the [validation package](https://github.com/kubernetes-sigs/gateway-api/tree/fa4b0a519b30a33b205ac0256876afc1456f2dd3/apis/v1/validation) -from the Gateway API. This will ensure that the webhook validation cannot be bypassed (it can be bypassed if the webhook -is not installed, misconfigured, or running a different version), and it will allow us to avoid repeating the same -validation in our code. - -If a resource is invalid: - -- NGF will not process it -- it will treat it as if the resource didn't exist. This also means that if the resource was - updated from a valid to an invalid state, NGF will also ignore any previous valid state. For example, it will remove - the generation configuration for an HTTPRoute resource. -- NGF will report the validation error as a - Warning [Event](https://kubernetes.io/docs/reference/kubernetes-api/cluster-resources/event-v1/) - for that resource. The Event message will describe the error and explain that the resource was ignored. We chose to - report an Event instead of updating the status, because to update the status, NGF first needs to look inside the - resource to determine whether it belongs to it or not. However, since the webhook validation applies to all parts of - the spec of resource, it means NGF has to look inside the invalid resource and parse potentially invalid parts. To - avoid that, NGF will report an Event. The owner of the resource will be able to see the Event. -- NGF will also report the validation error in the NGF logs. - ### NGF-specific validation -After re-running the webhook validation, NGF will run NGF-specific validation written in go. +NGF runs NGF-specific validation written in go. NGF-specific validation will: @@ -132,7 +105,6 @@ NGF-specific validation will not include: - *All* validation done by CRDs. NGF will only repeat the validation that addresses (1) and (2) in the list above with extra rules required by NGINX but missing in the CRDs. For example, NGF will not ensure the limits of field values. -- The validation done by the webhook (because it is done in the previous step). If a resource is invalid, NGF will report the error in its status. @@ -146,7 +118,6 @@ following methods in order of their appearance in the table. | Name | Type | Component | Scope | Feedback loop for errors | Can be bypassed? | |------------------------------|----------------------------|-----------------------|-------------------------|----------------------------------------------------------------------------------|--------------------------------| | CRD validation | OpenAPI and CEL validation | Kubernetes API server | Structure, field values | Kubernetes API server returns any errors a response for an API call. | Yes, if the CRDs are modified. | -| Re-run of webhook validation | Go code | NGF control plane | Field values | Errors are reported as Event for the resource. | No | | NGF-specific validation | Go code | NGF control plane | Field values | Errors are reported in the status of a resource after its creation/modification. | No | @@ -156,7 +127,6 @@ following methods in order of their appearance in the table. |------------------------------|---------|-----------------------|-------------------------|----------------------------------------------------------------------------------|--------------------------------------------------------------------------------------| | CRD validation | OpenAPI | Kubernetes API server | Structure, field values | Kubernetes API server returns any errors a response for an API call. | Yes, if the CRDs are modified. | | Webhook validation | Go code | Gateway API webhook | Field values | Kubernetes API server returns any errors a response for an API call. | Yes, if the webhook is not installed, misconfigured, or running a different version. | -| Re-run of webhook validation | Go code | NGF control plane | Field values | Errors are reported as Event for the resource. | No | | NGF-specific validation | Go code | NGF control plane | Field values | Errors are reported in the status of a resource after its creation/modification. | No | @@ -182,14 +152,6 @@ We will not introduce any NGF webhook in the cluster (it adds operational comple source of potential downtime -- a webhook failure disables CRUD operations on the relevant resources) unless we find good reasons for that. -### Upgrades - -Since NGF will use the validation package from the Gateway API project, when a new release happens, we will need to -upgrade the dependency and release a new version of NGF, provided that the validation code changed. However, if it did -not change, we do not need to release a new version. Note: other things from a new Gateway API release might prompt us -to release a new version like supporting a new field. See also -[GEP-922](https://gateway-api.sigs.k8s.io/geps/gep-922/#). - ### Reliability NGF processes two kinds of transactions: diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 210238f0fe..db035746c4 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -16,7 +16,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" v1 "sigs.k8s.io/gateway-api/apis/v1" - gwapivalidation "sigs.k8s.io/gateway-api/apis/v1/validation" "sigs.k8s.io/gateway-api/apis/v1alpha2" "sigs.k8s.io/gateway-api/apis/v1beta1" @@ -25,13 +24,6 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) -const ( - validationErrorLogMsg = "the resource failed validation: Gateway API CEL validation (Kubernetes 1.25+) " + - "by the Kubernetes API server and/or the Gateway API webhook validation (if installed) failed to reject " + - "the resource with the error; make sure Gateway API CRDs include CEL validation and/or (if installed) the " + - "webhook is running correctly." -) - // ChangeType is the type of change that occurred based on a k8s object event. type ChangeType int @@ -193,35 +185,8 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { }, ) - updater := newValidatingUpsertUpdater( - trackingUpdater, - cfg.EventRecorder, - func(obj client.Object) error { - // Add the validation for Gateway API resources which the webhook validates - - var err error - switch o := obj.(type) { - // We don't validate GatewayClass or ReferenceGrant, because as of the latest version, - // the webhook doesn't validate them. - // It only validates a GatewayClass update that requires the previous version of the resource, - // which NGF cannot reliably provide - for example, after NGF restarts). - // https://github.com/kubernetes-sigs/gateway-api/blob/v1.0.0/apis/v1/validation/gatewayclass.go#L28 - case *v1.Gateway: - err = gwapivalidation.ValidateGateway(o).ToAggregate() - case *v1.HTTPRoute: - err = gwapivalidation.ValidateHTTPRoute(o).ToAggregate() - } - - if err != nil { - return fmt.Errorf(validationErrorLogMsg+": %w", err) - } - - return nil - }, - ) - processor.getAndResetClusterStateChanged = trackingUpdater.getAndResetChangedStatus - processor.updater = updater + processor.updater = trackingUpdater return processor } diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 790f42e49e..fab678497e 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -11,7 +11,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log/zap" v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -2052,317 +2051,6 @@ var _ = Describe("ChangeProcessor", func() { ) }) }) - Describe("Webhook validation cases", Ordered, func() { - var ( - processor state.ChangeProcessor - fakeEventRecorder *record.FakeRecorder - - gc *v1.GatewayClass - - gwNsName, hrNsName types.NamespacedName - gw, gwInvalid *v1.Gateway - hr, hrInvalid *v1.HTTPRoute - ) - BeforeAll(func() { - fakeEventRecorder = record.NewFakeRecorder(2 /* number of buffered events */) - - processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ - GatewayCtlrName: controllerName, - GatewayClassName: gcName, - Logger: zap.New(), - Validators: createAlwaysValidValidators(), - EventRecorder: fakeEventRecorder, - Scheme: createScheme(), - }) - - gc = &v1.GatewayClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: gcName, - Generation: 1, - }, - Spec: v1.GatewayClassSpec{ - ControllerName: controllerName, - }, - } - - gwNsName = types.NamespacedName{Namespace: "test", Name: "gateway"} - hrNsName = types.NamespacedName{Namespace: "test", Name: "hr"} - - gw = &v1.Gateway{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: gwNsName.Namespace, - Name: gwNsName.Name, - }, - Spec: v1.GatewaySpec{ - GatewayClassName: gcName, - Listeners: []v1.Listener{ - { - Name: "listener-80-1", - Hostname: helpers.GetPointer[v1.Hostname]("foo.example.com"), - Port: 80, - Protocol: v1.HTTPProtocolType, - }, - }, - }, - } - - gwInvalid = gw.DeepCopy() - // cannot have hostname for TCP protocol - gwInvalid.Spec.Listeners[0].Protocol = v1.TCPProtocolType - - hr = &v1.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: hrNsName.Namespace, - Name: hrNsName.Name, - }, - Spec: v1.HTTPRouteSpec{ - CommonRouteSpec: v1.CommonRouteSpec{ - ParentRefs: []v1.ParentReference{ - { - Namespace: (*v1.Namespace)(&gw.Namespace), - Name: v1.ObjectName(gw.Name), - SectionName: (*v1.SectionName)( - helpers.GetPointer("listener-80-1"), - ), - }, - }, - }, - Hostnames: []v1.Hostname{ - "foo.example.com", - }, - Rules: []v1.HTTPRouteRule{ - { - Matches: []v1.HTTPRouteMatch{ - { - Path: &v1.HTTPPathMatch{ - Type: helpers.GetPointer(v1.PathMatchPathPrefix), - Value: helpers.GetPointer("/"), - }, - }, - }, - }, - }, - }, - } - - hrInvalid = hr.DeepCopy() - hrInvalid.Spec.Rules[0].Matches[0].Path.Type = nil // cannot be nil - }) - - assertHREvent := func() { - var e string - EventuallyWithOffset(1, fakeEventRecorder.Events).Should(Receive(&e)) - ExpectWithOffset(1, e).To(ContainSubstring("Rejected")) - ExpectWithOffset(1, e).To(ContainSubstring("spec.rules[0].matches[0].path.type")) - } - - assertGwEvent := func() { - var e string - EventuallyWithOffset(1, fakeEventRecorder.Events).Should(Receive(&e)) - ExpectWithOffset(1, e).To(ContainSubstring("Rejected")) - ExpectWithOffset(1, e).To(ContainSubstring("spec.listeners[0].hostname")) - } - - It("should process GatewayClass", func() { - processor.CaptureUpsertChange(gc) - - changed, graphCfg := processor.Process() - Expect(changed).To(Equal(state.ClusterStateChange)) - Expect(graphCfg.GatewayClass).ToNot(BeNil()) - Expect(fakeEventRecorder.Events).To(HaveLen(0)) - }) - - When("resources are invalid", func() { - It("should not process them", func() { - processor.CaptureUpsertChange(gwInvalid) - processor.CaptureUpsertChange(hrInvalid) - - changed, graphCfg := processor.Process() - - Expect(changed).To(Equal(state.NoChange)) - Expect(graphCfg).To(BeNil()) - - Expect(fakeEventRecorder.Events).To(HaveLen(2)) - assertGwEvent() - assertHREvent() - }) - }) - - When("resources are valid", func() { - It("should process them", func() { - processor.CaptureUpsertChange(gw) - processor.CaptureUpsertChange(hr) - - changed, graphCfg := processor.Process() - - Expect(changed).To(Equal(state.ClusterStateChange)) - Expect(graphCfg).ToNot(BeNil()) - Expect(graphCfg.Gateway).ToNot(BeNil()) - Expect(graphCfg.Routes).To(HaveLen(1)) - - Expect(fakeEventRecorder.Events).To(HaveLen(0)) - }) - }) - - When("a new version of HTTPRoute is invalid", func() { - It("it should delete the configuration for the old one and not process the new one", func() { - processor.CaptureUpsertChange(hrInvalid) - - changed, graphCfg := processor.Process() - - Expect(changed).To(Equal(state.ClusterStateChange)) - Expect(graphCfg.Routes).To(HaveLen(0)) - - Expect(fakeEventRecorder.Events).To(HaveLen(1)) - assertHREvent() - }) - }) - - When("a new version of Gateway is invalid", func() { - It("it should delete the configuration for the old one and not process the new one", func() { - processor.CaptureUpsertChange(gwInvalid) - - changed, graphCfg := processor.Process() - - Expect(changed).To(Equal(state.ClusterStateChange)) - Expect(graphCfg.Gateway).To(BeNil()) - - Expect(fakeEventRecorder.Events).To(HaveLen(1)) - assertGwEvent() - }) - }) - Describe("Webhook assumptions", func() { - 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, - Logger: zap.New(), - Validators: createAlwaysValidValidators(), - EventRecorder: fakeEventRecorder, - Scheme: createScheme(), - }) - }) - - createInvalidHTTPRoute := func(invalidator func(hr *v1.HTTPRoute)) *v1.HTTPRoute { - hr := createRoute( - "hr", - "gateway", - "foo.example.com", - createBackendRef( - helpers.GetPointer[v1.Kind]("Service"), - "test", - helpers.GetPointer[v1.Namespace]("namespace"), - ), - ) - invalidator(hr) - return hr - } - - createInvalidGateway := func(invalidator func(gw *v1.Gateway)) *v1.Gateway { - gw := createGateway("gateway") - invalidator(gw) - return gw - } - - assertRejectedEvent := func() { - EventuallyWithOffset(1, fakeEventRecorder.Events).Should(Receive(ContainSubstring("Rejected"))) - } - - DescribeTable("Invalid HTTPRoutes", - func(hr *v1.HTTPRoute) { - processor.CaptureUpsertChange(hr) - - changed, graphCfg := processor.Process() - - Expect(changed).To(Equal(state.NoChange)) - Expect(graphCfg).To(BeNil()) - - assertRejectedEvent() - }, - Entry( - "duplicate parentRefs", - createInvalidHTTPRoute(func(hr *v1.HTTPRoute) { - hr.Spec.ParentRefs = append(hr.Spec.ParentRefs, hr.Spec.ParentRefs[len(hr.Spec.ParentRefs)-1]) - }), - ), - Entry( - "nil path.Type", - createInvalidHTTPRoute(func(hr *v1.HTTPRoute) { - hr.Spec.Rules[0].Matches[0].Path.Type = nil - }), - ), - Entry("nil path.Value", - createInvalidHTTPRoute(func(hr *v1.HTTPRoute) { - hr.Spec.Rules[0].Matches[0].Path.Value = nil - }), - ), - Entry( - "nil request.Redirect", - createInvalidHTTPRoute(func(hr *v1.HTTPRoute) { - hr.Spec.Rules[0].Filters = append(hr.Spec.Rules[0].Filters, v1.HTTPRouteFilter{ - Type: v1.HTTPRouteFilterRequestRedirect, - RequestRedirect: nil, - }) - }), - ), - Entry("nil port in BackendRef", - createInvalidHTTPRoute(func(hr *v1.HTTPRoute) { - hr.Spec.Rules[0].BackendRefs[0].Port = nil - }), - ), - ) - - DescribeTable("Invalid Gateway resources", - func(gw *v1.Gateway) { - processor.CaptureUpsertChange(gw) - - changed, graphCfg := processor.Process() - - Expect(changed).To(Equal(state.NoChange)) - Expect(graphCfg).To(BeNil()) - - assertRejectedEvent() - }, - Entry("tls in HTTP listener", - createInvalidGateway(func(gw *v1.Gateway) { - gw.Spec.Listeners[0].TLS = &v1.GatewayTLSConfig{} - }), - ), - Entry("tls is nil in HTTPS listener", - createInvalidGateway(func(gw *v1.Gateway) { - gw.Spec.Listeners[0].Protocol = v1.HTTPSProtocolType - gw.Spec.Listeners[0].TLS = nil - }), - ), - Entry("zero certificateRefs in HTTPS listener", - createInvalidGateway(func(gw *v1.Gateway) { - gw.Spec.Listeners[0].Protocol = v1.HTTPSProtocolType - gw.Spec.Listeners[0].TLS = &v1.GatewayTLSConfig{ - Mode: helpers.GetPointer(v1.TLSModeTerminate), - CertificateRefs: nil, - } - }), - ), - Entry("listener hostnames conflict", - createInvalidGateway(func(gw *v1.Gateway) { - gw.Spec.Listeners = append(gw.Spec.Listeners, v1.Listener{ - Name: "listener-80-2", - Hostname: nil, - Port: 80, - Protocol: v1.HTTPProtocolType, - }) - }), - ), - ) - }) - }) Describe("Edge cases with panic", func() { var processor state.ChangeProcessor diff --git a/internal/mode/static/state/graph/backend_refs.go b/internal/mode/static/state/graph/backend_refs.go index 8d66d9b3a2..5e22743707 100644 --- a/internal/mode/static/state/graph/backend_refs.go +++ b/internal/mode/static/state/graph/backend_refs.go @@ -348,7 +348,8 @@ func validateBackendRef( } if ref.Port == nil { - panicForBrokenWebhookAssumption(fmt.Errorf("port is nil for ref %q", ref.Name)) + valErr := field.Required(path.Child("port"), "port cannot be nil") + return false, staticConds.NewRouteBackendRefUnsupportedValue(valErr.Error()) } // any value of port is OK diff --git a/internal/mode/static/state/graph/backend_refs_test.go b/internal/mode/static/state/graph/backend_refs_test.go index 32d963ed5c..d4c0a5b24e 100644 --- a/internal/mode/static/state/graph/backend_refs_test.go +++ b/internal/mode/static/state/graph/backend_refs_test.go @@ -208,6 +208,17 @@ func TestValidateBackendRef(t *testing.T) { "test.weight: Invalid value: -1: must be in the range [0, 1000000]", ), }, + { + name: "nil port", + ref: getModifiedRef(func(backend gatewayv1.BackendRef) gatewayv1.BackendRef { + backend.Port = nil + return backend + }), + expectedValid: false, + expectedCondition: staticConds.NewRouteBackendRefUnsupportedValue( + "test.port: Required value: port cannot be nil", + ), + }, } for _, test := range tests { diff --git a/internal/mode/static/state/graph/gateway_listener.go b/internal/mode/static/state/graph/gateway_listener.go index 12c2ade962..8fe03d7383 100644 --- a/internal/mode/static/state/graph/gateway_listener.go +++ b/internal/mode/static/state/graph/gateway_listener.go @@ -294,7 +294,9 @@ func createHTTPListenerValidator(protectedPorts ProtectedPorts) listenerValidato } if listener.TLS != nil { - panicForBrokenWebhookAssumption(fmt.Errorf("tls is not nil for HTTP listener %q", listener.Name)) + path := field.NewPath("tls") + valErr := field.Forbidden(path, "tls is not supported for HTTP listener") + conds = append(conds, staticConds.NewListenerUnsupportedValue(valErr.Error())...) } return conds, true @@ -322,7 +324,9 @@ func createHTTPSListenerValidator(protectedPorts ProtectedPorts) listenerValidat } if listener.TLS == nil { - panicForBrokenWebhookAssumption(fmt.Errorf("tls is nil for HTTPS listener %q", listener.Name)) + valErr := field.Required(field.NewPath("TLS"), "tls must be defined for HTTPS listener") + conds = append(conds, staticConds.NewListenerUnsupportedValue(valErr.Error())...) + return conds, true } tlsPath := field.NewPath("tls") @@ -343,7 +347,10 @@ func createHTTPSListenerValidator(protectedPorts ProtectedPorts) listenerValidat } if len(listener.TLS.CertificateRefs) == 0 { - panicForBrokenWebhookAssumption(fmt.Errorf("zero certificateRefs for HTTPS listener %q", listener.Name)) + msg := "certificateRefs must be defined for TLS mode terminate" + valErr := field.Required(tlsPath.Child("certificateRefs"), msg) + conds = append(conds, staticConds.NewListenerInvalidCertificateRef(valErr.Error())...) + return conds, true } certRef := listener.TLS.CertificateRefs[0] diff --git a/internal/mode/static/state/graph/gateway_listener_test.go b/internal/mode/static/state/graph/gateway_listener_test.go index 4e54ade569..dafe63e5a3 100644 --- a/internal/mode/static/state/graph/gateway_listener_test.go +++ b/internal/mode/static/state/graph/gateway_listener_test.go @@ -35,6 +35,17 @@ func TestValidateHTTPListener(t *testing.T) { expected: staticConds.NewListenerUnsupportedValue(`port: Invalid value: 0: port must be between 1-65535`), name: "invalid port", }, + { + l: v1.Listener{ + Port: 80, + TLS: &v1.GatewayTLSConfig{ + Mode: helpers.GetPointer(v1.TLSModeTerminate), + }, + Name: "http-listener", + }, + expected: staticConds.NewListenerUnsupportedValue(`tls: Forbidden: tls is not supported for HTTP listener`), + name: "invalid HTTP listener with TLS", + }, { l: v1.Listener{ Port: 9113, @@ -149,6 +160,16 @@ func TestValidateHTTPSListener(t *testing.T) { ), name: "invalid tls mode", }, + { + l: v1.Listener{ + Port: 443, + TLS: nil, + }, + expected: staticConds.NewListenerUnsupportedValue( + `TLS: Required value: tls must be defined for HTTPS listener`, + ), + name: "nil tls", + }, { l: v1.Listener{ Port: 443, @@ -162,6 +183,19 @@ func TestValidateHTTPSListener(t *testing.T) { ), name: "invalid cert ref group", }, + { + l: v1.Listener{ + Port: 443, + TLS: &v1.GatewayTLSConfig{ + Mode: helpers.GetPointer(v1.TLSModeTerminate), + CertificateRefs: []v1.SecretObjectReference{}, + }, + }, + expected: staticConds.NewListenerInvalidCertificateRef( + `tls.certificateRefs: Required value: certificateRefs must be defined for TLS mode terminate`, + ), + name: "zero cert refs", + }, { l: v1.Listener{ Port: 443, diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index d9135237d3..b1302b8c7f 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -1,7 +1,6 @@ package graph import ( - "errors" "fmt" "strings" @@ -99,7 +98,7 @@ func buildSectionNameRefs( parentRefs []v1.ParentReference, routeNamespace string, gatewayNsNames []types.NamespacedName, -) []ParentRef { +) ([]ParentRef, error) { sectionNameRefs := make([]ParentRef, 0, len(parentRefs)) type key struct { @@ -125,9 +124,7 @@ func buildSectionNameRefs( } if _, exist := uniqueSectionsPerGateway[k]; exist { - panicForBrokenWebhookAssumption( - fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gw.String()), - ) + return nil, fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gw.String()) } uniqueSectionsPerGateway[k] = struct{}{} @@ -137,7 +134,7 @@ func buildSectionNameRefs( }) } - return sectionNameRefs + return sectionNameRefs, nil } func findGatewayForParentRef( @@ -172,16 +169,21 @@ func buildRoute( ghr *v1.HTTPRoute, gatewayNsNames []types.NamespacedName, ) *Route { - sectionNameRefs := buildSectionNameRefs(ghr.Spec.ParentRefs, ghr.Namespace, gatewayNsNames) + r := &Route{ + Source: ghr, + } + sectionNameRefs, err := buildSectionNameRefs(ghr.Spec.ParentRefs, ghr.Namespace, gatewayNsNames) + if err != nil { + r.Valid = false + r.Conditions = append(r.Conditions, staticConds.NewRouteUnsupportedValue(err.Error())) + + return r + } // route doesn't belong to any of the Gateways if len(sectionNameRefs) == 0 { return nil } - - r := &Route{ - Source: ghr, - ParentRefs: sectionNameRefs, - } + r.ParentRefs = sectionNameRefs if err := validateHostnames( ghr.Spec.Hostnames, @@ -669,10 +671,10 @@ func validatePathMatch( } if path.Type == nil { - panicForBrokenWebhookAssumption(errors.New("path type cannot be nil")) + return field.ErrorList{field.Required(fieldPath.Child("type"), "path type cannot be nil")} } if path.Value == nil { - panicForBrokenWebhookAssumption(errors.New("path value cannot be nil")) + return field.ErrorList{field.Required(fieldPath.Child("value"), "path value cannot be nil")} } if *path.Type != v1.PathMatchPathPrefix && *path.Type != v1.PathMatchExact { @@ -725,13 +727,13 @@ func validateFilterRedirect( ) field.ErrorList { var allErrs field.ErrorList - if filter.RequestRedirect == nil { - panicForBrokenWebhookAssumption(errors.New("requestRedirect cannot be nil")) - } - redirect := filter.RequestRedirect redirectPath := filterPath.Child("requestRedirect") + if filter.RequestRedirect == nil { + return field.ErrorList{field.Required(redirectPath, "requestRedirect cannot be nil")} + } + if redirect.Scheme != nil { if valid, supportedValues := validator.ValidateRedirectScheme(*redirect.Scheme); !valid { valErr := field.NotSupported(redirectPath.Child("scheme"), *redirect.Scheme, supportedValues) @@ -775,13 +777,13 @@ func validateFilterRewrite( ) field.ErrorList { var allErrs field.ErrorList - if filter.URLRewrite == nil { - panicForBrokenWebhookAssumption(errors.New("urlRewrite cannot be nil")) - } - rewrite := filter.URLRewrite rewritePath := filterPath.Child("urlRewrite") + if filter.URLRewrite == nil { + return field.ErrorList{field.Required(rewritePath, "urlRewrite cannot be nil")} + } + if rewrite.Hostname != nil { if err := validator.ValidateHostname(string(*rewrite.Hostname)); err != nil { valErr := field.Invalid(rewritePath.Child("hostname"), *rewrite.Hostname, err.Error()) @@ -821,7 +823,7 @@ func validateFilterHeaderModifier( headerModifierPath := filterPath.Child("requestHeaderModifier") if headerModifier == nil { - panicForBrokenWebhookAssumption(errors.New("requestHeaderModifier cannot be nil")) + return field.ErrorList{field.Required(headerModifierPath, "requestHeaderModifier cannot be nil")} } return validateFilterHeaderModifierFields(validator, headerModifier, headerModifierPath) diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index ec3b5ea521..007a8b6751 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -30,14 +30,22 @@ func createHTTPRoute( paths ...string, ) *gatewayv1.HTTPRoute { rules := make([]gatewayv1.HTTPRouteRule, 0, len(paths)) + pathType := helpers.GetPointer(gatewayv1.PathMatchPathPrefix) for _, path := range paths { + if path == "/empty-type" { + pathType = nil + } + pathValue := helpers.GetPointer(path) + if path == "/empty-value" { + pathValue = nil + } rules = append(rules, gatewayv1.HTTPRouteRule{ Matches: []gatewayv1.HTTPRouteMatch{ { Path: &gatewayv1.HTTPPathMatch{ - Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), - Value: helpers.GetPointer(path), + Type: pathType, + Value: pathValue, }, }, }, @@ -189,54 +197,57 @@ func TestBuildSectionNameRefs(t *testing.T) { }, } - g := NewWithT(t) - - result := buildSectionNameRefs(parentRefs, routeNamespace, gwNsNames) - g.Expect(result).To(Equal(expected)) -} - -func TestBuildSectionNameRefsPanicsForDuplicateParentRefs(t *testing.T) { - gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} - tests := []struct { - name string - parentRefs []gatewayv1.ParentReference + name string + parentRefs []gatewayv1.ParentReference + expectedRefs []ParentRef + expectError bool }{ + { + name: "normal case", + parentRefs: parentRefs, + expectedRefs: expected, + expectError: false, + }, { parentRefs: []gatewayv1.ParentReference{ { - Name: gatewayv1.ObjectName(gwNsName.Name), + Name: gatewayv1.ObjectName(gwNsName1.Name), SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), }, { - Name: gatewayv1.ObjectName(gwNsName.Name), + Name: gatewayv1.ObjectName(gwNsName1.Name), SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), }, }, - name: "with sectionNames", + name: "duplicate sectionNames", + expectError: true, }, { parentRefs: []gatewayv1.ParentReference{ { - Name: gatewayv1.ObjectName(gwNsName.Name), + Name: gatewayv1.ObjectName(gwNsName1.Name), SectionName: nil, }, { - Name: gatewayv1.ObjectName(gwNsName.Name), + Name: gatewayv1.ObjectName(gwNsName1.Name), SectionName: nil, }, }, - name: "nil sectionNames", + name: "nil sectionNames", + expectError: true, }, } - gwNsNames := []types.NamespacedName{gwNsName} - for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - run := func() { buildSectionNameRefs(test.parentRefs, gwNsName.Namespace, gwNsNames) } - g.Expect(run).To(Panic()) + + result, err := buildSectionNameRefs(test.parentRefs, routeNamespace, gwNsNames) + g.Expect(result).To(Equal(test.expectedRefs)) + if !test.expectError { + g.Expect(err).To(BeNil()) + } }) } } @@ -330,6 +341,8 @@ func TestBuildRoute(t *testing.T) { const ( invalidPath = "/invalid" invalidRedirectHostname = "invalid.example.com" + emptyPathType = "/empty-type" + emptyPathValue = "/empty-value" ) gatewayNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} @@ -352,6 +365,9 @@ func TestBuildRoute(t *testing.T) { hrNotNGF := createHTTPRoute("hr", "some-gateway", "example.com", "/") hrInvalidMatches := createHTTPRoute("hr", gatewayNsName.Name, "example.com", invalidPath) + hrInvalidMatchesEmptyPathType := createHTTPRoute("hr", gatewayNsName.Name, "example.com", emptyPathType) + hrInvalidMatchesEmptyPathValue := createHTTPRoute("hr", gatewayNsName.Name, "example.com", emptyPathValue) + hrInvalidFilters := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/filter") addFilterToPath(hrInvalidFilters, "/filter", invalidFilter) @@ -368,6 +384,12 @@ func TestBuildRoute(t *testing.T) { addFilterToPath(hrDroppedInvalidFilters, "/filter", validFilter) addFilterToPath(hrDroppedInvalidFilters, "/", invalidFilter) + hrDuplicateSectionName := createHTTPRoute("hr", gatewayNsName.Name, "example.com", "/") + hrDuplicateSectionName.Spec.ParentRefs = append( + hrDuplicateSectionName.Spec.ParentRefs, + hrDuplicateSectionName.Spec.ParentRefs[0], + ) + validatorInvalidFieldsInRule := &validationfakes.FakeHTTPFieldsValidator{ ValidatePathInMatchStub: func(path string) error { if path == invalidPath { @@ -415,6 +437,73 @@ func TestBuildRoute(t *testing.T) { }, name: "normal case", }, + { + validator: &validationfakes.FakeHTTPFieldsValidator{}, + hr: hrInvalidMatchesEmptyPathType, + expected: &Route{ + Source: hrInvalidMatchesEmptyPathType, + Valid: false, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRouteUnsupportedValue( + `All rules are invalid: spec.rules[0].matches[0].path.type: Required value: path type cannot be nil`, + ), + }, + Rules: []Rule{ + { + ValidMatches: false, + ValidFilters: true, + }, + }, + }, + name: "invalid matches with empty path type", + }, + { + validator: &validationfakes.FakeHTTPFieldsValidator{}, + hr: hrDuplicateSectionName, + expected: &Route{ + Source: hrDuplicateSectionName, + Conditions: []conditions.Condition{ + staticConds.NewRouteUnsupportedValue( + `duplicate section name "test-section" for Gateway test/gateway`, + ), + }, + }, + name: "invalid route with duplicate sectionName", + }, + { + validator: &validationfakes.FakeHTTPFieldsValidator{}, + hr: hrInvalidMatchesEmptyPathValue, + expected: &Route{ + Source: hrInvalidMatchesEmptyPathValue, + Valid: false, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + }, + }, + Conditions: []conditions.Condition{ + staticConds.NewRouteUnsupportedValue( + `All rules are invalid: spec.rules[0].matches[0].path.value: Required value: path value cannot be nil`, + ), + }, + Rules: []Rule{ + { + ValidMatches: false, + ValidFilters: true, + }, + }, + }, + name: "invalid matches with empty path value", + }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, hr: hrNotNGF, @@ -1907,7 +1996,6 @@ func TestValidateFilterRedirect(t *testing.T) { validator *validationfakes.FakeHTTPFieldsValidator name string expectErrCount int - panic bool }{ { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -1915,8 +2003,8 @@ func TestValidateFilterRedirect(t *testing.T) { Type: gatewayv1.HTTPRouteFilterRequestRedirect, RequestRedirect: nil, }, - panic: true, - name: "nil filter", + name: "nil filter", + expectErrCount: 1, }, { validator: createAllValidValidator(), @@ -2042,15 +2130,9 @@ func TestValidateFilterRedirect(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - if test.panic { - validate := func() { - _ = validateFilterRedirect(test.validator, test.filter, filterPath) - } - g.Expect(validate).To(Panic()) - } else { - allErrs := validateFilterRedirect(test.validator, test.filter, filterPath) - g.Expect(allErrs).To(HaveLen(test.expectErrCount)) - } + + allErrs := validateFilterRedirect(test.validator, test.filter, filterPath) + g.Expect(allErrs).To(HaveLen(test.expectErrCount)) }) } } @@ -2061,7 +2143,6 @@ func TestValidateFilterRewrite(t *testing.T) { validator *validationfakes.FakeHTTPFieldsValidator name string expectErrCount int - panic bool }{ { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -2069,8 +2150,8 @@ func TestValidateFilterRewrite(t *testing.T) { Type: gatewayv1.HTTPRouteFilterURLRewrite, URLRewrite: nil, }, - panic: true, - name: "nil filter", + name: "nil filter", + expectErrCount: 1, }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -2191,15 +2272,8 @@ func TestValidateFilterRewrite(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - if test.panic { - validate := func() { - _ = validateFilterRewrite(test.validator, test.filter, filterPath) - } - g.Expect(validate).To(Panic()) - } else { - allErrs := validateFilterRewrite(test.validator, test.filter, filterPath) - g.Expect(allErrs).To(HaveLen(test.expectErrCount)) - } + allErrs := validateFilterRewrite(test.validator, test.filter, filterPath) + g.Expect(allErrs).To(HaveLen(test.expectErrCount)) }) } } @@ -2233,6 +2307,15 @@ func TestValidateFilterRequestHeaderModifier(t *testing.T) { expectErrCount: 0, name: "valid request header modifier filter", }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: nil, + }, + expectErrCount: 1, + name: "nil request header modifier filter", + }, { validator: func() *validationfakes.FakeHTTPFieldsValidator { v := createAllValidValidator() diff --git a/internal/mode/static/state/graph/validation.go b/internal/mode/static/state/graph/validation.go index 049721c20c..27d23bf135 100644 --- a/internal/mode/static/state/graph/validation.go +++ b/internal/mode/static/state/graph/validation.go @@ -2,7 +2,6 @@ package graph import ( "errors" - "fmt" "strings" "k8s.io/apimachinery/pkg/util/validation" @@ -30,11 +29,3 @@ func validateHostname(hostname string) error { return nil } - -// panicForBrokenWebhookAssumption panics with the error message because an assumption about the webhook validation -// (run by NGF) is broken. -// Use it when you expect a validated Gateway API resource, but the actual resource breaks the validation constraints. -// For example, a field that must not be nil is nil. -func panicForBrokenWebhookAssumption(err error) { - panic(fmt.Errorf("webhook validation assumption was broken: %w", err)) -} diff --git a/internal/mode/static/state/store.go b/internal/mode/static/state/store.go index ac0659fe7c..02a2585580 100644 --- a/internal/mode/static/state/store.go +++ b/internal/mode/static/state/store.go @@ -3,11 +3,9 @@ package state import ( "fmt" - apiv1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -254,47 +252,3 @@ func (s *changeTrackingUpdater) setChangeType(obj client.Object, changed bool) { } } } - -type upsertValidatorFunc func(obj client.Object) error - -// validatingUpsertUpdater is an Updater that validates an object before upserting it. -// If the validation fails, it deletes the object and records an event with the validation error. -type validatingUpsertUpdater struct { - updater Updater - eventRecorder record.EventRecorder - validator upsertValidatorFunc -} - -func newValidatingUpsertUpdater( - updater Updater, - eventRecorder record.EventRecorder, - validator upsertValidatorFunc, -) *validatingUpsertUpdater { - return &validatingUpsertUpdater{ - updater: updater, - eventRecorder: eventRecorder, - validator: validator, - } -} - -func (u *validatingUpsertUpdater) Upsert(obj client.Object) { - if err := u.validator(obj); err != nil { - u.updater.Delete(obj, client.ObjectKeyFromObject(obj)) - - u.eventRecorder.Eventf( - obj, - apiv1.EventTypeWarning, - "Rejected", - "%s; NGINX Gateway Fabric will delete any existing NGINX configuration that corresponds to the resource", - err.Error(), - ) - - return - } - - u.updater.Upsert(obj) -} - -func (u *validatingUpsertUpdater) Delete(objType client.Object, nsname types.NamespacedName) { - u.updater.Delete(objType, nsname) -} From fcc5ce84f0c044453bbdb28842bcb7d772535790 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Fri, 16 Feb 2024 11:59:40 +0000 Subject: [PATCH 2/3] Add validation for case insensitive header name repetition in filters --- internal/mode/static/state/graph/httproute.go | 51 +++++++++++++++++++ .../mode/static/state/graph/httproute_test.go | 19 +++++++ 2 files changed, 70 insertions(+) diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index b1302b8c7f..e7b48afd2b 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -836,6 +836,20 @@ func validateFilterHeaderModifierFields( ) field.ErrorList { var allErrs field.ErrorList + // Ensure that the header names are case-insensitive unique + allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( + headerModifier.Add, + headerModifierPath.Child("add"))..., + ) + allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( + headerModifier.Set, + headerModifierPath.Child("set"))..., + ) + allErrs = append(allErrs, validateRequestHeaderStringCaseInsensitiveUnique( + headerModifier.Remove, + headerModifierPath.Child("remove"))..., + ) + for _, h := range headerModifier.Add { if err := validator.ValidateRequestHeaderName(string(h.Name)); err != nil { valErr := field.Invalid(headerModifierPath.Child("add"), h, err.Error()) @@ -865,3 +879,40 @@ func validateFilterHeaderModifierFields( return allErrs } + +func validateRequestHeadersCaseInsensitiveUnique( + headers []v1.HTTPHeader, + path *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + + seen := make(map[string]struct{}) + + for _, h := range headers { + name := strings.ToLower(string(h.Name)) + if _, exists := seen[name]; exists { + valErr := field.Invalid(path, h, "header name is not unique") + allErrs = append(allErrs, valErr) + } + seen[name] = struct{}{} + } + + return allErrs +} + +func validateRequestHeaderStringCaseInsensitiveUnique(headers []string, path *field.Path) field.ErrorList { + var allErrs field.ErrorList + + seen := make(map[string]struct{}) + + for _, h := range headers { + name := strings.ToLower(h) + if _, exists := seen[name]; exists { + valErr := field.Invalid(path, h, "header name is not unique") + allErrs = append(allErrs, valErr) + } + seen[name] = struct{}{} + } + + return allErrs +} diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index 007a8b6751..98fee5643a 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -2388,6 +2388,25 @@ func TestValidateFilterRequestHeaderModifier(t *testing.T) { expectErrCount: 7, name: "request header modifier filter all fields invalid", }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + {Name: "mYbespokeHEader", Value: "duplicate"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + {Name: "accept-encodING", Value: "gzip"}, + }, + Remove: []string{"Cache-Control", "cache-control"}, + }, + }, + expectErrCount: 3, + name: "request header modifier filter not unique names", + }, } filterPath := field.NewPath("test") From a0e4b7f429c0f55e0854076ca450d0c96e336315 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Mon, 19 Feb 2024 11:48:52 +0000 Subject: [PATCH 3/3] Review feedback --- internal/mode/static/state/graph/httproute.go | 5 +-- .../mode/static/state/graph/httproute_test.go | 41 +++++++++---------- site/content/overview/resource-validation.md | 22 +--------- 3 files changed, 22 insertions(+), 46 deletions(-) diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index e7b48afd2b..d16e833f00 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -175,7 +175,6 @@ func buildRoute( sectionNameRefs, err := buildSectionNameRefs(ghr.Spec.ParentRefs, ghr.Namespace, gatewayNsNames) if err != nil { r.Valid = false - r.Conditions = append(r.Conditions, staticConds.NewRouteUnsupportedValue(err.Error())) return r } @@ -730,7 +729,7 @@ func validateFilterRedirect( redirect := filter.RequestRedirect redirectPath := filterPath.Child("requestRedirect") - if filter.RequestRedirect == nil { + if redirect == nil { return field.ErrorList{field.Required(redirectPath, "requestRedirect cannot be nil")} } @@ -780,7 +779,7 @@ func validateFilterRewrite( rewrite := filter.URLRewrite rewritePath := filterPath.Child("urlRewrite") - if filter.URLRewrite == nil { + if rewrite == nil { return field.ErrorList{field.Required(rewritePath, "urlRewrite cannot be nil")} } diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index 98fee5643a..7f4b99d376 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -21,6 +21,8 @@ import ( const ( sectionNameOfCreateHTTPRoute = "test-section" + emptyPathType = "/empty-type" + emptyPathValue = "/empty-value" ) func createHTTPRoute( @@ -33,11 +35,11 @@ func createHTTPRoute( pathType := helpers.GetPointer(gatewayv1.PathMatchPathPrefix) for _, path := range paths { - if path == "/empty-type" { + if path == emptyPathType { pathType = nil } pathValue := helpers.GetPointer(path) - if path == "/empty-value" { + if path == emptyPathValue { pathValue = nil } rules = append(rules, gatewayv1.HTTPRouteRule{ @@ -198,16 +200,16 @@ func TestBuildSectionNameRefs(t *testing.T) { } tests := []struct { - name string - parentRefs []gatewayv1.ParentReference - expectedRefs []ParentRef - expectError bool + expectedError error + name string + parentRefs []gatewayv1.ParentReference + expectedRefs []ParentRef }{ { - name: "normal case", - parentRefs: parentRefs, - expectedRefs: expected, - expectError: false, + name: "normal case", + parentRefs: parentRefs, + expectedRefs: expected, + expectedError: nil, }, { parentRefs: []gatewayv1.ParentReference{ @@ -220,8 +222,8 @@ func TestBuildSectionNameRefs(t *testing.T) { SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), }, }, - name: "duplicate sectionNames", - expectError: true, + name: "duplicate sectionNames", + expectedError: errors.New("duplicate section name \"http\" for Gateway test/gateway-1"), }, { parentRefs: []gatewayv1.ParentReference{ @@ -234,8 +236,8 @@ func TestBuildSectionNameRefs(t *testing.T) { SectionName: nil, }, }, - name: "nil sectionNames", - expectError: true, + name: "nil sectionNames", + expectedError: errors.New("duplicate section name \"\" for Gateway test/gateway-1"), }, } @@ -245,7 +247,9 @@ func TestBuildSectionNameRefs(t *testing.T) { result, err := buildSectionNameRefs(test.parentRefs, routeNamespace, gwNsNames) g.Expect(result).To(Equal(test.expectedRefs)) - if !test.expectError { + if test.expectedError != nil { + g.Expect(err).To(Equal(test.expectedError)) + } else { g.Expect(err).To(BeNil()) } }) @@ -341,8 +345,6 @@ func TestBuildRoute(t *testing.T) { const ( invalidPath = "/invalid" invalidRedirectHostname = "invalid.example.com" - emptyPathType = "/empty-type" - emptyPathValue = "/empty-value" ) gatewayNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} @@ -469,11 +471,6 @@ func TestBuildRoute(t *testing.T) { hr: hrDuplicateSectionName, expected: &Route{ Source: hrDuplicateSectionName, - Conditions: []conditions.Condition{ - staticConds.NewRouteUnsupportedValue( - `duplicate section name "test-section" for Gateway test/gateway`, - ), - }, }, name: "invalid route with duplicate sectionName", }, diff --git a/site/content/overview/resource-validation.md b/site/content/overview/resource-validation.md index 2c3c52a2f4..9ae2b37e3f 100644 --- a/site/content/overview/resource-validation.md +++ b/site/content/overview/resource-validation.md @@ -64,27 +64,7 @@ The HTTPRoute "coffee" is invalid: spec.hostnames[0]: Invalid value: "cafe.!@#$% {{< note >}}Bypassing this validation step is possible if the webhook is not running in the cluster. If this happens, Step 3 will reject the invalid values.{{< /note >}} -### Step 3 - Webhook validation by NGINX Gateway Fabric - -To ensure that the resources are validated with the webhook validation rules, even if the webhook is not running, NGINX Gateway Fabric performs the same validation. However, NGINX Gateway Fabric performs the validation _after_ the Kubernetes API server accepts the resource. - -Below is an example of how NGINX Gateway Fabric rejects an invalid resource (a Gateway resource with a TCP listener that configures a hostname) with a Kubernetes event: - -```shell -kubectl describe gateway some-gateway -``` - -```text -. . . -Events: - Type Reason Age From Message - ---- ------ ---- ---- ------- - Warning Rejected 6s nginx-gateway-fabric-nginx the resource failed webhook validation, however the Gateway API webhook failed to reject it with the error; make sure the webhook is installed and running correctly; validation error: spec.listeners[1].hostname: Forbidden: should be empty for protocol TCP; NGINX Gateway Fabric will delete any existing NGINX configuration that corresponds to the resource -``` - -{{< note >}}This validation step always runs and cannot be bypassed. NGINX Gateway Fabric will ignore any resources that fail the webhook validation, like in the example above. If the resource previously existed, NGINX Gateway Fabric will remove any existing NGINX configuration for that resource.{{< /note >}} - -### Step 4 - Validation by NGINX Gateway Fabric +### Step 3 - Validation by NGINX Gateway Fabric This step catches the following cases of invalid values: