GEP: Add Name to HTTPRouteRule and HTTPRouteMatch

[hzxuzhonghu]

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

[mehabhalodiya]

/assign @hzxuzhonghu

[k8s-triage-robot]

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:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

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:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[shaneutt]

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.

[robscott]

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.

[shaneutt]

Ok, then let's consider this closed in favor of the implementation in #1058 👍

[howardjohn]

/reopen

Route delegation is not moving forward, this is still useful

[k8s-ci-robot]

@howardjohn: Reopened this issue.

In response to this:

/reopen

Route delegation is not moving forward, this is still useful

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.

[youngnick]

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?

[howardjohn]

"", "-", "unknown", nil - not sure. It may be usage specific, some systems have different ways to describe "not present" I would think, which makes it hard to prescribe against them.

[swathinsankaran]

@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

[maleck13]

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

[robscott]

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.

[howardjohn]

@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.

[youngnick]

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 name field if there's a list.

However, versioning means that we have the following things to address in this proposal:

• The new name field must be optional, or it's a breaking change.
• Because the field must be optional, we need to define cases like:
  • What implementations should do with empty name fields for logging. Leave the field out, leave it empty? Doesn't really matter what we choose, we just need to choose something.
  • What happens when something (like a Policy or whatever) refers to a name field that doesn't exist? To me, this is a ResolvedRefs error, the same as if the object as a whole did not exist. But again, we need an answer.

As long as we carefully handle the implications of this field being optional, I think it's okay to proceed.

[maleck13]

My 2 cents:

The new name field must be optional, or it's a breaking change.

I think this is unfortunate but understandable.

What implementations should do with empty name fields for logging. Leave the field out, leave it empty? Doesn't really matter what we choose, we just need to choose something.

Leave it empty would be my preference.

What implementations should do with empty name fields for logging. Leave the field out, leave it empty? Doesn't really matter what we choose, we just need to choose something.

What happens when something (like a Policy or whatever) refers to a name field that doesn't exist? To me, this is a ResolvedRefs error, the same as if the object as a whole did not exist. But again, we need an answer.

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 sectionName ref.

[pleshakov]

@swathinsankaran

We have a similar requirement to apply some policies per rule as mentioned in the description.

Could this potentially be done by using a filter referenced from a routing rule?

[maleck13]

@youngnick Wondering if this has been discussed further for 1.0? If there is anything I can do to help progress it let me know

[arkodg]

@maleck13 this is not going to make it into v1.0 (ref to this slack conversation)

hey @hzxuzhonghu, are you planning on working on this GEP, (see some past work in #986) ?

[shaneutt]

We triaged this for GA and we think it's indeed not something that needs to block GA, but also we don't think it's actually ready for someone to pick up:

/label-remove help

We expect more conversation and considerations are needed before we can start taking action on this one:

/triage needs-information

[guicassolato]

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 matches []HTTPRouteMatch fields in our policy APIs, which we use to lookup for HTTPRouteRules that declare identical matches, to associate the policy rule portion with. We call these structures "route selectors" (which also include hostnames.) A typical Kuadrant policy can contain multiple rules, with multiple route selectors each. See example in a Kuadrant RateLimitPolicy and in a Kuadrant AuthPolicy.

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.

[costinm]

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.

[mikemorris]

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.

[shaneutt]

@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.

[shaneutt]

@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.

[guicassolato]

@shaneutt, absolutely yes. I think @maleck13 may have brought the topic to the GAMMA meeting today. The two of us will sync up tomorrow and proceed with the next steps to put the GEP together asap. Thank you!

[shaneutt]

Awesome! Thank you! Let us know if you get stuck or need help.

/assign @guicassolato

[guicassolato]

Opened the draft PR with TLDR, a few goal/non-goals and brief summary of alternatives considered for now. #2593

cc @shaneutt @maleck13

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[maleck13]

/remove-lifecycle stale