GEP-713 enhancements #3609
GEP-713 enhancements #3609
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
size/XXL
|
|
||
| Cons: | ||
| #### Target object status |
There was a problem hiding this comment.
can we give some examples of this? I think I understand it but not certain
There was a problem hiding this comment.
Added a simplified example, plus an extension of it for the case including sectionName.
Please let me know if that works or if you expected to see a full YAML.
There was a problem hiding this comment.
A full yaml would be nice
geps/gep-713/index.md
Outdated
| going to affect their object, at apply time, which helps a lot with discoverability. | ||
| * **Accepted**: the meta resource passed both syntactic validation by the API server and semantic validation enforced by the controller, such as whether the target objects exist. | ||
| * **Enforced**: the meta resource’s spec is guaranteed to be fully enforced, to the extent of what the controller can ensure. | ||
| * **Partially enforced**: parts of the meta resource’s spec is guaranteed to be enforced, while other parts are known to have been superseded by other specs, to the extent of what the controller can ensure. The status should include details highlighting which parts of the meta resource are enforced and which parts have been superseded, with the references to all other related meta resources. |
There was a problem hiding this comment.
As long as this is not a MUST then its not a problem, but this seems like it could be quite onerous to compute. For example, imagine I have a global policy and then 1000 namespaces any of which could partially conflict. Its not great to have to 'bubble up' these to the parent.
There was a problem hiding this comment.
A concrete example of this in existing Gateway API is attachedRoutes, which is similarly complex for implementations to compute (efficiently)
geps/gep-713/index.md
Outdated
|
|
||
| ## Background and concepts | ||
| The merge strategies typically include strategies for dealing with conflicting and/or missing specs, such as for applying default and/or override values on the target resources. |
There was a problem hiding this comment.
I think it's important to note that sometimes the merge strategy may be specified in the design of the object (that is, it's a defaults policy or something), rather than in a field?
In fact, I tend to think that, if the merge strategy is listed in a field, it should be in the status, not the spec, since it's relevant info for users of the Policy more than implementers (who will build the merge strategy into code when handling the Policy anyway).
There was a problem hiding this comment.
+1, this feels like something that belongs in status.
There was a problem hiding this comment.
+1 that merge strategy may be defined in the metaresource, e.g. the API contract is either only one is allowed per target, or multiple are allowed.
There was a problem hiding this comment.
I feel confused about the merge strategy in the status instead of the spec.
Are we talking about the metaresource's status and spec? Or the target's?
The merge strategy, if more than one is supported by the metaresource kind, is a choice of the user that declares an instance of the metaresource. How can it be in the status?
The user literally specify what merge strategy to use when merging that instance of the metaresource. It should be in the spec, no?
There was a problem hiding this comment.
Added a few lines about merge strategy as a user choice or not, and reflected in the status stanza of the metaresource.
geps/gep-713/index.md
Outdated
|
|
||
| **Ana**: _What the hell just happened??_ | ||
| If multiple meta resources target the same context, this is considered to be a conflict. |
There was a problem hiding this comment.
We need to define "context" again here, I think. (I'd forgotten the definition by the time I got to this part).
| If multiple meta resources target the same context, this is considered to be a conflict. | |
| If multiple meta resources target the same context (that is, multiple instances of the same or similar policies acting on the same hierarchy have an effective target of the same object), this is considered to be a conflict. |
There was a problem hiding this comment.
"same", yes; "similar", not a good idea IMO. I think the behavior for different kinds of policies should be undefined.
There was a problem hiding this comment.
I'm okay with removing "or similar", but I think that if we're going to leave this as undefined in some cases, we need to be specific in the ones where we do need to have opinions:
- For Gateway API Policy objects included in the specification, in the case of intent conflict with some other Policy on Gateway API objects, the Gateway API Policy must take precedence.
- For implementation specific Policy objects that affect the same properties across multiple implementations, it's up to the implementations to define behavior. If they don't then the behavior is, necessarily, undefined and could produce differing outcomes depending on unknown factors.
In other words, this is a terrible idea and users should try not to use multiple Policy objects that affect the same things.
geps/gep-713/index.md
Outdated
| **Chihiro**: _At a guess, all the workloads in the `baker` namespace actually | ||
| fail a lot, but they seem OK because there are retries across the whole | ||
| namespace?_ 🤔 | ||
| Conflicts must be resolved by applying a defined *merge strategy* (see further definition in the next section), where the meta resource considered higher between two conflicting specs dictates the merge strategy according to which the conflict must be resolved, defaulting to the lower spec (more specific) beating the higher one if not specified otherwise. |
There was a problem hiding this comment.
| Conflicts must be resolved by applying a defined *merge strategy* (see further definition in the next section), where the meta resource considered higher between two conflicting specs dictates the merge strategy according to which the conflict must be resolved, defaulting to the lower spec (more specific) beating the higher one if not specified otherwise. | |
| Conflicts must be resolved by applying a defined *merge strategy* (see further definition in the next section). | |
| When resolving conflicts, the meta resource higher in the relevant hierarchy dictates the merge strategy - that is, merge strategy conflict resolution works on a least-specific-wins basis. After that the merge strategy's conflict resolution rules apply. | |
| If no merge strategy is specified, then implementations should use more-specific-wins merge strategy by default. |
I think this is what you meant here @guicassolato?
There was a problem hiding this comment.
Yes, but I happen to find the suggested text more confusing than the original.
"least-specific-wins" and "more-specific-wins" have different subjects in the sentences, and therefore I would phrase it differently to avoid confusion.
A merge strategy is a function that takes as input 2 specs and outputs 1.
One thing is determining the merge strategy. When resolving a conflict posed by 2 metaresources, the least specific metaresource among the two dictates the merge strategy that will be used to solve the conflict, i.e. the function that will take both metaresource specs as input. It's always the least specific metaresource that determines it.
The determined merge strategy can be a merge strategy that resolves to "least-specific-wins" or "more-specific-wins" (and occasionally to things more sophisticated than that, like actual merges).
If the least specific metaresource does not specify a merge strategy, then the merge strategy used to resolve the conflict is "more-specific-wins".
There was a problem hiding this comment.
Rephrased a bit to break down as suggested but trying to avoid overloading terminology.
Signed-off-by: Guilherme Cassolato <[email protected]>
1. Define names and mechanisms for possible merge strategies (so both what e.g. “atomic default” means, but also that “atomic default” is the correct name for that strategy) 2. Define a status mechanism by which the strategy SHOULD be reported, and that a conformant implementation MUST use the names defined in 1 to report strategy. 3. Define what merge strategy is preferred for `defaults`, and define that implementations using the defaults clause SHOULD use that strategy. 4. Define what merge strategy is preferred for `overrides`, and define that implementations using the overrides clause SHOULD use that strategy. 5. Acknowledge that implementations MAY support other strategies, or selecting strategies at runtime, but that those are implementation-specific behaviors. Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
…lementations Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
…on rules section Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
… not must adopt Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
…nd of merge strategy, etc) + more detailed `status` stanza provided Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
guicassolato
force-pushed
the
geps/713-enhancements
branch
from
d3b062f to
94cbdd2
Compare
robscott
left a comment
robscott
left a comment
There was a problem hiding this comment.
Thanks @guicassolato, this is really well done!
Not at all a blocker for this PR, but this is by far our largest GEP now and may justify being split out into separate docs pages in the not too distant future.
geps/gep-713/index.md
Outdated
| !!! warning | ||
| This GEP specifies a _pattern_, not an API field or new object. It defines some terms, including _Metaresource_, _Policies_ and _Policy Attachment_, and their related concepts. |
There was a problem hiding this comment.
A concern here is that when I look at the deploy preview for this, the number of warnings at the top of the page are pretty overwhelming. I think the text here is good, but I don't think it needs to be included in a warning block.
There was a problem hiding this comment.
We have a warning, a danger and an info block here. Should them all be turned into regular paragraphs or just the warning one?
I'll make them all paragraphs for now, but happy to revert if needed.
geps/gep-713/index.md
Outdated
|
|
||
| #### gwctl | ||
|
|
||
| https://github.com/kubernetes-sigs/gwctl |
There was a problem hiding this comment.
Mind turning this into a link? (Same applies to similar items above and below)
geps/gep-713/index.md
Outdated
|
|
||
| This is an `Ancestor` status rather than a `Parent` status, as in the Route status, because, for Policy Attachment, the relevant object may or may not be the direct parent. | ||
|
|
||
| For example, `BackendTLSPolicy` directly attaches to a Service, which may be included in multiple Routes, in multiple Gateways. However, for many implementations, the status of the `BackendTLSPolicy` will be different only at the Gateway level, so Gateway is the relevant Ancestor for the status. Each Gateway that has a Route that includes a backend with an attached `BackendTLSPolicy` SHOULD have a separate `PolicyAncestorStatus` section in the `BackendTLSPolicy`'s `status.ancestors` stanza, which mandates that entries must be distinct using the combination of the `AncestorRef` and the `ControllerName` fields as a key. See [GEP-1897][../gep-1897/index.md] for the exact details. |
There was a problem hiding this comment.
The link to GEP-1897 isn't working
geps/gep-713/index.md
Outdated
|
|
||
| ##### Using upstream `PolicyAncestorStatus` struct | ||
|
|
||
| _Status:_ Provisional |
There was a problem hiding this comment.
This feels like it should be experimental at minimum, likely standard now that we have a GA resource relying on it.
There was a problem hiding this comment.
OK. Let's make it Experimental then. I think many implementations still need to catch up on this.
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
|
One tiny non-blocking nit, so I'm going to get this done. /lgtm |
k8s-ci-robot
added
the
lgtm
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
* GEP-713 resurrected 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]> * update metadata files Signed-off-by: Guilherme Cassolato <[email protected]> * not forcing annotations on objects not owned by the implementation Signed-off-by: Guilherme Cassolato <[email protected]> * conflict resolution section rewritten to clarify that the merge strategy is the only thing necessary to resolve conflicts Signed-off-by: Guilherme Cassolato <[email protected]> * Metaresource spelled as a single word Signed-off-by: Guilherme Cassolato <[email protected]> * addressing comments * "meta resource" -> "metaresource" (single word) * rephrasing/rearranging content for clarity * typos Signed-off-by: Guilherme Cassolato <[email protected]> * fixing escaping of arrows and gt/lw characters Signed-off-by: Guilherme Cassolato <[email protected]> * clarifying metaresource statuses' MUST vs SHOULD and example of target object status Signed-off-by: Guilherme Cassolato <[email protected]> * targeting strategy -> targeting method, to avoid overloading of the term 'strategy' Signed-off-by: Guilherme Cassolato <[email protected]> * fix target object status MUST versus SHOULD Signed-off-by: Guilherme Cassolato <[email protected]> * diagrams to illustrate the abstract process for calculating effective metaresources Signed-off-by: Guilherme Cassolato <[email protected]> * golang examples of target refs Signed-off-by: Guilherme Cassolato <[email protected]> * fix broken anchors Signed-off-by: Guilherme Cassolato <[email protected]> * diagrams for the end-to-end examples Signed-off-by: Guilherme Cassolato <[email protected]> * sentence rephrased for improved readability Signed-off-by: Guilherme Cassolato <[email protected]> * addressing a few comments by candita and robscott Signed-off-by: Guilherme Cassolato <[email protected]> * typos Signed-off-by: Guilherme Cassolato <[email protected]> * Get GEP-713 back to Memorandum Signed-off-by: Guilherme Cassolato <[email protected]> * Reorg (take 1) Signed-off-by: Guilherme Cassolato <[email protected]> * Reorg (take 2) Signed-off-by: Guilherme Cassolato <[email protected]> * minor fixes Signed-off-by: Guilherme Cassolato <[email protected]> * fix: typo in 'sectionName' Signed-off-by: Guilherme Cassolato <[email protected]> * minor fixes (2) Signed-off-by: Guilherme Cassolato <[email protected]> * fix: typos Signed-off-by: Guilherme Cassolato <[email protected]> * sentence about different kinds possibly implying different targetting semantics rephrased for improved readability Signed-off-by: Guilherme Cassolato <[email protected]> * small enhancements to description of' Hierarchy of target kinds' and 'Conflict resolution rules' subsections Signed-off-by: Guilherme Cassolato <[email protected]> * Clarifying enhancements to the definitions of merge strategy and related concepts Signed-off-by: Guilherme Cassolato <[email protected]> * added motivation to define 'classes of metaresources' Signed-off-by: Guilherme Cassolato <[email protected]> * Preferring 'Policy' over 'Metaresource' when prescribing rules/schema/etc in the spec Signed-off-by: Guilherme Cassolato <[email protected]> * Label selectors as a mechanism to target removed from the spec Ref.: kubernetes-sigs#3609 (comment) Signed-off-by: Guilherme Cassolato <[email protected]> * More thorough definitions for the spec'ed merge strategies Signed-off-by: Guilherme Cassolato <[email protected]> * Further enhancements to the definition of Merge strategies after review 1. Define names and mechanisms for possible merge strategies (so both what e.g. “atomic default” means, but also that “atomic default” is the correct name for that strategy) 2. Define a status mechanism by which the strategy SHOULD be reported, and that a conformant implementation MUST use the names defined in 1 to report strategy. 3. Define what merge strategy is preferred for `defaults`, and define that implementations using the defaults clause SHOULD use that strategy. 4. Define what merge strategy is preferred for `overrides`, and define that implementations using the overrides clause SHOULD use that strategy. 5. Acknowledge that implementations MAY support other strategies, or selecting strategies at runtime, but that those are implementation-specific behaviors. Signed-off-by: Guilherme Cassolato <[email protected]> * minor fix: plural targetRefs used in an example Signed-off-by: Guilherme Cassolato <[email protected]> * Tables highlighting the current state of policies across multiple implementations Signed-off-by: Guilherme Cassolato <[email protected]> * fix Envoy Gateway SecurityPolicy merge strategy Signed-off-by: Guilherme Cassolato <[email protected]> * cleanup redundant 'in other words' summary from the conflict resolution rules section Signed-off-by: Guilherme Cassolato <[email protected]> * Less negative disclaimer at the top Signed-off-by: Guilherme Cassolato <[email protected]> * minor fix: change and augment are not synonyms Signed-off-by: Guilherme Cassolato <[email protected]> * added goal: facilitate building tools + implementations should adopt, not must adopt Signed-off-by: Guilherme Cassolato <[email protected]> * keep support for singular form targetRef Signed-off-by: Guilherme Cassolato <[email protected]> * use example.com in the examples Signed-off-by: Guilherme Cassolato <[email protected]> * caveat to using sectionName Signed-off-by: Guilherme Cassolato <[email protected]> * fix: list items and broken links Signed-off-by: Guilherme Cassolato <[email protected]> * Reorg for more easily flagging of the status of each feature (each kind of merge strategy, etc) + more detailed `status` stanza provided Signed-off-by: Guilherme Cassolato <[email protected]> * fix: several typos Signed-off-by: Guilherme Cassolato <[email protected]> * warning, danger, and info blocks turned into regular paragraphs Signed-off-by: Guilherme Cassolato <[email protected]> * PolicyAncestorStatus: Provisional -> Experimental Signed-off-by: Guilherme Cassolato <[email protected]> * fix: broken link to GEP-1897 Signed-off-by: Guilherme Cassolato <[email protected]> * links to known implementations Signed-off-by: Guilherme Cassolato <[email protected]> --------- Signed-off-by: Guilherme Cassolato <[email protected]>
14 participants
What type of PR is this?
/kind gep
What this PR does / why we need it:
Rewriting of GEP-713 (Memorandum) to clarify concepts and incorporate enhancements discussed at #2927.
Which issue(s) this PR fixes:
Related to #713
Does this PR introduce a user-facing change?: