[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.

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.

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.

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.

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

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

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.

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

doing now, passing through the comments again

I second @youngnick and prefer option 1.

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

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?

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.

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

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

Once those two outstanding things are resolved, this LGTM.

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!

Yes, this LGTM now. Thanks @rikatz!

/hold cancel