Implementing GEP 724: Refresh Route-Gateway Binding #754

robscott · 2021-08-09T06:12:14Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Implements GEP #724

Which issue(s) this PR fixes:
Fixes #724

Does this PR introduce a user-facing change?:

- Routes attach to Gateways with direct object references
- Gateway Route selector has been removed
- Gateway Route group/kind restriction has changed into an optional list

Note: This PR does not include any docs yet. I will follow up with that either in this PR or a follow up.

k8s-ci-robot · 2021-08-09T06:12:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott

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

apis/v1alpha2/tlsroute_types.go

howardjohn

LGTM

hbagdi

first pass, looking good

apis/v1alpha2/gateway_types.go

hbagdi · 2021-08-09T18:54:34Z

apis/v1alpha2/tcproute_types.go

@@ -41,17 +41,29 @@ type TCPRoute struct {

 // TCPRouteSpec defines the desired state of TCPRoute
 type TCPRouteSpec struct {
+	// ParentRefs references the resources that can attach to this Route. The


// ParentRefs references the resources that can attach to this Route. The

Instead of resources, just use Gateway for now. Optimize for the vast majority.

Rewrote a good chunk of this, hopefully it's clearer now.

apis/v1alpha2/udproute_types.go

examples/v1alpha2/http-redirect.yaml

apis/v1alpha2/shared_types.go

stevesloka · 2021-08-10T01:33:35Z

apis/v1alpha2/gateway_types.go

+	//
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=253
+	Kind string `json:"kind"`


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

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.

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

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.

apis/v1alpha2/shared_types.go

robscott · 2021-08-10T21:31:17Z

Thanks for the great feedback on this @stevesloka @hbagdi and @howardjohn! I think I've responded to everything now, PTAL. I'm planning on covering docs and additional examples in a follow up PR so this one doesn't become too massive.

hbagdi

LGTM overall

apis/v1alpha2/gateway_types.go

hbagdi · 2021-08-10T21:56:21Z

apis/v1alpha2/gateway_types.go

 	// +optional
-	Hostname *Hostname `json:"hostname,omitempty"`
+	SupportedKinds []RouteGroupKind `json:"supportedKinds,omitempty"`


So if this field is not optional, does it mean minItem=1 is automatically imposed? Probably not.
What is the reason for not adding minItem=1 here?

apis/v1alpha2/shared_types.go

apis/v1alpha2/tcproute_types.go

apis/v1alpha2/udproute_types.go

apis/v1alpha2/gateway_types.go

jpeach · 2021-08-10T23:33:19Z

apis/v1alpha2/gateway_types.go

 	//
-	// +optional
-	Hostname *Hostname `json:"hostname,omitempty"`
+	// +kubebuilder:validation:MaxItems=8


Suggested change

// +kubebuilder:validation:MaxItems=8

// +kubebuilder:validation:MaxItems=16

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.

jpeach · 2021-08-10T23:36:00Z

apis/v1alpha2/gateway_types.go

-	//
-	// Support: Core
-	Kind string `json:"kind"`
+	// +kubebuilder:validation:MaxItems=8


Suggested change

// +kubebuilder:validation:MaxItems=8

// +kubebuilder:validation:MaxItems=16

Same comment as above.

apis/v1alpha2/gateway_types.go

jpeach · 2021-08-10T23:52:34Z

apis/v1alpha2/gateway_types.go

+	SupportedKinds []RouteGroupKind `json:"supportedKinds,omitempty"`
+
+	// AttachedRoutes represents the total number of Routes that have been
+	// successfully attached to this Listener.


Suggested change

// successfully attached to this Listener.

// successfully attached to this Listener.

//

// +kubebuilder:validation:Minimum=1

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

jpeach · 2021-08-10T23:54:04Z

Looks great, just a few nits to tidy up 👍

Miciah · 2021-08-11T02:37:55Z

apis/v1alpha2/gateway_types.go

+	// Support: Core
+	//
+	// +kubebuilder:validation:MaxLength=253
+	Name string `json:"name"`


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

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

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

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.

apis/v1alpha2/gateway_types.go

Miciah · 2021-08-11T02:57:41Z

apis/v1alpha2/shared_types.go

+	// ParentRef is a reference to the parent resource that the route wants to
+	// be attached to.


This is status—it should report actual state, not desired state.

Suggested change

// ParentRef is a reference to the parent resource that the route wants to

// be attached to.

// ParentRef is a reference to the parent resource that the route is attached to.

This was actually an intentional distinction. We want status to include entries for each parent the Route wants to be attached to so there is a way to explain when an attachment is not allowed and/or being prevented for some reason.

Suggested change

// ParentRef is a reference to the parent resource that the route wants to

// be attached to.

// ParentRef is a reference to the parent resource to which the route's spec

// requests to be attached.

Maybe this? Reads a little awkward, I think.

Agree it was confusing. I took another shot at this, let me know what you think

apis/v1alpha2/shared_types.go

Miciah · 2021-08-11T03:04:53Z

apis/v1alpha2/gateway_types.go

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


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?

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.

apis/v1alpha2/shared_types.go

robscott · 2021-08-11T23:12:58Z

Thanks for the great feedback on this PR! I think I've addressed everything now, PTAL.

apis/v1alpha2/gateway_types.go

youngnick · 2021-08-12T01:01:01Z

Only a few small nits from me, LGTM otherwise.

robscott · 2021-08-12T04:35:13Z

Thanks for the feedback @youngnick! PTAL.

youngnick · 2021-08-12T05:38:09Z

/lgtm

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 9, 2021

k8s-ci-robot requested review from bowei and youngnick August 9, 2021 06:12

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2021

robscott force-pushed the gep-724-implementation branch from 9dbb75c to 0366715 Compare August 9, 2021 06:13

howardjohn reviewed Aug 9, 2021

View reviewed changes

apis/v1alpha2/tlsroute_types.go Outdated Show resolved Hide resolved

howardjohn reviewed Aug 9, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021

hbagdi reviewed Aug 9, 2021

View reviewed changes

This was linked to issues Aug 9, 2021

Clarify how RouteGateways would work if we supported Route->Route delegation #411

Closed

Route selector improvements on Gateway #515

Closed

robscott force-pushed the gep-724-implementation branch from 0366715 to 1e7538a Compare August 9, 2021 21:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021

robscott force-pushed the gep-724-implementation branch from 1e7538a to 052bf1f Compare August 9, 2021 21:46

hbagdi linked an issue Aug 9, 2021 that may be closed by this pull request

Add support for multiple kinds within a single Gateway listener #699

Closed

robscott force-pushed the gep-724-implementation branch 2 times, most recently from 1edfec1 to 5a758ad Compare August 10, 2021 00:00

stevesloka reviewed Aug 10, 2021

View reviewed changes

robscott force-pushed the gep-724-implementation branch from 5a758ad to 2a686f0 Compare August 10, 2021 21:26

hbagdi reviewed Aug 10, 2021

View reviewed changes

jpeach reviewed Aug 10, 2021

View reviewed changes

apis/v1alpha2/gateway_types.go Show resolved Hide resolved

jpeach reviewed Aug 10, 2021

View reviewed changes

robscott mentioned this pull request Aug 10, 2021

Documenting Route attachment for v1alpha2 #762

Merged

jpeach reviewed Aug 10, 2021

View reviewed changes

apis/v1alpha2/gateway_types.go Outdated Show resolved Hide resolved

jpeach reviewed Aug 10, 2021

View reviewed changes

Miciah reviewed Aug 11, 2021

View reviewed changes

hbagdi mentioned this pull request Aug 11, 2021

Stricter regex-based validation #767

Closed

robscott added this to the v1alpha2 milestone Aug 11, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021

robscott added 3 commits August 11, 2021 15:53

Implementing GEP 724: Refresh Route-Gateway Binding

07dd063

Tweaks in response to PR feedback

a3b92b7

Introducing CommonRouteSpec struct, other tweaks from PR feedback

718de05

robscott force-pushed the gep-724-implementation branch from 3843d86 to 90ebdde Compare August 11, 2021 23:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021

Tweaks to wording and capitalization in godocs

bad1af9

robscott force-pushed the gep-724-implementation branch from 90ebdde to bad1af9 Compare August 11, 2021 23:07

robscott mentioned this pull request Aug 12, 2021

Implementing and Documenting GEP 746 (TLS Updates) #768

Merged

youngnick reviewed Aug 12, 2021

View reviewed changes

apis/v1alpha2/gateway_types.go Outdated Show resolved Hide resolved

Additional tweaks to godocs

67902eb

k8s-ci-robot assigned youngnick Aug 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2021

k8s-ci-robot merged commit f573729 into kubernetes-sigs:master Aug 12, 2021

robscott mentioned this pull request Aug 18, 2021

v1alpha2 feedback #790

Closed

10 tasks

robscott deleted the gep-724-implementation branch January 8, 2022 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing GEP 724: Refresh Route-Gateway Binding #754

Implementing GEP 724: Refresh Route-Gateway Binding #754

robscott commented Aug 9, 2021

k8s-ci-robot commented Aug 9, 2021

howardjohn left a comment

hbagdi left a comment

hbagdi Aug 9, 2021

robscott Aug 9, 2021

stevesloka Aug 10, 2021

robscott Aug 10, 2021

jpeach Aug 10, 2021

robscott Aug 11, 2021

robscott commented Aug 10, 2021

hbagdi left a comment

hbagdi Aug 10, 2021

jpeach Aug 10, 2021

robscott Aug 11, 2021

jpeach Aug 10, 2021

robscott Aug 11, 2021

jpeach Aug 10, 2021

robscott Aug 11, 2021

jpeach commented Aug 10, 2021

Miciah Aug 11, 2021

hbagdi Aug 11, 2021

robscott Aug 11, 2021

hbagdi Aug 11, 2021

robscott Aug 11, 2021

Miciah Aug 11, 2021

robscott Aug 11, 2021

youngnick Aug 12, 2021

robscott Aug 12, 2021

Miciah Aug 11, 2021

robscott Aug 11, 2021

robscott commented Aug 11, 2021

youngnick commented Aug 12, 2021

robscott commented Aug 12, 2021

youngnick commented Aug 12, 2021

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

		// ParentRef is a reference to the parent resource that the route wants to
		// be attached to.

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

Implementing GEP 724: Refresh Route-Gateway Binding #754

Implementing GEP 724: Refresh Route-Gateway Binding #754

Conversation

robscott commented Aug 9, 2021

k8s-ci-robot commented Aug 9, 2021

howardjohn left a comment

Choose a reason for hiding this comment

hbagdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Aug 10, 2021

hbagdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Aug 11, 2021

youngnick commented Aug 12, 2021

robscott commented Aug 12, 2021

youngnick commented Aug 12, 2021