Panic for broken webhook validation assumption (#565)

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

Author: pleshakov

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)

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

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 {

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]
 

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
 

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