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

GEP-713 resurrected #3609

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

guicassolato
Copy link
Contributor

What type of PR is this?
/kind gep

What this PR does / why we need it:
Resurrects GEP-713 all the way from being a Memorandum straight to Standard, while it buries its forks GEP-2648 and GEP-2649.

Which issue(s) this PR fixes:
Related to #713

Does this PR introduce a user-facing change?:

Updates and promotes GEP-713 to Standard, along with moving its previous forks GEP-2648 and GEP-2649 to becoming Memoranda.

Addresses most of the up voted suggestions discussed at https://github.com/kubernetes-sigs/gateway-api/discussions/2927, such as:
* merging Direct and Inherited back into a single spec;
* introducing the concept of **merge strategy**

Additionally to:
* targetRef supporting label selectors as an option;
* reduction of targetRef.sectionName to the base case of "it's just another (virtual) resource kind"; and
* algorithm for calculating effective meta resources (effective policies)

And general enhacements to the spec aiming to:
* acknowledge the current known support of the pattern across Gateway API implementations;
* broaden the definitions to potentially welcome known implementation of other meta resource-like concepts into the pattern (or at least acknowledge their similarities with Gateway API)

Resurrects GEP-713 all the way from being a Memorandum straight to Standard, while it buries its forks GEP-2648 and GEP-2649.

Addresses most of the up voted suggestions discussed at kubernetes-sigs#2927, such as:
* merging Direct and Inherited back into a single spec;
* introducing the concept of **merge strategy**

Additionally to:
* targetRef supporting label selectors as an option;
* reduction of targetRef.sectionName to the base case of "it's just another (virtual) resource kind"; and
* algorithm for calculating effective meta resources (effective policies)

And general enhacements to the spec aiming to:
* acknowledge the current known support of the pattern across Gateway API implementations;
* broaden the definitions to potentially welcome known implementation of other meta resource-like concepts into the pattern (or at least acknowledge their similarities with Gateway API)

Signed-off-by: Guilherme Cassolato <[email protected]>
@k8s-ci-robot 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 Feb 11, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: guicassolato
Once this PR has been reviewed and has the lgtm label, please assign mlavacca for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 11, 2025
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 11, 2025
Signed-off-by: Guilherme Cassolato <[email protected]>

**Ana**: _What the hell just happened??_
If multiple meta resources target the same context, the following criteria MUST be applied in order to resolve the conflict and thus calculate the effective spec, continuing on ties:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit unclear to me why we are being prescriptive on conflict resolution here? It seems like we have previously said an implementation can make it's own choices on how to merge or not merge resources - shouldn't it be allowed to have it's own mechanics for conflict too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole section at L213 seems to contradict this as well (or likely I just misunderstood) - that seems to say an implementation can make it's own conflict resolution choices, and one choice is "merge" which implies we are not doing the things listed here

Copy link
Contributor Author

@guicassolato guicassolato Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementations still get to choose.

Criteria 1 to 3 following this line establish (try to establish) the distinction between a conflict of policies at different levels of the hierarchy (e.g. xPolicy1 -> Gateway <- HTTPRoute <- xPolicy2) versus a conflict that happens at the same level (e.g. 2 xPolicy resources that target the exact same HTTPRouteRule.)

When the policies are at different levels, the merge strategy dictates how the merge will happen (or not happen). The merge strategy can be one of the "prescribed" ones or implementation-specific. To this rule, I'm adding that the higher policy controls the strategy, which I think has always been implied. E.g. if the Gateway policy says defaults (or nothing at all), then the merge strategy will be "gateway defaults", regardless of what the HTTPRoute policy says. The HTTPRoute policy may say defaults or overrides (or again nothing at all), and it will still be merged as gateway defaults, not because of what the HTTPRoute policy says, but because of what the Gateway policy does.

As for policies conflicting at the same level, I chose to stick with two of the existing criteria of resolving based on creation timestamp and then namespaced names, dropping only the 'overrides always beats defaults' one, which has been shown to be confusing to wrap our heads around (as I mentioned in the community call a couple weeks ago.)

I can try some different writing to make this clearer but from my POV I think it's significantly more permissive now in favour of the implementations than before. I guess the TLDR is: custom merge strategies for policies conflicting at different levels of the hierarchy if necessary and one less imposition for same level policies.


Pro:
The basic status conditions are:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this mean? an implementation should or must have these? Or if they want similar concepts they should consider these names for consistency

Copy link
Contributor Author

@guicassolato guicassolato Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically means I'm a coward 😜 I didn't want to push for a particular closed set of meta resource/policy status conditions, but to take a step back and collect more feedback first.

What you see here is my take on what can be done to enhance the current proposed conditions (which only has one prescribed condition really – i.e.,Accepted), and that is still generic enough IMO that all implementations could consider implementing (a bold guess of mine perhaps).

Do you have an opinion on MUST, SHOULD, MAY (...want to consider adopting similar concepts)? Or even on the list of conditions itself?


### Conditions
For objects that do not have a `status.Conditions` field available (`Secret` is a good example), that object MUST instead have an annotation of `colors.controller.k8s.io/ColorPolicyAffected: true` added instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot require an implementation to write an annotation to an object they don't own IMO. It may make sense in some cases but I cannot imagine users would be too happy with write access to secrets in most cases for example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can change it to SHOULD.


Cons:
#### Target object status
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we give some examples of this? I think I understand it but not certain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/gep PRs related to Gateway Enhancement Proposal(GEP) release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants