ACM-39570: fix namespace deletion blocked by HCP proxy rejecting cleanup requests - #773
Conversation
…espace deletion The HCP proxy's handleRoute unconditionally required the hostingCluster query parameter, causing the Kubernetes namespace controller's cleanup DELETE/LIST requests to be rejected with 400 BadRequest. This blocked namespace deletion cluster-wide since the namespace controller cannot confirm resource cleanup for the hcp.ocm.io/v1alpha1 API group. Return an empty HostedClusterList for collection-level requests without hostingCluster, which is semantically correct since the proxy stores no resources locally. Also add list and deletecollection to the discovery verbs so the namespace controller knows these operations are supported. Ref: https://issues.redhat.com/browse/ACM-39570 Signed-off-by: kurwang <kurwang@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: stolostron/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe HCP proxy now advertises ChangesHostedCluster collection handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✨ 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. Comment |
Root causeWhen any namespace is deleted, the Kubernetes namespace controller enumerates all registered API groups and sends |
kurwang
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/manager/hcp_proxy_test.go (1)
338-351: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete empty-list payload.
The GET and DELETE tests verify only
kindand emptyitems. They do not verifyapiVersionormetadata.resourceVersion, whichhandleEmptyCollectionnow sets. Add assertions for these fields so an incomplete Kubernetes list response cannot pass.Also applies to: 353-366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/manager/hcp_proxy_test.go` around lines 338 - 351, The collection GET and DELETE tests around handleRoute and handleEmptyCollection only validate kind and empty items. Extend both test assertions to verify the response apiVersion and metadata.resourceVersion fields match the values produced by handleEmptyCollection, while preserving the existing empty-list checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/manager/hcp_proxy_test.go`:
- Around line 320-325: Update the handler tests around the status checks at
lines 286, 298, 311, 345, and 360 to use require.Equal with descriptive failure
messages, change collection items assertions to require.Empty, and add a
descriptive message to the JSON decode require.NoError call. Keep setup-failure
assertions fatal so subsequent test logic does not run on invalid responses.
In `@pkg/manager/hcp_proxy.go`:
- Line 499: Update the JSON response encoding call in the surrounding proxy
handler to handle the error returned by json.Encoder.Encode instead of assigning
it to _. Wrap encoding failures with fmt.Errorf("context: %w", err), then log or
return the wrapped error through the existing proxy error-handling path with
useful context.
- Around line 411-419: Update the collection shortcut around sanitizeProxyName
and handleEmptyCollection so it applies only when the hostingCluster query key
is absent and the request method is GET or DELETE. When the key is present,
validate every value using the existing allow-list validation and return 400 for
invalid or empty values; unsupported collection methods must continue through
normal handling rather than returning HostedClusterList. Add regression tests
covering invalid values and unsupported methods.
- Line 388: Update the resource discovery entry near the APIResourceList verbs
and dispatchCollection consistently: either implement delegation for collection
GET/DELETE to their intended handlers, or remove list and deletecollection from
the advertised verbs so discovery exposes only supported operations. Preserve
create and delete behavior already supported by dispatchCollection.
---
Nitpick comments:
In `@pkg/manager/hcp_proxy_test.go`:
- Around line 338-351: The collection GET and DELETE tests around handleRoute
and handleEmptyCollection only validate kind and empty items. Extend both test
assertions to verify the response apiVersion and metadata.resourceVersion fields
match the values produced by handleEmptyCollection, while preserving the
existing empty-list checks.
🪄 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: stolostron/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 29a26000-e21c-4b35-a285-9a144b06bfcc
📒 Files selected for processing (2)
pkg/manager/hcp_proxy.gopkg/manager/hcp_proxy_test.go
Signed-off-by: kurwang <kurwang@redhat.com>
Signed-off-by: kurwang <kurwang@redhat.com>
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kurwang, rokej 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 |
2 participants
Description of the change(s):
handleEmptyCollectionto return an emptyHostedClusterListfor collection-level GET/DELETE requests that arrive without ahostingClusterquery parameterhandleRouteto detect collection endpoints and route tohandleEmptyCollectionwhenhostingClusteris absent, instead of returning 400 BadRequestlistanddeletecollectionto the advertised verbs in the API discovery document for thehostedclustersresourceWhy do we need this PR:
DELETE /apis/hcp.ocm.io/v1alpha1/namespaces/{ns}/hostedclusters) during namespace cleanup without the custom?hostingClusterquery parameterhostingClusteron all requests and rejects these with 400 BadRequest, causing every namespace deletion on the cluster to stall indefinitely withNamespaceDeletionContentFailureIssue reference:
Test API/Unit - Success