Rewrite Listeners in terms of an endpoint concept.

[jpeach]

This change explores one way to solve the problem described here,
which is that there is an implied endpoint abstraction that is awkwardly
split across the cluster operator and application owner roles.

Rewrite Listeners to model the concept of a logical network endpoint.
This change defines a Gateway more strongly as the network element to
which addresses can be attached, and Listeners as logical endpoints
within the network element.

The main goal of this change is to consolidate the name of an application
(i.e. the endpoint) into a single part of the API that is controlled
by one user persona. In this change, the application name is strictly
part of the Gateway spec and controlled by the cluster operator role,
while routes are controlled by the application owner.

The Listener protocol is now strongly defined to a core set of common
protocol stacks. This loss of flexibility improves clarity because it
makes it possible to specify how Listeners should be collapsed into
virtual hosts in an underlying proxy implementation.

This change doesn't update ListenerStatus, though clearly it no longer
makes complete sense. The most likely change needed for GatewayStatus
is to drop the Listeners field and fold listener errors directly into
the Conditions field.

[k8s-ci-robot]

Hi @jpeach. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jpeach]

/hold

[robscott]

This an interesting concept, thanks for the work on it! The diff is relatively large, but if I'm understanding it correctly, the change entails moving hostname from routes to listeners and moving the route selector into listeners. Did I miss any other significant changes in the diff?

[robscott]

I may be misunderstanding the scope of this PR, but on the surface these protocol additions feel out of scope.

[jpeach]

Not fully. The problem is that certain behaviour can only be specified in terms of the protocol field. This means that, maybe, the open-ended protocol is a false abstraction and that in reality, the set of supported protocols is fixed.

[robscott]

The additional validation complexity here is an unfortunate consequence of this change.

[jpeach]

I think that the complexity was already there, but this change lets us make it explicit. Previously, there was no apparant complexity exposed in the API because all the complexity of how to reconcile the hostname and TLS was implicit. The controller has to do something but the API provides no rules or other guidance.

[robscott]

Under this model it seems like it would be helpful to have multiple ports/protocols per listener. I know we'd already discussed that as an option, but now that routes are per listener, it seems to be even more necessary.

[jpeach]

There's no need for that, and I'd argue that it doesn't work. You can't have multiple processes accepting on the same socket unless you know the application protocol and that protocol has some method of discrimination. This means that you are, by definition, doing something protocol-specific, so trying to make that generic is a false abstraction that obscures rather than clarifies.

In this model, you can have a listener per port, and the rules for collapsing listeners onto the same underlying network port are clear. If you need more ports, then add more listeners with the same selector.

A further aspect that I didn't draw out in this PR is that the protocol and the route selector fields are tightly coupled and there have to be rules about how to handle this. Tying them together in the same struct makes it easier to elaborate the rules for handling that.

[robscott]

Yep, as I've thought about this more it really does make sense to keep listeners unique per port/protocol combination.

[jpeach]

This an interesting concept, thanks for the work on it! The diff is relatively large, but if I'm understanding it correctly, the change entails moving hostname from routes to listeners and moving the route selector into listeners. Did I miss any other significant changes in the diff?

Nope, you have nailed the main intent. See attached doc for my reasoning that there is a "logical endpoint" object that we should model.

[bowei]

/ok-to-test

[bowei]

/assign

[bowei]

Splitting the ports by listener seems to clean things up.
I'm not so sure about the hostname requirement:

How to have wildcard hostnames.
This is a common use case (in fact we added support for it Ingress GA, maybe step backwards).

We at least need to be able to handle default hostname.

Given that with the listener + TLS + associated routes => you can validate the hostname matches and certs w/ all of the objects involved, can we remove hostname from the Gateway level?

[hbagdi]

Is this defining compatibility (if yes, then compatibility with what) or validity of a Listener?

[jpeach]

This is defining compatibility for the purposes of "collapsing compatible Listeners" in the preceding paragraph. The concept of collapsing Listeners is what allows virtual hosting).

[jpeach]

I added some more text to specify the rules around collapsing Listeners into a virtual host configuration. I think it's clearer, but could still be better.

[hbagdi]

Given that with the listener + TLS + associated routes => you can validate the hostname matches and certs w/ all of the objects involved, can we remove hostname from the Gateway level?

That works for HTTP but not HTTPS.

I think this change is a positive one and we should proceed.
I have the same thought as @bowei on the hostname. Default and wildcard domains need to be supported here.
Here is what I think might work:

Keep Hostname in HTTPRoute. This is used for route matching.
In Gateway's Listener, we keep a field AllowedHostnames []string which acts as a filter.
Cluster Operator populate the field after they do the due diligence on their part (ensure DNS setup, TLS certs, approvals, etc).
App owners are now free to bind routes as they see fit as long as the hostname they wish to use is allowed in the Gateway Listener. If not, then they go and talk to the Cluster Operator.

AllowedHostnames can be strict, meaning each and every domain needs to be added, semi-strict by allowed a wildcard set of domains, or relaxed, where it allows for all domains (the easy path).

[jpeach]

