AP Defaults #503

KevFan · 2024-03-21T13:27:11Z

Description

Closes: #462

Adds the defaults field to the AuthPolicy spec. This field is mutually exclusive with the pre-existing fields for defining auth rules in the bare spec.

Verification

The explicit default path is already tests by integration tests but it you want to verify manually:

Follow the guide at https://github.com/Kuadrant/kuadrant-operator/blob/main/doc/user-guides/authenticated-rl-for-app-developers.md and ensure you can complete it successfully
Repeat the guide but at step 3, use the following AuthPolicy that defines explicit defaults instead:

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  defaults:
    rules:
      authentication:
        "api-key-users":
          apiKey:
            selector:
              matchLabels:
                app: toystore
            allNamespaces: true
          credentials:
            authorizationHeader:
              prefix: APIKEY
      response:
        success:
          dynamicMetadata:
            "identity":
              json:
                properties:
                  "userid":
                    selector: auth.identity.metadata.annotations.secret\.kuadrant\.io/user-id
EOF

Verify you can complete the guide successfully
Try creating AuthPolicy with both implicit and explicit defaults

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  defaults:
    rules:
      authentication:
        "api-key-users":
          apiKey:
            selector:
              matchLabels:
                app: toystore
            allNamespaces: true
          credentials:
            authorizationHeader:
              prefix: APIKEY
  rules:
    authentication:
      "api-key-users":
        apiKey:
          selector:
            matchLabels:
              app: toystore
          allNamespaces: true
        credentials:
          authorizationHeader:
            prefix: APIKEY
EOF

Verify policy is rejected due to:

The AuthPolicy "toystore" is invalid: spec: Invalid value: "object": Implicit and explicit defaults are mutually exclusive

KevFan · 2024-03-21T15:40:36Z

api/v1beta2/authpolicy_types.go

+	// Defaults define explicit default values for this policy and for policies inheriting this policy.
+	// Defaults are mutually exclusive with implicit defaults defined by CommonSpec.
+	// +optional
+	Defaults *CommonSpec `json:"defaults"`
+
+	// CommonSpec defines implicit default values for this policy and for policies inheriting this policy.
+	// CommonSpec is mutually exclusive with explicit defaults defined by Defaults.
+	CommonSpec `json:""`
+}
+
+// CommonSpec contains common shared fields for defaults and overrides
+type CommonSpec struct {


This gives issues with CEL due to the cost of route selector validation is and size of the CRD

The CustomResourceDefinition "authpolicies.kuadrant.io" is invalid: * metadata.annotations: Too long: must have at most 262144 bytes * spec.validation.openAPIV3Schema.properties[spec].properties[rules].properties[authentication].additionalProperties.properties[routeSelectors].items.properties[matches].items.properties[path].x-kubernetes-validations[10].rule: Forbidden: contributed to estimated rule cost total exceeding cost limit for entire OpenAPIv3 schema * spec.validation.openAPIV3Schema.properties[spec].properties[rules].properties[authorization].additionalProperties.properties[routeSelectors].items.properties[matches].items.properties[path].x-kubernetes-validations[10].rule: Forbidden: contributed to estimated rule cost total exceeding cost limit for entire OpenAPIv3 schema * spec.validation.openAPIV3Schema.properties[spec].properties[rules].properties[callbacks].additionalProperties.properties[routeSelectors].items.properties[matches].items.properties[path].x-kubernetes-validations[10].rule: Forbidden: contributed to estimated rule cost total exceeding cost limit for entire OpenAPIv3 schema * spec.validation.openAPIV3Schema.properties[spec].properties[rules].properties[metadata].additionalProperties.properties[routeSelectors].items.properties[matches].items.properties[path].x-kubernetes-validations[10].rule: Forbidden: contributed to estimated rule cost total exceeding cost limit for entire OpenAPIv3 schema * spec.validation.openAPIV3Schema: Forbidden: x-kubernetes-validations estimated rule cost total for entire OpenAPIv3 schema exceeds budget by factor of 1.7x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared) make: *** [install] Error 1

So we might want to rethink of whether we truely want the 2 pathways to achieving defaults 🤔

Added a workaround at 6a01a4b decreasing the max number of route selectors allowed, but would likely hit this problem again for overrides also which would decrease this number again

Boomatang

Verification worked as expected. Unsure what the best manual verification steps are for the CEL expression that have being add.

Added a couple of comments.

Boomatang · 2024-04-02T08:38:51Z

api/v1beta2/authpolicy_types.go

@@ -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=15
+	// +kubebuilder:validation:MaxItems=8


Do we need any test to cover this being changed in the future, as it was reduced from 15 to 8?

Sure, can add a test. Though, I'm expecting this value to decrease again when the overrides field is added in #464

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?

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 RouteSelectors that the fields itself also have CEL validation for and how frequently we have this slice of RouteSelectors in the various structs used within the CRD. This will fail the CEL cost estimation when applying the CRD even if switch to using a validating webhook 🤔

