OCPBUGS-49738: fix handling of host conflict#736
OCPBUGS-49738: fix handling of host conflict#736jcmoraisjr wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
jira/severity-important
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-49738, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-49738, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/assign @gcs278 |
|
Able to reproduce the issue 'HostAlreadyClaimed' on a unfixed build and also tested the same on cluster using this PR. Hence marking as verified |
openshift-ci-robot
added
the
verified
|
@melvinjoseph86: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@melvinjoseph86 I'm not sure we have an e2e that covers this scenario, it was unknown for us up to now. So it'd need to be added if we want the coverage. |
|
@CodeRabbit can you review this PR? |
|
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughRoute host tracking and removal were reworked: Changes
Sequence Diagram(s)sequenceDiagram
participant Watch as K8sWatch
participant HostAdmitter
participant RouteMap
participant ClaimMaps as claimed/blocked maps
K8sWatch->>HostAdmitter: Added/Modified Route (with UID, Host, WildcardPolicy)
HostAdmitter->>HostAdmitter: lookup routeHosts[UID] -> oldHost?
alt oldHost exists
HostAdmitter->>RouteMap: RemoveRoute(keyFor(oldHost), route)
HostAdmitter->>ClaimMaps: remove oldHost and wildcardKey from claimedHosts/blockedWildcards/claimedWildcards
end
HostAdmitter->>HostAdmitter: apply current WildcardPolicy -> attempt claim
HostAdmitter->>RouteMap: Insert/claim new host entry
HostAdmitter->>HostAdmitter: update routeHosts[UID] = newHost
Note over HostAdmitter,ClaimMaps: On delete events, similar pre-removal uses cached host then clears routeHosts[UID]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci-robot
added
jira/invalid-bug
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-49738, which is invalid:
Comment DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/router/controller/host_admitter.go (1)
172-210:⚠️ Potential issue | 🟠 MajorMove stale-entry cleanup before conflict detection.
Line 174 can return before Lines 207-210 run. If a route changes host and the new host is rejected as already claimed, its old host entry remains in the maps, so the original stale-claim bug still survives on the rejection path.
🐛 Proposed fix
func (p *HostAdmitter) addRoute(route *routev1.Route) error { + // Remove old entries for this route from all hosts before conflict checks; + // this handles host changes even when the new host is rejected. + p.claimedHosts.RemoveRouteFromAllKeys(route) + p.blockedWildcards.RemoveRouteFromAllKeys(route) + p.claimedWildcards.RemoveRouteFromAllKeys(route) + // Find displaced routes (or error if an existing route displaces us) displacedRoutes, err, ownerNamespace := p.displacedRoutes(route) if err != nil { msg := fmt.Sprintf("a route in another namespace holds host %s", route.Spec.Host) if ownerNamespace == route.Namespace { @@ p.plugin.HandleRoute(watch.Deleted, displacedRoute) } - // Remove old entries for this route from all hosts, this handles the change of the host value - p.claimedHosts.RemoveRouteFromAllKeys(route) - p.blockedWildcards.RemoveRouteFromAllKeys(route) - p.claimedWildcards.RemoveRouteFromAllKeys(route) - if len(route.Spec.WildcardPolicy) == 0 { route.Spec.WildcardPolicy = routev1.WildcardPolicyNone }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/controller/host_admitter.go` around lines 172 - 210, The stale host entries cleanup must run before checking/early-returning on displaced routes so a rejected new-host doesn't leave the old host claimed; move the calls to p.claimedHosts.RemoveRouteFromAllKeys(route), p.blockedWildcards.RemoveRouteFromAllKeys(route), and p.claimedWildcards.RemoveRouteFromAllKeys(route) to just before invoking p.displacedRoutes(route) (i.e., run cleanup on the incoming route first), then call p.displacedRoutes(route) and proceed with the existing error handling and displacedRoute removals as-is; ensure you reference all three RemoveRouteFromAllKeys methods and keep the displacedRoutes/error flow unchanged besides relocating the cleanup.
🧹 Nitpick comments (1)
pkg/router/controller/host_admitter.go (1)
38-42: Consider avoiding full-map scans on every route admission.This helper is O(total routes) per call; used on every add/modify, bulk syncs can become O(n²). If route volume is high, consider tracking previous keys per namespace/name or limiting the full scan to update paths that can actually have stale entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/controller/host_admitter.go` around lines 38 - 42, RemoveRouteFromAllKeys currently scans the entire RouteMap on every call which is O(total routes) and leads to quadratic behavior during bulk syncs; change the implementation to avoid full-map scans by tracking previously associated keys per route (e.g., maintain a map[string]map[string]struct{} keyed by namespace/name -> set of keys) so RemoveRouteFromAllKeys(RouteMap.RemoveRouteFromAllKeys) can iterate only the known keys for the given route (routev1.Route) and call RouteMap.RemoveRoute for those entries, or alternatively update callers to pass the specific keys to remove rather than relying on a global scan.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/router/controller/host_admitter.go`:
- Around line 172-210: The stale host entries cleanup must run before
checking/early-returning on displaced routes so a rejected new-host doesn't
leave the old host claimed; move the calls to
p.claimedHosts.RemoveRouteFromAllKeys(route),
p.blockedWildcards.RemoveRouteFromAllKeys(route), and
p.claimedWildcards.RemoveRouteFromAllKeys(route) to just before invoking
p.displacedRoutes(route) (i.e., run cleanup on the incoming route first), then
call p.displacedRoutes(route) and proceed with the existing error handling and
displacedRoute removals as-is; ensure you reference all three
RemoveRouteFromAllKeys methods and keep the displacedRoutes/error flow unchanged
besides relocating the cleanup.
---
Nitpick comments:
In `@pkg/router/controller/host_admitter.go`:
- Around line 38-42: RemoveRouteFromAllKeys currently scans the entire RouteMap
on every call which is O(total routes) and leads to quadratic behavior during
bulk syncs; change the implementation to avoid full-map scans by tracking
previously associated keys per route (e.g., maintain a
map[string]map[string]struct{} keyed by namespace/name -> set of keys) so
RemoveRouteFromAllKeys(RouteMap.RemoveRouteFromAllKeys) can iterate only the
known keys for the given route (routev1.Route) and call RouteMap.RemoveRoute for
those entries, or alternatively update callers to pass the specific keys to
remove rather than relying on a global scan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a4014cee-4bb7-40ef-87a0-3c4f7d28ca9b
📒 Files selected for processing (1)
pkg/router/controller/host_admitter.go
jcmoraisjr
force-pushed
the
OCPBUGS-49738-fix-routemap
branch
from
d6ed14d to
daab4aa
Compare
openshift-ci-robot
removed
the
verified
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-49738, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/router/controller/host_admitter_test.go (1)
157-259:⚠️ Potential issue | 🟡 MinorAvoid masking host conflicts with zero UIDs after enabling wildcard tracking.
Line 159 now enables the RouteMap conflict path, but these table fixtures still share the empty UID.
HostAdmitter.displacedRoutesskips same-UID routes inpkg/router/controller/host_admitter.go:259-262, so duplicate-host cases can pass as “same route” rather than validating admission behavior.🧪 Proposed test isolation fix
func TestWildcardHostDeny(t *testing.T) { - p := &fakePlugin{} - admitter := NewHostAdmitter(p, wildcardAdmitter, true, false, LogRejections) tests := []struct { name string host string @@ for _, tc := range tests { - route := &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: tc.name, - Namespace: "deny", - }, - Spec: routev1.RouteSpec{Host: tc.host, WildcardPolicy: tc.policy}, - } + t.Run(tc.name, func(t *testing.T) { + p := &fakePlugin{} + admitter := NewHostAdmitter(p, wildcardAdmitter, true, false, LogRejections) + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.name, + Namespace: "deny", + UID: types.UID(tc.name), + }, + Spec: routev1.RouteSpec{Host: tc.host, WildcardPolicy: tc.policy}, + } - err := admitter.HandleRoute(watch.Added, route) - if tc.errors { - if err == nil { - t.Fatalf("Test case %s expected errors, got none", tc.name) - } - } else { - if err != nil { - t.Fatalf("Test case %s expected no errors, got %v", tc.name, err) + err := admitter.HandleRoute(watch.Added, route) + if tc.errors { + if err == nil { + t.Fatalf("Test case %s expected errors, got none", tc.name) + } + } else { + if err != nil { + t.Fatalf("Test case %s expected no errors, got %v", tc.name, err) + } } - } + }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/controller/host_admitter_test.go` around lines 157 - 259, The test uses multiple Route fixtures with empty ObjectMeta.UIDs, which masks conflict detection because HostAdmitter.displacedRoutes ignores routes with the same UID; update TestWildcardHostDeny to give each test route a unique UID (e.g., route.ObjectMeta.UID = types.UID(fmt.Sprintf("%s-uid", tc.name)) or similar) before calling NewHostAdmitter/HandleRoute so duplicate-host cases are evaluated as distinct routes and the wildcard conflict path is exercised correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/router/controller/host_admitter_test.go`:
- Around line 262-329: TestWildcardHostUpdate reuses one admitter and duplicate
UIDs across subtests and doesn't validate cleanup of route2's old host key;
update the test to create a fresh NewHostAdmitter per t.Run and assign unique
UIDs for each route (avoid reusing "1"/"2"), and add a third route (e.g.,
route3) that uses tc.oldHostRoute2 before modifying route2, call
admitter.HandleRoute(watch.Added, route3) to ensure the map entry for
oldHostRoute2 is exercised, then perform the Modified updates for route1/route2
with unique UIDs to assert the cleanup behavior implemented around
RemoveRouteFromAllKeys (host_admitter.go lines ~211-213) is covered.
---
Outside diff comments:
In `@pkg/router/controller/host_admitter_test.go`:
- Around line 157-259: The test uses multiple Route fixtures with empty
ObjectMeta.UIDs, which masks conflict detection because
HostAdmitter.displacedRoutes ignores routes with the same UID; update
TestWildcardHostDeny to give each test route a unique UID (e.g.,
route.ObjectMeta.UID = types.UID(fmt.Sprintf("%s-uid", tc.name)) or similar)
before calling NewHostAdmitter/HandleRoute so duplicate-host cases are evaluated
as distinct routes and the wildcard conflict path is exercised correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ae9cb993-942f-43a8-a1ce-81641a5396ba
📒 Files selected for processing (2)
pkg/router/controller/host_admitter.gopkg/router/controller/host_admitter_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/router/controller/host_admitter.go
jcmoraisjr
force-pushed
the
OCPBUGS-49738-fix-routemap
branch
from
daab4aa to
fe0db75
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
gcs278
left a comment
gcs278
left a comment
There was a problem hiding this comment.
Nice PR - generally looks good, I just have a concern about performance and a minor unit test coverage addition.
| require.NoError(t, err) | ||
| err = admitter.HandleRoute(watch.Modified, route2) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
nit For complete coverage, consider adding a delete then recreate here with another route that is using the same hostname, since the bug https://redhat.atlassian.net/browse/OCPBUGS-49738 called out a "phantom" hostname claim even while the route was deleted.
| }) | |
| route3name := tc.namePrefix + "-route3" | |
| route3 := &routev1.Route{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: route3name, | |
| Namespace: "host-update", | |
| UID: types.UID(route3name), | |
| }, | |
| Spec: routev1.RouteSpec{Host: tc.oldHostRoute1, WildcardPolicy: tc.policy}, | |
| } | |
| [...] | |
| // Delete both routes | |
| err = admitter.HandleRoute(watch.Deleted, route1) | |
| require.NoError(t, err) | |
| err = admitter.HandleRoute(watch.Deleted, route2) | |
| require.NoError(t, err) | |
| // New route should be able to claim route1's original host | |
| err = admitter.HandleRoute(watch.Added, route3) | |
| require.NoError(t, err) | |
| }) |
There was a problem hiding this comment.
Good idea, just added.
| p.claimedHosts.RemoveRouteFromAllKeys(route) | ||
| p.blockedWildcards.RemoveRouteFromAllKeys(route) | ||
| p.claimedWildcards.RemoveRouteFromAllKeys(route) |
There was a problem hiding this comment.
I had claude do a rough benchmarking for me.
Master (before) PR (after) Ratio
routes-100 ~3,800 ns/op ~7,600 ns/op 2x slower
routes-1,000 ~8,500 ns/op ~85,500 ns/op 10x slower
routes-10,000 ~170,000 ns/op ~1,100,000 ns/op 6x slower
Nothing unexpected here - iterating over the routes can be expensive for large clusters.
My experience is that the router is particularly performance sensitive - and sometimes have customers with an enormous amount of routes (10k+).
The alternative to looping through is to use a data structure to do some "bookkeeping" of the previous route (key) to hostname (value), so that you could look up the route's previous hostname quickly and remove that without scanning.
Could look something like:
| p.claimedHosts.RemoveRouteFromAllKeys(route) | |
| p.blockedWildcards.RemoveRouteFromAllKeys(route) | |
| p.claimedWildcards.RemoveRouteFromAllKeys(route) | |
| if oldHost, ok := p.routeHosts[route.UID]; ok { | |
| oldWildcardKey := routeapihelpers.GetDomainForHost(oldHost) | |
| p.claimedHosts.RemoveRoute(oldHost, route) | |
| p.blockedWildcards.RemoveRoute(oldWildcardKey, route) | |
| p.claimedWildcards.RemoveRoute(oldWildcardKey, route) | |
| } | |
| p.routeHosts[route.UID] = route.Spec.Host |
Did you consider adding a cache of route hostnames like this for better performance?
There was a problem hiding this comment.
This is a fair point, in fact I was focusing more in the problem instead of in the solution. Just changed pretty much like you suggested, changing a bit in naming and location of the code.
jcmoraisjr
force-pushed
the
OCPBUGS-49738-fix-routemap
branch
from
fe0db75 to
5f8894c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/router/controller/host_admitter.go (1)
145-150:⚠️ Potential issue | 🟠 MajorMemory leak:
routeHostscache not cleaned up on route deletion.When a route is deleted, the entry in
p.routeHosts[route.UID]is never removed. This causes a memory leak over time as routes are deleted, and could cause stale behavior if a route UID is reused.🔧 Proposed fix
case watch.Deleted: p.claimedHosts.RemoveRoute(route.Spec.Host, route) wildcardKey := routeapihelpers.GetDomainForHost(route.Spec.Host) p.claimedWildcards.RemoveRoute(wildcardKey, route) p.blockedWildcards.RemoveRoute(wildcardKey, route) + delete(p.routeHosts, route.UID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/controller/host_admitter.go` around lines 145 - 150, The Deleted case in the watch handler is missing cleanup of the per-route cache entry in p.routeHosts; add code in the watch.Deleted branch to remove the entry for the deleted route (e.g., delete(p.routeHosts, route.UID) or p.routeHosts.Delete(route.UID) depending on the container type) and, if p.routeHosts is protected by a mutex, perform the deletion under the same lock used elsewhere to avoid races.
🧹 Nitpick comments (1)
pkg/router/controller/host_admitter.go (1)
24-35: Misleading variable name:removedcontains remaining elements, not removed ones.
slices.DeleteFuncreturns the modified slice with matching elements removed, soremovedactually holds the elements that were not removed. Consider renaming toremainingfor clarity.♻️ Suggested rename for clarity
func (srm RouteMap) RemoveRoute(key string, route *routev1.Route) { source := srm[key] - removed := slices.DeleteFunc(source, func(r *routev1.Route) bool { + remaining := slices.DeleteFunc(source, func(r *routev1.Route) bool { return r.Namespace == route.Namespace && r.Name == route.Name }) - if len(removed) > 0 { - srm[key] = removed + if len(remaining) > 0 { + srm[key] = remaining } else { delete(srm, key) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/controller/host_admitter.go` around lines 24 - 35, The variable name `removed` in RouteMap.RemoveRoute is misleading because slices.DeleteFunc returns the slice after deletion (the remaining elements); rename `removed` to `remaining` (or similar) and update uses in the function (the assignment srm[key] = remaining and the else branch) to reflect that it contains the elements that remain rather than the ones removed—this keeps semantics clear in the RemoveRoute function and matches the behavior of slices.DeleteFunc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/router/controller/host_admitter.go`:
- Around line 145-150: The Deleted case in the watch handler is missing cleanup
of the per-route cache entry in p.routeHosts; add code in the watch.Deleted
branch to remove the entry for the deleted route (e.g., delete(p.routeHosts,
route.UID) or p.routeHosts.Delete(route.UID) depending on the container type)
and, if p.routeHosts is protected by a mutex, perform the deletion under the
same lock used elsewhere to avoid races.
---
Nitpick comments:
In `@pkg/router/controller/host_admitter.go`:
- Around line 24-35: The variable name `removed` in RouteMap.RemoveRoute is
misleading because slices.DeleteFunc returns the slice after deletion (the
remaining elements); rename `removed` to `remaining` (or similar) and update
uses in the function (the assignment srm[key] = remaining and the else branch)
to reflect that it contains the elements that remain rather than the ones
removed—this keeps semantics clear in the RemoveRoute function and matches the
behavior of slices.DeleteFunc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e42df920-e088-45f1-bf5f-a1a2e2247e97
📒 Files selected for processing (2)
pkg/router/controller/host_admitter.gopkg/router/controller/host_admitter_test.go
jcmoraisjr
force-pushed
the
OCPBUGS-49738-fix-routemap
branch
from
5f8894c to
704aaf6
Compare
Routes are grouped by host on three distinct hashmaps when allowWildcardRoutes is enabled. These hashmaps are updated whenever routes are added, modified or deleted, and their content is used to evaluate host conflicts. This hashmaps handling is done using the current host of the route resource, so, if the host value changes, the router does not find the entry on the hashmaps because it is using the current - and still not used - host value as the key. The approach used on this update is to cache the host from the router when it's added, and remove the resource from the cached host, which is the key of the hashmaps used to evaluate conflicts.
jcmoraisjr
force-pushed
the
OCPBUGS-49738-fix-routemap
branch
from
704aaf6 to
31a6c97
Compare
|
@jcmoraisjr: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| @@ -153,6 +147,7 @@ func (p *HostAdmitter) HandleRoute(eventType watch.EventType, route *routev1.Rou | |||
| wildcardKey := routeapihelpers.GetDomainForHost(route.Spec.Host) | |||
There was a problem hiding this comment.
I think there is another nuanced variation of this bug with delete behavior that we should consider fixing while we are here. Consider the scenario:
- RouteX added with hostA: maps have entries under A,
routeHosts[UID] = A - RouteX modified to hostB, but rejected by
displacedRoutes: maps still have entries under hostA,routeHosts[UID] still = A - RouteX deleted:
route.Spec.Host = B, but maps have the route under A:RemoveRoute("B", route)is a no-op, stale entry leaks
In this scenario, we'd be attempting to clean up entries in claimedHosts, claimedWildcards, and blockedWildcards using a host that was never admitted. routeHosts is effectively the "last successfully admitted host" while the route might have failed to admit it's current host.
Should we also use routeHosts here to fix that bug? Also, if you do end up fixing this, consider a unit test, like TestDeleteAfterRejectedHostChange to verify the bug I'm describing & the fix.
| p.blockedWildcards.RemoveRoute(oldWildcardKey, route) | ||
| p.claimedWildcards.RemoveRoute(oldWildcardKey, route) | ||
| } | ||
| p.routeHosts[route.UID] = route.Spec.Host |
There was a problem hiding this comment.
Should this go below the switch statement? The switch statement's default case is a rejection for an unsupported wildcard policy, which means we'd be storing a route host in our map that was rejected.
Probably more of defensive coding comment - I don't know if it's possible to trigger the Default case with API validation.
5 participants
Routes are grouped by host on three distinct hashmaps when allowWildcardRoutes is enabled. These hashmaps are updated whenever routes are added, modified or deleted, and their content is used to evaluate host conflicts. This hashmaps handling is done using the current host of the route resource, so, if the host value changes, the router does not find the entry on the hashmaps because it is using the current - and still not used - host value as the key.
The approach used on this update is to ignore the host from the router, and remove the resource from all the hosts instead, since the only match on all the hashmaps should be found on the key corresponding to the former host value.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests