Skip to content

Conversation

@davidjumani
Copy link

@davidjumani davidjumani commented Jun 30, 2025

What type of PR is this?

/kind test
/area conformance-test

What this PR does / why we need it:

This PR contains an initial conformance test for listenersets. This aims to verify the following :

  • Translation of a listenerset by an implementer
  • Route inheritance of gateway-level routes by the listenerset
  • Route isolation between gateways and listenersets based on parentRef and sectionName
  • Gateway, ListenerSet, Route and Service in different namespaces
  • ListenerSets not allowed on the gateway
  • ListenerSets with protocol and hostname conflict

Run against kgateway

--- PASS: TestConformance/ListenerSetHostnameConflict (4.21s)
    --- PASS: TestConformance/ListenerSetHostnameConflict/5_request_to_'hostname-conflict-listener-1.com/listenerset-2-route'_should_receive_one_of_[] (0.03s)
    --- PASS: TestConformance/ListenerSetHostnameConflict/7_request_to_'hostname-conflict-listener-2.com/listenerset-2-route'_should_receive_one_of_[] (0.03s)
    --- PASS: TestConformance/ListenerSetHostnameConflict/4_request_to_'hostname-conflict-listener-1.com/listenerset-1-route'_should_receive_one_of_[] (0.03s)
    --- PASS: TestConformance/ListenerSetHostnameConflict/6_request_to_'hostname-conflict-listener-2.com/listenerset-1-route'_should_go_to_infra-backend-v2 (0.03s)
    --- PASS: TestConformance/ListenerSetHostnameConflict/1_request_to_'listenerset-1-listener.com/listenerset-1-route'_should_go_to_infra-backend-v2 (0.03s)
    --- PASS: TestConformance/ListenerSetHostnameConflict/3_request_to_'hostname-conflict-listener-1.com/gateway-route'_should_go_to_infra-backend-v1 (0.04s)
    --- PASS: TestConformance/ListenerSetHostnameConflict/2_request_to_'listenerset-2-listener.com/listenerset-2-route'_should_go_to_infra-backend-v3 (0.04s)
    --- PASS: TestConformance/ListenerSetHostnameConflict/0_request_to_'gateway-listener.com/gateway-route'_should_go_to_infra-backend-v1 (0.04s)
--- PASS: TestConformance/ListenerSetCrossNamespace (3.33s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/6_request_to_'listenerset-1-listener-1.com/gateway-listener-2-listener-route'_should_receive_one_of_[] (0.03s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/8_request_to_'gateway-listener-1.com/listenerset-1-route'_should_receive_one_of_[] (0.03s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/16_request_to_'listenerset-2-listener-1.com/listenerset-2-route'_should_receive_one_of_[] (0.04s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/9_request_to_'gateway-listener-2.com/listenerset-1-route'_should_receive_one_of_[] (0.04s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/4_request_to_'gateway-listener-1.com/gateway-listener-2-listener-route'_should_receive_one_of_[] (0.03s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/2_request_to_'listenerset-1-listener-1.com/gateway-route'_should_receive_one_of_[] (0.04s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/7_request_to_'listenerset-1-listener-2.com/gateway-listener-2-listener-route'_should_receive_one_of_[] (0.04s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/1_request_to_'gateway-listener-2.com/gateway-route'_should_go_to_infra-backend-v1 (0.04s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/5_request_to_'gateway-listener-2.com/gateway-listener-2-listener-route'_should_go_to_infra-backend-v1 (0.04s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/0_request_to_'gateway-listener-1.com/gateway-route'_should_go_to_infra-backend-v1 (0.05s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/12_request_to_'gateway-listener-1.com/listenerset-1-listener-2-listener-route'_should_receive_one_of_[] (0.01s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/14_request_to_'listenerset-1-listener-1.com/listenerset-1-listener-2-listener-route'_should_receive_one_of_[] (0.01s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/3_request_to_'listenerset-1-listener-2.com/gateway-route'_should_receive_one_of_[] (0.02s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/13_request_to_'gateway-listener-2.com/listenerset-1-listener-2-listener-route'_should_receive_one_of_[] (0.02s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/15_request_to_'listenerset-1-listener-2.com/listenerset-1-listener-2-listener-route'_should_go_to_infra-backend-v1 (0.02s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/11_request_to_'listenerset-1-listener-2.com/listenerset-1-route'_should_go_to_infra-backend-v1 (0.02s)
    --- PASS: TestConformance/ListenerSetCrossNamespace/10_request_to_'listenerset-1-listener-1.com/listenerset-1-route'_should_go_to_infra-backend-v1 (0.02s)
--- PASS: TestConformance/ListenerSetNotAllowed (3.11s)
    --- PASS: TestConformance/ListenerSetNotAllowed/2_request_to_'bar.com/route'_should_receive_one_of_[] (0.04s)
    --- PASS: TestConformance/ListenerSetNotAllowed/1_request_to_'foo.com/route'_should_receive_one_of_[] (0.04s)
    --- PASS: TestConformance/ListenerSetNotAllowed/0_request_to_'example.com/route'_should_go_to_infra-backend-v1 (0.05s)
--- PASS: TestConformance/ListenerSetProtocolConflict (4.21s)
    --- PASS: TestConformance/ListenerSetProtocolConflict/4_request_to_'protocol-conflict-listener-1.com/listenerset-1-route'_should_receive_one_of_[] (0.03s)
    --- PASS: TestConformance/ListenerSetProtocolConflict/5_request_to_'protocol-conflict-listener-1.com/listenerset-2-route'_should_receive_one_of_[] (0.03s)
    --- PASS: TestConformance/ListenerSetProtocolConflict/3_request_to_'protocol-conflict-listener-1.com/gateway-route'_should_go_to_infra-backend-v1 (0.04s)
    --- PASS: TestConformance/ListenerSetProtocolConflict/0_request_to_'gateway-listener.com/gateway-route'_should_go_to_infra-backend-v1 (0.04s)
    --- PASS: TestConformance/ListenerSetProtocolConflict/7_request_to_'protocol-conflict-listener-2.com/listenerset-2-route'_should_receive_one_of_[] (0.04s)
    --- PASS: TestConformance/ListenerSetProtocolConflict/6_request_to_'protocol-conflict-listener-2.com/listenerset-1-route'_should_go_to_infra-backend-v2 (0.04s)
    --- PASS: TestConformance/ListenerSetProtocolConflict/1_request_to_'listenerset-1-listener.com/listenerset-1-route'_should_go_to_infra-backend-v2 (0.04s)
    --- PASS: TestConformance/ListenerSetProtocolConflict/2_request_to_'listenerset-2-listener.com/listenerset-2-route'_should_go_to_infra-backend-v3 (0.04s)


Which issue(s) this PR fixes:

Fixes #3785

Does this PR introduce a user-facing change?:

Adding initial conformance tests for XListenerSets

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added kind/test release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/conformance-test Issues or PRs related to Conformance tests. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 30, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: davidjumani
Once this PR has been reviewed and has the lgtm label, please assign youngnick 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:

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

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 30, 2025
@davidjumani
Copy link
Author

/test all

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2025
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 16, 2025
@davidjumani davidjumani marked this pull request as ready for review August 16, 2025 07:44
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2025
@davidjumani
Copy link
Author

/retest

@robscott
Copy link
Member

/cc @rikatz @dprotaso

Comment on lines 66 to 70
// Requests to the listener with domain name conflict should not work
{
Request: http.Request{Host: "conflict.com", Path: "/gateway-route"},
Response: http.Response{StatusCode: 404},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't know if we were clear enough about this in the GEP, but in this case, because there is a Gateway Listener, it should win over the ListenerSet listener. (see https://gateway-api.sigs.k8s.io/geps/gep-1713/#listener-precedence for the details here)

In this and other conflict cases, there should always be one "winning" Listener from somewhere, and that Listener should end up Accepted, and traffic should flow.

The intent of that is to stop the creation of conflicts from stopping traffic flowing (that's why "oldest first wins" for ListenerSets).

Copy link
Author

@davidjumani davidjumani Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused since the GEP mentions

Implementations MUST treat the parent Gateways as having the merged list of all listeners from itself and attached ListenerSets and validation of this list of listeners MUST behave the same as if the list were part of a single Gateway with the relaxed listener name constraints.

and so I based the validation tests as though they were defined on a single gateway - My understanding was that the ordering was purely for merging them into a gateway and not validating them any different

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I see #3978 has been updated. Shall I wait for it to merge before making the changes here ?

@davidjumani
Copy link
Author

Thanks @youngnick @dprotaso @rikatz for your reviews! I've updated this to reflect the discussion in #3978

@davidjumani
Copy link
Author

@youngnick @dprotaso @rikatz could you please have another look at this ? Thanks

port: 80
protocol: HTTP
hostname: "hostname-conflict-listener-1.com"
allowedRoutes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated with this PR, but this got my attention:

@dprotaso @youngnick do we really want this kind of deep allowance on a ListenerSet? The way I see a ListenerSet, it is mostly related to "Ana" persona, allowing all of these dereference will be really hard for controllers to track.

Imagine this situation:

  • Chihiro creates a Gateway and tells "I allow Listeners and routes from namespace with 'production' label"
  • Then Ana creates a ListenerSet on a namespace with 'production' label but tells that the ListenerSet should accept routes from any namespace
  • Erin then comes and attaches the HTTPRoute of a development namespace into a ListenerSet of production, and now can rely on a production hostname, a production certificate, etc

IMO ListenerSet should be scoped to the namespace they belong, and in case of a cross-namespace need of a Listener this should then be a case where a Route attaches not to a ListenerSet, but to a Gateway listener as it is today.

Request: http.Request{Host: "hostname-conflict-listener-1.com", Path: "/listenerset-2-route"},
Response: http.Response{StatusCode: 404},
},
// Requests to the listener with domain name conflict should work on the first listener (based on listener precedence - alphabetic / creation time)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was now wondering on something on GEP that should be clarified again (sorry @davidjumani !)

I think this conflict management may be a bit unsecure. The CreationTimestamp reflects when the resource was created, but we can modify it in a way that a Listener can be stolen:

  • Erin creates the ListenerSet on her namespace, with something like:
creationTimestamp: "2025-08-11T15:44:05Z" # this is set by the APIserver
- listeners:
  - hostname: www.evil.tld
  • Ana creates the ListenerSet on her namespace, with something like:
creationTimestamp: "2025-08-11T16:44:05Z"
- listeners:
  - hostname: www.mything.tld

Now, given the situation above, Erin's ListenerSet is older than Ana's ListenerSet. We don't have a conflict here, but what happens if Erin then changes her ListenerSet to something like:

creationTimestamp: "2025-08-11T15:44:05Z" # this is set by the APIserver
- listeners:
  - hostname: www.evil.tld
  - hostname: www.mything.tld

I don't think we have a way to say "Erin's resource is older, but the ListenerSet inside Ana's array is older" which means effectively per the conflict management that Erin's ListenerSet will win the "conflict resolution" per the resource age, and steal www.mything.tld from Ana

Am I missing something here?

GatewayMustHaveCondition: 180 * time.Second,
GatewayStatusMustHaveListeners: 60 * time.Second,
GatewayListenersMustHaveConditions: 60 * time.Second,
ListenerSetMustHaveCondition: 180 * time.Second,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't 3 minutes too much? The timeout above (GatewayListenersMustHaveConditions) is 1 minute, maybe we can follow the same here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set ListenerSetMustHaveCondition to match GatewayMustHaveCondition
ListenerSetListenersMustHaveConditions matches GatewayListenersMustHaveConditions
Do you still think it should be reduced ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's see how this goes with 3 minutes, and be careful if this becomes a problem for test execution time

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2025
@davidjumani
Copy link
Author

Thanks @rikatz for the review! Wrt the open questions, can we get the conformance tests in right now? When we amend the GEP, the conformance tests can also be updated

@davidjumani
Copy link
Author

@youngnick @dprotaso @rikatz Could you please have a look at this? Thanks

@rikatz
Copy link
Member

rikatz commented Oct 22, 2025

This is next on my queue, will be my first thing tomorrow (thursday)

hostname: "listenerset-1-listener-2.com"
allowedRoutes:
namespaces:
from: All
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting: do we expect this to work, if the allowedListener defines just ListenerSet of the same namespace, or not?

I think the idea is to allow, just checking

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused. Are you asking if the Listener on the ListenerSet will accept routes from all namespaces if the Gateway's allowedListener is set to only listenerSets from the same namespace?
If so, it should allow since the allowedListener restricts listenersets and should have no effect on the routes

@rikatz
Copy link
Member

rikatz commented Oct 23, 2025

I still have some questions, but overall lgtm and I would like to see these tests running with other implementations. I will try to trigger it tomorrow against Istio and kgateway to see how they behave (failing is fine, as this may not be implemented yet)

I have mapped some missing tests, but not for this PR:

  • ListenerSet entry name unique on same set
  • count of attached routes (maybe it is ther,e but I didn't spotted it)
  • Port is now optional to allow for dynamic port assignment. If the port is unspecified or set to zero, the implementation will assign a unique port.
  • When a ReferenceGrant is applied to a Gateway it MUST NOT be inherited by child ListenerSets. Thus a ListenerSet listener MUST NOT access secrets granted to the Gateway listeners

@davidjumani
Copy link
Author

Thanks @rikatz I've updated the PR and description with the passing run on kgateway
I'll also add the tests that have been suggested in a follow up

@davidjumani
Copy link
Author

/retest

This reverts commit 3ec5493.
@davidjumani
Copy link
Author

I have mapped some missing tests, but not for this PR:

Thanks @rikatz
I've raised a PR with the list of conformance tests I believe would be sufficient to validate this feature
Add conformance details to ListenerSets (GEP-1713)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/conformance-test Issues or PRs related to Conformance tests. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/test release-blocker MUST be completed to complete the milestone release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Conformance Tests for ListenerSets

6 participants