-
Notifications
You must be signed in to change notification settings - Fork 459
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: Add Name to HTTPRouteRule and HTTPRouteMatch #995
Comments
/assign @hzxuzhonghu |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
If I'm understanding correctly based on the closed PR, we've decided not to move forward with this at this time. If that's not the case, please feel free to drop a comment and we can re-open. |
I think we decided this would be reasonable if we had a bigger use case associated with it. This will likely end up being included as part of route delegation in #1058. |
Ok, then let's consider this closed in favor of the implementation in #1058 👍 |
/reopen Route delegation is not moving forward, this is still useful |
@howardjohn: Reopened this issue. In response to this:
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. |
I think it makes sense in general, but will need careful writing of the spec - we can't add the new field as required, but what value should be used in contexts where it's expected (like log messages or telemetry) if it's not set? |
|
@shaneutt @youngnick @howardjohn We would appreciate having this in the next version of the HTTPRoute. We have a similar requirement to apply some policies per rule as mentioned in the description. cc: @apalsule |
Within the Kuadrant project, we also see value in adding this to support policy. Would really like to see it in the next version also |
One of the most common arguments against this feature is that we really shouldn't be encouraging users to have large Routes. Routes with lots of rules are remarkably hard to deal with in cases like partially invalid config where many Route rules are valid, but one is not. We still have not solved this problem so the API only provides guidance for when a Route is invalid. It seems like any request for attaching policies and other resources to a specific Route Rule name has to answer this key question: Why not just use more than one Route? Any Route that has 10 rules could also be equally represented by 10 Routes with 1 rule each, or 5 Routes with 2 rules each, or 2 Routes with 5 rules each, etc. I'm concerned that adding name to Route rules would encourage even larger Routes, which would only exacerbate other problems that we have not solved yet like partially valid Routes. |
@robscott I think everything you said is true, but also if you have >1 route having a name is useful. So even for a small 2 route service its helpful. If its such a concern, why do we even allow multiple routes at all? Its very likely too late to change that so mostly a theoretical argument. But it does seem compelling that if you have multiple things, you ought to have a way to refer to them. Listener names, service port names, container names, etc all follow this pattern. |
We currently have a maximum items set of 16 in the validation for rules across all the rule types, so I think we're managing the larger routes concern to some extent. I think I should be clear that we will be very reluctant to allow more than that (I actually think 16 is probably too large as it is, but we can't make it smaller without an API version bump). That said, I think I'm in agreement with @howardjohn here - it is useful for things to have a However, versioning means that we have the following things to address in this proposal:
As long as we carefully handle the implications of this field being optional, I think it's okay to proceed. |
My 2 cents:
I think this is unfortunate but understandable.
Leave it empty would be my preference.
Agree again. As I have a particular interest in policy, and so should be part of the target ref as called out in the future work section of the policy attachment GEP. For me the all of the target ref in a policy has to be valid before the policy is applied. So it would be invalid to have a policy with a target ref that matched the resource but had an invalid |
Could this potentially be done by using a filter referenced from a routing rule? |
It turns out not having the name field available in the HTTPRouteRule (and perhaps also in the HTTPRouteMatch) has been biting us in more ways, due to the CEL validations in the HTTPRoute (v0.8+). Kuadrant policies (based on Policy Attachment, GEP-713) support associating parts of a policy spec ("policy rules") to specific sections of the main targeted network resource – particularly to HTTPRouteRules, when a HTTPRoute is the target. Because HTTPRouteRules cannot be referred by name, our solution was to define This would work fine until v0.7, but since validation rules based on Common Expression Language (CEL) have been added to the HTTPRoute (#2253), the estimated cost limit exceeds the thresholds, and we have been struggling with having to set some hard constraints on the number of policy rules a user can declare. In particular, these HTTPRoute validations have been flagged as the ones adding most to the estimated cost of our APIs: https://github.com/kubernetes-sigs/gateway-api/blame/14197584076cbc6853caf4a6cef29d6d5810595c/apis/v1beta1/httproute_types.go#L372-L382 A much better alternative – that would not only fix the issue of the estimated complexity of the imported validation rules, but also improve assertiveness in how those associations between policy rules and HTTPRouteRules ("HTTPRouteRule refs") are declared – would be having name fields available in the HTTPRouteRule type. |
For default values: "1", "2"... Or match-1 if we want to avoid confusion with array syntax. Since most routes have few matchers - just allowing attachment with an array syntax or the default names would work very well with no change to the route. Ideally it would be similar to jq. |
This is starting to feel more useful with steps towards standardizing support for partial validity of routes in #2429 and could greatly help clarify those corresponding error messages. Would strongly prefer @youngnick and @maleck13's suggestions that unset optional names be empty/nil and a mismatched sectionName in policy targetRef should be a ResolvedRefs error - array-based indexing would be too easy to accidentally switch targets of a policy when adding or removing a rule from a route. |
@hzxuzhonghu given the dates on this, we don't expect there's active work on this currently /unassign @hzxuzhonghu But if that's incorrect, and you still want to work on this please let us know. |
@guicassolato you recently discussed your strong need and use case for this, are you interested in being one of the assigned people and to try and move this forward by starting a GEP? We would ask the first pass at this be focused entirely on the WHAT and WHY and try to stay away from the how (e.g. no API design in the first PR) so we can build consensus on what it is and why we want to do it, but not get lost (yet) in the how we do it. |
Awesome! Thank you! Let us know if you get stuck or need help. /assign @guicassolato |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
/remove-lifecycle rotten |
What would you like to be added:
Add a name field to HTTPRouteRule and HTTPRouteMatch structs to allow users to identify different routes
Why this is needed:
Policy Attachment
Route Attachment/Delegation: Ability to have more fine grained matches for policy references IF we add sectionName to the policy object
Improved Debugging: Ability to add debugging information, such as adding the route matched to some access log
The text was updated successfully, but these errors were encountered: