test(source): add regression coverage for TCPRoute referencing a ListenerSet - #6578
test(source): add regression coverage for TCPRoute referencing a ListenerSet#6578pujitha24 wants to merge 2 commits into
Conversation
Motivation: Issue kubernetes-sigs#6557 reports that a TCPRoute referencing a Gateway API ListenerSet (e.g. an Envoy Gateway ListenerSet) fails to resolve via --source=gateway-tcproute, logging "ListenerSet ns/name not found for TCPRoute ns/name", while the reporter says an equivalent HTTPRoute against the same ListenerSet worked. ListenerSet resolution lives in a single code path shared by every Gateway API route kind (resolveParentRef/routeIsAllowed in source/gateway.go), but source/gateway_listenerset_test.go only ever exercised it against HTTPRoute. TCPRoute+ListenerSet had zero test coverage, so a regression there could ship unnoticed. Approach: Investigated by adding a probe test that builds a Gateway (TCP listener) + ListenerSet (TCP listener) + TCPRoute referencing the ListenerSet, with GatewayListenerSets: true. It passed on current HEAD: TCPRoute already resolves correctly through a ListenerSet. The existing TestGatewayHTTPRouteWithListenerSetDisabled test also shows that with the flag left at its default (false), even HTTPRoute against a ListenerSet produces zero endpoints (not an error) - the same "not found" behavior the issue describes applies uniformly to every route kind, since the gating logic has no per-kind branching. That points to the issue being caused by --gateway-listener-sets not being enabled (it defaults to false and requires Gateway API v1.5+ CRDs), matching the suggestion already left on the issue by another community member, rather than a code defect in this repository. Fabricating a source change for a bug that doesn't reproduce would be worse than doing nothing, so instead this PR turns the probe into a permanent regression test, TestGatewayTCPRouteWithListenerSetParentRef, modeled on the existing HTTPRoute version and using the hostname-annotation pattern already used by TestGatewayTCPRouteSourceEndpoints (TCPRoute has no Hostnames() field). Mutation testing (temporarily short-circuiting the ListenerSet lookup for TCPRoute only) confirmed this new test fails when TCPRoute-specific resolution breaks, while the existing HTTPRoute test does not catch that case - so it adds real, non-overlapping coverage of this shared path. No production code changes; user-visible behavior is unchanged. Validation: go test ./source/... -run TestGatewayTCPRouteWithListenerSetParentRef -v (PASS) go test -race ./source/... (PASS, no regressions) go build ./... (clean) go vet ./source/... (clean) gofmt -l source/gateway_listenerset_test.go (no output) Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
kubernetes-prow
Bot
added
the
do-not-merge/invalid-commit-message
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
source
cncf-cla: no
|
Welcome @pujitha24! |
|
Hi @pujitha24. 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 Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
kubernetes-prow
Bot
added
needs-ok-to-test
There was a problem hiding this comment.
Pull request overview
Adds a regression test to ensure the gateway-tcproute source correctly resolves a TCPRoute whose parentRef targets a ListenerSet when --gateway-listener-sets is enabled, preventing a repeat of #6557.
Changes:
- Add
TestGatewayTCPRouteWithListenerSetParentRefcovering the Gateway → ListenerSet → TCPRoute resolution path. - Use the shared hostname annotation key constant for the TCPRoute hostname (
annotations.HostnameKey).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kubernetes-prow
Bot
added
cncf-cla: yes
mloiseleur
changed the title
|
@pujitha24 You need to fix the invalid commit message before we can proceed with the review. See #6578 (comment) |
547b0ff to
382cc67
Compare
kubernetes-prow
Bot
removed
the
do-not-merge/invalid-commit-message
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
Coverage Report for CI Build 30717423597Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 81.697%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
I'm not fully understand what this test is asserting. it does correctly assert that TCPRoute+ListenerSet resolution works, mirroring existing HTTPRoute coverage. But it is not a regression guard for issue #6557 as claimed. If #6557 recurs, this test would not have caught it, and merging this PR with "Fixes #6557" would close the issue without addressing its actual root cause. This test never goes red. It doesn't fail without a fix and pass with one — there is no fix in this PR (confirmed: zero production code changed), and it also doesn't fail on the pre-issue codebase. So it can't be "locking in" a fix, because nothing here demonstrates the bug ever existed in this code path. Just my assumption, but the bug itself seems like an informer cache-sync race, or most likely RBAC restricting list/watch on ListenerSet (which would silently produce an empty lister in prod but isn't modeled by the fake clientset at all), or some interaction specific to running --source=gateway-tcproute and --source=gateway-httproute together in the same long-running process. None of that is exercised here. Maybe it should be reframed the PR as "adds TCPRoute+ListenerSet coverage" The bug from issue #6557 :
|
The test comment claimed it guards against kubernetes-sigs#6557, but as pointed out in review it can't be a regression guard: nothing in this PR fails without a fix and passes with one. Reword it to describe what it actually does — TCPRoute+ListenerSet coverage for the shared resolveParentRef/routeIsAllowed path — without claiming to reproduce or guard against the reported issue. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
You're right, and I've reworded the test comment and the PR description accordingly — it's coverage for the TCPRoute+ListenerSet path, not a regression guard for #6557, and I dropped |
|
/assign @mloiseleur Reworded per the earlier feedback (now framed as added coverage, not a fix, and |
What this does
Adds TCPRoute+ListenerSet coverage for the
gateway-tcproutesource.TestGatewayTCPRouteWithListenerSetParentRefbuilds aGateway->ListenerSet->TCPRoute(whoseparentReftargets the ListenerSet) and asserts thegateway-tcproutesource resolves the expected endpoint (app.example.com) when--gateway-listener-setsis enabled — mirroring the existing HTTPRoute + ListenerSet coverage in this file.Why
Issue #6557 reported that a
TCPRoutereferencing aListenerSetfailed to resolve while an equivalentHTTPRouteworked.resolveParentRef/routeIsAllowedinsource/gateway.goalready handle TCPRoute correctly on currentHEAD(this test passes without any production-code change), but that path had zero TCPRoute-specific test coverage before this PR.As pointed out in review, this test is not a regression guard for #6557: nothing here fails without a fix and passes with one, so it can't demonstrate the reported bug ever existed in this code path. The root cause remains unconfirmed — possibly
--gateway-listener-setsnot being enabled in the reporter's environment (it defaults tofalse), an informer cache-sync race, RBAC restricting list/watch on ListenerSet, or an interaction specific to running--source=gateway-tcprouteand--source=gateway-httproutetogether that isn't exercised by a single-source unit test. This PR only adds missing coverage for the TCPRoute+ListenerSet path; it does not close or resolve #6557.Notes
go test ./source/...passes locally.Relates to #6557