test(conformance): preserve foreign status entries on BackendTLSPolicy and Gateway - #5206
test(conformance): preserve foreign status entries on BackendTLSPolicy and Gateway#5206lexfrei wants to merge 4 commits into
Conversation
…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>
kubernetes-prow
Bot
added
release-note-none
|
[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
cncf-cla: yes
kubernetes-prow
Bot
added
the
needs-ok-to-test
|
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. |
|
Ran the two new tests against a live implementation, my cloudflare-tunnel-gateway-controller on kind with a real Cloudflare Tunnel. Both pass; dropped the "not run yet" line from the description. |
|
I ran the tests against cloud-provider-kind, which turned out to need a fix: kubernetes-sigs/cloud-provider-kind#471. |
This satisfies upcoming conformance tests in kubernetes-sigs/gateway-api#5206 . Signed-off-by: Bittrance <bittrance@gmail.com>
|
I do not know who is reviewing conformance , tagging @snorwin and @rikatz for routing Once there is agreement please review kubernetes-sigs/cloud-provider-kind#471 for being conformant |
Tests that seed a status entry owned by another controller need the status helpers to stop waiting for that entry to catch up with the object generation: nothing reconciles it, so it never does. Extend the StaleControllerName exemption RouteMustHaveParents already has to the BackendTLSPolicy ancestor helpers, and add a StaleConditionType exemption to FilterStaleConditions for plain condition lists (Gateway and Listener conditions have no controllerName to key on, so those tests seed a foreign condition type instead). Signed-off-by: Aleksei Sviridkin <f@lex.la>
… controllers Same contract as the HTTPRoute foreign-status test, applied to the other status list that is scoped by controllerName: policy status.ancestors. The test seeds an ancestor entry under a stale controllerName, forces a generation bump, waits for the implementation's own ancestor to catch up, and requires the seeded entry to survive byte for byte. Signed-off-by: Aleksei Sviridkin <f@lex.la>
…tions Gateway and Listener conditions carry no controllerName, so the cross-controller contract there is different from status.parents and status.ancestors: implementations must not remove, change or update conditions whose type they are not responsible for. The test seeds a condition of a foreign type on the Gateway and on one listener, forces a generation bump, waits for the implementation's own conditions to catch up on both levels, and requires the seeded conditions to survive byte for byte. Signed-off-by: Aleksei Sviridkin <f@lex.la>
fa6c9a6 to
00da1c5
Compare
|
@youngnick, reworked all my PRs here. |
4 participants
What type of PR is this?
/kind test
/area conformance-test
What this PR does / why we need it:
Two more tests for the cross-controller status contract, stacked on #5138 (the first commit here is that PR, the last three are new).
BackendTLSPolicy status.ancestors is the only other status list scoped by controllerName, so its test is a structural copy of the HTTPRoute one: seed an ancestor entry under a stale controllerName, bump the generation, wait for the implementation's own ancestor to catch up, require the seeded entry to survive byte for byte.
Gateway and Listener conditions carry no controllerName. The API scopes them by condition type instead ("Implementations MUST NOT remove or reorder Conditions that they are not directly responsible for"), so that test seeds a condition of a foreign type on the Gateway and on one listener and checks survival the same way.
The status helpers needed the same observedGeneration exemption RouteMustHaveParents already has: nothing reconciles a seeded entry, so it never catches up. Unit tests pin both exemptions. The two new tests pass against a live implementation: cloudflare-tunnel-gateway-controller on kind with a real Cloudflare Tunnel.
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.