kuadrant-operator/api/v1beta2/route_selectors.go

Lines 13 to 24 in 4f6a138

type RouteSelector struct {

// Hostnames defines a set of hostname that should match against the HTTP Host header to select a HTTPRoute to process the request

// https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec

// +optional

Hostnames []gatewayapiv1.Hostname `json:"hostnames,omitempty"`

// Matches define conditions used for matching the rule against incoming HTTP requests.

// https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec

// +optional

// +kubebuilder:validation:MaxItems=8

Matches []gatewayapiv1.HTTPRouteMatch `json:"matches,omitempty"`

}

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:

wait for Adding GEP-995 for support for named route rules kubernetes-sigs/gateway-api#2593 to be implemented and then simplify the route selectors

redefine the HTTPRouteMatch types ourselves without the validation – we could do this because the validation that matters is the one enforced on the HTTPRoute, not on the policy; on the policy, the only consequence of an "invalid" route selector is that the selector will not select any route rule.

Boomatang · 2024-04-02T09:55:36Z

controllers/authpolicy_authconfig.go

@@ -112,39 +112,39 @@ func (r *AuthPolicyReconciler) desiredAuthConfig(ctx context.Context, ap *api.Au
 	authConfig.Spec.Hosts = hosts

 	// named patterns
-	if namedPatterns := ap.Spec.NamedPatterns; len(namedPatterns) > 0 {
+	if namedPatterns := ap.GetNamedPatterns(); len(namedPatterns) > 0 {


I have a thought/concern, which I am not sure what the best resolution is. There is nothing stop someone from accessing Spec.NamedPatterns directly. This could lead to errors later. One way would be to force access through methods by making the fields in the Spec private. In this case the methods would want to be on the Spec struct and not the AP struct. Access would then look like ap.Spec.GetNamedPatterns().

This would need to apply to all data fields in the Spec and not just the NamedPatterns. It should also apply to the spec in #456.

What do you think? Personal if I have access to the fields of a struct at some point I will access them directly.

I have a thought/concern, which I am not sure what the best resolution is. There is nothing stop someone from accessing Spec.NamedPatterns directly. This could lead to errors later. One way would be to force access through methods by making the fields in the Spec private. In this case the methods would want to be on the Spec struct and not the AP struct. Access would then look like ap.Spec.GetNamedPatterns().

This would need to apply to all data fields in the Spec and not just the NamedPatterns. It should also apply to the spec in #456.

Hmm, I dont think this will quite work as intended. If we make fields private / not exported, I guess we need setters for all the fields also, while also having setters for the defaults fields. The will work for using the struct via code.

The other one is since the field is not exported, the json field will not be available via kubectl get policy x -o json commands which I think makes this not viable unless I'm missing something.

What do you think? Personal if I have access to the fields of a struct at some point I will access them directly.

Yeah, unfortunately I think is an artefact of having multiple pathways of achieving the same result. I guess the assumption is that if people are acessing the fields directly via code, they know which pathway they are using, either spec.X or spec.defaults.x but the getters will be the best way to get the data without needing to know which pathway is used

…n tests

KevFan · 2024-04-03T11:28:38Z

api/v1beta2/authpolicy_types.go

@@ -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"`
+	AuthScheme *AuthSchemeSpec `json:"rules,omitempty"`


Switched to pointer so that this will not show on json spec as empty object {} when empty which would be a bit confusing to a user, especially if spec.defaults is used instead 🤔 Also makes the mutual exclusive CEL rule a bit easier by switching to pointer

KevFan · 2024-04-03T11:33:01Z

controllers/authpolicy_authconfig.go

+	commonSpec := ap.Spec.CommonSpec()
+


There's no nil check for AuthScheme here as this function is only called from r.desiredAuthConfig() which has been updated with this nil check for AuthScheme. This function will not be called in this case

KevFan · 2024-04-03T11:38:14Z

Makefile

@@ -406,14 +406,15 @@ kind-load-bundle: ## Load image to local cluster
 ##@ Deployment

 install: manifests kustomize ## Install CRDs into the K8s cluster specified in ~/.kube/config.
-	$(KUSTOMIZE) build config/crd | kubectl apply -f -
+	# Use server side apply, otherwise will hit into this issue https://medium.com/pareture/kubectl-install-crd-failed-annotations-too-long-2ebc91b40c7d
+	$(KUSTOMIZE) build config/crd | kubectl apply --server-side -f -


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 🤔

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.)

guicassolato · 2024-04-03T15:11:48Z

doc/reference/authpolicy.md

+| `rules`          | [AuthScheme](#authscheme)                                                                                                                   | No           | Implicit default authentication/authorization rules                                                                                                                                                                                                                                             |
+| `routeSelectors` | [][RouteSelector](route-selectors.md#routeselector)                                                                                         | No           | List of implicit default selectors of HTTPRouteRules whose matching rules activate the policy. At least one HTTPRouteRule must be selected to activate the policy. If omitted, all HTTPRouteRules of the targeted HTTPRoute activate the policy. Do not use it in policies targeting a Gateway. |
+| `patterns`       | Map<String: [NamedPattern](#namedpattern)>                                                                                                  | No           | Implicit default named patterns of lists of `selector`, `operator` and `value` tuples, to be reused in `when` conditions and pattern-matching authorization rules.                                                                                                                              |
+| `when`           | [][PatternExpressionOrRef](https://docs.kuadrant.io/authorino/docs/features/#common-feature-conditions-when)                                | No           | List of implicit default additional dynamic conditions (expressions) to activate the policy. Use it for filtering attributes that cannot be expressed in the targeted HTTPRoute's `spec.hostnames` and `spec.rules.matches` fields, or when targeting a Gateway.                                |


The when conditions at this level will be a problem when we get to Tier 2 of D/O and introduce constraints.

guicassolato

LGTM. Nice work, @KevFan!

Boomatang

LGTM - Happy with changes, all my comments and concerns were addressed.

* feat: ap default field * refactor: Getters for common shared default fields * workaround: reduce route selector length & use server side apply * refactor: use explicit default in AP integration tests * refactor: AP CEL validation * refactor: AuthPolicyCommonSpec * docs: update AuthPolicy doc with defaults field * refactor: remove unneccessary getters * refactor: reset number of root level route selectors & add integration tests * refactor: remove debug log from new AP route selector integration tests

KevFan commented Mar 21, 2024

View reviewed changes

KevFan force-pushed the ap-default branch 3 times, most recently from 909a1cb to 2798ac1 Compare March 25, 2024 15:38

KevFan self-assigned this Mar 25, 2024

KevFan added kind/enhancement New feature or request size/medium area/api Changes user facing APIs labels Mar 25, 2024

KevFan changed the title ~~[WIP] AP Defaults~~ AP Defaults Mar 26, 2024

KevFan marked this pull request as ready for review March 26, 2024 16:49

KevFan requested a review from a team as a code owner March 26, 2024 16:49

KevFan force-pushed the ap-default branch 2 times, most recently from 6c4decc to b4d066d Compare March 27, 2024 16:51

Boomatang reviewed Apr 2, 2024

View reviewed changes

KevFan added 8 commits April 2, 2024 16:44

feat: ap default field

957889d

refactor: Getters for common shared default fields

e987c91

workaround: reduce route selector length & use server side apply

38629a2

refactor: use explicit default in AP integration tests

9a6b535

refactor: AP CEL validation

7d98454

refactor: AuthPolicyCommonSpec

bf908a2

docs: update AuthPolicy doc with defaults field

00a0343

refactor: remove unneccessary getters

4e8d06e

KevFan force-pushed the ap-default branch from b4d066d to 4e8d06e Compare April 2, 2024 15:44

refactor: reset number of root level route selectors & add integratio…

4bcd167

…n tests

KevFan commented Apr 3, 2024

View reviewed changes

guicassolato reviewed Apr 3, 2024

View reviewed changes

guicassolato approved these changes Apr 3, 2024

View reviewed changes

refactor: remove debug log from new AP route selector integration tests

bf701f8

Boomatang approved these changes Apr 8, 2024

View reviewed changes

KevFan merged commit d818530 into Kuadrant:main Apr 8, 2024
13 checks passed

martinhesko mentioned this pull request Apr 12, 2024

Atomic defaults tests Kuadrant/testsuite#375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP Defaults #503

AP Defaults #503

KevFan commented Mar 21, 2024 •

edited

Loading

KevFan Mar 21, 2024 •

edited

Loading

KevFan Mar 25, 2024

Boomatang left a comment

Boomatang Apr 2, 2024

KevFan Apr 2, 2024

Boomatang Apr 3, 2024

KevFan Apr 3, 2024

guicassolato Apr 3, 2024

Boomatang Apr 2, 2024

KevFan Apr 2, 2024

KevFan Apr 3, 2024

KevFan Apr 3, 2024

KevFan Apr 3, 2024

guicassolato Apr 3, 2024

guicassolato Apr 3, 2024

guicassolato left a comment

Boomatang left a comment

	type RouteSelector struct {
	// Hostnames defines a set of hostname that should match against the HTTP Host header to select a HTTPRoute to process the request
	// https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec
	// +optional
	Hostnames []gatewayapiv1.Hostname `json:"hostnames,omitempty"`

	// Matches define conditions used for matching the rule against incoming HTTP requests.
	// https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteSpec
	// +optional
	// +kubebuilder:validation:MaxItems=8
	Matches []gatewayapiv1.HTTPRouteMatch `json:"matches,omitempty"`
	}

AP Defaults #503

AP Defaults #503

Conversation

KevFan commented Mar 21, 2024 • edited Loading

Description

Verification

KevFan Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Boomatang 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

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

guicassolato left a comment

Choose a reason for hiding this comment

Boomatang left a comment

Choose a reason for hiding this comment

KevFan commented Mar 21, 2024 •

edited

Loading

KevFan Mar 21, 2024 •

edited

Loading