Updating GEP-2648 to support multiple target refs #2966

robscott · 2024-04-11T21:33:22Z

What type of PR is this?
/kind gep

What this PR does / why we need it:
This PR is a follow up to both #2955 and #2927. It makes two changes to the direct policy attachment GEP:

Policy targetRefs are namespace-local by default
Policy targetRefs can be plural

Both of these changes are after significant discussion at KubeCon and in a follow up meeting around how we can improve policy attachment. They are particularly timely because in this release we are already making breaking changes to BackendTLSPolicy and we're introducing another policy (BackendLBPolicy). I'd like to avoid future breaking changes to both of those policies by including these changes for both policies in v1.1.

Which issue(s) this PR fixes:
Continues on #2648

Does this PR introduce a user-facing change?:

1. Direct policies should use local target references by default
2. Direct policies can reference multiple target references

/cc @arkodg @candita @gcs278 @guicassolato @mikemorris @shaneutt

k8s-ci-robot · 2024-04-11T21:33:27Z

@robscott: GitHub didn't allow me to request PR reviews from the following users: guicassolato.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind gep

What this PR does / why we need it:
This PR is a follow up to both #2955 and #2927. It makes two changes to the direct policy attachment GEP:

Policy targetRefs are namespace-local by default

Policy targetRefs can be plural

Both of these changes are after significant discussion at KubeCon and in a follow up meeting around how we can improve policy attachment. They are particularly timely because in this release we are already making breaking changes to BackendTLSPolicy and we're introducing another policy (BackendLBPolicy). I'd like to get avoid future breaking changes to both of those policies by including these changes for both policies in v1.1.

Which issue(s) this PR fixes:
Continues on #2648

Does this PR introduce a user-facing change?:
1. Direct policies should use local target references by default
2. Direct policies can reference multiple target references
/cc @arkodg @candita @gcs278 @guicassolato @mikemorris @shaneutt

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mikemorris · 2024-04-11T22:59:23Z

apis/v1alpha2/policy_types.go

+// in different namespaces. For more information on how this policy attachment
+// model works, and a sample Policy resource, refer to the policy attachment
+// documentation for Gateway API.
+type NamespacedPolicyTargetReference struct {


I'm not sure if it's needed, but it looks like this type may be missing from some places where PolicyTargetReference has been swapped to LocalPolicyTargetReference like apis/applyconfiguration/internal/internal.go?

Good catch - that was an intentional omission for now, basically leaving out NamespacedPolicyTargetReferenceWithSectionName until we're very sure we need it. It should be easy for us to add in the future or for someone else to recreate on their own if needed, just generally think that namespaced targets in policies should be a rare exception. Happy to change though if you'd prefer I add all the types up front.

Make sense, agreed keeping this simpler unless/until we have an actual need is best!

mikemorris

/approve

Based on kubernetes-sigs/gateway-api#2966. Note that we do not HAVE to follow the GatewayAPI here; we can make our own decision. There is, however, a general desire to allow multiple for ergonomics. In this proposal, I hide `targetRef`, but the API will remain + be implemented forever. Implementation cost here is near zero, as we can easily translate it to a single `targetRefs`; we just hide from docs to push users toward the new ones.

k8s-ci-robot · 2024-04-12T14:12:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, mikemorris, 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

linsun · 2024-04-12T14:43:34Z

LGTM

linsun

LGTM assuming #2966 will be merged soon.

candita · 2024-04-12T19:05:07Z

geps/gep-2648/index.md

+
+### Multiple
+
+In some cases, it may be desirable for a policy to target more than one resource


@robscott have you considered allowing multiple targetRefs ONLY for inherited policies, and disallowing in direct attach policies? I thought the spirit of direct attach policies was that they were one-to-one and the ancestor status was clear.

I personally see value in supporting multiple targetRefs at both, Direct and Inherited policies. Rationale:

One who targets multiple HTTPRoutes with a Direct policy can potentially manage only one policy object instead of also many, less repetition, etc.

Simplicity, the only difference between the two would continue to be, as of today, the sectionName.

Especially the latter I believe to be consistent in the long run with the possibility of Direct vs Inherited being reduced to different merge strategies, and not necessarily two big classes of policies – see #2927 (comment).

I just added a separate response that I think is related to this question too. Essentially I agree that we're headed in that direction with #2927, but I'm trying to focus this on the smallest set of changes required to enable changes to our Backend policies in v1.1, which both happen to be direct. I think once someone has time to formally propose the rest of the changes discussed in #2927 and make the corresponding updates to the GEP, all of this will apply equally to direct and inherited policies.

geps/gep-2648/index.md

candita · 2024-04-12T19:12:53Z

geps/gep-2648/index.md

-* The number or scope of objects is exactly _one_ object. No label selectors or
-  lists of `targetRef` are allowed.


I think this rule should stay for Direct Attached. It is the Inherited policy that should reference multiple objects.

Good point, I'm looking at this PR as the smallest subset of #2927 that is required for v1.1. Since both BackendTLSPolicy and BackendLBPolicy are considered "direct" policies, that seemed like the most important type of policy to extend with this capability. With that said, I think there's broad consensus as part of the discussion around #2927 that both direct and inherited will effectively be merged when the full set of changes described there are merged, so this will eventually affect all policies.

Even with that background, I think the primary differentiator between direct and inherited is if policies could affect multiple layers (ie Route and Gateway) or not. In the case of our Backend policies, they still can only affect the backend layer, so I think we're still meeting the original goals of direct policies.

* policy attachment: allow `targetRefs` Based on kubernetes-sigs/gateway-api#2966. Note that we do not HAVE to follow the GatewayAPI here; we can make our own decision. There is, however, a general desire to allow multiple for ergonomics. In this proposal, I hide `targetRef`, but the API will remain + be implemented forever. Implementation cost here is near zero, as we can easily translate it to a single `targetRefs`; we just hide from docs to push users toward the new ones. * codegen * Align documentation * consistency

arkodg · 2024-04-15T04:15:57Z

geps/gep-2648/index.md

+
+#### Migration from Single to Multiple Targets
+
+Existing policies with a single `targetRef` may want to transition to supporting


happy to see the focus on trying to solve this issue of how a user can author intent once (one policy), and apply it to multiple targets. Pluralizing targetRef seems to be the simplest way to support this, but imo this style only works well when the count of targetRefs is 2-10.
When a user wants to define an implementation specific SecurityPolicy resource and apply it to 50 or even 1000 xRoutes, this becomes tedious. This is where label selectors can help reduce the number of lines to author to get the job done.
This style is used in other field within Gateway API like AllowedRoutes.

We may want to take smaller steps when supporting multiple targets, but wanted to highlight that there is a real need to support other ways to target resources, and hoping we leaving space for that in the API

thinking out loud, here's one way to achieve it

targets: refs: - .....

+1 for this.

Although matching the policy by labels may be complex and less efficient than the current KV matching used by targetRefs , providing this solution as an optional implementation would be more user-friendly.

This has been mentioned previously somewhere (I believe by @youngnick, but I can't find the reference currently) as a possible future addition if needed, but intentionally omitted to start to keep complexity constrained while we figure out how to handle status and discoverability in a reasonably performant way.

Agreed that targetRefs is not ideal for scaling beyond a handful of referenced objects (I think that's intentional and not necessarily a bad thing).

Before considering this addition, I'd first want to dig in to whether using an inherited policy targeted at a namespace or gateway may be sufficient for these use cases rather than targeting dozens or hundreds or routes individually. The best argument I've seen for supporting a selector so far is for #1812, and Istio does already implement a split targetRef or selector API model on https://istio.io/latest/docs/reference/config/security/authorization-policy/#AuthorizationPolicy and other resources so there are other implementations with prior art, but I'd consider this beyond the scope of this immediate change for now.

Agree with @mikemorris that label selection here is a bigger problem than I'm trying to solve in this PR. Likely worth adding this idea to the broader policy discussion in #2927 though to see what kind of traction it gets.

@arkodg was part of your original comment here also an argument for starting with a smaller max length than the "16" that I'd originally proposed?

@robscott my orig comment was to make sure we can extend this API easily in the future in case we want to add selector support

candita · 2024-04-15T19:57:21Z

apis/v1alpha2/policy_types.go

Are there few enough people that have implemented Policy that we wouldn't want to make this change by deleting the v1a2 Policy and instead migrate it to v1a3 with the changes? For the same reasons as mentioned in #2955 (comment)?

Hmm good suggestion, that might be the best way to handle this.

Good question - I think this is different since we're not directly generating CRDs from these types, so versioning doesn't seem to carry the same weight. It might actually be better to move these types to an internal/unversioned package instead since these primarily exist as a reference point + for use in our relatively new Policy CRDs (BackendTLSPolicy and BackendLBPolicy).

+1 There may be other CRDs that don't require specific versioning. It would be great to have a policy in place, no pun intended.

These feel similar to the types in https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/shared_types.go - moving them to internal/unversioned could be reasonable if we have some safeguards in place to protect against breaking changes that may propagate up to a published CRD that is versioned (like for instance if we just removed a namespace field without considering the implications for whether any published CRD embedding this type may need a revision bump).

This also clarifies cross-namespace references.

kflynn · 2024-04-15T22:25:41Z

Let's land this as well. 🙂

/lgtm

robscott · 2024-04-16T00:08:26Z

/hold cancel

k8s-ci-robot requested a review from gcs278 April 11, 2024 21:33

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/gep PRs related to Gateway Enhancement Proposal(GEP) labels Apr 11, 2024

k8s-ci-robot requested review from mikemorris, shaneutt, arkodg and candita April 11, 2024 21:33

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 11, 2024

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 11, 2024

robscott force-pushed the policy-targetrefs branch from 6d82760 to 381de51 Compare April 11, 2024 22:04

k8s-ci-robot added the area/gwctl label Apr 11, 2024

robscott mentioned this pull request Apr 11, 2024

Update API and docs for GEP-2907 changes (BackendTLSPolicy) #2955

Merged

mikemorris reviewed Apr 11, 2024

View reviewed changes

robscott force-pushed the policy-targetrefs branch from 381de51 to 16c2d12 Compare April 11, 2024 23:38

mikemorris approved these changes Apr 12, 2024

View reviewed changes

howardjohn mentioned this pull request Apr 12, 2024

policy attachment: allow targetRefs istio/api#3159

Merged

howardjohn approved these changes Apr 12, 2024

View reviewed changes

linsun reviewed Apr 12, 2024

View reviewed changes

robscott mentioned this pull request Apr 12, 2024

Add SessionPersistence API as experimental #2969

Merged

candita reviewed Apr 12, 2024

View reviewed changes

geps/gep-2648/index.md Outdated Show resolved Hide resolved

candita reviewed Apr 12, 2024

View reviewed changes

arkodg reviewed Apr 15, 2024

View reviewed changes

arkodg mentioned this pull request Apr 15, 2024

Support attachment of policies to multiple Gateways envoyproxy/gateway#2999

Closed

candita reviewed Apr 15, 2024

View reviewed changes

Updating GEP-2648 to support multiple target refs

f7abd3d

This also clarifies cross-namespace references.

robscott force-pushed the policy-targetrefs branch from 16c2d12 to f7abd3d Compare April 15, 2024 21:29

k8s-ci-robot assigned kflynn Apr 15, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2024

k8s-ci-robot merged commit f6910e2 into kubernetes-sigs:main Apr 16, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating GEP-2648 to support multiple target refs #2966

Updating GEP-2648 to support multiple target refs #2966

robscott commented Apr 11, 2024 •

edited

Loading

k8s-ci-robot commented Apr 11, 2024

mikemorris Apr 11, 2024 •

edited

Loading

robscott Apr 11, 2024

mikemorris Apr 12, 2024

mikemorris left a comment

k8s-ci-robot commented Apr 12, 2024

linsun commented Apr 12, 2024

linsun left a comment

candita Apr 12, 2024

guicassolato Apr 13, 2024

robscott Apr 15, 2024

candita Apr 12, 2024

robscott Apr 15, 2024

arkodg Apr 15, 2024

arkodg Apr 15, 2024

spacewander Apr 15, 2024

mikemorris Apr 15, 2024 •

edited

Loading

robscott Apr 15, 2024

robscott Apr 15, 2024

arkodg Apr 24, 2024

candita Apr 15, 2024 •

edited

Loading

mikemorris Apr 15, 2024

robscott Apr 15, 2024

candita Apr 15, 2024

mikemorris Apr 15, 2024 •

edited

Loading

kflynn commented Apr 15, 2024

robscott commented Apr 16, 2024


		### Multiple

		In some cases, it may be desirable for a policy to target more than one resource

		* The number or scope of objects is exactly _one_ object. No label selectors or
		lists of `targetRef` are allowed.


		#### Migration from Single to Multiple Targets

		Existing policies with a single `targetRef` may want to transition to supporting

Updating GEP-2648 to support multiple target refs #2966

Updating GEP-2648 to support multiple target refs #2966

Conversation

robscott commented Apr 11, 2024 • edited Loading

k8s-ci-robot commented Apr 11, 2024

mikemorris Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemorris left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 12, 2024

linsun commented Apr 12, 2024

linsun 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

mikemorris Apr 15, 2024 • edited Loading

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

candita Apr 15, 2024 • edited Loading

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

mikemorris Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

kflynn commented Apr 15, 2024

robscott commented Apr 16, 2024

robscott commented Apr 11, 2024 •

edited

Loading

mikemorris Apr 11, 2024 •

edited

Loading

mikemorris Apr 15, 2024 •

edited

Loading

candita Apr 15, 2024 •

edited

Loading

mikemorris Apr 15, 2024 •

edited

Loading