GEP 735 L4 Traffic Matching

[shaneutt]

What type of PR is this?
/kind feature
/kind design
/kind gep
/kind api-change
-->

What this PR does / why we need it:

This adds traffic matching to enable more expressive routing for TCPRoute and UDPRoute.

Which issue(s) this PR fixes:

Fixes #735

Does this PR introduce a user-facing change?:

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[howardjohn]

Can we add type? Like listener.address.

The reason is for many cases you don't want to go type out a giant IPv6 address, which is either long or dynamic. However, the controller may have higher level constructs. For example, address: my-gcp-address-with-a-name or even address: httpbin.default.svc.cluster.local (ie use the Cluster IP of the service)

If you know what FE80::0202:B3FF:FE1E:8329 is you probably have a name for it somewhere that is more ergonomic to reference.

[shaneutt]

Just want to make sure I'm clear: are you suggesting:

a) type for IP addresses (v4, v6)
b) type for address (e.g. can include hostnames)
c) both

[robscott]

I think the idea of a "NamedAddress" here makes sense, similar to the address type we have on Gateway. I don't think we'd need to distinguish between v4 and v6 addresses though, but don't feel too strongly about that.

[shaneutt]

1462088 changes this to support both IPAddress and NamedAddress options, let me know your thoughts 🙇

[hbagdi]

NamedAddress feels a bit like ImplementationSpecific in path match (which we got rid off).
@robscott @howardjohn Does NamedAddress by itself (without the additional context of the provider) enough here?

[robscott]

That's a good point, NamedAddress is going to have a different meaning to most implementations. That's easier to solve on Gateways because each will only be implemented by a single controller. This feels similar in nature to RegEx path matching - each implementation will have a different take on it, but it means something conceptually similar.

In most cases you would not want to specify a RegEx match on a Route implemented by multiple controllers unless you knew they supported the same variation of RegEx. I think this would come with the same constraints. Although that's not ideal, I do think it's a sufficiently valuable concept to include. We likely need to document the limitations of these implementation-specific types though.

[shaneutt]

@howardjohn I would like to get your feedback on this: given the contention I'm leaning towards removing the NamedAddress for now such as to get the groundwork completed more quickly and allow for a follow up to suggest NamedAddress or something similar as a separate iteration so that we can focus on it specifically, let me know if you have any thoughts or concerns here.

[robscott]

NamedAddress feels a bit like ImplementationSpecific in path match

@hbagdi I think there's a significant difference here. One of the key problems with "ImplementationSpecific" path matching was that any given implementation may have multiple extended kinds of path matching they'd like to support, and "ImplementationSpecific" could only really support one per implementation. With NamedAddress, I think there will be at most one interpretation per implementation. We already have a "NamedAddress" concept in the API that's fairly well defined, so it's not so much a question of if we should add the concept of a "NamedAddress", but if it makes sense here.

given the contention I'm leaning towards removing the NamedAddress for now

@shaneutt my personal preference is that we leave this in place. I think it's a natural extension point and helps show the purpose of even having an address type here. Although it is something that will have a different meaning per implementation, that matches how NamedAddress already works elsewhere in the API, and seems conceptually similar to RegEx matching.

[shaneutt]

Ok, @hbagdi does that sound reasonable to you?

[robscott]

Thanks for your work on this @shaneutt!

[robscott]

I think the idea of a "NamedAddress" here makes sense, similar to the address type we have on Gateway. I don't think we'd need to distinguish between v4 and v6 addresses though, but don't feel too strongly about that.

[tokers]

Another useful route match type is the TLS matching (according to the SNI), is this also in the plan?

[Miciah]

Have you considered making AddressMatch a union type, with Type being the discriminator? Having a generic Value field looks odd in a Kubernetes API, and it forfeits the opportunity to enforce stricter validation.

[shaneutt]

I had not as I was very intentionally trying to make this similar to GatewayAddress so that we didn't end up with two different ways of describing an address, do you have any thoughts on that?

[Miciah]

Ah, I can see how making AddressMatch similar to GatewayAddress could reduce confusion if the user wanted to specify the same IP address or named address on both the listener and an address match. On the other hand, AddressMatch is missing Hostname, the semantics of NamedAddress across the two types gets a little funny, and I'm wary of making things appear alike superficially when they are fundamentally different.

