Skip to content

Commit 1fc8942

Browse files
miledxzmiledxz
committed
Improve Gateway validation error messages (nginx#503)
The PR nginx#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 nginx#473
1 parent 3d71b58 commit 1fc8942

File tree

2 files changed

+89
-44
lines changed

2 files changed

+89
-44
lines changed

‎internal/state/graph/gateway.go

+55-24
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"sort"
66

77
"k8s.io/apimachinery/pkg/types"
8+
"k8s.io/apimachinery/pkg/util/validation/field"
89
"sigs.k8s.io/controller-runtime/pkg/client"
910
"sigs.k8s.io/gateway-api/apis/v1beta1"
1011

@@ -200,13 +201,16 @@ func validateListener(
200201
conds = validate(gl)
201202

202203
if len(gw.Spec.Addresses) > 0 {
203-
conds = append(conds, conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"))
204+
path := field.NewPath("spec", "addresses")
205+
valErr := field.Forbidden(path, "addresses are not supported")
206+
conds = append(conds, conditions.NewListenerUnsupportedAddress(valErr.Error()))
204207
}
205208

206209
validHostnameErr := validateListenerHostname(gl.Hostname)
207210
if validHostnameErr != nil {
208-
msg := fmt.Sprintf("Invalid hostname: %v", validHostnameErr)
209-
conds = append(conds, conditions.NewListenerUnsupportedValue(msg))
211+
path := field.NewPath("hostname")
212+
valErr := field.Invalid(path, gl.Hostname, validHostnameErr.Error())
213+
conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error()))
210214
}
211215

212216
return conds, validHostnameErr == nil
@@ -248,13 +252,21 @@ func (c *httpListenerConfigurator) loadSecretIntoListener(l *Listener) {
248252

249253
l.SecretPath, err = c.secretMemoryMgr.Request(nsname)
250254
if err != nil {
251-
msg := fmt.Sprintf("Failed to get the certificate %s: %v", nsname.String(), err)
252-
l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(msg)...)
255+
path := field.NewPath("tls", "certificateRefs").Index(0)
256+
// field.NotFound could be better, but it doesn't allow us to set the error message.
257+
valErr := field.Invalid(path, nsname, err.Error())
258+
259+
l.Conditions = append(l.Conditions, conditions.NewListenerInvalidCertificateRef(valErr.Error())...)
253260
l.Valid = false
254261
}
255262
}
256263

257264
func (c *httpListenerConfigurator) configure(gl v1beta1.Listener) *Listener {
265+
// The functions called by configure() generate conditions for invalid fields of the listener.
266+
// Because the Gateway status includes a status field for each listener, the messages in those conditions
267+
// don't need to include the full path to the field (e.g. "spec.listeners[0].hostname"). They will include
268+
// a path starting from the field of a listener (e.g. "hostname", "tls.options").
269+
258270
conds, validHostname := validateListener(gl, c.gateway, c.validate)
259271

260272
l := &Listener{
@@ -283,16 +295,19 @@ func newInvalidProtocolListenerConfigurator() *invalidProtocolListenerConfigurat
283295
}
284296

285297
func (c *invalidProtocolListenerConfigurator) configure(gl v1beta1.Listener) *Listener {
286-
msg := fmt.Sprintf("Protocol %q is not supported, use %q or %q",
287-
gl.Protocol, v1beta1.HTTPProtocolType, v1beta1.HTTPSProtocolType)
298+
valErr := field.NotSupported(
299+
field.NewPath("protocol"),
300+
gl.Protocol,
301+
[]string{string(v1beta1.HTTPProtocolType), string(v1beta1.HTTPSProtocolType)},
302+
)
288303

289304
return &Listener{
290305
Source: gl,
291306
Valid: false,
292307
Routes: make(map[types.NamespacedName]*Route),
293308
AcceptedHostnames: make(map[string]struct{}),
294309
Conditions: []conditions.Condition{
295-
conditions.NewListenerUnsupportedProtocol(msg),
310+
conditions.NewListenerUnsupportedProtocol(valErr.Error()),
296311
},
297312
}
298313
}
@@ -301,8 +316,9 @@ func validateHTTPListener(listener v1beta1.Listener) []conditions.Condition {
301316
var conds []conditions.Condition
302317

303318
if listener.Port != 80 {
304-
msg := fmt.Sprintf("Port %d is not supported for HTTP, use 80", listener.Port)
305-
conds = append(conds, conditions.NewListenerPortUnavailable(msg))
319+
path := field.NewPath("port")
320+
valErr := field.NotSupported(path, listener.Port, []string{"80"})
321+
conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error()))
306322
}
307323

