Skip to content
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.

Sign up for GitHub

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

feat: implement L4 traffic matching #976

Merged
merged 2 commits into from
Jan 18, 2022
Merged

feat: implement L4 traffic matching #976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26dec1c
docs: tweak GEP 735 for PR reviews
shaneutt Jan 18, 2022
5f448f5
feat: implement L4 traffic matching (GEP-735)
shaneutt Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 1 addition & 34 deletions apis/v1alpha2/gateway_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,7 @@ type GatewayAddress struct {
// Type of the address.
//
// +optional
// +kubebuilder:validation:Enum=IPAddress;Hostname;NamedAddress
// +kubebuilder:default=IPAddress
Type *AddressType `json:"type,omitempty"`

Expand All @@ -462,40 +463,6 @@ type GatewayAddress struct {
Value string `json:"value"`
}

// AddressType defines how a network address is represented as a text string.
//
// If the requested address is unsupported, the controller
// should raise the "Detached" listener status condition on
// the Gateway with the "UnsupportedAddress" reason.
//
// +kubebuilder:validation:Enum=IPAddress;Hostname;NamedAddress
type AddressType string

const (
// A textual representation of a numeric IP address. IPv4
// addresses must be in dotted-decimal form. IPv6 addresses
// must be in a standard IPv6 text representation
// (see [RFC 5952](https://tools.ietf.org/html/rfc5952)).
//
// Support: Extended
IPAddressType AddressType = "IPAddress"

// A Hostname represents a DNS based ingress point. This is similar to the
// corresponding hostname field in Kubernetes load balancer status. For
// example, this concept may be used for cloud load balancers where a DNS
// name is used to expose a load balancer.
//
// Support: Extended
HostnameAddressType AddressType = "Hostname"

// A NamedAddress provides a way to reference a specific IP address by name.
// For example, this may be a name or other unique identifier that refers
// to a resource on a cloud provider such as a static IP.
//
// Support: Implementation-Specific
NamedAddressType AddressType = "NamedAddress"
)

// GatewayStatus defines the observed state of Gateway.
type GatewayStatus struct {
// Addresses lists the IP addresses that have actually been
Expand Down
83 changes: 83 additions & 0 deletions apis/v1alpha2/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,3 +399,86 @@ type AnnotationKey string
// +kubebuilder:validation:MinLength=0
// +kubebuilder:validation:MaxLength=4096
type AnnotationValue string

// AddressRouteMatches defines AddressMatch rules for inbound traffic according to
// source and/or destination address of a packet or connection.
type AddressRouteMatches struct {
// SourceAddresses indicates the originating (source) network
// addresses which are valid for routing traffic.
//
// Support: Extended
SourceAddresses []AddressMatch `json:"sourceAddresses"`

// DestinationAddresses indicates the destination network addresses
// which are valid for routing traffic.
//
// Support: Extended
DestinationAddresses []AddressMatch `json:"destinationAddresses"`
}

// AddressMatch defines matching rules for network addresses by type.
type AddressMatch struct {
// Type of the address, either IPAddress or NamedAddress.
//
// If NamedAddress is used this is a custom and specific value for each
// implementation to handle (and add validation for) according to their
// own needs.
//
// For IPAddress the implementor may expect either IPv4 or IPv6.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream differentiates between v4 and v6:

Should we?

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

//
// Support: Core (IPAddress)
// Support: Custom (NamedAddress)
//
// +optional
// +kubebuilder:validation:Enum=IPAddress;NamedAddress
// +kubebuilder:default=IPAddress
Type *AddressType `json:"type,omitempty"`

// Value of the address. The validity of the values will depend
// on the type and support by the controller.
//
// If implementations support proxy-protocol (see:
// https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt) they
// must respect the connection metadata from proxy-protocol
// in the match logic implemented for these address values.
//
// Examples: `1.2.3.4`, `128::1`, `my-named-address`.
//
// Support: Core
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Value string `json:"value"`
}

// AddressType defines how a network address is represented as a text string.
type AddressType string

const (
// A textual representation of a numeric IP address. IPv4
// addresses must be in dotted-decimal form. IPv6 addresses
// must be in a standard IPv6 text representation
// (see [RFC 5952](https://tools.ietf.org/html/rfc5952)).
//
// This type is intended for specific addresses. Address ranges are not
// supported (e.g. you can not use a CIDR range like 127.0.0.0/24 as an
// IPAddress).
//
// Support: Extended
IPAddressType AddressType = "IPAddress"

// A Hostname represents a DNS based ingress point. This is similar to the
// corresponding hostname field in Kubernetes load balancer status. For
// example, this concept may be used for cloud load balancers where a DNS
// name is used to expose a load balancer.
//
// Support: Extended
HostnameAddressType AddressType = "Hostname"

// A NamedAddress provides a way to reference a specific IP address by name.
// For example, this may be a name or other unique identifier that refers
// to a resource on a cloud provider such as a static IP.
//
// Support: Implementation-Specific
NamedAddressType AddressType = "NamedAddress"
)
7 changes: 7 additions & 0 deletions apis/v1alpha2/tcproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ type TCPRouteStatus struct {

// TCPRouteRule is the configuration for a given rule.
type TCPRouteRule struct {
// Matches are rules for routing traffic to backends based on addresses.
//
// +optional
// +kubebuilder:validation:MaxItems=16
// <gateway:experimental>
Matches []AddressRouteMatches `json:"matches,omitempty"`

// BackendRefs defines the backend(s) where matching requests should be
// sent. If unspecified or invalid (refers to a non-existent resource or a
// Service with no endpoints), the underlying implementation MUST actively
Expand Down
7 changes: 7 additions & 0 deletions apis/v1alpha2/udproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ type UDPRouteStatus struct {

// UDPRouteRule is the configuration for a given rule.
type UDPRouteRule struct {
// Matches add rules for filtering traffic to backends based on addresses.
//
// +optional
// +kubebuilder:validation:MaxItems=16
// <gateway:experimental>
Matches []AddressRouteMatches `json:"matches,omitempty"`

// BackendRefs defines the backend(s) where matching requests should be
// sent. If unspecified or invalid (refers to a non-existent resource or a
// Service with no endpoints), the underlying implementation MUST actively
Expand Down
63 changes: 63 additions & 0 deletions apis/v1alpha2/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

88 changes: 88 additions & 0 deletions config/crd/experimental/gateway.networking.k8s.io_tcproutes.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading