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..d16e833f00 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,20 @@ 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 + + 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 +670,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 +726,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 redirect == 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 +776,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 rewrite == 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 +822,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) @@ -834,6 +835,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()) @@ -863,3 +878,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 ec3b5ea521..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( @@ -30,14 +32,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 == emptyPathType { + pathType = nil + } + pathValue := helpers.GetPointer(path) + if path == emptyPathValue { + 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 +199,59 @@ 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 + expectedError error + name string + parentRefs []gatewayv1.ParentReference + expectedRefs []ParentRef }{ + { + name: "normal case", + parentRefs: parentRefs, + expectedRefs: expected, + expectedError: nil, + }, { 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", + expectedError: errors.New("duplicate section name \"http\" for Gateway test/gateway-1"), }, { 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", + expectedError: errors.New("duplicate section name \"\" for Gateway test/gateway-1"), }, } - 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.expectedError != nil { + g.Expect(err).To(Equal(test.expectedError)) + } else { + g.Expect(err).To(BeNil()) + } }) } } @@ -352,6 +367,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 +386,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 +439,68 @@ 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, + }, + 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 +1993,6 @@ func TestValidateFilterRedirect(t *testing.T) { validator *validationfakes.FakeHTTPFieldsValidator name string expectErrCount int - panic bool }{ { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -1915,8 +2000,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 +2127,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 +2140,6 @@ func TestValidateFilterRewrite(t *testing.T) { validator *validationfakes.FakeHTTPFieldsValidator name string expectErrCount int - panic bool }{ { validator: &validationfakes.FakeHTTPFieldsValidator{}, @@ -2069,8 +2147,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 +2269,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 +2304,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() @@ -2305,6 +2385,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") 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) -} 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: