GEP: introduce GEP 820 #822
Conversation
k8s-ci-robot
added
kind/gep
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hbagdi 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
hbagdi
force-pushed
the
gep/820
branch
3 times, most recently
from
23c6a94 to
2e93ddc
Compare
| more match criterias might be added to L4 routes. | ||
|
|
||
| - Do the same to UDPRoute and TLSRoute | ||
|
|
There was a problem hiding this comment.
I think we should still give a patch to support more match types -- as they are going to come up.
What about this:
Modify HTTPPathMatch.Type:
- Allow qualified names (e.g.
acme.io/my-custom-match). These represent custom match types - The standard match types (
Exact,Prefix,RegularExpression) stay - Remove
ImplementationSpecific
There was a problem hiding this comment.
A few related conversations going on here, but it's helpful to reference Dan's earlier comment in the sig-net review that showed the limitations of a single ImplementationSpecific match type: #780 (comment).
Although not completely in scope for the original purpose of this GEP, I think there's enough overlap to discuss it here. I think @bowei's suggestion to bundle this removal of the ExtensionRef with the ability to provide custom match types would help here. Although we'd be removing one way to extend path matching, we'd be adding other more targeted/clearly scoped extension points. It would also match the pattern we've already chosen to take with Protocol on Gateway listeners (a core set of supported values with clear meaning + a way to use your own custom types when necessary).
What do you think @hbagdi @jpeach @youngnick?
There was a problem hiding this comment.
If this is something we want to move forward with, should we take the same approach for header and query param matching that also include ImplementationSpecific match types? I don't think we had a clear purpose for the match types in those cases other than to be consistent with path match types, where we were really trying to be consistent with Ingress...
There was a problem hiding this comment.
I'm happy to just remove it in this iteration. Specifying a match as a separate object is awkward and verbose, and I haven't heard anyone that plans to implement this.
I also don't think that "ImplementationSpecific" is workable in general, because it doesn't actually let you say what kind of match the implementation wants to make.
There was a problem hiding this comment.
I think we're all in agreement on removing it. I hate to scope creep, but it does seem like it could make sense to combine the removal of this extension mechanism with loosened validation on match types as an alternative extension point.
There was a problem hiding this comment.
I'm happy to just remove it in this iteration
@jpeach To clarify, were you referring to "extensionRef" or "ImplementationSpecific" match types?
There was a problem hiding this comment.
I'm happy to just remove it in this iteration
@jpeach To clarify, were you referring to "extensionRef" or "ImplementationSpecific" match types?
The former, per the proposal in this GEP. I was just saying that I don't really think we should also try to replace it in this GEP.
There was a problem hiding this comment.
I think that the idea of adding qualified path match types is fine, but I don't think that we should push to get it into v1alpha2. It's additive anyway.
I'd support just dumping ImplementationSpecific for now, and ensuring that the thing we're designing actually does what we want.
|
One small suggestion, the rest LGTM |
|
/lgtm |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
I think it's fine to keep this GEP simple and leave any potential "ImplementationSpecific" match type changes to a future GEP. /lgtm |
|
/lgtm -- I think as long as there is a reasonable looking proposal for how to add in extensbility, removal of extensionRef lgtm |
k8s-ci-robot
removed
the
do-not-merge/hold
|
Sigh, also needs #830 merged. |
|
/retest |
What type of PR is this?
/kind gep
What this PR does / why we need it:
See #820
Which issue(s) this PR fixes:
None
Does this PR introduce a user-facing change?:
Preview URL: https://deploy-preview-822--kubernetes-sigs-gateway-api.netlify.app/geps/gep-820/