AP Defaults #503
+9,225
−162
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,7 +57,7 @@ type CommonAuthRuleSpec struct { | |
| // At least one selected HTTPRoute rule must match to trigger the auth rule. | ||
| // If no route selectors are specified, the auth rule will be evaluated at all requests to the protected routes. | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=8 | ||
|
There was a problem hiding this comment. Do we need any test to cover this being changed in the future, as it was reduced from 15 to 8? There was a problem hiding this comment. Sure, can add a test. Though, I'm expecting this value to decrease again when the overrides field is added in #464 There was a problem hiding this comment. Cool, and thinking out loud. If we need to increase that number in the future, is the workaround to create a webhook to do the validation and drop the use of the CEL? There was a problem hiding this comment. Added integration test for this. I don't think just simply switching to using validation webhook would work, as I believe this more so relates to using a slice of kuadrant-operator/api/v1beta2/route_selectors.go Lines 13 to 24 in 4f6a138 There was a problem hiding this comment. It's because we're importing Gateway API types and, with those, their CEL validation as well. I guess our best options to avoid this hard limit are:
|
||
| RouteSelectors []RouteSelector `json:"routeSelectors,omitempty"` | ||
| } | ||
|
|
||
|
|
@@ -128,19 +128,42 @@ type CallbackSpec struct { | |
| CommonAuthRuleSpec `json:""` | ||
| } | ||
|
|
||
| // RouteSelectors - implicit default validation | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.routeSelectors)",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules) || !has(self.rules.authentication) || !self.rules.authentication.exists(x, has(self.rules.authentication[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules) || !has(self.rules.metadata) || !self.rules.metadata.exists(x, has(self.rules.metadata[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules) || !has(self.rules.authorization) || !self.rules.authorization.exists(x, has(self.rules.authorization[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules) || !has(self.rules.response) || !has(self.rules.response.success) || !has(self.rules.response.success.headers) || !self.rules.response.success.headers.exists(x, has(self.rules.response.success.headers[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules) || !has(self.rules.response) || !has(self.rules.response.success) || !has(self.rules.response.success.dynamicMetadata) || !self.rules.response.success.dynamicMetadata.exists(x, has(self.rules.response.success.dynamicMetadata[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.rules) || !has(self.rules.callbacks) || !self.rules.callbacks.exists(x, has(self.rules.callbacks[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // RouteSelectors - explicit default validation | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.defaults) || !has(self.defaults.routeSelectors)",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.defaults) || !has(self.defaults.rules) || !has(self.defaults.rules.authentication) || !self.defaults.rules.authentication.exists(x, has(self.defaults.rules.authentication[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.defaults) || !has(self.defaults.rules) || !has(self.defaults.rules.metadata) || !self.defaults.rules.metadata.exists(x, has(self.defaults.rules.metadata[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.defaults) || !has(self.defaults.rules) || !has(self.defaults.rules.authorization) || !self.defaults.rules.authorization.exists(x, has(self.defaults.rules.authorization[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.defaults) || !has(self.defaults.rules) || !has(self.defaults.rules.response) || !has(self.defaults.rules.response.success) || !has(self.defaults.rules.response.success.headers) || !self.defaults.rules.response.success.headers.exists(x, has(self.defaults.rules.response.success.headers[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.defaults) || !has(self.defaults.rules) || !has(self.defaults.rules.response) || !has(self.defaults.rules.response.success) || !has(self.defaults.rules.response.success.dynamicMetadata) || !self.defaults.rules.response.success.dynamicMetadata.exists(x, has(self.defaults.rules.response.success.dynamicMetadata[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.defaults) || !has(self.defaults.rules) || !has(self.defaults.rules.callbacks) || !self.defaults.rules.callbacks.exists(x, has(self.defaults.rules.callbacks[x].routeSelectors))",message="route selectors not supported when targeting a Gateway" | ||
| // Mutual Exclusivity Validation | ||
| // +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && (has(self.routeSelectors) || has(self.patterns) || has(self.when) || has(self.rules)))",message="Implicit and explicit defaults are mutually exclusive" | ||
| type AuthPolicySpec struct { | ||
| // TargetRef identifies an API object to apply policy to. | ||
| // +kubebuilder:validation:XValidation:rule="self.group == 'gateway.networking.k8s.io'",message="Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'" | ||
| // +kubebuilder:validation:XValidation:rule="self.kind == 'HTTPRoute' || self.kind == 'Gateway'",message="Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'" | ||
| TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"` | ||
|
|
||
| // Defaults define explicit default values for this policy and for policies inheriting this policy. | ||
| // Defaults are mutually exclusive with implicit defaults defined by AuthPolicyCommonSpec. | ||
| // +optional | ||
| Defaults *AuthPolicyCommonSpec `json:"defaults,omitempty"` | ||
|
|
||
| // AuthPolicyCommonSpec defines implicit default values for this policy and for policies inheriting this policy. | ||
| // AuthPolicyCommonSpec is mutually exclusive with explicit defaults defined by Defaults. | ||
| AuthPolicyCommonSpec `json:""` | ||
| } | ||
|
|
||
| // AuthPolicyCommonSpec contains common shared fields for defaults and overrides | ||
| type AuthPolicyCommonSpec struct { | ||
| // Top-level route selectors. | ||
| // If present, the elements will be used to select HTTPRoute rules that, when activated, trigger the external authorization service. | ||
| // At least one selected HTTPRoute rule must match to trigger the AuthPolicy. | ||
|
|
@@ -161,13 +184,13 @@ type AuthPolicySpec struct { | |
|
|
||
| // The auth rules of the policy. | ||
| // See Authorino's AuthConfig CRD for more details. | ||
| AuthScheme *AuthSchemeSpec `json:"rules,omitempty"` | ||
|
There was a problem hiding this comment. Switched to pointer so that this will not show on json |
||
| } | ||
|
|
||
| // GetRouteSelectors returns the top-level route selectors of the auth scheme. | ||
| // impl: RouteSelectorsGetter | ||
| func (c AuthPolicyCommonSpec) GetRouteSelectors() []RouteSelector { | ||
| return c.RouteSelectors | ||
| } | ||
|
|
||
| type AuthPolicyStatus struct { | ||
|
|
@@ -262,28 +285,36 @@ func (ap *AuthPolicy) GetRulesHostnames() (ruleHosts []string) { | |
| } | ||
| } | ||
|
|
||
| appendCommonSpecRuleHosts := func(c *AuthPolicyCommonSpec) { | ||
| if c.AuthScheme == nil { | ||
| return | ||
| } | ||
|
|
||
| for _, config := range c.AuthScheme.Authentication { | ||
| appendRuleHosts(config) | ||
| } | ||
| for _, config := range c.AuthScheme.Metadata { | ||
| appendRuleHosts(config) | ||
| } | ||
| for _, config := range c.AuthScheme.Authorization { | ||
| appendRuleHosts(config) | ||
| } | ||
| if response := c.AuthScheme.Response; response != nil { | ||
| for _, config := range response.Success.Headers { | ||
| appendRuleHosts(config) | ||
| } | ||
| for _, config := range response.Success.DynamicMetadata { | ||
| appendRuleHosts(config) | ||
| } | ||
| } | ||
| for _, config := range c.AuthScheme.Callbacks { | ||
| appendRuleHosts(config) | ||
| } | ||
| } | ||
|
|
||
| appendRuleHosts(ap.Spec.CommonSpec()) | ||
| appendCommonSpecRuleHosts(ap.Spec.CommonSpec()) | ||
|
|
||
| return | ||
| } | ||
|
|
||
|
|
@@ -299,6 +330,14 @@ func (ap *AuthPolicy) DirectReferenceAnnotationName() string { | |
| return AuthPolicyDirectReferenceAnnotationName | ||
| } | ||
|
|
||
| func (ap *AuthPolicySpec) CommonSpec() *AuthPolicyCommonSpec { | ||
| if ap.Defaults != nil { | ||
| return ap.Defaults | ||
| } | ||
|
|
||
| return &ap.AuthPolicyCommonSpec | ||
| } | ||
|
|
||
| //+kubebuilder:object:root=true | ||
|
|
||
| // AuthPolicyList contains a list of AuthPolicy | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure will this be an issue for OLM upgrading CRDs. I'm wishful that this will not be an issue for OLM, and this is just a local issue 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AuthPolicy CRD itself has reached ~600 Kb already with this change. IIRC, by default, the maximum request size accepted by etcd is 1.5 Mb – actual value size between 2 Mb (default) and 8 Mb (recommended maximum.)