Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support attachment of policies to multiple Gateways #2999

Closed
guydc opened this issue Mar 23, 2024 · 17 comments · Fixed by #3581
Closed

Support attachment of policies to multiple Gateways #2999

guydc opened this issue Mar 23, 2024 · 17 comments · Fixed by #3581
Assignees
Labels
area/api API-related issues area/policy kind/enhancement New feature or request
Milestone

Comments

@guydc
Copy link
Contributor

guydc commented Mar 23, 2024

Description:
Gateway operators may want to attach common policies to a group of gateways. Currently, duplicated instances of a policy need to be created and attached to each Gateway to achieve this. This can lead to significant configuration duplication, especially when many virtual gateways are merged to a single envoy deployment.

EnvoyPatchPolicy introduced a concept of GatewayClass-atttached policies for the merge gateways scenario.

This form of attachment can be extended to other policies like ClientTrafficPolicy, BackendTrafficPolicy, SecurityPolicy, EnvoyExtensionPolicy and BackendTLSPolicy.

@guydc guydc added kind/enhancement New feature or request area/api API-related issues area/policy labels Mar 23, 2024
@arkodg
Copy link
Contributor

arkodg commented Mar 26, 2024

Im a -1 on this

2 ways to apply policies (Gateway, Route) is sufficient to solve most cases, 3 ways makes the concepts of defaults, overrides and merges very complicated to comprehend for the user and compute for the translator

  • For EnvoyPatchPolicy we only allow attaching to the GatewayClass if mergeGateways is set

@arkodg
Copy link
Contributor

arkodg commented Mar 26, 2024

im a +1 for pluralizing targetRefs so multiple Gateways can be targeted , would ideally like to get guidance on this from upstream before implementing this, this is something we can time box

@guydc
Copy link
Contributor Author

guydc commented Mar 26, 2024

im a +1 for pluralizing targetRefs so multiple Gateways can be targeted , would ideally like to get guidance on this from upstream before implementing this, this is something we can time box

Yes, that can also work pretty well, as long as selectors are supported, and users don't have to maintain an explicit gateway list.

@guydc guydc changed the title Support GatewayClass attachment for additional policies Support attachment of policies to multiple Gateways Mar 26, 2024
@arkodg
Copy link
Contributor

arkodg commented Mar 26, 2024

cc @AliceProxy who was a big +1 for ^

@davidalger
Copy link
Contributor

davidalger commented Apr 2, 2024

+1 on pluralizing targetRefs, this would also be useful for targeting multiple HTTPRoutes via the same BackendTrafficPolicy for example where groups of routes may require different timeouts or circuit breaker settings from the defaults on the Gateway

@arkodg
Copy link
Contributor

arkodg commented Apr 15, 2024

related discussion in upstream kubernetes-sigs/gateway-api#2966 (comment)

@arkodg arkodg added this to the v1.1.0-rc1 milestone Apr 24, 2024
@arkodg arkodg added the kind/decision A record of a decision made by the community. label Apr 24, 2024
@sadovnikov
Copy link
Contributor

Gateway API v1.1.0-rc1 was released yesterday. It includes "the targetRef field is now a targetRefs list and these references no longer include a namespace field", which is good. However, it seems to be done only for the BackendTLSPolicy and to be implemented as a list of targetRef.

It would be great to see it applied to the Direct Policy Attachment in general and as a choice between targetRef and selector

@arkodg
Copy link
Contributor

arkodg commented May 7, 2024

The decision that needs to be made here is two fold

  1. What does targeting multiple resources look like - is it similar to the upstream recommendation of using targetRefs or should Envoy Gateway adopt another way to target multiple resources
  2. should the current way to target a single resource using targetRef be deprecated (not removed) or deprecated and removed in a future release ?

@arkodg arkodg self-assigned this May 7, 2024
Copy link

github-actions bot commented Jun 7, 2024

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

@github-actions github-actions bot added the stale label Jun 7, 2024
@arkodg
Copy link
Contributor

arkodg commented Jun 7, 2024

unassigning myself, I won't have cycles to work on this issue for v1.1, if anyone else wants to drive this, please go ahead

@arkodg arkodg removed the stale label Jun 7, 2024
@arkodg arkodg removed their assignment Jun 7, 2024
@shawnh2 shawnh2 added help wanted Extra attention is needed and removed kind/decision A record of a decision made by the community. labels Jun 7, 2024
@liorokman liorokman self-assigned this Jun 9, 2024
@liorokman liorokman removed the help wanted Extra attention is needed label Jun 9, 2024
@liorokman
Copy link
Contributor

The decision that needs to be made here is two fold

  1. What does targeting multiple resources look like - is it similar to the upstream recommendation of using targetRefs or should Envoy Gateway adopt another way to target multiple resources
  2. should the current way to target a single resource using targetRef be deprecated (not removed) or deprecated and removed in a future release ?

What was the decision that was reached?

@arkodg
Copy link
Contributor

arkodg commented Jun 10, 2024

thanks for picking this one up @liorokman
reg 1. - requires a little more brainstorming - should we start off with targetRefs or also support label based targets , this affects the top level API field we introduce
reg 2. there was consensus in keeping the existing targetRef field for now

@liorokman
Copy link
Contributor

reg 1. - requires a little more brainstorming - should we start off with targetRefs or also support label based targets , this > affects the top level API field we introduce

I've started implementing with targetRefs in addition to the existing targetRef for now. You can take a look at the attached PR to get some idea of how it would be - I've already done BackendTrafficPolicy and ClientTrafficPolicy.

reg 2. there was consensus in keeping the existing targetRef field for now

100% agree.

@arkodg
Copy link
Contributor

arkodg commented Jun 10, 2024

@liorokman with targetRefs, attaching to 10-20 should be fine, then it may become tedious. So if we do decide to support the case of attaching to 100 Gateways, will that field we part of PolicyReferences as a member field e.g. targetSelector ?

@liorokman
Copy link
Contributor

Yes, I think that a target selector can definitely be a part of the PolicyReferences structure.

@liorokman
Copy link
Contributor

I ended up implementing both the plural targetRefs and a targetSelector.

@sadovnikov
Copy link
Contributor

@liorokman, thank you very much for #3581 👍
Watching #3607 now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api API-related issues area/policy kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants