Improve Gateway validation error messages (#503)

The PR #455
brought NKG-specific validation for HTTPRoutes. The implementation uses
https://pkg.go.dev/k8s.io/apimachinery/pkg/util/validation/field 
to generate validation errors.

This commit makes generated Gateway-related errors consistent with
HTTPRoute-related errors by starting using that package above.

Fixes #473

Author: pleshakov

internal/state/graph/gateway.go

@@ -5,6 +5,7 @@ import (
 	"sort"
 
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/gateway-api/apis/v1beta1"
 
@@ -200,13 +201,16 @@ func validateListener(
 	conds = validate(gl)
 
 	if len(gw.Spec.Addresses) > 0 {
-		conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"))
+		path := field.NewPath("spec", "addresses")
+		valErr := field.Forbidden(path, "addresses are not supported")
+		conds = append(conds, conditions.NewListenerUnsupportedAddress(valErr.Error()))
 	}
 
 	validHostnameErr := validateListenerHostname(gl.Hostname)
 	if validHostnameErr != nil {
-		msg := fmt.Sprintf("Invalid hostname: %v", validHostnameErr)
-		conds = append(conds, conditions.NewListenerUnsupportedValue(msg))
+		path := field.NewPath("hostname")
+		valErr := field.Invalid(path, gl.Hostname, validHostnameErr.Error())
+		conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error()))
 	}
 
 	return conds, validHostnameErr == nil
@@ -248,13 +252,21 @@ func (c *httpListenerConfigurator) loadSecretIntoListener(l *Listener) {
 
 	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)...)
+		path := field.NewPath("tls", "certificateRefs").Index(0)
+		// field.NotFound could be better, but it doesn't allow us to set the error message.
+		valErr := field.Invalid(path, nsname, err.Error())
+
+		l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(valErr.Error())...)
 		l.Valid = false
 	}
 }
 
 func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *Listener {
+	// The functions called by configure() generate conditions for invalid fields of the listener.
+	// Because the Gateway status includes a status field for each listener, the messages in those conditions
+	// don't need to include the full path to the field (e.g. "spec.listeners[0].hostname"). They will include
+	// a path starting from the field of a listener (e.g. "hostname", "tls.options").
+
 	conds, validHostname := validateListener(gl, c.gateway, c.validate)
 
 	l := &Listener{
@@ -283,16 +295,19 @@ func newInvalidProtocolListenerConfigurator() *invalidProtocolListenerConfigurat
 }
 
 func (c *invalidProtocolListenerConfigurator) configure(gl v1beta1.Listener) *Listener {
-	msg := fmt.Sprintf("Protocol %q is not supported, use %q or %q",
-		gl.Protocol, v1beta1.HTTPProtocolType, v1beta1.HTTPSProtocolType)
+	valErr := field.NotSupported(
+		field.NewPath("protocol"),
+		gl.Protocol,
+		[]string{string(v1beta1.HTTPProtocolType), string(v1beta1.HTTPSProtocolType)},
+	)
 
 	return &Listener{
 		Source:            gl,
 		Valid:             false,
 		Routes:            make(map[types.NamespacedName]*Route),
 		AcceptedHostnames: make(map[string]struct{}),
 		Conditions: []conditions.Condition{
-			conditions.NewListenerUnsupportedProtocol(msg),
+			conditions.NewListenerUnsupportedProtocol(valErr.Error()),
 		},
 	}
 }
@@ -301,8 +316,9 @@ func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition {
 	var conds []conditions.Condition
 
 	if listener.Port != 80 {
-		msg := fmt.Sprintf("Port %d is not supported for HTTP, use 80", listener.Port)
-		conds = append(conds, conditions.NewListenerPortUnavailable(msg))
+		path := field.NewPath("port")
+		valErr := field.NotSupported(path, listener.Port, []string{"80"})
+		conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error()))
 	}
 
 	// The imported Webhook validation ensures the tls field is not set for an HTTP listener.
@@ -315,48 +331,63 @@ func validateHTTPSListener(listener v1beta1.Listener, gwNsName string) []conditi
 	var conds []conditions.Condition
 
 	if listener.Port != 443 {
-		msg := fmt.Sprintf("Port %d is not supported for HTTPS, use 443", listener.Port)
-		conds = append(conds, conditions.NewListenerPortUnavailable(msg))
+		path := field.NewPath("port")
+		valErr := field.NotSupported(path, listener.Port, []string{"443"})
+		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.
 
+	tlsPath := field.NewPath("tls")
+
 	if *listener.TLS.Mode != v1beta1.TLSModeTerminate {
-		msg := fmt.Sprintf("tls.mode %q is not supported, use %q", *listener.TLS.Mode, v1beta1.TLSModeTerminate)
-		conds = append(conds, conditions.NewListenerUnsupportedValue(msg))
+		valErr := field.NotSupported(
+			tlsPath.Child("mode"),
+			*listener.TLS.Mode,
+			[]string{string(v1beta1.TLSModeTerminate)},
+		)
+		conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error()))
 	}
 
 	if len(listener.TLS.Options) > 0 {
-		conds = append(conds, conditions.NewListenerUnsupportedValue("tls.options are not supported"))
+		path := tlsPath.Child("options")
+		valErr := field.Forbidden(path, "options are not supported")
+		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.
 
 	certRef := listener.TLS.CertificateRefs[0]
 
+	certRefPath := tlsPath.Child("certificateRefs").Index(0)
+
 	if certRef.Kind != nil && *certRef.Kind != "Secret" {
-		msg := fmt.Sprintf("Kind must be Secret, got %q", *certRef.Kind)
-		conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...)
+		path := certRefPath.Child("kind")
+		valErr := field.NotSupported(path, *certRef.Kind, []string{"Secret"})
+		conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...)
 	}
 
 	// for Kind Secret, certRef.Group must be nil or empty
 	if certRef.Group != nil && *certRef.Group != "" {
-		msg := fmt.Sprintf("Group must be empty, got %q", *certRef.Group)
-		conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...)
+		path := certRefPath.Child("group")
+		valErr := field.NotSupported(path, *certRef.Group, []string{""})
+		conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...)
 	}
 
 	// secret must be in the same namespace as the gateway
 	if certRef.Namespace != nil && string(*certRef.Namespace) != gwNsName {
-		const msg = "Referenced Secret must belong to the same namespace as the Gateway"
-		conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...)
-
+		const detail = "Referenced Secret must belong to the same namespace as the Gateway"
+		path := certRefPath.Child("namespace")
+		valErr := field.Invalid(path, certRef.Namespace, detail)
+		conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...)
 	}
 
 	if l := len(listener.TLS.CertificateRefs); l > 1 {
-		msg := fmt.Sprintf("Only 1 certificateRef is supported, got %d", l)
-		conds = append(conds, conditions.NewListenerUnsupportedValue(msg))
+		path := tlsPath.Child("certificateRefs")
+		valErr := field.TooMany(path, l, 1)
+		conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error()))
 	}
 
 	return conds

internal/state/graph/gateway_test.go

@@ -244,7 +244,7 @@ func TestBuildGateway(t *testing.T) {
 	}
 
 	const (
-		invalidHostnameMsg = "Invalid hostname: a lowercase RFC 1123 subdomain " +
+		invalidHostnameMsg = `hostname: Invalid value: "$example.com": 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])?)*')`
@@ -325,8 +325,9 @@ func TestBuildGateway(t *testing.T) {
 						Routes:            map[types.NamespacedName]*Route{},
 						AcceptedHostnames: map[string]struct{}{},
 						Conditions: []conditions.Condition{
-							conditions.NewListenerUnsupportedProtocol(`Protocol "TCP" is not supported, use "HTTP" ` +
-								`or "HTTPS"`),
+							conditions.NewListenerUnsupportedProtocol(
+								`protocol: Unsupported value: "TCP": supported values: "HTTP", "HTTPS"`,
+							),
 						},
 					},
 				},
@@ -344,7 +345,7 @@ func TestBuildGateway(t *testing.T) {
 						Routes:            map[types.NamespacedName]*Route{},
 						AcceptedHostnames: map[string]struct{}{},
 						Conditions: []conditions.Condition{
-							conditions.NewListenerPortUnavailable("Port 81 is not supported for HTTP, use 80"),
+							conditions.NewListenerPortUnavailable(`port: Unsupported value: 81: supported values: "80"`),
 						},
 					},
 				},
@@ -362,7 +363,7 @@ func TestBuildGateway(t *testing.T) {
 						Routes:            map[types.NamespacedName]*Route{},
 						AcceptedHostnames: map[string]struct{}{},
 						Conditions: []conditions.Condition{
-							conditions.NewListenerPortUnavailable("Port 444 is not supported for HTTPS, use 443"),
+							conditions.NewListenerPortUnavailable(`port: Unsupported value: 444: supported values: "443"`),
 						},
 					},
 				},
@@ -406,8 +407,9 @@ func TestBuildGateway(t *testing.T) {
 						Valid:             false,
 						Routes:            map[types.NamespacedName]*Route{},
 						AcceptedHostnames: map[string]struct{}{},
-						Conditions: conditions.NewListenerInvalidCertificateRef("Failed to get the certificate " +
-							"test/does-not-exist: secret not found"),
+						Conditions: conditions.NewListenerInvalidCertificateRef(
+							`tls.certificateRefs[0]: Invalid value: test/does-not-exist: secret not found`,
+						),
 					},
 				},
 			},
@@ -505,7 +507,9 @@ func TestBuildGateway(t *testing.T) {
 						Routes:            map[types.NamespacedName]*Route{},
 						AcceptedHostnames: map[string]struct{}{},
 						Conditions: []conditions.Condition{
-							conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"),
+							conditions.NewListenerUnsupportedAddress(
+								"spec.addresses: Forbidden: addresses are not supported",
+							),
 						},
 					},
 					"listener-443-1": {
@@ -515,7 +519,9 @@ func TestBuildGateway(t *testing.T) {
 						AcceptedHostnames: map[string]struct{}{},
 						SecretPath:        "",
 						Conditions: []conditions.Condition{
-							conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"),
+							conditions.NewListenerUnsupportedAddress(
+								"spec.addresses: Forbidden: addresses are not supported",
+							),
 						},
 					},
 				},
@@ -564,7 +570,7 @@ func TestValidateHTTPListener(t *testing.T) {
 				Port: 81,
 			},
 			expected: []conditions.Condition{
-				conditions.NewListenerPortUnavailable("Port 81 is not supported for HTTP, use 80"),
+				conditions.NewListenerPortUnavailable(`port: Unsupported value: 81: supported values: "80"`),
 			},
 			name: "invalid port",
 		},
@@ -633,7 +639,7 @@ func TestValidateHTTPSListener(t *testing.T) {
 				},
 			},
 			expected: []conditions.Condition{
-				conditions.NewListenerPortUnavailable("Port 80 is not supported for HTTPS, use 443"),
+				conditions.NewListenerPortUnavailable(`port: Unsupported value: 80: supported values: "443"`),
 			},
 			name: "invalid port",
 		},
@@ -647,7 +653,7 @@ func TestValidateHTTPSListener(t *testing.T) {
 				},
 			},
 			expected: []conditions.Condition{
-				conditions.NewListenerUnsupportedValue("tls.options are not supported"),
+				conditions.NewListenerUnsupportedValue("tls.options: Forbidden: options are not supported"),
 			},
 			name: "invalid options",
 		},
@@ -660,7 +666,9 @@ func TestValidateHTTPSListener(t *testing.T) {
 				},
 			},
 			expected: []conditions.Condition{
-				conditions.NewListenerUnsupportedValue(`tls.mode "Passthrough" is not supported, use "Terminate"`),
+				conditions.NewListenerUnsupportedValue(
+					`tls.mode: Unsupported value: "Passthrough": supported values: "Terminate"`,
+				),
 			},
 			name: "invalid tls mode",
 		},
@@ -672,8 +680,10 @@ func TestValidateHTTPSListener(t *testing.T) {
 					CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefGroup},
 				},
 			},
-			expected: conditions.NewListenerInvalidCertificateRef(`Group must be empty, got "some-group"`),
-			name:     "invalid cert ref group",
+			expected: conditions.NewListenerInvalidCertificateRef(
+				`tls.certificateRefs[0].group: Unsupported value: "some-group": supported values: ""`,
+			),
+			name: "invalid cert ref group",
 		},
 		{
 			l: v1beta1.Listener{
@@ -683,8 +693,10 @@ func TestValidateHTTPSListener(t *testing.T) {
 					CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefKind},
 				},
 			},
-			expected: conditions.NewListenerInvalidCertificateRef(`Kind must be Secret, got "ConfigMap"`),
-			name:     "invalid cert ref kind",
+			expected: conditions.NewListenerInvalidCertificateRef(
+				`tls.certificateRefs[0].kind: Unsupported value: "ConfigMap": supported values: "Secret"`,
+			),
+			name: "invalid cert ref kind",
 		},
 		{
 			l: v1beta1.Listener{
@@ -694,8 +706,10 @@ func TestValidateHTTPSListener(t *testing.T) {
 					CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefTNamespace},
 				},
 			},
-			expected: conditions.NewListenerInvalidCertificateRef("Referenced Secret must belong to the same " +
-				"namespace as the Gateway"),
+			expected: conditions.NewListenerInvalidCertificateRef(
+				`tls.certificateRefs[0].namespace: Invalid value: "diff-ns": Referenced Secret must belong to ` +
+					`the same namespace as the Gateway`,
+			),
 			name: "invalid cert ref namespace",
 		},
 		{
@@ -707,7 +721,7 @@ func TestValidateHTTPSListener(t *testing.T) {
 				},
 			},
 			expected: []conditions.Condition{
-				conditions.NewListenerUnsupportedValue("Only 1 certificateRef is supported, got 2"),
+				conditions.NewListenerUnsupportedValue("tls.certificateRefs: Too many: 2: must have at most 1 items"),
 			},
 			name: "too many cert refs",
 		},