Skip to content

Improve Gateway validation error messages #503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Improve Gateway validation error messages #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 55 additions & 24 deletions internal/state/graph/gateway.go
View file
Open in desktop
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
Open in desktop
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