Splitting the ports by listener seems to clean things up.
I'm not so sure about the hostname requirement:

How to have wildcard hostnames.
This is a common use case (in fact we added support for it Ingress GA, maybe step backwards).

I need to go dig up and read the Ingress spec for this! I changed the Listener hostname to be a match type, so you can express wildcards as a domain match. The default would be an exact host name match.

We at least need to be able to handle default hostname.

I documented the case where the Listener can omit the Hostname. This would allow your to specify routes without pinning to any name.

This, together with domain matching, required a fair amount of new text to specify how Listeners should be collapsed into a virtual host configuration.

Given that with the listener + TLS + associated routes => you can validate the hostname matches and certs w/ all of the objects involved, can we remove hostname from the Gateway level?

Yeh I agree with @hbagdi on this. You need some deterministic information about the hostname in the Listener to do the right certificate selection. In the Google doc linked from the initial comment I discussed some of the corner cases here. Keeping some hostname spec in the Listener means that the semantics are consistent across changes in what the route selector selects.

[jpeach]

Given that with the listener + TLS + associated routes => you can validate the hostname matches and certs w/ all of the objects involved, can we remove hostname from the Gateway level?

That works for HTTP but not HTTPS.

I think this change is a positive one and we should proceed.
I have the same thought as @bowei on the hostname. Default and wildcard domains need to be supported here.
Here is what I think might work:

Keep Hostname in HTTPRoute. This is used for route matching.
In Gateway's Listener, we keep a field AllowedHostnames []string which acts as a filter.
Cluster Operator populate the field after they do the due diligence on their part (ensure DNS setup, TLS certs, approvals, etc).
App owners are now free to bind routes as they see fit as long as the hostname they wish to use is allowed in the Gateway Listener. If not, then they go and talk to the Cluster Operator.

AllowedHostnames can be strict, meaning each and every domain needs to be added, semi-strict by allowed a wildcard set of domains, or relaxed, where it allows for all domains (the easy path).

I restored the Hostname in HTTPRoute as per this suggestion. Probably we need some more explanatory text to describe how we intend it to work.

To address the default and wildcard cases, I borrowed the match concept from routes and change the Listener hostname to have "domain" and "exact" match types. A hostname can be omitted, which would give the Listener default route semantics.

[costinm]

Why not be able to accept ? If you don't specify any listener it could default to HTTP/HTTPS.

[jpeach]

If there are no Listeners, there's no way to know where to send the traffic.

[hbagdi]

If there are no listeners, you don't even know which port to listen on to accept any traffic in the first place.

[bowei]

What is the use case for associated an address but not accepting connections? IF it's not meaningful/useful, we are better off making it an invalid configuration.

[jpeach]

I can resolve this by making Listeners required?

[bowei]

Yes -- that's probably easiest rather than trying to figure out what empty listeners mean (for now)

[jpeach]

Updated field tags and examples so that all examples can be applies to a cluster. Filed the following issues to track follow on work:

#206 Discuss domain match vs. wildcard match for listener hostnames
#205 WebSocket protocol support
#204 h2c (cleartext HTTP/2) protocol support
#203 Protocol type extensibility
#202 Re-address Gateway and Listener error conditions

[hanlins]

Can you change gateway conditions since route is not part of Gateway? Also it would be nice to add GatewayConditionType for Addresses like InvalidAddress.

[jpeach]

Can you change gateway conditions since route is not part of Gateway? Also it would be nice to add GatewayConditionType for Addresses like InvalidAddress.

There's a lot of conditions that don't make sense after this change, and also conditions that are missing. Since this PR is quite intrusive already, I'm proposing to spin that out into #202.

[bowei]

We can fix this in a later commit, but it would be good to frame this from the user's standpoint, as the current description is somewhat hard to parse.

[jpeach]

On Jun 6, 2020, at 8:46 AM, Bowei Du ***@***.***> wrote:  @bowei commented on this pull request. In api/v1alpha1/gateway_types.go: > @@ -54,78 +55,212 @@ type GatewayList struct { type GatewaySpec struct { // Class used for this Gateway. This is the name of a GatewayClass resource. Class string `json:"class" protobuf:"bytes,1,opt,name=class"` - // Listeners associated with this Gateway. Listeners define what addresses, - // ports, protocols are bound on this Gateway. + + // Listeners associated with this Gateway. Listeners define + // logical endpoints that are bound on this Gateway's addresses. + // At least one Listener MUST be specified. + // + // Each Listener in this array must have a unique Port field, + // however a GatewayClass may collapse compatible Listener + // definitions into single implementation-defined acceptor + // configuration even if their Port fields would otherwise conflict. + // + // Listeners are compatible if all of the following conditions are true: We can fix this in a later commit, but it would be good to frame this from the user's standpoint, as the current description is somewhat hard to parse.

I’ve been through a number of revisions of this, but I still agree that it could be improved. Happy to take editorial suggestions or discuss on a call :)

[bowei]

/approve

let's merge and iterate

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, jpeach

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 [bowei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[robscott]

Just confirmed with @jpeach that we can remove the hold and get this in.

/hold cancel
/lgtm