fix(webhook): scope domain uniqueness to the listener - #235
H34rtUnd3rB14d3 wants to merge 2 commits into
Conversation
Domain collisions were keyed by (nodeID, domain), so the same domain on two VirtualServices attached to different listeners was rejected as a duplicate. Envoy resolves an incoming connection to a listener first and only then picks a filter chain inside it, so domains only have to be unique within a single listener. '*' hit this hardest, being the usual way to say "accept anything on this port": two listeners could not both have a catch-all. Key collisions by (nodeID, listener, domain) instead. The listener's namespaced name is the scope - two listeners stay independent even when they share a port but differ by address. Duplicates within one listener are still rejected, and the error now names the listener. The store index keeps its map shape and holds the composite key, since nothing else reads it. In the light validation path domains come from route configurations, which do not carry a listener; filter chains and route configurations are both named after their VirtualService, so the listener is recovered by matching those names, falling back to the heavy dry-run when a route configuration cannot be attributed. Three e2e specs cover the acceptance criteria: the same domain served from two listeners on different ports, the same for '*', and a duplicate within one listener still being rejected. They were verified to fail without this change with "duplicate domain dup.kaasops.io for node test". Every VirtualService in them gets its own listener on purpose - a non-TLS listener puts no match criteria on its filter chain, so two VirtualServices on one such listener collide in Envoy whatever their domains are. Running the e2e suite also surfaced three chart CRDs - endpoints, secrets and virtualhosts - that had drifted from config/crd/bases. They have no Go types left, so controller-gen does not regenerate them, and only the config/ copies were refreshed during an earlier refactoring. The stale copies made the chart install fail against CRDs already applied from config/crd, which is what broke the suite locally. Copy the current versions over so both sources agree again.
unparam flags it now that the helper has four callers, all passing "ns". The parameter never carried any variation, so replace it with a constant shared with the listener fixtures.
| } | ||
| } | ||
|
|
||
| func TestLightValidator_UpdatePrevVSExcluded(t *testing.T) { |
There was a problem hiding this comment.
This test wasn't moved to the listener-scoped key: the index is seeded with a bare b.com, and the stub returns Resources without Listener, so the candidate looks up default/|b.com and never finds anything. It stays green even with the exclusion in excludePreviousVSDomains turned off (I tried: this one passes, the multi-node test next to it fails). Could you seed ldk(testListenerA, "b.com") and set Listener: testListenerA in both stub branches, like the neighbouring tests do?
| nodeDom := nodeIDDomain(nodeID, ldKey) | ||
| if _, ok := nodeIDDomainsSet[nodeDom]; ok { | ||
| return fmt.Errorf("duplicate domain %s for node %s", domain, nodeID), nil, vsStatuses, metrics | ||
| return fmt.Errorf("duplicate domain %s for node %s on listener %s", |
There was a problem hiding this comment.
Here the domain is unquoted, while the light path (L1096) and validateDomainsWithinVS quote it. The e2e check for duplicate domain dup.kaasops.io matches only this form, so it passes only while webhook.lightDryRun is off. Maybe switch both heavy-path messages (here and L475) to '%s' and update the e2e string?
| // they come from, so the listener owning a filter chain also owns the route | ||
| // configuration with the same name. That gives us the listener scope domains | ||
| // have to be unique within. | ||
| rcListener, err := c.routeConfigToListener(nodeID) |
There was a problem hiding this comment.
Route configurations (L980) and listeners (here) come from two separate cache reads, each under its own read lock, and SetSnapshot can land in between. Then route configs from one snapshot get attributed using listeners from the next one. Would it make sense to take the snapshot once and read both resource types from it?
This branch has not been deployed
2 participants
Problem
The validating webhook rejects a
VirtualServicewhen anotherVirtualServiceon thesame node already uses one of its domains — even when the two are attached to different
listeners:
Envoy resolves an incoming connection to a listener first and only then picks a filter
chain inside it, so domains only have to be unique within a single listener. Two
listeners are independent even when they share a port and differ only by address.
*is hit hardest, because it is the usual way to say "accept anything on this port":two listeners could not both have a catch-all.
Cause
Collisions were keyed by
(nodeID, domain)—nodeIDDomain()ininternal/xds/updater/updater.go. The listener never entered the key, so the check wasstrictly broader than the constraint it was meant to enforce.
Change
Key collisions by
(nodeID, listener, domain). The listener's namespaced name is thescope, not its
address:port— that keeps two listeners independent when they share aport. Duplicates within one listener are still rejected, and the error now names the
listener:
Three chart CRDs —
endpoints,secrets,virtualhosts— are also synced withconfig/crd/bases, from which they had drifted. They have no Go types left since #178,so
controller-gendoes not regenerate them; without the sync the chart cannot beinstalled over CRDs already applied from
config/crd, which breaks the e2e suite.Tests
Three e2e specs matching the acceptance criteria, in
test/e2e/domain_per_listener_test.go:its own
VirtualService;*;They were verified to fail without this change, with
duplicate domain dup.kaasops.io for node test.Every
VirtualServicein them gets its own listener on purpose: a non-TLS listener putsno match criteria on its filter chain, so two
VirtualServices on one such listenercollide in Envoy whatever their domains are — the specs would otherwise fail for an
unrelated reason.
Unit tests in
internal/xds/updater/light_validator_test.gocover the light path,including two listeners that share a port and differ only by address.
Verification
make lint— no issuesmake test— no failuresmake test-e2e— 52/52 specs pass on a clean kind cluster