feat: implement L4 traffic matching

[shaneutt]

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This implements GEP 735.

Does this PR introduce a user-facing change?:

- optional L4 traffic matching has been added for `TCPRoute` and `UDPRoute`

[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

[hbagdi]

Upstream differentiates between v4 and v6:

• https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services
• https://kubernetes.io/docs/concepts/services-networking/endpoint-slices/

Should we?

Furthermore, do we need to think of cases where an implementation doesn't support IPv6 for any reason? Too hypothetical?

[shaneutt]

Upstream differentiates between v4 and v6 should we?

My initial thoughts are: one reason we might want to differentiate would be if one or the other is not available on the Gateway network (e.g. the Gateway only has IPv4 OR IPv6). Given that we're simply using the existing IPAddress type which already supported both IPv4 and IPv6 I feel like we would need a strong reason and a separate body of work to add this differentiation as it seems possible it would end up being backwards incompatible.

Furthermore, do we need to think of cases where an implementation doesn't support IPv6 for any reason? Too hypothetical?

To expand on that I think there could be cases where IPv4 isn't supported too, but as you've already suggested I personally wouldn't want to spend cycles trying to predict these kinds of situations. We have until February (at least) before we're going to start talking beta release, there's time in the interim for such use cases to present themselves.

[hbagdi]

Given that we're simply using the existing IPAddress type which already supported both IPv4 and IPv6 I feel like we would need a strong reason and a separate body of work to add this differentiation as it seems possible it would end up being backwards incompatible.

Let's discuss this in the office hour today. We will be asked about dual-stack as soon as this API hits beta and we need to have a good answer/plan for it. Best to follow upstream closely.

[hbagdi]

My bad. No meeting today.
cc @robscott who may be more familiar with upstream dual-stack work

[robscott]

Upstream differentiates between v4 and v6

Although that's true, at least part of that was motivated by simpler implementation. For example, EndpointSlices initially started with a single IPAddress type in v1alpha1 that was replaced with separate v4 and v6 types in large part to simplify proxy implementations where v4 and v6 addresses were handled entirely separately.

It does seem like it would be useful to be able to request the IP families you want to have assigned to a Gateway. I'm less sure that it's helpful to have separate matching types for v4 and v6 addresses. In that case, the families can already be derived by the value provided and a separate type does not seem as useful to me.

@khenidak has been most involved in upstream dual stack work, what do you think about representing IP matching? Should we have separate address types per IP family or rely on deriving that information from the address/cidr itself?

[shaneutt]

This comment seems to be the lingering question, but has not gotten any further feedback in over a week. Given that this PR is not actually changing how we differentiate between v4 and v6 (this uses the pre-existing AddressType which already accepts both) I would like to suggest that we should split out reworking how we configure and differentiate IP versions into a separate GEP or body of work so that it doesn't have to hold this PR up?

[brianehlert]

I am looking at this now for CRDs (outside of Gateway) and I am leaning to follow Service which uses ipfamilies to follow existing semantics.
To easily express: match ipv6 traffic, match ipv4 traffic, match both
@shaneutt is that what you are thinking AddressType would do, equivalent-ish to Service ipfamilies?
This could allow matching on family alone without requiring the person to define a match pattern.
To your point, always requiring a match pattern to signify anything other than 'both' is only one key.

[shaneutt]

If I'm understanding correctly, yes that's basically what I had in mind.

[bowei]

As the value is a specific Value, having an additional IP family field seems to be redundant.

Minor style point, we should write all of the comment from the perspective of the users (the implemetors should...) style comments need to live somewhere else...

[robscott]

Thanks for your work on this @shaneutt! Mostly LGTM, just a few small nits.

[robscott]

Thanks for all the work on this @shaneutt! I think you and @jpeach may be working on bigger changes here, but just remembered that we need to guard all these new fields as experimental to start, so added some comments related to that.

[robscott]

Thanks for the work on this @shaneutt! This LGTM other than a couple tiny nits.

[robscott]

Thanks @shaneutt!

/lgtm
/approve

[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