Add initial set of CEL validations for HTTPRoute #2253

gauravkghildiyal · 2023-07-31T07:25:56Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add initial set of CEL validations for HTTPRoute.

This includes most (but not all) of the HTTPRoute validations offered by the webhook.

What remains?

Only 2 kinds of validations remain.

The validations which have not been ported along with the reason and details are available in https://github.com/gauravkghildiyal/gateway-api/blob/pending-httproute-validations/apis/v1beta1/validation/httproute.go. All validations which were successfully ported have been removed from this file.

1 "Namespace" is a reserved word in CEL.

Since namespace is a reserved word, a validation that involves a field named namespace seems to getting rejected. We need to figure out if there is a way around this.

2 Estimated cost limit exceeds threshold

CEL attempts to estimate the runtime cost of a validation. Validations that require O(n^2) algorithms on larger cardinality fields end up crossing the allowed threshold. One such place is within []gatewayv1b1.HTTPHeaderMatch where we want to ensure unique (case-insensitive) header names.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Introduce validations through CEL for HTTPRoute.

linux-foundation-easycla · 2023-07-31T15:38:20Z

The committers listed above are authorized under a signed CLA.

✅ login: gauravkghildiyal / name: Gaurav Kumar Ghildiyal (a20f261)

gauravkghildiyal · 2023-07-31T16:06:54Z

/check-cla

gauravkghildiyal · 2023-08-03T06:02:50Z

Added another validation which was blocked on figuring out some regexp parsing behaviour. Now only two category of validations remain:

Seems like CEL might consider namespace field to be a reserved word. This seems to be blocking validations that involve the any field named namespace

The second category of validations remaining are still the "cost exceeded" kind.

(Will update the description with latest details)

gauravkghildiyal · 2023-08-03T21:03:33Z

@jpbetz Offered some really GREAT insights into the problems we're facing.

Problem 1 can be solved by https://kubernetes.io/docs/reference/using-api/cel/#escaping. To fully work, it requires some hacks like the custom generation logic used in Completing CEL validation for Gateway #2246 so I will prepare a separate PR for that.
Problem 2 seems like a bug which Joe has raised an issue for: CEL Estimated cost incorrect for lowerAscii kubernetes/kubernetes#119749

robscott

Thanks @gauravkghildiyal, this is really great! A few small nits, but overall both the validation and corresponding tests look remarkably thorough.

robscott · 2023-08-03T23:33:51Z

apis/v1beta1/object_reference_types.go

@@ -91,6 +91,8 @@ type SecretObjectReference struct {
 // References to objects with invalid Group and Kind are not valid, and must
 // be rejected by the implementation, with appropriate Conditions set
 // on the containing object.
+//
+// +kubebuilder:validation:XValidation:message="Must have port for Service reference",rule="((!has(self.group) || size(self.group) == 0) && (!has(self.kind) || self.kind == 'Service')) ? has(self.port) : true"


Nit: In my experimenting I thought that defaults applied before CEL validation so it may be possible to simplify this. With that said, I could also be remembering wrong here. In any case this is not a blocker.

Yes you are absolutely right! Sorry didn't notice the defaults. Appreciate the closer look :)

robscott · 2023-08-03T23:50:06Z

hack/cel-validation/httproute_test.go

+			},
+		},
+		{
+			name:       "invalid because multipler filters are repeated",


Suggested change

name: "invalid because multipler filters are repeated",

name: "invalid because multiple filters are repeated",

Thanks! Changed.

robscott · 2023-08-03T23:51:01Z

hack/cel-validation/httproute_test.go

+			},
+		},
+		{
+			name: "valie ReplacePrefixMatch",


Suggested change

name: "valie ReplacePrefixMatch",

name: "valid ReplacePrefixMatch",

Thanks! Changed

robscott · 2023-08-03T23:57:33Z

config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml

+                              - message: filter.RequestHeaderModifier must be nil
+                                  if the HTTPRouteFilter.Type is not RequestHeaderModifier


I don't think we've been very consistent with this, but we should try to match the upstream pattern here where field names/paths match what a user would see when using YAML. For example, look at the core validation here: https://github.com/kubernetes/kubernetes/blob/2c6c4566eff972d6c1320b5f8ad795f88c822d09/pkg/apis/core/validation/validation.go#L1041-L1045.

This comment applies throughout the PR, but will not repeat it everywhere to reduce noise.

Suggested change

- message: filter.RequestHeaderModifier must be nil

if the HTTPRouteFilter.Type is not RequestHeaderModifier

- message: filter.requestHeaderModifier must be nil

if the HTTPRouteFilter.Type is not RequestHeaderModifier

Sure can do! I've changed all such occurrences. The places which are still kept capitalized have the intent of referring to the enum (like RequestRedirect) instead of the field (requestRedirect). If in some place, the intent is not clear enough, I can try and reframe the statement.

gauravkghildiyal

Appreciate the thorough reviews!

gauravkghildiyal · 2023-08-04T04:01:14Z

hack/cel-validation/httproute_test.go

+			},
+		},
+		{
+			name: "valie ReplacePrefixMatch",


Thanks! Changed

gauravkghildiyal · 2023-08-04T04:01:38Z

hack/cel-validation/httproute_test.go

+			},
+		},
+		{
+			name:       "invalid because multipler filters are repeated",


Thanks! Changed.

gauravkghildiyal · 2023-08-04T04:05:38Z

apis/v1beta1/object_reference_types.go

@@ -91,6 +91,8 @@ type SecretObjectReference struct {
 // References to objects with invalid Group and Kind are not valid, and must
 // be rejected by the implementation, with appropriate Conditions set
 // on the containing object.
+//
+// +kubebuilder:validation:XValidation:message="Must have port for Service reference",rule="((!has(self.group) || size(self.group) == 0) && (!has(self.kind) || self.kind == 'Service')) ? has(self.port) : true"


Yes you are absolutely right! Sorry didn't notice the defaults. Appreciate the closer look :)

gauravkghildiyal · 2023-08-04T04:43:54Z

config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml

+                              - message: filter.RequestHeaderModifier must be nil
+                                  if the HTTPRouteFilter.Type is not RequestHeaderModifier


Sure can do! I've changed all such occurrences. The places which are still kept capitalized have the intent of referring to the enum (like RequestRedirect) instead of the field (requestRedirect). If in some place, the intent is not clear enough, I can try and reframe the statement.

robscott

Thanks @gauravkghildiyal!

/lgtm
/approve

k8s-ci-robot · 2023-08-04T17:01:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauravkghildiyal, robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 31, 2023

k8s-ci-robot requested review from robscott and shaneutt July 31, 2023 07:26

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 31, 2023

gauravkghildiyal force-pushed the cel-validation branch 2 times, most recently from 27f092f to a8ea615 Compare July 31, 2023 15:38

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 31, 2023

gauravkghildiyal force-pushed the cel-validation branch from a8ea615 to 79f800e Compare July 31, 2023 16:17

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 31, 2023

robscott mentioned this pull request Jul 31, 2023

Updating GEP-1742 to clarify timeout formatting #2155

Closed

robscott added this to the v0.8.0 milestone Aug 1, 2023

gauravkghildiyal force-pushed the cel-validation branch from 79f800e to adb94d2 Compare August 3, 2023 05:57

robscott reviewed Aug 3, 2023

View reviewed changes

Add initial set of CEL validations for HTTPRoute

a20f261

gauravkghildiyal force-pushed the cel-validation branch from adb94d2 to a20f261 Compare August 4, 2023 04:38

gauravkghildiyal commented Aug 4, 2023

View reviewed changes

robscott reviewed Aug 4, 2023

View reviewed changes

k8s-ci-robot assigned robscott Aug 4, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2023

k8s-ci-robot merged commit f638d3f into kubernetes-sigs:main Aug 4, 2023

gauravkghildiyal deleted the cel-validation branch August 4, 2023 23:10

guicassolato mentioned this pull request Oct 31, 2023

GEP: Add Name to HTTPRouteRule and HTTPRouteMatch #995

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial set of CEL validations for HTTPRoute #2253

Add initial set of CEL validations for HTTPRoute #2253

gauravkghildiyal commented Jul 31, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jul 31, 2023 •

edited

Loading

gauravkghildiyal commented Jul 31, 2023

gauravkghildiyal commented Aug 3, 2023

gauravkghildiyal commented Aug 3, 2023

robscott left a comment

robscott Aug 3, 2023

gauravkghildiyal Aug 4, 2023

robscott Aug 3, 2023

gauravkghildiyal Aug 4, 2023

robscott Aug 3, 2023

gauravkghildiyal Aug 4, 2023

robscott Aug 3, 2023

gauravkghildiyal Aug 4, 2023

gauravkghildiyal left a comment

gauravkghildiyal Aug 4, 2023

gauravkghildiyal Aug 4, 2023

gauravkghildiyal Aug 4, 2023

gauravkghildiyal Aug 4, 2023

robscott left a comment

k8s-ci-robot commented Aug 4, 2023

	name: "invalid because multipler filters are repeated",
	name: "invalid because multiple filters are repeated",

	name: "valie ReplacePrefixMatch",
	name: "valid ReplacePrefixMatch",

		- message: filter.RequestHeaderModifier must be nil
		if the HTTPRouteFilter.Type is not RequestHeaderModifier

Add initial set of CEL validations for HTTPRoute #2253

Add initial set of CEL validations for HTTPRoute #2253

Conversation

gauravkghildiyal commented Jul 31, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

What remains?

1 "Namespace" is a reserved word in CEL.

2 Estimated cost limit exceeds threshold

linux-foundation-easycla bot commented Jul 31, 2023 • edited Loading

gauravkghildiyal commented Jul 31, 2023

gauravkghildiyal commented Aug 3, 2023

gauravkghildiyal commented Aug 3, 2023

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravkghildiyal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 4, 2023

gauravkghildiyal commented Jul 31, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jul 31, 2023 •

edited

Loading