Change validation in Duration type in CRDs and NGF (#2525)

bjee19 · web-flow · commit 8fb93bbd6306 · 2024-09-11T13:01:26.000-07:00
Problem: Duration type in crds was limited to being in seconds or milliseconds. This is inconvenient with the keepalive_time nginx directive which defaults to 1 hour.

Solution: Relax the validation to allow for minutes and hours.

Testing: Tested that crds can use minutes and hours for their duration fields.
diff --git a/apis/v1alpha1/shared_types.go b/apis/v1alpha1/shared_types.go
@@ -1,10 +1,11 @@
 package v1alpha1
 
 // Duration is a string value representing a duration in time.
-// Duration can be specified in milliseconds (ms) or seconds (s) A value without a suffix is seconds.
-// Examples: 120s, 50ms.
+// Duration can be specified in milliseconds (ms), seconds (s), minutes (m), hours (h).
+// A value without a suffix is seconds.
+// Examples: 120s, 50ms, 5m, 1h.
 //
-// +kubebuilder:validation:Pattern=`^\d{1,4}(ms|s)?$`
+// +kubebuilder:validation:Pattern=`^[0-9]{1,4}(ms|s|m|h)?$`
 type Duration string
 
 // SpanAttribute is a key value pair to be added to a tracing span.
diff --git a/config/crd/bases/gateway.nginx.org_clientsettingspolicies.yaml b/config/crd/bases/gateway.nginx.org_clientsettingspolicies.yaml
@@ -70,7 +70,7 @@ spec:
                       If a client does not transmit anything within this time, the request is terminated with the
                       408 (Request Time-out) error.
                       Default: https://nginx.org/en/docs/http/ngx_http_core_module.html#client_body_timeout.
-                    pattern: ^\d{1,4}(ms|s)?$
+                    pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                     type: string
                 type: object
               keepAlive:
@@ -91,21 +91,21 @@ spec:
                       Time defines the maximum time during which requests can be processed through one keep-alive connection.
                       After this time is reached, the connection is closed following the subsequent request processing.
                       Default: https://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_time.
-                    pattern: ^\d{1,4}(ms|s)?$
+                    pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                     type: string
                   timeout:
                     description: Timeout defines the keep-alive timeouts for clients.
                     properties:
                       header:
                         description: 'Header sets the timeout in the "Keep-Alive:
                           timeout=time" response header field.'
-                        pattern: ^\d{1,4}(ms|s)?$
+                        pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                         type: string
                       server:
                         description: |-
                           Server sets the timeout during which a keep-alive client connection will stay open on the server side.
                           Setting this value to 0 disables keep-alive client connections.
-                        pattern: ^\d{1,4}(ms|s)?$
+                        pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                         type: string
                     type: object
                     x-kubernetes-validations:
diff --git a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml
@@ -156,7 +156,7 @@ spec:
                         description: |-
                           Interval is the maximum interval between two exports.
                           Default: https://nginx.org/en/docs/ngx_otel_module.html#otel_exporter
-                        pattern: ^\d{1,4}(ms|s)?$
+                        pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                         type: string
                     required:
                     - endpoint
diff --git a/deploy/crds.yaml b/deploy/crds.yaml
@@ -69,7 +69,7 @@ spec:
                       If a client does not transmit anything within this time, the request is terminated with the
                       408 (Request Time-out) error.
                       Default: https://nginx.org/en/docs/http/ngx_http_core_module.html#client_body_timeout.
-                    pattern: ^\d{1,4}(ms|s)?$
+                    pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                     type: string
                 type: object
               keepAlive:
@@ -90,21 +90,21 @@ spec:
                       Time defines the maximum time during which requests can be processed through one keep-alive connection.
                       After this time is reached, the connection is closed following the subsequent request processing.
                       Default: https://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_time.
-                    pattern: ^\d{1,4}(ms|s)?$
+                    pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                     type: string
                   timeout:
                     description: Timeout defines the keep-alive timeouts for clients.
                     properties:
                       header:
                         description: 'Header sets the timeout in the "Keep-Alive:
                           timeout=time" response header field.'
-                        pattern: ^\d{1,4}(ms|s)?$
+                        pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                         type: string
                       server:
                         description: |-
                           Server sets the timeout during which a keep-alive client connection will stay open on the server side.
                           Setting this value to 0 disables keep-alive client connections.
-                        pattern: ^\d{1,4}(ms|s)?$
+                        pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                         type: string
                     type: object
                     x-kubernetes-validations:
@@ -741,7 +741,7 @@ spec:
                         description: |-
                           Interval is the maximum interval between two exports.
                           Default: https://nginx.org/en/docs/ngx_otel_module.html#otel_exporter
-                        pattern: ^\d{1,4}(ms|s)?$
+                        pattern: ^[0-9]{1,4}(ms|s|m|h)?$
                         type: string
                     required:
                     - endpoint
diff --git a/internal/mode/static/nginx/config/policies/clientsettings/validator_test.go b/internal/mode/static/nginx/config/policies/clientsettings/validator_test.go
@@ -103,13 +103,19 @@ func TestValidator_Validate(t *testing.T) {
 				return p
 			}),
 			expConditions: []conditions.Condition{
-				staticConds.NewPolicyInvalid("[spec.body.timeout: Invalid value: \"invalid\": \\d{1,4}(ms|s)? " +
-					"(e.g. '5ms',  or '10s', regex used for validation is 'must contain a number followed by 'ms' or 's''), " +
-					"spec.keepAlive.time: Invalid value: \"invalid\": \\d{1,4}(ms|s)? (e.g. '5ms',  or '10s', regex used for " +
-					"validation is 'must contain a number followed by 'ms' or 's''), spec.keepAlive.timeout.server: Invalid value: " +
-					"\"invalid\": \\d{1,4}(ms|s)? (e.g. '5ms',  or '10s', regex used for validation is 'must contain a number " +
-					"followed by 'ms' or 's''), spec.keepAlive.timeout.header: Invalid value: \"invalid\": \\d{1,4}(ms|s)? " +
-					"(e.g. '5ms',  or '10s', regex used for validation is 'must contain a number followed by 'ms' or 's'')]"),
+				staticConds.NewPolicyInvalid(
+					"[spec.body.timeout: Invalid value: \"invalid\": ^[0-9]{1,4}(ms|s|m|h)? " +
+						"(e.g. '5ms',  or '10s',  or '500m',  or '1000h', regex used for validation is " +
+						"'must contain an, at most, four digit number followed by 'ms', 's', 'm', or 'h''), " +
+						"spec.keepAlive.time: Invalid value: \"invalid\": ^[0-9]{1,4}(ms|s|m|h)? " +
+						"(e.g. '5ms',  or '10s',  or '500m',  or '1000h', regex used for validation is " +
+						"'must contain an, at most, four digit number followed by 'ms', 's', 'm', or 'h''), " +
+						"spec.keepAlive.timeout.server: Invalid value: \"invalid\": ^[0-9]{1,4}(ms|s|m|h)? " +
+						"(e.g. '5ms',  or '10s',  or '500m',  or '1000h', regex used for validation is " +
+						"'must contain an, at most, four digit number followed by 'ms', 's', 'm', or 'h''), " +
+						"spec.keepAlive.timeout.header: Invalid value: \"invalid\": ^[0-9]{1,4}(ms|s|m|h)? " +
+						"(e.g. '5ms',  or '10s',  or '500m',  or '1000h', regex used for validation is " +
+						"'must contain an, at most, four digit number followed by 'ms', 's', 'm', or 'h'')]"),
 			},
 		},
 		{
diff --git a/internal/mode/static/nginx/config/validation/generic.go b/internal/mode/static/nginx/config/validation/generic.go
@@ -39,8 +39,8 @@ func (GenericValidator) ValidateServiceName(name string) error {
 }
 
 const (
-	durationStringFmt    = `\d{1,4}(ms|s)?`
-	durationStringErrMsg = "must contain a number followed by 'ms' or 's'"
+	durationStringFmt    = `^[0-9]{1,4}(ms|s|m|h)?`
+	durationStringErrMsg = "must contain an, at most, four digit number followed by 'ms', 's', 'm', or 'h'"
 )
 
 var durationStringFmtRegexp = regexp.MustCompile("^" + durationStringFmt + "$")
@@ -51,6 +51,8 @@ func (GenericValidator) ValidateNginxDuration(duration string) error {
 		examples := []string{
 			"5ms",
 			"10s",
+			"500m",
+			"1000h",
 		}
 
 		return errors.New(k8svalidation.RegexError(durationStringFmt, durationStringErrMsg, examples...))
diff --git a/internal/mode/static/nginx/config/validation/generic_test.go b/internal/mode/static/nginx/config/validation/generic_test.go
@@ -56,14 +56,16 @@ func TestValidateNginxDuration(t *testing.T) {
 		`5ms`,
 		`10s`,
 		`123ms`,
+		`5m`,
+		`2h`,
 	)
 
 	testInvalidValuesForSimpleValidator(
 		t,
 		validator.ValidateNginxDuration,
 		`test`,
 		`12345`,
-		`5m`,
+		`5k`,
 	)
 }
 
diff --git a/site/content/reference/api.md b/site/content/reference/api.md
@@ -812,8 +812,9 @@ Support: Gateway, HTTPRoute, GRPCRoute.</p>
 </p>
 <p>
 <p>Duration is a string value representing a duration in time.
-Duration can be specified in milliseconds (ms) or seconds (s) A value without a suffix is seconds.
-Examples: 120s, 50ms.</p>
+Duration can be specified in milliseconds (ms), seconds (s), minutes (m), hours (h).
+A value without a suffix is seconds.
+Examples: 120s, 50ms, 5m, 1h.</p>
 </p>
 <h3 id="gateway.nginx.org/v1alpha1.IPFamilyType">IPFamilyType
 (<code>string</code> alias)</p><a class="headerlink" href="#gateway.nginx.org%2fv1alpha1.IPFamilyType" title="Permanent link">¶</a>

Original file line number	Diff line number	Diff line change
`@@ -39,8 +39,8 @@ func (GenericValidator) ValidateServiceName(name string) error {`
`39`	`39`	`}`
`40`	`40`
`41`	`41`	`const (`
`42`		- durationStringFmt = `\d{1,4}(ms\|s)?`
`43`		`- durationStringErrMsg = "must contain a number followed by 'ms' or 's'"`
	`42`	+ durationStringFmt = `^[0-9]{1,4}(ms\|s\|m\|h)?`
	`43`	`+ durationStringErrMsg = "must contain an, at most, four digit number followed by 'ms', 's', 'm', or 'h'"`
`44`	`44`	`)`
`45`	`45`
`46`	`46`	`var durationStringFmtRegexp = regexp.MustCompile("^" + durationStringFmt + "$")`
`@@ -51,6 +51,8 @@ func (GenericValidator) ValidateNginxDuration(duration string) error {`
`51`	`51`	`examples := []string{`
`52`	`52`	`"5ms",`
`53`	`53`	`"10s",`
	`54`	`+ "500m",`
	`55`	`+ "1000h",`
`54`	`56`	`}`
`55`	`57`
`56`	`58`	`return errors.New(k8svalidation.RegexError(durationStringFmt, durationStringErrMsg, examples...))`
Original file line number	Diff line number	Diff line change
`@@ -56,14 +56,16 @@ func TestValidateNginxDuration(t *testing.T) {`
`56`	`56`	`5ms`,
`57`	`57`	`10s`,
`58`	`58`	`123ms`,
	`59`	+ `5m`,
	`60`	+ `2h`,
`59`	`61`	`)`
`60`	`62`
`61`	`63`	`testInvalidValuesForSimpleValidator(`
`62`	`64`	`t,`
`63`	`65`	`validator.ValidateNginxDuration,`
`64`	`66`	`test`,
`65`	`67`	`12345`,
`66`		- `5m`,
	`68`	+ `5k`,
`67`	`69`	`)`
`68`	`70`	`}`
`69`	`71`