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

Jump to bottom

Implementing GEP 724: Refresh Route-Gateway Binding #754

Merged
merged 5 commits into from
Aug 12, 2021
Merged

Implementing GEP 724: Refresh Route-Gateway Binding #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
07dd063
Implementing GEP 724: Refresh Route-Gateway Binding
robscott Aug 9, 2021
a3b92b7
Tweaks in response to PR feedback
robscott Aug 10, 2021
718de05
Introducing CommonRouteSpec struct, other tweaks from PR feedback
robscott Aug 11, 2021
bad1af9
Tweaks to wording and capitalization in godocs
robscott Aug 11, 2021
67902eb
Additional tweaks to godocs
robscott Aug 12, 2021
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
131 changes: 65 additions & 66 deletions apis/v1alpha2/gateway_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ type GatewaySpec struct {
//
// Support: Core
//
// +listType=map
// +listMapKey=name
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=64
Listeners []Listener `json:"listeners"`
Expand Down Expand Up @@ -141,6 +143,15 @@ type GatewaySpec struct {
// combination of Hostname, Port, and Protocol. This will be enforced by a
// validating webhook.
type Listener struct {
// Name is the name of the Listener. If more than one Listener is present
// each Listener MUST specify a name. The names of Listeners MUST be unique
// within a Gateway.
//
// Support: Core
//
// +kubebuilder:validation:MaxLength=253
Name string `json:"name"`
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to restrict the name a little more, for example with +kubebuilder:validation:Pattern=`^[0-9A-Za-z_-]+$`.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL, I didn't know this was even possible.
Out of scope of this PR, should we use this in most places?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I've been thinking this would be a good time to introduce some standard regex validation for these fields as part of v1alpha2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added #767

Copy link
Member Author

Choose a reason for hiding this comment

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

I completely agree that we should do this, but want to leave this for a follow up PR so we can establish consistent regexes for different types of fields. #767 is in the v1alpha2 milestone so we can make sure it gets in.


// Hostname specifies the virtual hostname to match for protocol types that
// define this concept. When unspecified, "", or `*`, all hostnames are
// matched. This field can be omitted for protocols that don't require
Expand Down Expand Up @@ -198,18 +209,7 @@ type Listener struct {
// +optional
TLS *GatewayTLSConfig `json:"tls,omitempty"`

// Routes specifies a schema for associating routes with the
// Listener using selectors. A Route is a resource capable of
// servicing a request and allows a cluster operator to expose
// a cluster resource (i.e. Service) by externally-reachable
// URL, load-balance traffic and terminate SSL/TLS. Typically,
// a route is a "HTTPRoute" or "TCPRoute" in group
// "gateway.networking.k8s.io", however, an implementation may support
// other types of resources.
//
// The Routes selector MUST select a set of objects that
// are compatible with the application protocol specified in
// the Protocol field.
// Routes specifies which Routes may be attached to this Listener.
hbagdi marked this conversation as resolved.
Show resolved Hide resolved
//
// Although a client request may technically match multiple route rules,
// only one rule may ultimately receive the request. Matching precedence
Expand All @@ -232,7 +232,9 @@ type Listener struct {
// invalid, the rest of the Route should still be supported.
//
// Support: Core
Routes RouteBindingSelector `json:"routes"`
// +kubebuilder:default={namespaces:{from: Same}}
// +optional
Routes *ListenerRoutes `json:"routes,omitempty"`
}

// ProtocolType defines the application protocol accepted by a Listener.
Expand Down Expand Up @@ -374,59 +376,33 @@ const (
TLSModePassthrough TLSModeType = "Passthrough"
)

// RouteBindingSelector defines a schema for associating routes with the Gateway.
// If Namespaces and Selector are defined, only routes matching both selectors are
// associated with the Gateway.
type RouteBindingSelector struct {
// Namespaces indicates in which namespaces Routes should be selected
// for this Gateway. This is restricted to the namespace of this Gateway by
// ListenerRoutes defines which Routes may be attached to this Listener.
type ListenerRoutes struct {
// Namespaces indicates which namespaces Routes may be attached to this
// Listener from. This is restricted to the namespace of this Gateway by
// default.
//
// Support: Core
//
// +optional
// +kubebuilder:default={from: Same}
Namespaces *RouteNamespaces `json:"namespaces,omitempty"`
// Selector specifies a set of route labels used for selecting
// routes to associate with the Gateway. If this Selector is defined,
// only routes matching the Selector are associated with the Gateway.
// An empty Selector matches all routes.
//
// Support: Core
//
// +optional
Selector *metav1.LabelSelector `json:"selector,omitempty"`
// Group is the group of the route resource to select. Omitting the value
// indicates the gateway.networking.k8s.io API group.
// For example, use the following to select an HTTPRoute:
//
// routes:
// kind: HTTPRoute
//
// Otherwise, if an alternative API group is desired, specify the desired
// group:

// Kinds specifies the groups and kinds of Routes that are allowed to bind
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Kinds specifies the groups and kinds of Routes that are allowed to bind
// Kinds specifies the groups and kinds of Routes that are allowed to attach

for consistency and to save me from wondering whether "bind" and "attach" mean different things... which they don't, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not intended to mean anything different, just ended up having more attach terminology in the godocs this time around because the field started with an attachTo name.

// to this Gateway Listener. When unspecified or empty, the kinds of Routes
// selected are determined using the Listener protocol.
//
// routes:
// group: acme.io
// kind: FooRoute
// A RouteGroupKind MUST correspond to kinds of Routes that are compatible
// with the application protocol specified in the Listener's Protocol field.
// If an implementation does not support or recognize this resource type, it
// MUST set the "ResolvedRefs" condition to false for this Listener with the
// "InvalidRoutesRef" reason.
Copy link
Contributor

Choose a reason for hiding this comment

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

kinds the plural - that's not a violation of upstream conventions, is that right? Just making sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I understand this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

The field is called Kinds, I think that's fine because it's a list of Kind objects?

//
// Support: Core
//
// +optional
// +kubebuilder:default=gateway.networking.k8s.io
// +kubebuilder:validation:MaxLength=253
Group *string `json:"group,omitempty"`
// Kind is the kind of the route resource to select.
//
// Kind MUST correspond to kinds of routes that are compatible with the
// application protocol specified in the Listener's Protocol field.
//
// If an implementation does not support or recognize this
// resource type, it SHOULD set the "ResolvedRefs" condition to false for
// this listener with the "InvalidRoutesRef" reason.
//
// Support: Core
Kind string `json:"kind"`
// +kubebuilder:validation:MaxItems=8
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// +kubebuilder:validation:MaxItems=8
// +kubebuilder:validation:MaxItems=16

Copy link
Member Author

Choose a reason for hiding this comment

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

Same comment as above.

Kinds []RouteGroupKind `json:"kinds,omitempty"`
}

// RouteSelectType specifies where Routes should be selected by a Gateway.
Expand Down Expand Up @@ -468,6 +444,22 @@ type RouteNamespaces struct {
Selector *metav1.LabelSelector `json:"selector,omitempty"`
}

// RouteGroupKind indicates the group and kind of a Route resource.
type RouteGroupKind struct {
robscott marked this conversation as resolved.
Show resolved Hide resolved
// Group is the group of the Route.
//
// +optional
// +kubebuilder:default=gateway.networking.k8s.io
// +kubebuilder:validation:MaxLength=253
Group *string `json:"group,omitempty"`

// Kind is the kind of the Route.
robscott marked this conversation as resolved.
Show resolved Hide resolved
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Kind string `json:"kind"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a status that should be set if the Kind is invalid or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great question, but unfortunately I don't have a good answer. This is one of the many situations where it would be unclear which controller was responsible for communicating that status. Of course every controller only knows the resources it supports, not what other implementations may support. I think the best indication of this for now would just be if this resource was missing from the RouteParents status altogether since no controller would have added it.

Copy link
Contributor

Choose a reason for hiding this comment

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

  // Kind MUST correspond to kinds of Routes that are compatible with the    
  // application protocol specified in the Listener's Protocol field. If an    
  // implementation does not support or recognize this resource type, it    
  // SHOULD set the "ResolvedRefs" condition to false for this listener with
  // the "InvalidRoutesRef" reason.

ListenerRoutes says this case should raise a status condition (on the gateway).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh woops, I misinterpreted this as a question about an invalid ref from a Route to a Gateway. The answer from @jpeach is completely correct for what was actually being asked here.

}

// GatewayAddress describes an address that can be bound to a Gateway.
type GatewayAddress struct {
// Type of the address.
Expand Down Expand Up @@ -558,7 +550,7 @@ type GatewayStatus struct {
//
// +optional
// +listType=map
// +listMapKey=port
robscott marked this conversation as resolved.
Show resolved Hide resolved
// +listMapKey=name
// +kubebuilder:validation:MaxItems=64
Listeners []ListenerStatus `json:"listeners,omitempty"`
}
Expand Down Expand Up @@ -663,19 +655,26 @@ const (

// ListenerStatus is the status associated with a Listener.
type ListenerStatus struct {
// Port is the unique Listener port value for which this message is
// reporting the status.
Port PortNumber `json:"port"`

// Protocol is the Listener protocol value for which this message is
// reporting the status.
Protocol ProtocolType `json:"protocol"`
// Name is the name of the Listener. If the Gateway has more than one
// Listener present, each ListenerStatus MUST specify a name. The names of
// ListenerStatus objects MUST be unique within a Gateway.
//
// +kubebuilder:validation:MaxLength=253
Name string `json:"name"`

// Hostname is the Listener hostname value for which this message is
// reporting the status.
// SupportedKinds is the list indicating the Kinds supported by this
// listener. When this is not specified on the Listener, this MUST represent
// the kinds an implementation supports for the specified protocol. When
// there are kinds specified on the Listener, this MUST represent the
// intersection of those kinds and the kinds supported by the implementation
// for the specified protocol.
//
// +optional
Hostname *Hostname `json:"hostname,omitempty"`
// +kubebuilder:validation:MaxItems=8
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// +kubebuilder:validation:MaxItems=8
// +kubebuilder:validation:MaxItems=16

Copy link
Member Author

Choose a reason for hiding this comment

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

At present we only have 4 valid Route kinds, and it's unlikely/impossible that they'd all be valid for a given listener. We can always increase this limit in the future if needed, but I'd be hesitant to start too high here.

SupportedKinds []RouteGroupKind `json:"supportedKinds"`

// AttachedRoutes represents the total number of Routes that have been
// successfully attached to this Listener.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// successfully attached to this Listener.
// successfully attached to this Listener.
//
// +kubebuilder:validation:Minimum=1

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't it valid for this to be 0? No guarantee that a Route will be attached to a Listener.

AttachedRoutes int32 `json:"attachedRoutes"`
Copy link
Contributor

Choose a reason for hiding this comment

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

How about throwing an upper limit here and seeing if someone crosses that limit?
Helpful for future maintainers as they can be certain about n in O(n).

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually I think of upper limits as being useful to help limit the max size/complexity of a resource. In this case I don't think a limit would help with either of those. I think it could be reasonable for implementations on their own to limit the number of attached routes they'd support per Listener, but I'm not sure there's as much value for us limiting this value here.


// Conditions describe the current condition of this listener.
//
Expand Down
6 changes: 1 addition & 5 deletions apis/v1alpha2/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,7 @@ type HTTPRouteList struct {

// HTTPRouteSpec defines the desired state of HTTPRoute
type HTTPRouteSpec struct {
// Gateways defines which Gateways can use this Route.
//
// +optional
// +kubebuilder:default={allow: "SameNamespace"}
Gateways *RouteGateways `json:"gateways,omitempty"`
CommonRouteSpec `json:",inline"`

// Hostnames defines a set of hostname that should match against
// the HTTP Host header to select a HTTPRoute to process the request.
Expand Down
Loading