Skip to content

Commit

Permalink
Improve Gateway validation error messages (#503)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@pleshakov
pleshakov authored Mar 23, 2023
1 parent c9d912c commit c473aba
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 44 deletions.
79 changes: 55 additions & 24 deletions internal/state/graph/gateway.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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()),
},
}
}
Expand All @@ -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.
Expand All @@ -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
Expand Down
54 changes: 34 additions & 20 deletions internal/state/graph/gateway_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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])?)*')`
Expand Down Expand Up @@ -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"`,
),
},
},
},
Expand All @@ -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"`),
},
},
},
Expand All @@ -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"`),
},
},
},
Expand Down Expand Up @@ -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`,
),
},
},
},
Expand Down Expand Up @@ -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": {
Expand All @@ -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",
),
},
},
},
Expand Down Expand Up @@ -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",
},
Expand Down Expand Up @@ -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",
},
Expand All @@ -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",
},
Expand All @@ -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",
},
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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",
},
{
Expand All @@ -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",
},
Expand Down

0 comments on commit c473aba

Please sign in to comment.