Elaborating on what I mean by funny semantics, do the names that the user can specify for a NamedAddress in the listener refer to the same set of things as the names that the user can specify for a NamedAddress in a source match? What about for destination matches? Is this relationship (or lack thereof) entirely up to the implementation to define?

(I now kind of wish we had used a union type for GatewayAddress, but it's probably too late to change that now without an extremely compelling need.)

[robscott]

For better or worse, we've used the Type and Value pair heavily throughout the API, including path, header, and query param matching. I believe HTTPRoute filters are the only place we've used a proper union type. Although union types are common in upstream Kubernetes, I think the pattern proposed here is also quite common.

Since we're dealing with CRDs and there is not proper union support yet, we'd be stuck with adding additional webhook validation if we transitioned to a union type. My personal preference would be to continue with the proposed approach to more closely match the pattern we've used elsewhere in Gateway API.

[Miciah]

Using union types probably would have made sense in those other contexts (and in hindsight, it would have simplified defining support levels). It makes even more sense for AddressMatch because we can specify OpenAPI validation to enforce that any value that the user puts in, say, the "address" field must be either an IPv4 or an IPv6 address (the following is incomplete and not tested, but this example should illustrate what I mean):

sourceAddresses:
  items:
    oneOf:
    - properties:
        address:
          format: ipv4
    - properties:
        address:
          format: ipv6
    properties:
      address:
        type: string
    type: object
  type: array

Granted, CRD validation may have gaps (if I recall correctly, it admits an object definition that specifies a field that doesn't correspond to the discriminator value). However, I figure that some validation is better than none.

I think we'd need webhook validation anyway to validate that the user provided a valid regexp for the path, header, and query param matching, but regexp support is "Custom" and underspecified anyway, so it's fair to leave that up to implementors.

Using a union type is just a suggestion for enabling slightly improved validation and clarity, so I won't press it if the goal is to make AddressMatch match GatewayAddress.

[youngnick]

I think the problem with both using a union type and having more interesting OpenAPI validation is that neither fits well through the CRD-validation-shaped hole we have available for now.

I also think that having some more complex validation in either the webhook or an exported library (as we've discussed briefly a few times) would be beneficial.

[shaneutt]

@Miciah I like your suggestion to use a union type, but I'm also aware of some of the other implications that were brought up by @robscott and @youngnick. I'm inclined to think that the use of union types are generally preferable but given the existing precedents perhaps doing that needs to be a holistic effort and not this specific PR starting some of the APIs in a different direction than the rest and starting an implementation dichotomy. I think a reasonable action and resolution for this comment may be to use what we have now for this PR and create a follow up issue that effectively states "convert everything to union types and significantly improve our CRD validation" what are your thoughts on that?

[shaneutt]

@Miciah just wanted to double check this?

[Miciah]

I agree, please disregard the suggestion to use a union type as far as this PR is concerned.

[hbagdi]

Another useful route match type is the TLS matching (according to the SNI), is this also in the plan?

Gateway API already supports that. Please take a look at TLSRoute.

[youngnick]

I really like the shape of this, but have a couple of small things about the exact field semantics. In general, I think we should add enum fields very carefully, as they are tricky to manage, because adding values to an enum is a breaking API change (by the API guidelines).

[youngnick]

I think the problem with both using a union type and having more interesting OpenAPI validation is that neither fits well through the CRD-validation-shaped hole we have available for now.

I also think that having some more complex validation in either the webhook or an exported library (as we've discussed briefly a few times) would be beneficial.

[hbagdi]

LGTM once there is clarity on the proxy protocol.

[hbagdi]

/lgtm

/hold for other reviews

[robscott]

Thanks! Will leave final LGTM for someone else (cc @bowei @hbagdi @jpeach @youngnick).

/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, shaneutt

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:

• OWNERS [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[youngnick]

/lgtm

[candita]

@shaneutt what happens once the GEP is accepted? Who completes the work?

Update: I found the PR in v1alpha2, and the revert of the GEP to provisional. Bummer.

[shaneutt]

@shaneutt what happens once the GEP is accepted? Who completes the work?

Update: I found the PR in v1alpha2, and the revert of the GEP to provisional. Bummer.

Indeed, given the current state this GEP's actual status is closest to being considered declined. If you are feeling strongly about it, you could move it forward or even create a new one that follows up on it with a different implemention. Extra context: I created this GEP entirely because there was an open ask to do this, I did not have and still do not have a strong need for this functionality in my own implementations currently so I'm inclined to call it declined and wait for someone else who has a need to come along later.