Implement NKG-specific field validation for Gateways

internal/state/conditions/conditions_test.go

@@ -7,7 +7,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func TestDeduplicateDeduplicateRouteConditions(t *testing.T) {
+func TestDeduplicateConditions(t *testing.T) {
 	g := NewGomegaWithT(t)
 
 	conds := []Condition{

internal/state/listener.go

@@ -1,6 +1,7 @@
 package state
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
@@ -35,7 +36,7 @@ type listenerConfigurator interface {
 }
 
 type listenerConfiguratorFactory struct {
-	https *httpsListenerConfigurator
+	https *httpListenerConfigurator
 	http  *httpListenerConfigurator
 }
 
@@ -60,68 +61,115 @@ func newListenerConfiguratorFactory(
 	}
 }
 
-type httpsListenerConfigurator struct {
+type httpListenerConfigurator struct {
 	gateway         *v1beta1.Gateway
 	secretMemoryMgr SecretDiskMemoryManager
 	usedHostnames   map[string]*listener
+	validate        func(gl v1beta1.Listener) []conditions.Condition
+}
+
+func newHTTPListenerConfigurator(gw *v1beta1.Gateway) *httpListenerConfigurator {
+	return &httpListenerConfigurator{
+		usedHostnames: make(map[string]*listener),
+		gateway:       gw,
+		validate:      validateHTTPListener,
+	}
 }
 
 func newHTTPSListenerConfigurator(
 	gateway *v1beta1.Gateway,
 	secretMemoryMgr SecretDiskMemoryManager,
-) *httpsListenerConfigurator {
-	return &httpsListenerConfigurator{
+) *httpListenerConfigurator {
+	return &httpListenerConfigurator{
 		gateway:         gateway,
 		secretMemoryMgr: secretMemoryMgr,
 		usedHostnames:   make(map[string]*listener),
+		validate: func(gl v1beta1.Listener) []conditions.Condition {
+			return validateHTTPSListener(gl, gateway.Namespace)
+		},
+	}
+}
+
+func buildListener(
+	gl v1beta1.Listener,
+	gw *v1beta1.Gateway,
+	validate func(gl v1beta1.Listener) []conditions.Condition,
+) (l *listener, validHostname bool) {
+	conds := validate(gl)
+
+	if len(gw.Spec.Addresses) > 0 {
+		conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"))
+	}
+
+	validHostnameErr := validateListenerHostname(gl.Hostname)
+	if validHostnameErr != nil {
+		msg := fmt.Sprintf("Invalid hostname: %v", validHostnameErr)
+		conds = append(conds, conditions.NewListenerUnsupportedValue(msg))
 	}
+
+	return &listener{
+		Source:            gl,
+		Valid:             len(conds) == 0,
+		Routes:            make(map[types.NamespacedName]*route),
+		AcceptedHostnames: make(map[string]struct{}),
+		Conditions:        conds,
+	}, validHostnameErr == nil
 }
 
-func (c *httpsListenerConfigurator) configure(gl v1beta1.Listener) *listener {
-	validate := func(gl v1beta1.Listener) []conditions.Condition {
-		return validateHTTPSListener(gl, c.gateway.Namespace)
+func (c *httpListenerConfigurator) ensureUniqueHostnamesAmongListeners(l *listener) {
+	h := getHostname(l.Source.Hostname)
+
+	if holder, exist := c.usedHostnames[h]; exist {
+		l.Valid = false
+
+		holder.Valid = false   // all listeners for the same hostname become conflicted
+		holder.SecretPath = "" // ensure secret path is unset for invalid listeners
+
+		format := "Multiple listeners for the same port use the same hostname %q; " +
+			"ensure only one listener uses that hostname"
+		conflictedConds := conditions.NewListenerConflictedHostname(fmt.Sprintf(format, h))
+
+		holder.Conditions = append(holder.Conditions, conflictedConds...)
+		l.Conditions = append(l.Conditions, conflictedConds...)
+
+		return
 	}
 
-	l := configureListenerAndUseHostname(gl, c.gateway, c.usedHostnames, validate)
+	c.usedHostnames[h] = l
+}
 
+func (c *httpListenerConfigurator) loadSecretIntoListener(l *listener) {
 	if !l.Valid {
-		return l
+		return
 	}
 
 	nsname := types.NamespacedName{
 		Namespace: c.gateway.Namespace,
-		Name:      string(gl.TLS.CertificateRefs[0].Name),
+		Name:      string(l.Source.TLS.CertificateRefs[0].Name),
 	}
 
-	path, err := c.secretMemoryMgr.Request(nsname)
-	if err != nil {
-		l.Valid = false
+	var err error
 
+	l.SecretPath, err = c.secretMemoryMgr.Request(nsname)
+	if err != nil {
 		msg := fmt.Sprintf("Failed to get the certificate %s: %v", nsname.String(), err)
 		l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(msg)...)
-
-		return l
+		l.Valid = false
 	}
-
-	l.SecretPath = path
-
-	return l
 }
 
-type httpListenerConfigurator struct {
-	usedHostnames map[string]*listener
-	gateway       *v1beta1.Gateway
-}
+func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener {
+	l, validHostname := buildListener(gl, c.gateway, c.validate)
 
-func newHTTPListenerConfigurator(gw *v1beta1.Gateway) *httpListenerConfigurator {
-	return &httpListenerConfigurator{
-		usedHostnames: make(map[string]*listener),
-		gateway:       gw,
+	if validHostname {
+		c.ensureUniqueHostnamesAmongListeners(l)
 	}
-}
 
-func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *listener {
-	return configureListenerAndUseHostname(gl, c.gateway, c.usedHostnames, validateHTTPListener)
+	if gl.Protocol == v1beta1.HTTPSProtocolType {
+		c.loadSecretIntoListener(l)
+	}
+
+	return l
 }
 
 type invalidProtocolListenerConfigurator struct{}
@@ -145,60 +193,6 @@ func (c *invalidProtocolListenerConfigurator) configure(gl v1beta1.Listener) *li
 	}
 }
 
-func configureListenerAndUseHostname(
-	gl v1beta1.Listener,
-	gw *v1beta1.Gateway,
-	usedHostnames map[string]*listener,
-	validate func(listener v1beta1.Listener) []conditions.Condition,
-) *listener {
-	conds := validate(gl)
-
-	if len(gw.Spec.Addresses) > 0 {
-		conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"))
-	}
-
-	valid := len(conds) == 0
-
-	// we validate hostnames separately from validate() because we don't want to check for conflicts for
-	// invalid hostnames
-	validHostname, hostnameCond := validateListenerHostname(gl.Hostname)
-	if validHostname {
-		h := getHostname(gl.Hostname)
-
-		if holder, exist := usedHostnames[h]; exist {
-			valid = false
-			holder.Valid = false   // all listeners for the same hostname become conflicted
-			holder.SecretPath = "" // ensure secret path is unset for invalid listeners
-
-			format := "Multiple listeners for the same port use the same hostname %q; " +
-				"ensure only one listener uses that hostname"
-			conflictedConds := conditions.NewListenerConflictedHostname(fmt.Sprintf(format, h))
-			holder.Conditions = append(holder.Conditions, conflictedConds...)
-			conds = append(conds, conflictedConds...)
-		}
-	} else {
-		valid = false
-		conds = append(conds, hostnameCond)
-	}
-
-	l := &listener{
-		Source:            gl,
-		Valid:             valid,
-		Routes:            make(map[types.NamespacedName]*route),
-		AcceptedHostnames: make(map[string]struct{}),
-		Conditions:        conds,
-	}
-
-	if !validHostname {
-		return l
-	}
-
-	h := getHostname(gl.Hostname)
-	usedHostnames[h] = l
-
-	return l
-}
-
 func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition {
 	var conds []conditions.Condition
 
@@ -264,27 +258,27 @@ func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []conditi
 	return conds
 }
 
-func validateListenerHostname(host *v1beta1.Hostname) (valid bool, cond conditions.Condition) {
+func validateListenerHostname(host *v1beta1.Hostname) error {
 	if host == nil {
-		return true, conditions.Condition{}
+		return nil
 	}
 
 	h := string(*host)
 
 	if h == "" {
-		return true, conditions.Condition{}
+		return nil
 	}
 
 	// FIXME(pleshakov): For now, we don't support wildcard hostnames
 	if strings.HasPrefix(h, "*") {
-		return false, conditions.NewListenerUnsupportedValue("Wildcard hostnames are not supported")
+		return fmt.Errorf("wildcard hostnames are not supported")
 	}
 
 	msgs := validation.IsDNS1123Subdomain(h)
 	if len(msgs) > 0 {
 		combined := strings.Join(msgs, ",")
-		return false, conditions.NewListenerUnsupportedValue(fmt.Sprintf("Invalid hostname: %s", combined))
+		return errors.New(combined)
 	}
 
-	return true, conditions.Condition{}
+	return nil
 }

internal/state/listener_test.go

@@ -189,55 +189,48 @@ func TestValidateHTTPSListener(t *testing.T) {
 
 func TestValidateListenerHostname(t *testing.T) {
 	tests := []struct {
-		hostname          *v1beta1.Hostname
-		expectedCondition conditions.Condition
-		name              string
-		expectedValid     bool
+		hostname  *v1beta1.Hostname
+		name      string
+		expectErr bool
 	}{
 		{
-			hostname:          nil,
-			expectedValid:     true,
-			expectedCondition: conditions.Condition{},
-			name:              "nil hostname",
+			hostname:  nil,
+			expectErr: false,
+			name:      "nil hostname",
 		},
 		{
-			hostname:          (*v1beta1.Hostname)(helpers.GetStringPointer("")),
-			expectedValid:     true,
-			expectedCondition: conditions.Condition{},
-			name:              "empty hostname",
+			hostname:  (*v1beta1.Hostname)(helpers.GetStringPointer("")),
+			expectErr: false,
+			name:      "empty hostname",
 		},
 		{
-			hostname:          (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")),
-			expectedValid:     true,
-			expectedCondition: conditions.Condition{},
-			name:              "valid hostname",
+			hostname:  (*v1beta1.Hostname)(helpers.GetStringPointer("foo.example.com")),
+			expectErr: false,
+			name:      "valid hostname",
 		},
 		{
-			hostname:          (*v1beta1.Hostname)(helpers.GetStringPointer("*.example.com")),
-			expectedValid:     false,
-			expectedCondition: conditions.NewListenerUnsupportedValue("Wildcard hostnames are not supported"),
-			name:              "wildcard hostname",
+			hostname:  (*v1beta1.Hostname)(helpers.GetStringPointer("*.example.com")),
+			expectErr: true,
+			name:      "wildcard hostname",
 		},
 		{
-			hostname:      (*v1beta1.Hostname)(helpers.GetStringPointer("example$com")),
-			expectedValid: false,
-			expectedCondition: conditions.NewListenerUnsupportedValue(
-				"Invalid hostname: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric " +
-					"characters, '-' or '.', and must start and end with an alphanumeric character " +
-					"(e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?" +
-					`(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')`),
-			name: "invalid hostname",
+			hostname:  (*v1beta1.Hostname)(helpers.GetStringPointer("example$com")),
+			expectErr: true,
+			name:      "invalid hostname",
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			g := NewGomegaWithT(t)
 
-			valid, cond := validateListenerHostname(test.hostname)
+			err := validateListenerHostname(test.hostname)
 
-			g.Expect(valid).To(Equal(test.expectedValid))
-			g.Expect(cond).To(Equal(test.expectedCondition))
+			if test.expectErr {
+				g.Expect(err).ToNot(BeNil())
+			} else {
+				g.Expect(err).To(BeNil())
+			}
 		})
 	}
 }

internal/state/statuses.go

@@ -99,6 +99,8 @@ func buildStatuses(graph *graph) Statuses {
 		for name, l := range graph.Gateway.Listeners {
 			conds := make([]conditions.Condition, 0, len(l.Conditions)+len(defaultConds)+1) // 1 is for missing GC
 
+			// We add default conds first, so that any additional conditions will override them, which is
+			// ensured by DeduplicateConditions.
 			conds = append(conds, defaultConds...)
 			conds = append(conds, l.Conditions...)
 
@@ -140,6 +142,8 @@ func buildStatuses(graph *graph) Statuses {
 		for ref, cond := range r.InvalidSectionNameRefs {
 			baseConds := buildBaseRouteConditions(gcValidAndExist)
 
+			// We add baseConds first, so that any additional conditions will override them, which is
+			// ensured by DeduplicateConditions.
 			conds := make([]conditions.Condition, 0, len(baseConds)+1)
 			conds = append(conds, baseConds...)
 			conds = append(conds, cond)