From ba2f63cf9a70af539adcac327527167c50e60285 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 6 Apr 2023 12:59:29 -0700 Subject: [PATCH] Panic for broken webhook validation assumption - Add panics for cases when an assumption about how webhook validation (run by NKG) validates a Gateway API resource is broken. - Add unit tests for those cases to ensure that invalid resources don't reach the code that makes those assumptions. Note: panics are not unit tested as we considered it'd be redundant. Fixes https://github.com/nginxinc/nginx-kubernetes-gateway/issues/515 --- internal/nginx/config/servers.go | 4 + internal/state/change_processor_test.go | 136 +++++++++++++++++++++++- internal/state/graph/backend_refs.go | 8 +- internal/state/graph/gateway.go | 15 +-- internal/state/graph/httproute.go | 23 ++-- internal/state/graph/validation.go | 9 ++ 6 files changed, 175 insertions(+), 20 deletions(-) diff --git a/internal/nginx/config/servers.go b/internal/nginx/config/servers.go index 513ffe6d8c..2c0b1c52c5 100644 --- a/internal/nginx/config/servers.go +++ b/internal/nginx/config/servers.go @@ -120,6 +120,10 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int) []http.Lo // For example, type is v1beta1.HTTPRouteFilterRequestRedirect, but RequestRedirect field is nil. // The imported Webhook validation webhook catches that. + // FIXME(pleshakov): Ensure dataplane.Configuration -related types don't include v1beta1 types, so that + // we don't need to make any assumptions like above here. After fixing this, ensure that there is a test + // for checking the imported Webhook validation catches the case above. + // RequestRedirect and proxying are mutually exclusive. if r.Filters.RequestRedirect != nil { loc.Return = createReturnValForRedirectFilter(r.Filters.RequestRedirect, listenerPort) diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index 28a5226ffe..cef061248c 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -1995,13 +1995,15 @@ var _ = Describe("ChangeProcessor", func() { }) assertHREvent := func() { - e := <-fakeEventRecorder.Events + 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() { - e := <-fakeEventRecorder.Events + 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")) } @@ -2185,6 +2187,136 @@ var _ = Describe("ChangeProcessor", func() { assertGwEvent() }) }) + + Describe("Webhook assumptions", func() { + var processor state.ChangeProcessor + + BeforeEach(func() { + fakeSecretMemoryMgr = &secretsfakes.FakeSecretDiskMemoryManager{} + fakeEventRecorder = record.NewFakeRecorder(1 /* number of buffered events */) + + processor = state.NewChangeProcessorImpl(state.ChangeProcessorConfig{ + GatewayCtlrName: controllerName, + GatewayClassName: gcName, + SecretMemoryManager: fakeSecretMemoryMgr, + RelationshipCapturer: relationship.NewCapturerImpl(), + Logger: zap.New(), + Validators: createAlwaysValidValidators(), + EventRecorder: fakeEventRecorder, + Scheme: createScheme(), + }) + }) + + createInvalidHTTPRoute := func(invalidator func(hr *v1beta1.HTTPRoute)) *v1beta1.HTTPRoute { + hr := createRoute( + "hr", + "gateway", + "foo.example.com", + createBackendRef( + helpers.GetPointer[v1beta1.Kind]("Service"), + "test", + helpers.GetPointer[v1beta1.Namespace]("namespace"), + ), + ) + invalidator(hr) + return hr + } + + createInvalidGateway := func(invalidator func(gw *v1beta1.Gateway)) *v1beta1.Gateway { + gw := createGateway("gateway") + invalidator(gw) + return gw + } + + assertRejectedEvent := func() { + EventuallyWithOffset(1, fakeEventRecorder.Events).Should(Receive(ContainSubstring("Rejected"))) + } + + DescribeTable("Invalid HTTPRoutes", + func(hr *v1beta1.HTTPRoute) { + processor.CaptureUpsertChange(hr) + + expectedConf := dataplane.Configuration{} + expectedStatuses := state.Statuses{} + + changed, conf, statuses := processor.Process(context.Background()) + + Expect(changed).To(BeFalse()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) + + assertRejectedEvent() + }, + Entry( + "duplicate parentRefs", + createInvalidHTTPRoute(func(hr *v1beta1.HTTPRoute) { + hr.Spec.ParentRefs = append(hr.Spec.ParentRefs, hr.Spec.ParentRefs[len(hr.Spec.ParentRefs)-1]) + }), + ), + Entry( + "nil path.Type", + createInvalidHTTPRoute(func(hr *v1beta1.HTTPRoute) { + hr.Spec.Rules[0].Matches[0].Path.Type = nil + }), + ), + Entry("nil path.Value", + createInvalidHTTPRoute(func(hr *v1beta1.HTTPRoute) { + hr.Spec.Rules[0].Matches[0].Path.Value = nil + }), + ), + Entry( + "nil request.Redirect", + createInvalidHTTPRoute(func(hr *v1beta1.HTTPRoute) { + hr.Spec.Rules[0].Filters = append(hr.Spec.Rules[0].Filters, v1beta1.HTTPRouteFilter{ + Type: v1beta1.HTTPRouteFilterRequestRedirect, + RequestRedirect: nil, + }) + }), + ), + Entry("nil port in BackendRef", + createInvalidHTTPRoute(func(hr *v1beta1.HTTPRoute) { + hr.Spec.Rules[0].BackendRefs[0].Port = nil + }), + ), + ) + + DescribeTable("Invalid Gateway resources", + func(gw *v1beta1.Gateway) { + processor.CaptureUpsertChange(gw) + + expectedConf := dataplane.Configuration{} + expectedStatuses := state.Statuses{} + + changed, conf, statuses := processor.Process(context.Background()) + + Expect(changed).To(BeFalse()) + Expect(helpers.Diff(expectedConf, conf)).To(BeEmpty()) + assertStatuses(expectedStatuses, statuses) + + assertRejectedEvent() + }, + Entry("tls in HTTP listener", + createInvalidGateway(func(gw *v1beta1.Gateway) { + gw.Spec.Listeners[0].TLS = &v1beta1.GatewayTLSConfig{} + }), + ), + Entry("tls is nil in HTTPS listener", + createInvalidGateway(func(gw *v1beta1.Gateway) { + gw.Spec.Listeners[0].Protocol = v1beta1.HTTPSProtocolType + gw.Spec.Listeners[0].TLS = nil + }), + ), + Entry("zero certificateRefs in HTTPS listener", + createInvalidGateway(func(gw *v1beta1.Gateway) { + gw.Spec.Listeners[0].Protocol = v1beta1.HTTPSProtocolType + gw.Spec.Listeners[0].TLS = &v1beta1.GatewayTLSConfig{ + Mode: helpers.GetPointer(v1beta1.TLSModeTerminate), + CertificateRefs: nil, + } + }), + ), + ) + }) }) Describe("Edge cases with panic", func() { diff --git a/internal/state/graph/backend_refs.go b/internal/state/graph/backend_refs.go index 10b8d64765..6ba2c0337a 100644 --- a/internal/state/graph/backend_refs.go +++ b/internal/state/graph/backend_refs.go @@ -167,9 +167,11 @@ func validateBackendRef( return false, conditions.NewRouteBackendRefRefNotPermitted(valErr.Error()) } - // The imported Webhook validation ensures ref.Port is set - // any value is OK - // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. + if ref.Port == nil { + panicForBrokenWebhookAssumption(fmt.Errorf("port is nil for ref %q", ref.Name)) + } + + // any value of port is OK if ref.Weight != nil { if err := validateWeight(*ref.Weight); err != nil { diff --git a/internal/state/graph/gateway.go b/internal/state/graph/gateway.go index b8c9ba8d96..b8ff28a83c 100644 --- a/internal/state/graph/gateway.go +++ b/internal/state/graph/gateway.go @@ -321,8 +321,9 @@ func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition { conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error())) } - // The imported Webhook validation ensures the tls field is not set for an HTTP listener. - // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. + if listener.TLS != nil { + panicForBrokenWebhookAssumption(fmt.Errorf("tls is not nil for HTTP listener %q", listener.Name)) + } return conds } @@ -336,8 +337,9 @@ func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []conditi conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error())) } - // The imported Webhook validation ensures the tls field is not nil for an HTTPS listener. - // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. + if listener.TLS == nil { + panicForBrokenWebhookAssumption(fmt.Errorf("tls is nil for HTTPS listener %q", listener.Name)) + } tlsPath := field.NewPath("tls") @@ -356,8 +358,9 @@ func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []conditi conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error())) } - // The imported Webhook validation ensures len(listener.TLS.Certificates) is not 0. - // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. + if len(listener.TLS.CertificateRefs) == 0 { + panicForBrokenWebhookAssumption(fmt.Errorf("zero certificateRefs for HTTPS listener %q", listener.Name)) + } certRef := listener.TLS.CertificateRefs[0] diff --git a/internal/state/graph/httproute.go b/internal/state/graph/httproute.go index ba3ee4e5f6..ee23f26d01 100644 --- a/internal/state/graph/httproute.go +++ b/internal/state/graph/httproute.go @@ -1,6 +1,7 @@ package graph import ( + "errors" "fmt" "k8s.io/apimachinery/pkg/types" @@ -148,17 +149,16 @@ func buildSectionNameRefs( continue } - // The imported Webhook validation ensures unique section names. - // Additionally, it ensures that if multiple refs reference the same Gateway, their section names - // are not empty - // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. - // FIXME(pleshakov): SectionNames across multiple Gateways might collide. Fix that. var sectionName string if p.SectionName != nil { sectionName = string(*p.SectionName) } + if _, exist := sectionNameRefs[sectionName]; exist { + panicForBrokenWebhookAssumption(fmt.Errorf("duplicate section name %q", sectionName)) + } + sectionNameRefs[sectionName] = ParentRef{ Idx: i, Gateway: gw, @@ -520,8 +520,12 @@ func validatePathMatch( return allErrs } - // The imported Webhook validation ensures the path type and value are not nil - // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. + if path.Type == nil { + panicForBrokenWebhookAssumption(errors.New("path type cannot be nil")) + } + if path.Value == nil { + panicForBrokenWebhookAssumption(errors.New("path value cannot be nil")) + } if *path.Type != v1beta1.PathMatchPathPrefix { valErr := field.NotSupported(fieldPath, *path.Type, []string{string(v1beta1.PathMatchPathPrefix)}) @@ -553,8 +557,9 @@ func validateFilter( return allErrs } - // The imported Webhook validation ensures filter.RequestRedirect is not nil - // FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case. + if filter.RequestRedirect == nil { + panicForBrokenWebhookAssumption(errors.New("requestRedirect cannot be nil")) + } redirect := filter.RequestRedirect diff --git a/internal/state/graph/validation.go b/internal/state/graph/validation.go index 8825d32b95..1141680011 100644 --- a/internal/state/graph/validation.go +++ b/internal/state/graph/validation.go @@ -2,6 +2,7 @@ package graph import ( "errors" + "fmt" "strings" "k8s.io/apimachinery/pkg/util/validation" @@ -24,3 +25,11 @@ func validateHostname(hostname string) error { return nil } + +// panicForBrokenWebhookAssumption panics with the error message because an assumption about the webhook validation +// (run by NKG) 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)) +}