test(conformance): preserve Route status entries owned by other controllers - #5138
test(conformance): preserve Route status entries owned by other controllers#5138lexfrei wants to merge 1 commit into
Conversation
kubernetes-prow
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lexfrei 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
size/L
|
Hi @lexfrei. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
|
/cc |
| // foreignControllerName stands in for a second implementation sharing the | ||
| // cluster. Nothing reconciles it, so any change to its status entry can only | ||
| // have come from the implementation under test. |
There was a problem hiding this comment.
In general, AI-generated comments aren't worth to be added to the code, because if I want them, I can easily generate them myself by asking a model of my choice to explain me the code.
However, if you feel some part of the code needs further explanation because it does something unusual, goes against best practices, or is really complex, feel free to write those comments by hand.
| // foreignControllerName stands in for a second implementation sharing the | |
| // cluster. Nothing reconciles it, so any change to its status entry can only | |
| // have come from the implementation under test. |
There was a problem hiding this comment.
Dropped. Swept the rest of the PR too, kept only the comments that say something the code cannot, like the lastTransitionTime truncation.
| // foreignControllerName stands in for a second implementation sharing the | ||
| // cluster. Nothing reconciles it, so any change to its status entry can only | ||
| // have come from the implementation under test. | ||
| const foreignControllerName = gatewayv1.GatewayController("example.com/foreign-controller") |
There was a problem hiding this comment.
Since we want to create further tests with a second controller, I would introduce a sentinel in helpers.go, e.g., gateway.networking.k8s.io/skip-this-for-observed-generation or gateway.networking.k8s.io/stale-controller.
This would also simplify the changes in RouteMustHaveParents, which could then be used to assert the parent status of both controllers.
There was a problem hiding this comment.
Done. Went with stale-controller over skip-this-for-observed-generation: the constant is also the fixture owner in the test, so it has to read like a controller name.
One gap: a real second implementation still stalls the wait with its own controllerName, same as on main today. Per the scope rule in the troubleshooting guide it should not be writing status on another implementation Routes at all, so I left it. I can keep the filter alongside the sentinel if you want that covered too.
123ea37 to
ddb3e4b
Compare
|
hi lexfrei thank's for this PR. Can run this test with at least one implemenatation to verify that the flow is correct? I will try to run it against GKE today |
|
Ran it against my own implementation (cloudflare-tunnel-gateway-controller) on kind with a real Cloudflare Tunnel. Passes, and it is not silently skipping: Worth knowing what the run actually shows. After the spec bump my controller entry moves to observedGeneration 2 while the seeded entry stays at 1 and is otherwise untouched. That gap never closes, so the sentinel skip is what lets RouteMustHaveParents converge at all. Curious what GKE does with it. |
|
|
||
| // The implementation's own entry catching up to the bumped generation is | ||
| // what proves it wrote status while the seeded entry was there. | ||
| kubernetes.HTTPRouteMustHaveParents(t, suite.Client, suite.TimeoutConfig, routeNN, |
There was a problem hiding this comment.
@lexfrei is there a reason the stale controller status isn't also asserted using HTTPRouteMustHaveParents?
There was a problem hiding this comment.
No reason, added it.
Kept the comparison below because findConditionInList only looks at type, status and reason, and RouteMustHaveParents skips observedGeneration for the sentinel. An implementation that rewrites the entry with its own observedGeneration and lastTransitionTime, still Accepted=True, passes the helper. So the helper catches removal and the comparison catches a rewrite.
ddb3e4b to
95f45c5
Compare
7e57ab5 to
d75d825
Compare
|
@snorwin added the HTTPRouteMustHaveParents assertion you asked about. Writing the tests turned up that the reorder fixes more than I thought. On main @kl52752 did you get to run this against GKE? Still needs an /ok-to-test before prow picks it up. |
…ollers RouteStatus.Parents is namespaced by (parentRef, controllerName), and an implementation MUST NOT update entries whose controllerName is not its own. Nothing in the suite checked that, and the harness could not have hosted such a check: RouteMustHaveParents required every entry in status.parents to carry the Route's current generation, so a stale entry left behind by a second controller stalled the wait until it timed out. Add a StaleControllerName sentinel that tests can put on a parent status entry to stand in for another implementation, and skip those entries in the observedGeneration check. Nothing reconciles them, so they cannot be expected to keep up with the Route's generation. On top of that, add a test that seeds such an entry, bumps the Route's generation to force a status write, and checks that the seeded entry comes back byte for byte alongside the implementation's own entry. The observedGeneration check also ran against the parent list read on the previous poll rather than the one just fetched. On the first iteration that list was still empty, so a Route whose status already matched satisfied the wait without the check running at all. It now runs on the snapshot it just read, which tightens every RouteMustHaveParents caller in the suite: a status that matches but has not caught up with the Route generation no longer ends the wait early. The check moves into staleParentStatus so both halves of it can be pinned directly. Signed-off-by: Aleksei Sviridkin <f@lex.la>
d75d825 to
ab6839b
Compare
|
@kl52752 did you get a chance to run this against GKE? I rebased the branch on current main in the meantime; the Netlify failure was stale and is green now. |
|
@lexfrei I managed to run this test on GKE with success. |
kubernetes-prow
Bot
added
the
lgtm
3 participants
What type of PR is this?
/kind test
/area conformance-test
/area conformance-machinery
What this PR does / why we need it:
Two controllers on one cluster is a normal setup, and the spec is explicit that an implementation may only touch
status.parentsentries carrying its owncontrollerName. The suite had no coverage for that, so an implementation could wipe another controller's entry and still pass.The harness had to move first.
RouteMustHaveParentswalked every entry instatus.parentsand required each one's conditions to observe the Route's current generation. A second controller's entry does not track our Route's generation (and per the same MUST, the implementation under test is forbidden from refreshing it), so any Route carrying a foreign entry stalled the wait until timeout. That is a real problem on a shared cluster, and this PR fixes only as much of it as the test needs: the check now skips entries whose controllerName isStaleControllerName, a reserved value tests put on a status entry to stand in for a second implementation, so a seeded entry no longer stalls the wait. An entry carrying any other controllerName still has to observe the generation. Generalising that means passing the caller's controllerName into the helper, which is the signature change the scope note below defers. One side note on the same helper: the exact-set worry in the issue turned out not to apply,parentsForRouteMatchis already a subset check and ignores extra entries.One behaviour change worth calling out. The
observedGenerationcheck inRouteMustHaveParentswas dead on the fast path:actualwas assigned after the loop that reads it, so the first poll iteration ran the loop over an empty slice, and ifparentsForRouteMatchwas already satisfied the helper returned right there. That is the normal path on a passing run, so for those runs the check never ran at all. It runs now, and that reaches every*RouteMustHaveParentscaller in the suite: an implementation whose status matches on type, status and reason but has not caught up with the Route generation will now keep the wait going and can time out where it passed before. A unit test pins the ordering, it fails with the old ordering and passes with the new one.The new test attaches an HTTPRoute to
same-namespaceplus a second parentRef the implementation cannot resolve, writes the entry a second controller would have written for that parentRef, then changes the Route spec so the implementation has to do a full read-modify-write of a status that already holds the foreign entry. Waiting for the implementation's own entry to observe the bumped generation is what proves it wrote status while the foreign entry was there; the entry is then compared field for field, so a removal, a rewrite, or a dropped condition all fail.It is marked
Provisional, and gated onSupportGateway+SupportHTTPRouteonly: no new feature is involved, the MUST applies to every implementation that writes Route status.The harness change is covered by unit tests against a fake client.
Test_staleParentStatuspins which entries theobservedGenerationcheck skips: the reserved sentinel is exempt, an entry carrying any other controllerName is not.TestRouteMustHaveParentsIgnoresStaleControllerEntriesdrivesHTTPRouteMustHaveParentsover a Route holding a deliberately stale sentinel entry; it times out onmainand passes here.TestRouteMustHaveParentsChecksTheStatusItJustReadpins the read ordering by counting reads: with the old ordering the wait is satisfied by the first read, and the test fails.Scope note, per the plan in the issue: this is Route parent status only. Gateway and Listener conditions and policy ancestor status are separate follow-ups. Some sibling helpers are still single-controller by construction (
HTTPRouteMustHaveNoAcceptedParentsand its TLS/TCP/UDP twins hard-require at most one entry, andHTTPRouteMustHaveLatestConditionswalks every entry), but none of them takes a controllerName today, so relaxing them means changing exported signatures. I left them alone rather than smuggle that into this PR.Locally: build, vet, lint, and the unit tests above. I also ran the new conformance test against my own implementation on kind with a real Cloudflare Tunnel, it passes and does not silently skip. That says something about the test, not about anyone's conformance. Prow has not run anything here at all, the
needs-ok-to-testlabel is still on it. I have not done a full suite run producing a fresh conformance report.AI assistance: drafted with Claude, reviewed and verified by me before submitting.
Which issue(s) this PR fixes:
Part of #5105
Does this PR introduce a user-facing change?:
AI disclosure
This PR was prepared with AIL:3. I personally reviewed each line of the submission prior to opening this PR.