Another round of v1alpha2 cleanup #839

robscott · 2021-08-27T23:09:47Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This takes care of another big chunk of feedback from #780 as well as some of the items in #790.

Does this PR introduce a user-facing change?:

* "Controller" has been renamed to "ControllerName"
* "Admitted" condition has been renamed to "Accepted" and now defaults to an "Unknown" state instead of "False"

k8s-ci-robot · 2021-08-27T23:09:55Z

[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

howardjohn · 2021-08-29T18:56:34Z

apis/v1alpha2/gateway_types.go

@@ -72,6 +72,9 @@ type GatewaySpec struct {
 	// logical endpoints that are bound on this Gateway's addresses.
 	// At least one Listener MUST be specified.
 	//
+	// Each listener in a Gateway must have a unique combination of Hostname,
+	// Port, and Protocol. This will be enforced by a validating webhook.


nit: how it will be enforced doesn't seem like it should be in the docs?

Recommend adding an issue instead.

I'd include the TLS details in that list as well, tbh.

Created #847 to track webhook validation.

I'd include the TLS details in that list as well, tbh.

Do you mean that hostname, listener, and protocol can be identical across listener if TLS is different?

Removed reference to webhook in godocs here.

Do you mean that hostname, listener, and protocol can be identical across listener if TLS is different?

Oh, you're quite right, that makes no sense. I was more thinking that TLS details are part of the key for this, but you're right that if the hostname, port, and protocol are identical, then the TLS details must not be different.

apis/v1alpha2/gatewayclass_types.go

apis/v1alpha2/tlsroute_types.go

howardjohn · 2021-08-29T19:02:13Z

apis/v1alpha2/tlsroute_types.go

+	//
+	// If hostnames do not match with the criteria above, then the TLSRoute is
+	// not accepted, and the implementation must raise an 'Accepted' Condition
+	// with a status of `False` for the target Listener(s).


This is a bit confusing when not using sectionName in parentRefs. Since the target is not a listener

Yeah, this comes down to the behavior with partial Listener acceptance, I think if we mandate "all", and have a ResolvedRefs in the event not all match, then this covers the sectionName case.

I agree that we need a better way to handle status when sectionName is not specified. 2 options I've been thinking of:

A standard/reserved listener name such as "all" that is used in status to cover failed attachment to the Gateway as a whole.

A new set of condition types for Gateway status specifically for this purpose. (We could potentially reuse the existing listener condition types).

I think option 2 seems better - if no listeners are valid, that seems very relevant for top-level status, but I would have thought we had that covered already somehow.

howardjohn · 2021-08-29T19:02:42Z

apis/v1alpha2/tlsroute_types.go

+	// TLSRoute specified `test.example.com` and `test.example.net`,
+	// `test.example.net` must not be considered for a match.
+	//
+	// If hostnames do not match with the criteria above, then the TLSRoute is


I would clarify: If <any,all> hostnames .... And should be All IMO

+1 that it the Route should only not be accepted if all hostnames do not match, and if at least one does, a ResolvedRefs condition should be raised as well.

+1 that it the Route should only not be accepted if all hostnames do not match

Updated, thanks!

and if at least one does, a ResolvedRefs condition should be raised as well.

Wouldn't this result in a successful attachment? Why would we raise a ResolvedRefs condition?

I guess you're right, the thing I wanted to make sure we have is to indicate what hostnames are in use somehow. If you've got a bunch of hostnames and only one is in use, that seems like it would be good to know.

hbagdi · 2021-08-30T20:21:06Z

apis/v1alpha2/gateway_types.go

@@ -72,6 +72,9 @@ type GatewaySpec struct {
 	// logical endpoints that are bound on this Gateway's addresses.
 	// At least one Listener MUST be specified.
 	//
+	// Each listener in a Gateway must have a unique combination of Hostname,
+	// Port, and Protocol. This will be enforced by a validating webhook.


Recommend adding an issue instead.

apis/v1alpha2/gateway_types.go

hbagdi · 2021-08-30T20:25:15Z

apis/v1alpha2/gateway_types.go

+	//   [effective request URI](https://tools.ietf.org/html/rfc7230#section-5.5)
+	//   or the [:authority pseudo-header](https://tools.ietf.org/html/rfc7540#section-8.1.2.3)
+	// * HTTPS: The Hostname match MUST be applied at both the TLS and HTTP
+	//   protocol layers.


@youngnick Is this sufficient to close the domain fronting issue (#72) as well?

Although this isn't entirely new content, now that I'm seeing it here, I think this wording may be a bit too strong. I'm not sure that every implementation can apply a match at both layers.

I think the direction is good, but maybe we need to specify this in terms of API objects, like:

- TLS: the Listener hostname must match the SNI. - HTTP: The HTTPRoute hostname must match the Host header of the request. - HTTPS: The Listener Hostname and the HTTPRoute hostname must agree with each other, and also match request properties as above.

Because the important part is that we expect that Listeners and HTTPRoutes that do not match these rules should not end up attached.

I don't think we should mix up the route type with the listener protocol here. While there's likely a ~99% overlap here, if we tie route types too closely here, we prevent or at least complicate extended route types. I've copied most of your suggestions from above, but removed any references to Routes - the following paragraph already covers that intersection.

apis/v1alpha2/tcproute_types.go

apis/v1alpha2/tlsroute_types.go

youngnick

Overall LGTM, some small comments.

youngnick · 2021-08-31T05:01:02Z

apis/v1alpha2/gateway_types.go

@@ -72,6 +72,9 @@ type GatewaySpec struct {
 	// logical endpoints that are bound on this Gateway's addresses.
 	// At least one Listener MUST be specified.
 	//
+	// Each listener in a Gateway must have a unique combination of Hostname,
+	// Port, and Protocol. This will be enforced by a validating webhook.


I'd include the TLS details in that list as well, tbh.

youngnick · 2021-08-31T05:04:02Z

apis/v1alpha2/gateway_types.go

+	//   [effective request URI](https://tools.ietf.org/html/rfc7230#section-5.5)
+	//   or the [:authority pseudo-header](https://tools.ietf.org/html/rfc7540#section-8.1.2.3)
+	// * HTTPS: The Hostname match MUST be applied at both the TLS and HTTP
+	//   protocol layers.


I think the direction is good, but maybe we need to specify this in terms of API objects, like:

- TLS: the Listener hostname must match the SNI. - HTTP: The HTTPRoute hostname must match the Host header of the request. - HTTPS: The Listener Hostname and the HTTPRoute hostname must agree with each other, and also match request properties as above.

youngnick · 2021-08-31T05:04:54Z

apis/v1alpha2/gateway_types.go

+	//   [effective request URI](https://tools.ietf.org/html/rfc7230#section-5.5)
+	//   or the [:authority pseudo-header](https://tools.ietf.org/html/rfc7540#section-8.1.2.3)
+	// * HTTPS: The Hostname match MUST be applied at both the TLS and HTTP
+	//   protocol layers.


Because the important part is that we expect that Listeners and HTTPRoutes that do not match these rules should not end up attached.

apis/v1alpha2/gateway_types.go

apis/v1alpha2/shared_types.go

apis/v1alpha2/tlsroute_types.go

youngnick · 2021-08-31T05:17:13Z

apis/v1alpha2/tlsroute_types.go

+	// TLSRoute specified `test.example.com` and `test.example.net`,
+	// `test.example.net` must not be considered for a match.
+	//
+	// If hostnames do not match with the criteria above, then the TLSRoute is


+1 that it the Route should only not be accepted if all hostnames do not match, and if at least one does, a ResolvedRefs condition should be raised as well.

youngnick · 2021-08-31T05:17:47Z

apis/v1alpha2/tlsroute_types.go

+	//
+	// If hostnames do not match with the criteria above, then the TLSRoute is
+	// not accepted, and the implementation must raise an 'Accepted' Condition
+	// with a status of `False` for the target Listener(s).


Yeah, this comes down to the behavior with partial Listener acceptance, I think if we mandate "all", and have a ResolvedRefs in the event not all match, then this covers the sectionName case.

robscott · 2021-09-02T00:03:35Z

@youngnick @hbagdi @howardjohn Thanks for the great feedback on this! I think I've responded to everything, PTAL.

youngnick

LGTM aside from me being nitpicky about SHOULD.

I think that we need to ensure that every time we use SHOULD, we clearly explain the reasons that is not MUST, and consider using "MUST, except if" instead.

I think that SHOULD leaves too much room for interpretation without clarification.

youngnick · 2021-09-02T03:53:59Z

apis/v1alpha2/gateway_types.go

+	// * HTTPS: The Listener Hostname SHOULD match at both the TLS and HTTP
+	//   protocol layers as described above.


Sorry to go around again, but if this is a SHOULD, then I think we need to clarify, with something like:

If an implementation allows this to be the case, it MUST clearly document its behavior and how it manages the resulting security issue (different SNI and Host header can be used to bypass client certificate authentication).

I think we need to ensure that everytime we have a SHOULD, we have an explanation of why it's a SHOULD rather than a MUST, and we should (ironically) prefer must.

At the very least, the SNI MUST match the listener hostname I would think. And the HTTP Host MUST match the HTTPRoute hostname. And the listener/route hostnames MUST form an intersection. Therefor, there MUST be some intersection between the two if both are specified. For example, if I have listener hostname *.example.com, then both MUST have *.example.com.

Not that we should write that as is.. just brain dumping.

I took a shot at updating this. What I'm trying to communicate here is that if implementations can implement the match at both levels they should, but not every implementation can.

apis/v1alpha2/gateway_types.go

apis/v1alpha2/httproute_types.go

youngnick · 2021-09-06T23:31:47Z

I think you got it @robscott, nice work.

/lgtm

/hold
for another lgtm though.

apis/v1alpha2/gateway_types.go

apis/v1alpha2/httproute_types.go

jpeach · 2021-09-07T04:15:47Z

Just a couple of formatting nits.

/lgtm

youngnick · 2021-09-07T22:44:21Z

/lgtm

/hold cancel

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 27, 2021

k8s-ci-robot requested review from hbagdi and thockin August 27, 2021 23:09

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2021

robscott mentioned this pull request Aug 27, 2021

SIG-NETWORK v1alpha2 Review #780

Closed

robscott force-pushed the cleanup-v1alpha2 branch 2 times, most recently from 0c787e6 to d784985 Compare August 27, 2021 23:27

robscott mentioned this pull request Aug 27, 2021

Invalid parent references are challenging to implement status for #836

Closed

howardjohn reviewed Aug 29, 2021

View reviewed changes

hbagdi reviewed Aug 30, 2021

View reviewed changes

robscott mentioned this pull request Aug 30, 2021

v1alpha2 feedback #790

Closed

10 tasks

youngnick reviewed Aug 31, 2021

View reviewed changes

robscott force-pushed the cleanup-v1alpha2 branch from d784985 to 4ddc359 Compare September 2, 2021 00:01

youngnick reviewed Sep 2, 2021

View reviewed changes

robscott force-pushed the cleanup-v1alpha2 branch from 4ddc359 to b22e4e6 Compare September 2, 2021 19:25

This was referenced Sep 2, 2021

Add Hostnames to Status for HTTP and TLS Routes #848

Closed

Add top level Gateway conditions when Routes fail to attach #849

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2021

k8s-ci-robot assigned youngnick Sep 6, 2021

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

jpeach reviewed Sep 7, 2021

View reviewed changes

apis/v1alpha2/gateway_types.go Show resolved Hide resolved

jpeach reviewed Sep 7, 2021

View reviewed changes

apis/v1alpha2/gateway_types.go Show resolved Hide resolved

jpeach reviewed Sep 7, 2021

View reviewed changes

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

k8s-ci-robot assigned jpeach Sep 7, 2021

Another round of v1alpha2 cleanup

58e1dd2

robscott force-pushed the cleanup-v1alpha2 branch from b22e4e6 to 58e1dd2 Compare September 7, 2021 20:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 7, 2021

k8s-ci-robot merged commit ff66fd9 into kubernetes-sigs:master Sep 7, 2021

robscott added a commit to robscott/gateway-api that referenced this pull request Sep 10, 2021

Changes from kubernetes-sigs#839 and kubernetes-sigs#859

5bb78f1

youngnick mentioned this pull request Sep 21, 2021

Address SNI binding and bypass for TLS listeners #72

Closed

robscott added a commit to robscott/gateway-api that referenced this pull request Sep 27, 2021

Changes from kubernetes-sigs#839 and kubernetes-sigs#859

1fd1953

robscott added a commit to robscott/gateway-api that referenced this pull request Sep 27, 2021

Changes from kubernetes-sigs#839 and kubernetes-sigs#859

a4431d4

robscott deleted the cleanup-v1alpha2 branch January 8, 2022 01:06

mikemorris mentioned this pull request Oct 10, 2022

discrepancies between Accepted, Ready, and Detached condition types and reasons #1111

Closed

7 tasks

		// * HTTPS: The Listener Hostname SHOULD match at both the TLS and HTTP
		// protocol layers as described above.

Another round of v1alpha2 cleanup #839

Another round of v1alpha2 cleanup #839

Conversation

robscott commented Aug 27, 2021

k8s-ci-robot commented Aug 27, 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

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

youngnick 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

robscott commented Sep 2, 2021

youngnick 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

youngnick commented Sep 6, 2021

jpeach commented Sep 7, 2021

youngnick commented Sep 7, 2021