Improve some ListenetSet gep descriptions

[rikatz]

What type of PR is this?
/kind gep

What this PR does / why we need it:
This PR is a review of ListenerSet GEP, fixing some manifests and adding some comments to be further discussed

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rikatz
Once this PR has been reviewed and has the lgtm label, please assign thockin for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• geps/OWNERS

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

[LiorLieberman]

Thanks!

[rikatz]

One extra question I have on ListenerSet that is not clear for me, is the deduplication of listeners from different namespaces.

Let me add an example:

apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: parent-gateway
  namespace: infra
spec:
  allowedListeners:
    namespaces:
      from: All
---
apiVersion: gateway.networking.x-k8s.io/v1alpha1
kind: ListenerSet
metadata:
  name: listener01
  namespace: user01
spec:
  parentRef:
    name: parent-gateway
    namespace: infra
    kind: Gateway
    group: gateway.networking.k8s.io
  listeners:
  - name: something
    hostname: something.foo.com
    protocol: HTTPS
    port: 443
    tls:
      mode: Terminate
      certificateRefs:
      - kind: Secret
        group: ""
        name: cert
---
apiVersion: gateway.networking.x-k8s.io/v1alpha1
kind: ListenerSet
metadata:
  name: listener01
  namespace: user02
spec:
  parentRef:
    name: parent-gateway
    namespace: infra
    kind: Gateway
    group: gateway.networking.k8s.io
  listeners:
  - name: something
    hostname: something.foo.com
    protocol: HTTPS
    port: 443
    tls:
      mode: Terminate
      certificateRefs:
      - kind: Secret
        group: ""
        name: othercert

Who owns something.foo.com here? because in this case, if we are not explicitly deduplicating, we may have a situation where 2 listeners for the same host and different cert are set.

[rikatz]

on ListenerSet conflict management: at some point on this GEP there is a mention to it:

Listeners in a Gateway and their attached ListenerSets are concatenated as a list when programming the underlying infrastructure

Listeners should be merged using the following precedence:

    "parent" Gateway
    ListenerSet ordered by creation time (oldest first)
    ListenerSet ordered alphabetically by "{namespace}/{name}".

This is the statement for conflict management, but it is not really clear on what a conflict means. This way, during a discussion on gateway-api channel, we've reached the consensus of writing some conflict examples and how to deal with them, as part of the GEP, to make it easier on implementation and conformance.

[youngnick]

Thanks @rikatz.

To bring the comments I had in the Slack thread here:

I think there should be two rules:

• ListenerSets cannot be Accepted unless all Listeners are distinct (see the Gateway Spec listeners field for an exact definition).
• If Listeners within a group of ListenerSets are not distinct (that is, if Listeners in different ListenerSet objects that attach to the same Gateway are not distinct), then the Listeners are conflicting, and the Listener from the oldest ListenerSet object wins and is Accepted. ListenerSets containing conflicting Listeners MUST set the Conflicted Condition to true and clearly indicate which Listeners are conflicted.

Examples that illustrate those rules would be awesome.

[rikatz]

@youngnick added some examples and a whole section for conflict management, let me know wdyt :)

[davidjumani]

@dprotaso thanks for taking the time to explain this. Could you please help me understand this a bit further :

Let's say folks don't read the GEP. If I attach a Route to a Gateway you would naturally think that it only attaches to said parent only and not all child ListenerSets.

When I first heard about ListenerSets I assumed that since ListenerSets are "children" of the Gateway, they would inherit its routes. Reading the GEP, I came to a similar conclusion based on some of the lines quoted

The exception here might be Policy

Would this mean that a policy attached to the Gateway is inherited by the listenersets ? I'm concerned about the discrepancy between how attachments might be inherited

I understand the potential misunderstanding by users, but I believe that inheritance is more intuitive and beneficial. Would it be better to amend the GEP to reflect this ? Happy to discuss this further offline if that's easier

[dprotaso]

Would this mean that a policy attached to the Gateway is inherited by the listenersets ? I'm concerned about the discrepancy between how attachments might be inherited

It's policy specific - there are two types - direct and inherited - so there's no default policy behaviour.

https://gateway-api.sigs.k8s.io/geps/gep-2648/
https://gateway-api.sigs.k8s.io/geps/gep-2649/

I understand the potential misunderstanding by users, but I believe that inheritance is more intuitive and beneficial. Would it be better to amend the GEP to reflect this ? Happy to discuss this further offline if that's easier

I'm not sure it's worth the complexity when a new route is sufficient and more explicit (direct) about what listener's a route is attaching to. Secondly, we don't have semantics to prevent route inheritance that would just further complicate the GEP and introduce more complexity into the API to do something you can already do.

The /health use case isn't a compelling enough reason.

[rikatz]

I have fixed some comments on the GEP, yet I still need to hit the other conflict comments here, will take a look again tomorrow :)

[davidjumani]

Thanks for clearing it up @dprotaso
Let me get more info from the user on their listenerset use case

[youngnick]

So, it seems clear that the current GEP does not make the behavior clear for when a HTTPRoute attaches directly to a Gateway that has or could have ListenerSets attached.

We have two main options here:

• the implied behavior from the rules as written in the GEP today, which is that when a HTTPRoute has a Gateway parent with no SectionName, and that Gateway also has ListenerSets attached, the HTTPRoute only attaches to any Listeners explicitly specified on the Gateway.
• the behavior currently in these conformance tests, which is that when a HTTPRoute has a Gateway parent with no SectionName, and that Gateway also has ListenerSets attached, the HTTPRoute attaches to all Listeners, no matter what their source.

To me, it seems like Option 1 has some pros and cons:

• Pro: very explicit, with minimal risk of unintended consequences.
• Con: more difficult to explain, a bit harder to understand.
• Not sure if Pro or Con: requires implementations to keep track of where Listeners came from in order to implement it.

For Option 2:

• Pro: extension of existing "no sectionName" behavior to the larger context - a HTTPRoute with no sectionName parents to all Listeners, regardless of source.
• Pro: Implementations don't have to care about Listener provenance, all Listeners that make it into a Gateway are equal.
• Con: higher risk of unintended Route exposure for Routes attached directly to a Gateway with no sectionName.
• Con: the HTTPRoute could conceivably attach to many Listeners, which could produce problems with status size, since each Listener it attaches do should be listed in the HTTPRoute.

We've also discussed possibly making Listeners in a Gateway optional in the future, and requiring only one of Listeners and AllowedListeners to be set - which would make this problem less of an issue, although we would still need to define what happens in the case that a HTTPRoute with no sectionName attaches to a Gateway with AllowedListeners.

In that case:

• Option 1 implies the HTTPRoute should probably not attach, as there's no valid Listener match to produce dataplane config. That would mean the HTTPRoute should be Accepted: false.
• Option 2 implies that the HTTPRoute would attach to all ListenerSets attached to the Gateway, and be Accepted: True. However, if there are no ListenerSets attached to the Gateway, then probably the HTTPRoute should also fail to attach and be Accepted: false.

After writing all that out, I think I am slightly in favor of option 1 - keeping what we have now - mainly because it would also be very easy to end up making a Route that attached to hundreds or thousands of Listeners, which would be difficult to impossible to reflect in the Route's status.

I should note that, whatever option we choose, we MUST put some explicit explanation of the behavior in the GEP updates.

[rikatz]

So, @rikatz, any chance you could add two things to this?

doing now, passing through the comments again

[dprotaso]

I second @youngnick and prefer option 1.

[rikatz]

I think I am slightly in favor of option 1

So now after reading all of the comments: IMO we should always prioritize the most restrictive approach in favor of security/avoiding leaks (which seems option 1 for me as well)

I can imagine the following situation (forget about classes, internal vs external loadbalancers, let's think on a very simple scenario):

• ns-a, that runs some internal product, uses ListenerSet to define that the certificate on the endpoint should be something.internal.evil.corp and should never be exposed to outside world
• ns-b, that runs the company website, does not use ListenerSet so any Listener will be attached/merged to it.
  Now if I don't select any Listener or ListenerSet on ns-b, I will have my application available on any listener, regardless I should be consuming other user namespace or not. In fact, this can even allow me to "impersonate" some other user namespace/app (think on...compromised namespaces, etc).

I have a personal preference that the behavior should be explicit:

• If you have an HTTPRoute, and you have a ListenerSet on your own namespace, and you can consume it, is a bingo
• If you have an HTTPRoute, and you specify just a parent gateway, you should be using just the parent gateway Listeners but not the merged set of others ListenerSets from other namespaces
• If there is no Listener nor ListenerSet, the HTTPRoute (or any other route) should not be accepted due to the lack of Listeners (I guess this can be exposed on the Gateway)

As Nick pointed at some point here, I guess I would expect in the future that we transition to something that is more "user centric" without removing the "power" of a cluster admin:

• Cluster admin creates a gateway and tells "where I accept listeners from", but does not set any Listener anymore (maybe the only hill I would die on is that the HTTP/HTTPs port are established by cluster admin, and that the "vhosts" are merged from a ListenerSet for each namespace)
• A namespace owner can create a ListenerSet with a hostname. We should avoid hostname conflict between namespaces. So as a user, I define my ListenerSet, only pointing to a gateway and an authorized port, and it will be instantly accepted or conflicted if someone else is already using that protocol:hostname:port combination. This will help avoid vhost confusion + the "certificate" secret is restricted to the hostname owner (eg.: the first one defining it, per listener conflict management)
• HTTPRoutes on the same namespace now know which ListenerSet can be used, and the admin can even in the future block/disallow HTTPRoutes to bind directly to Gateway

I don't know if all of this makes sense, thinking loud on how we should try to rely ListenerSet to avoid some common problems like:

• hostname/vhost hijack
• certificate hijack

[rikatz]

I should note that, whatever option we choose, we MUST put some explicit explanation of the behavior in the GEP updates.

@youngnick @dprotaso I think this is the last missing piece for this update? If so, I will work on making this statement explicit on the GEP review, sounds good?

[youngnick]

I should note that, whatever option we choose, we MUST put some explicit explanation of the behavior in the GEP updates.

@youngnick @dprotaso I think this is the last missing piece for this update? If so, I will work on making this statement explicit on the GEP review, sounds good?

Yes, I think that's fine.

I think that we are stuck with ListenerSet also defining a port, because we can't change the structure of Listener easily. But I agree that having conflict detection work as quickly as possible is ideal - although I don't think that we can do it with CEL because it requires coordination across resources.

[rikatz]

The GEP already has a section called Optional Section Name that states:

If a `sectionName` in a Route's `parentRef` is not set then the Route MUST attach to only the listeners in the referenced parent. As an example given a `Gateway` and it's child `ListenerSets` a route attaching to the `Gateway` with an empty `sectionName` shall only attach to the listeners in the `Gateways` immediate `spec.listeners` list. In other words, the Route will not attach to any listeners in the `ListenerSets`. This is necessary because, for UX reasons, the `name` field does not have to be unique across all Listeners merged into a Gateway (see the section below for details).

I will make it more explicit and containing some examples so it clarifies the current situation of a Gateway without a sectionName and attached ListenerSet

[rikatz]

my last commit contains the changes to make the behavior more explicit, please let me know if this is adequate now

/cc @youngnick @dprotaso @davidjumani

[davidjumani]

Should this be the case now that routes are not inherited ?
If the kind is not specified, it can default to the gateway listener, but if the kind is ListenerSet, then it can attach to the listenerset listener
Asking since section names don't need to be unique between gateway / listenersets

[rikatz]

hum, @dprotaso I need your thoughts here

[youngnick]

Actually, that HTTPRoute should, as written, fail to attach, because the kind is Gateway, not ListenerSet (and that really should be, for now, XListenerSet, here and everywhere else).

Assuming that the Kind is corrected, then I actually don't think this should be an error on the HTTPRoute, because there is only one Listener it could attach to.

Now that we've clarified that there's no Listener attachment across parentRefs, then we can make the "can this attach" rule a bit simpler by saying "When a Route tries to attach to a Listener using a sectionName, then that sectionName must be distinct _within that Listener group (which could be a Gateway or a ListenerSet)." I think the situation shouldn't arise because listener.name should also be distinct within a Listener group, but it's good to be specific, I think.

[davidjumani]

Assuming that the Kind is corrected, then I actually don't think this should be an error on the HTTPRoute, because there is only one Listener it could attach to.

Agreed - it should attach to the listener defined in the parentref (either gateway or listenerset) and if a listener with that name does not exist, it should follow the default behaviour of erroring out

[rikatz]

so, echoing as a manifest because I understand yaml better than people :P

apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: parent-gateway
spec:
  gatewayClassName: example
  allowedListeners:
    namespaces:
      from: Same
  listeners:
  - name: foo
    hostname: foo.com
    protocol: HTTP
    port: 80
  - name: foo
    hostname: foo1.com
    protocol: HTTP
    port: 80
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: httproute-example
spec:
  parentRefs:
  - name: parent-gateway
    kind: Gateway
    sectionName: foo

But isn't this the case already? Don't we have a conflict management already in place for it, that does not allow two liteners on a listenergroup to have the same name?

[youngnick]

I feel like we should have the Kind listed as XListenerSet everywhere in this doc, with a note in the API section that this prefix is for during the experimental period.

Then, when we graduate this to Standard, we can remove the X prefix everywhere, and remove the API section note.

As it stands, since folks will need to use XListenerSet for the actual objects, and this will be a major source of examples, it feels like we could end up with user confusion if we don't make this super clear.

@dprotaso, @rikatz, thoughts?

[youngnick]

Once those two outstanding things are resolved, this LGTM.

[rikatz]

One last comment on this route disambiguation, just so I can be sure on the statement.

@youngnick @davidjumani if this is not right, and if you have some time I would appreciate if you can provide an example of a yaml/manifest with what you mean :)

Thanks!