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.
|
WalkthroughRoute host ownership tracking was changed: ChangesHost admitter ownership flow
Sequence Diagram(s)sequenceDiagram
participant K8sWatch as K8sWatch
participant Admitter as HostAdmitter
participant Map as RouteMap
participant Claims as claimed/blocked maps
K8sWatch->>Admitter: Add/Modify/Delete Route (UID, Spec{Host,Path,WildcardPolicy})
Admitter->>Admitter: lookup routeHosts[UID] -> oldHost?
alt oldHost exists
Admitter->>Map: RemoveRoute(keyFor(oldHost), route)
Admitter->>Claims: remove oldHost and wildcardKey from claimedHosts/blockedWildcards/claimedWildcards
end
alt event != Deleted
Admitter->>Admitter: evaluate WildcardPolicy and attempt claim
Admitter->>Map: Insert/claim new host entry
Admitter->>Admitter: routeHosts[UID] = newHost
else Deleted
Admitter->>Admitter: delete routeHosts[UID]
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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. |
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
2 times, most recently
from
704aaf6 to
31a6c97
Compare
| @@ -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.
There was a problem hiding this comment.
Good catch, got it, makes sense, just updated as suggested.
I reused the same tests to cover this new scenario, since this is a complement of what the test already implements, how do the changes sound?
| 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.
There was a problem hiding this comment.
In fact I think the API validation blocks this path to happen, but it worth to use a defensive code here. Just changed as suggested.
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
31a6c97 to
c223ef7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/router/controller/host_admitter.go (1)
218-238: ⚡ Quick winPre-removal of old claims happens before wildcard-policy validation — state can become inconsistent if the
defaultbranch is reached.When the pre-removal block (lines 218–224) runs, it removes the route's old claims from
claimedHosts,blockedWildcards, andclaimedWildcards. If the switch then hitsdefault, it returns an error after those removals but before inserting any new claims. The downstream plugin'sHandleRouteis never called (the caller returns on error), so the route continues to be served — but the admitted-host entry is now gone from all three maps, allowing another route to claim the same host.routeHosts[route.UID]is also stale (still holds the old host). The recovery path on a subsequent add/modify is a no-op pre-removal followed by correct re-insertion, so the state is eventually corrected, but there is a window of inconsistency.While
WildcardPolicyNoneandWildcardPolicySubdomainare the only values currently valid via API admission, a forward-compatibility edge exists if a new policy type is added to the API without a corresponding router update.The simplest defensive fix is to validate the policy before mutating any state:
🛡️ Suggested defensive fix: validate policy before pre-removal
+ // Validate the wildcard policy before touching claimed/blocked maps so that + // an unsupported policy cannot leave the maps in an inconsistent state. + switch route.Spec.WildcardPolicy { + case routev1.WildcardPolicyNone, routev1.WildcardPolicySubdomain: + // valid, fall through + default: + err := fmt.Errorf("unsupported wildcard policy %s", route.Spec.WildcardPolicy) + p.recorder.RecordRouteRejection(route, "RouteNotAdmitted", err.Error()) + return err + } + // Restore the old host and old wildcard key, removing from the previously claimed ones if oldHost, found := p.routeHosts[route.UID]; found { oldWildcardKey := routeapihelpers.GetDomainForHost(oldHost) // ensure the route doesn't exist as a claimed host or blocked wildcard - in case it previously was p.claimedHosts.RemoveRoute(oldHost, route) p.blockedWildcards.RemoveRoute(oldWildcardKey, route) p.claimedWildcards.RemoveRoute(oldWildcardKey, route) } switch route.Spec.WildcardPolicy { case routev1.WildcardPolicyNone: p.claimedHosts.InsertRoute(route.Spec.Host, route) p.blockedWildcards.InsertRoute(wildcardKey, route) case routev1.WildcardPolicySubdomain: p.claimedWildcards.InsertRoute(wildcardKey, route) - default: - err := fmt.Errorf("unsupported wildcard policy %s", route.Spec.WildcardPolicy) - p.recorder.RecordRouteRejection(route, "RouteNotAdmitted", err.Error()) - return err }🤖 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 218 - 238, Pre-validate the route's WildcardPolicy before mutating admission state: move or add a policy validation step at the start of the admission path in host_admitter (where p.routeHosts is referenced) to return an error and call p.recorder.RecordRouteRejection for unsupported policies before performing the pre-removal block that calls p.claimedHosts.RemoveRoute, p.blockedWildcards.RemoveRoute and p.claimedWildcards.RemoveRoute; keep the existing switch (route.Spec.WildcardPolicy -> routev1.WildcardPolicyNone/routev1.WildcardPolicySubdomain/default) but ensure the default branch can never run after state was already mutated by performing the validation first and only then doing the removals and subsequent InsertRoute calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/router/controller/host_admitter.go`:
- Around line 218-238: Pre-validate the route's WildcardPolicy before mutating
admission state: move or add a policy validation step at the start of the
admission path in host_admitter (where p.routeHosts is referenced) to return an
error and call p.recorder.RecordRouteRejection for unsupported policies before
performing the pre-removal block that calls p.claimedHosts.RemoveRoute,
p.blockedWildcards.RemoveRoute and p.claimedWildcards.RemoveRoute; keep the
existing switch (route.Spec.WildcardPolicy ->
routev1.WildcardPolicyNone/routev1.WildcardPolicySubdomain/default) but ensure
the default branch can never run after state was already mutated by performing
the validation first and only then doing the removals and subsequent InsertRoute
calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4690f66-7e46-4c8e-99e7-72e407829202
📒 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_test.go
|
@jcmoraisjr: all tests passed! 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. |
|
LGTM. Code Rabbit did point out that there is an minor inconsistency with removing the old routes from the hashmaps lookups before the wildcard policy is validated. Practically this can't happen today since the API validates the policy before it reaches the router, so I'm okay with moving forward. Feel free to address if you want. Thanks for the quick turn around! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
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