308324
// 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
315331
var conds []conditions.Condition
316332

317333
if listener.Port != 443 {
318-
msg := fmt.Sprintf("Port %d is not supported for HTTPS, use 443", listener.Port)
319-
conds = append(conds, conditions.NewListenerPortUnavailable(msg))
334+
path := field.NewPath("port")
335+
valErr := field.NotSupported(path, listener.Port, []string{"443"})
336+
conds = append(conds, conditions.NewListenerPortUnavailable(valErr.Error()))
320337
}
321338

322339
// The imported Webhook validation ensures the tls field is not nil for an HTTPS listener.
323340
// FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case.
324341

342+
tlsPath := field.NewPath("tls")
343+
325344
if *listener.TLS.Mode != v1beta1.TLSModeTerminate {
326-
msg := fmt.Sprintf("tls.mode %q is not supported, use %q", *listener.TLS.Mode, v1beta1.TLSModeTerminate)
327-
conds = append(conds, conditions.NewListenerUnsupportedValue(msg))
345+
valErr := field.NotSupported(
346+
tlsPath.Child("mode"),
347+
*listener.TLS.Mode,
348+
[]string{string(v1beta1.TLSModeTerminate)},
349+
)
350+
conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error()))
328351
}
329352

330353
if len(listener.TLS.Options) > 0 {
331-
conds = append(conds, conditions.NewListenerUnsupportedValue("tls.options are not supported"))
354+
path := tlsPath.Child("options")
355+
valErr := field.Forbidden(path, "options are not supported")
356+
conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error()))
332357
}
333358

334359
// The imported Webhook validation ensures len(listener.TLS.Certificates) is not 0.
335360
// FIXME(pleshakov): Add a unit test for the imported Webhook validation code for this case.
336361

337362
certRef := listener.TLS.CertificateRefs[0]
338363

364+
certRefPath := tlsPath.Child("certificateRefs").Index(0)
365+
339366
if certRef.Kind != nil && *certRef.Kind != "Secret" {
340-
msg := fmt.Sprintf("Kind must be Secret, got %q", *certRef.Kind)
341-
conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...)
367+
path := certRefPath.Child("kind")
368+
valErr := field.NotSupported(path, *certRef.Kind, []string{"Secret"})
369+
conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...)
342370
}
343371

344372
// for Kind Secret, certRef.Group must be nil or empty
345373
if certRef.Group != nil && *certRef.Group != "" {
346-
msg := fmt.Sprintf("Group must be empty, got %q", *certRef.Group)
347-
conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...)
374+
path := certRefPath.Child("group")
375+
valErr := field.NotSupported(path, *certRef.Group, []string{""})
376+
conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...)
348377
}
349378

350379
// secret must be in the same namespace as the gateway
351380
if certRef.Namespace != nil && string(*certRef.Namespace) != gwNsName {
352-
const msg = "Referenced Secret must belong to the same namespace as the Gateway"
353-
conds = append(conds, conditions.NewListenerInvalidCertificateRef(msg)...)
354-
381+
const detail = "Referenced Secret must belong to the same namespace as the Gateway"
382+
path := certRefPath.Child("namespace")
383+
valErr := field.Invalid(path, certRef.Namespace, detail)
384+
conds = append(conds, conditions.NewListenerInvalidCertificateRef(valErr.Error())...)
355385
}
356386

357387
if l := len(listener.TLS.CertificateRefs); l > 1 {
358-
msg := fmt.Sprintf("Only 1 certificateRef is supported, got %d", l)
359-
conds = append(conds, conditions.NewListenerUnsupportedValue(msg))
388+
path := tlsPath.Child("certificateRefs")
389+
valErr := field.TooMany(path, l, 1)
390+
conds = append(conds, conditions.NewListenerUnsupportedValue(valErr.Error()))
360391
}
361392

362393
return conds

‎internal/state/graph/gateway_test.go

+34-20
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ func TestBuildGateway(t *testing.T) {
244244
}
245245

246246
const (
247-
invalidHostnameMsg = "Invalid hostname: a lowercase RFC 1123 subdomain " +
247+
invalidHostnameMsg = `hostname: Invalid value: "$example.com": a lowercase RFC 1123 subdomain ` +
248248
"must consist of lower case alphanumeric characters, '-' or '.', and must start and end " +
249249
"with an alphanumeric character (e.g. 'example.com', regex used for validation is " +
250250
`'[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) {
325325
Routes: map[types.NamespacedName]*Route{},
326326
AcceptedHostnames: map[string]struct{}{},
327327
Conditions: []conditions.Condition{
328-
conditions.NewListenerUnsupportedProtocol(`Protocol "TCP" is not supported, use "HTTP" ` +
329-
`or "HTTPS"`),
328+
conditions.NewListenerUnsupportedProtocol(
329+
`protocol: Unsupported value: "TCP": supported values: "HTTP", "HTTPS"`,
330+
),
330331
},
331332
},
332333
},
@@ -344,7 +345,7 @@ func TestBuildGateway(t *testing.T) {
344345
Routes: map[types.NamespacedName]*Route{},
345346
AcceptedHostnames: map[string]struct{}{},
346347
Conditions: []conditions.Condition{
347-
conditions.NewListenerPortUnavailable("Port 81 is not supported for HTTP, use 80"),
348+
conditions.NewListenerPortUnavailable(`port: Unsupported value: 81: supported values: "80"`),
348349
},
349350
},
350351
},
@@ -362,7 +363,7 @@ func TestBuildGateway(t *testing.T) {
362363
Routes: map[types.NamespacedName]*Route{},
363364
AcceptedHostnames: map[string]struct{}{},
364365
Conditions: []conditions.Condition{
365-
conditions.NewListenerPortUnavailable("Port 444 is not supported for HTTPS, use 443"),
366+
conditions.NewListenerPortUnavailable(`port: Unsupported value: 444: supported values: "443"`),
366367
},
367368
},
368369
},
@@ -406,8 +407,9 @@ func TestBuildGateway(t *testing.T) {
406407
Valid: false,
407408
Routes: map[types.NamespacedName]*Route{},
408409
AcceptedHostnames: map[string]struct{}{},
409-
Conditions: conditions.NewListenerInvalidCertificateRef("Failed to get the certificate " +
410-
"test/does-not-exist: secret not found"),
410+
Conditions: conditions.NewListenerInvalidCertificateRef(
411+
`tls.certificateRefs[0]: Invalid value: test/does-not-exist: secret not found`,
412+
),
411413
},
412414
},
413415
},
@@ -505,7 +507,9 @@ func TestBuildGateway(t *testing.T) {
505507
Routes: map[types.NamespacedName]*Route{},
506508
AcceptedHostnames: map[string]struct{}{},
507509
Conditions: []conditions.Condition{
508-
conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"),
510+
conditions.NewListenerUnsupportedAddress(
511+
"spec.addresses: Forbidden: addresses are not supported",
512+
),
509513
},
510514
},
511515
"listener-443-1": {
@@ -515,7 +519,9 @@ func TestBuildGateway(t *testing.T) {
515519
AcceptedHostnames: map[string]struct{}{},
516520
SecretPath: "",
517521
Conditions: []conditions.Condition{
518-
conditions.NewListenerUnsupportedAddress("Specifying Gateway addresses is not supported"),
522+
conditions.NewListenerUnsupportedAddress(
523+
"spec.addresses: Forbidden: addresses are not supported",
524+
),
519525
},
520526
},
521527
},
@@ -564,7 +570,7 @@ func TestValidateHTTPListener(t *testing.T) {
564570
Port: 81,
565571
},
566572
expected: []conditions.Condition{
567-
conditions.NewListenerPortUnavailable("Port 81 is not supported for HTTP, use 80"),
573+
conditions.NewListenerPortUnavailable(`port: Unsupported value: 81: supported values: "80"`),
568574
},
569575
name: "invalid port",
570576
},
@@ -633,7 +639,7 @@ func TestValidateHTTPSListener(t *testing.T) {
633639
},
634640
},
635641
expected: []conditions.Condition{
636-
conditions.NewListenerPortUnavailable("Port 80 is not supported for HTTPS, use 443"),
642+
conditions.NewListenerPortUnavailable(`port: Unsupported value: 80: supported values: "443"`),
637643
},
638644
name: "invalid port",
639645
},
@@ -647,7 +653,7 @@ func TestValidateHTTPSListener(t *testing.T) {
647653
},
648654
},
649655
expected: []conditions.Condition{
650-
conditions.NewListenerUnsupportedValue("tls.options are not supported"),
656+
conditions.NewListenerUnsupportedValue("tls.options: Forbidden: options are not supported"),
651657
},
652658
name: "invalid options",
653659
},
@@ -660,7 +666,9 @@ func TestValidateHTTPSListener(t *testing.T) {
660666
},
661667
},
662668
expected: []conditions.Condition{
663-
conditions.NewListenerUnsupportedValue(`tls.mode "Passthrough" is not supported, use "Terminate"`),
669+
conditions.NewListenerUnsupportedValue(
670+
`tls.mode: Unsupported value: "Passthrough": supported values: "Terminate"`,
671+
),
664672
},
665673
name: "invalid tls mode",
666674
},
@@ -672,8 +680,10 @@ func TestValidateHTTPSListener(t *testing.T) {
672680
CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefGroup},
673681
},
674682
},
675-
expected: conditions.NewListenerInvalidCertificateRef(`Group must be empty, got "some-group"`),
676-
name: "invalid cert ref group",
683+
expected: conditions.NewListenerInvalidCertificateRef(
684+
`tls.certificateRefs[0].group: Unsupported value: "some-group": supported values: ""`,
685+
),
686+
name: "invalid cert ref group",
677687
},
678688
{
679689
l: v1beta1.Listener{
@@ -683,8 +693,10 @@ func TestValidateHTTPSListener(t *testing.T) {
683693
CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefKind},
684694
},
685695
},
686-
expected: conditions.NewListenerInvalidCertificateRef(`Kind must be Secret, got "ConfigMap"`),
687-
name: "invalid cert ref kind",
696+
expected: conditions.NewListenerInvalidCertificateRef(
697+
`tls.certificateRefs[0].kind: Unsupported value: "ConfigMap": supported values: "Secret"`,
698+
),
699+
name: "invalid cert ref kind",
688700
},
689701
{
690702
l: v1beta1.Listener{
@@ -694,8 +706,10 @@ func TestValidateHTTPSListener(t *testing.T) {
694706
CertificateRefs: []v1beta1.SecretObjectReference{invalidSecretRefTNamespace},
695707
},
696708
},
697-
expected: conditions.NewListenerInvalidCertificateRef("Referenced Secret must belong to the same " +
698-
"namespace as the Gateway"),
709+
expected: conditions.NewListenerInvalidCertificateRef(
710+
`tls.certificateRefs[0].namespace: Invalid value: "diff-ns": Referenced Secret must belong to ` +
711+
`the same namespace as the Gateway`,
712+
),
699713
name: "invalid cert ref namespace",
700714
},
701715
{
@@ -707,7 +721,7 @@ func TestValidateHTTPSListener(t *testing.T) {
707721
},
708722
},
709723
expected: []conditions.Condition{
710-
conditions.NewListenerUnsupportedValue("Only 1 certificateRef is supported, got 2"),
724+
conditions.NewListenerUnsupportedValue("tls.certificateRefs: Too many: 2: must have at most 1 items"),
711725
},
712726
name: "too many cert refs",
713727
},

0 commit comments

Comments
 (0)