ACM-39570, ACM-39868: fix HCP proxy namespace deletion and cluster-wide list - #774
Conversation
|
Warning Review limit reached
Next review available in: 54 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 (2)
WalkthroughThe HCP proxy now advertises ChangesHCP proxy routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
Add cluster-wide list path handling so "oc get hostedclusters -A" returns an empty HostedClusterList instead of 400 BadRequest. Also reject watch requests early with 405 since the proxy cannot maintain event streams. Ref: https://issues.redhat.com/browse/ACM-39868 Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/manager/hcp_proxy.go (1)
512-519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHandle the JSON encoding error.
Line 514 discards the error from
Encode. Propagate a contextual error if the handler contract permits it. Otherwise, record the error with the proxy structured logger.As per path instructions, “Never ignore error returns; wrap with fmt.Errorf("context: %w", err)”.
🤖 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.go` around lines 512 - 519, Update handleEmptyCollection in hcpProxy so the json.NewEncoder(w).Encode call no longer discards its error. If the surrounding handler flow can return an error, propagate it with contextual wrapping from handleEmptyCollection; otherwise, log the encoding failure through the proxy’s structured logger with enough context to identify the response path. Keep the existing HostedClusterList response payload and header setup unchanged.Source: Path instructions
pkg/manager/hcp_proxy_test.go (1)
320-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd failure messages to the changed assertions.
The new
assertandrequirecalls do not explain the expected behavior or why it matters. Add messages that identify the route contract under test, such as watch rejection, empty collection behavior, or invalid parameter rejection.As per coding guidelines, “Assertions should include meaningful failure messages.” As per path instructions, “Both: assertion messages must explain what was expected and why it matters.”
🤖 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 320 - 410, Add explicit failure messages to the new assertions in handleRoute tests so each check states the route contract being verified. Update the affected assert.Equal, assert.Contains, assert.Empty, and require.NoError calls in Test_handleRoute_WhenWatchRequested_ItShouldReturn405, Test_handleRoute_WhenMissingHostingCluster_OnNamedEndpoint_ItShouldReturn400, Test_handleRoute_WhenMissingHostingCluster_OnCollectionGET_ItShouldReturnEmptyList, Test_handleRoute_WhenMissingHostingCluster_OnCollectionDELETE_ItShouldReturnEmptyList, Test_handleRoute_WhenMissingHostingCluster_OnCollectionPOST_ItShouldReturn400, Test_handleRoute_WhenClusterWideGET_WithoutHostingCluster_ItShouldReturnEmptyList, and Test_handleRoute_WhenInvalidHostingCluster_OnCollection_ItShouldReturn400 to include messages that mention the expected route behavior and why it matters.Sources: Coding guidelines, Path instructions
test/e2e/hcp_proxy_test.go (1)
151-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd failure messages to the changed Gomega assertions.
The assertions do not identify the failed route contract. Add messages that state the expected status and empty-list behavior.
As per coding guidelines, “Assertions should include meaningful failure messages.” As per path instructions, “Both: assertion messages must explain what was expected and why it matters.”
Also applies to: 378-389
🤖 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 `@test/e2e/hcp_proxy_test.go` around lines 151 - 157, Update the changed Gomega assertions in the e2e route check to include meaningful failure messages in the hcp_proxy_test flow, especially around the status code and empty HostedClusterList response. Use the existing resp, doc, and json.NewDecoder assertions to attach messages that state the expected route contract (HTTP 200 and an empty items list) so failures clearly identify what was expected and why the proxy response matters.Sources: Coding guidelines, Path instructions
🤖 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.go`:
- Around line 416-429: Update the request handling around hostingClusterParam in
hcp_proxy.go to distinguish an absent query parameter from an explicitly empty
one, since Query().Get() collapses both to "". Keep the empty-collection
shortcut in this path only for truly missing hostingCluster on collection
GET/DELETE requests, and let ?hostingCluster= continue through sanitizeProxyName
so it returns HTTP 400. Add a focused test covering the ?hostingCluster= case to
lock in the behavior.
In `@test/e2e/hcp_proxy_test.go`:
- Around line 154-157: The HostedClusterList assertions must validate the full
empty-list payload on both routes. Update the tests at
test/e2e/hcp_proxy_test.go lines 154-157 and 383-389 to decode each response
body, assert kind equals HostedClusterList, and verify items is present as an
empty array rather than accepting missing or null items; also ensure the
APIService route validates this payload instead of only HTTP 200.
- Around line 148-150: Update the e2e cluster proxy request around client.Get to
create a per-request context with a timeout, pass it via an HTTP request instead
of using client.Get directly, and register its cancellation with
ginkgo.DeferCleanup. Preserve the existing proxy URL and response handling while
ensuring each Eventually attempt has a bounded round-trip.
---
Nitpick comments:
In `@pkg/manager/hcp_proxy_test.go`:
- Around line 320-410: Add explicit failure messages to the new assertions in
handleRoute tests so each check states the route contract being verified. Update
the affected assert.Equal, assert.Contains, assert.Empty, and require.NoError
calls in Test_handleRoute_WhenWatchRequested_ItShouldReturn405,
Test_handleRoute_WhenMissingHostingCluster_OnNamedEndpoint_ItShouldReturn400,
Test_handleRoute_WhenMissingHostingCluster_OnCollectionGET_ItShouldReturnEmptyList,
Test_handleRoute_WhenMissingHostingCluster_OnCollectionDELETE_ItShouldReturnEmptyList,
Test_handleRoute_WhenMissingHostingCluster_OnCollectionPOST_ItShouldReturn400,
Test_handleRoute_WhenClusterWideGET_WithoutHostingCluster_ItShouldReturnEmptyList,
and Test_handleRoute_WhenInvalidHostingCluster_OnCollection_ItShouldReturn400 to
include messages that mention the expected route behavior and why it matters.
In `@pkg/manager/hcp_proxy.go`:
- Around line 512-519: Update handleEmptyCollection in hcpProxy so the
json.NewEncoder(w).Encode call no longer discards its error. If the surrounding
handler flow can return an error, propagate it with contextual wrapping from
handleEmptyCollection; otherwise, log the encoding failure through the proxy’s
structured logger with enough context to identify the response path. Keep the
existing HostedClusterList response payload and header setup unchanged.
In `@test/e2e/hcp_proxy_test.go`:
- Around line 151-157: Update the changed Gomega assertions in the e2e route
check to include meaningful failure messages in the hcp_proxy_test flow,
especially around the status code and empty HostedClusterList response. Use the
existing resp, doc, and json.NewDecoder assertions to attach messages that state
the expected route contract (HTTP 200 and an empty items list) so failures
clearly identify what was expected and why the proxy response matters.
🪄 Autofix
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: 17357e0a-cc17-45dd-a40f-b21450caf09c
📒 Files selected for processing (3)
pkg/manager/hcp_proxy.gopkg/manager/hcp_proxy_test.gotest/e2e/hcp_proxy_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/manager/hcp_proxy.go (1)
512-519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHandle the JSON encoding error.
Line 514 discards the error from
Encode. Propagate a contextual error if the handler contract permits it. Otherwise, record the error with the proxy structured logger.As per path instructions, “Never ignore error returns; wrap with fmt.Errorf("context: %w", err)”.
🤖 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.go` around lines 512 - 519, Update handleEmptyCollection in hcpProxy so the json.NewEncoder(w).Encode call no longer discards its error. If the surrounding handler flow can return an error, propagate it with contextual wrapping from handleEmptyCollection; otherwise, log the encoding failure through the proxy’s structured logger with enough context to identify the response path. Keep the existing HostedClusterList response payload and header setup unchanged.Source: Path instructions
pkg/manager/hcp_proxy_test.go (1)
320-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd failure messages to the changed assertions.
The new
assertandrequirecalls do not explain the expected behavior or why it matters. Add messages that identify the route contract under test, such as watch rejection, empty collection behavior, or invalid parameter rejection.As per coding guidelines, “Assertions should include meaningful failure messages.” As per path instructions, “Both: assertion messages must explain what was expected and why it matters.”
🤖 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 320 - 410, Add explicit failure messages to the new assertions in handleRoute tests so each check states the route contract being verified. Update the affected assert.Equal, assert.Contains, assert.Empty, and require.NoError calls in Test_handleRoute_WhenWatchRequested_ItShouldReturn405, Test_handleRoute_WhenMissingHostingCluster_OnNamedEndpoint_ItShouldReturn400, Test_handleRoute_WhenMissingHostingCluster_OnCollectionGET_ItShouldReturnEmptyList, Test_handleRoute_WhenMissingHostingCluster_OnCollectionDELETE_ItShouldReturnEmptyList, Test_handleRoute_WhenMissingHostingCluster_OnCollectionPOST_ItShouldReturn400, Test_handleRoute_WhenClusterWideGET_WithoutHostingCluster_ItShouldReturnEmptyList, and Test_handleRoute_WhenInvalidHostingCluster_OnCollection_ItShouldReturn400 to include messages that mention the expected route behavior and why it matters.Sources: Coding guidelines, Path instructions
test/e2e/hcp_proxy_test.go (1)
151-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd failure messages to the changed Gomega assertions.
The assertions do not identify the failed route contract. Add messages that state the expected status and empty-list behavior.
As per coding guidelines, “Assertions should include meaningful failure messages.” As per path instructions, “Both: assertion messages must explain what was expected and why it matters.”
Also applies to: 378-389
🤖 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 `@test/e2e/hcp_proxy_test.go` around lines 151 - 157, Update the changed Gomega assertions in the e2e route check to include meaningful failure messages in the hcp_proxy_test flow, especially around the status code and empty HostedClusterList response. Use the existing resp, doc, and json.NewDecoder assertions to attach messages that state the expected route contract (HTTP 200 and an empty items list) so failures clearly identify what was expected and why the proxy response matters.Sources: Coding guidelines, Path instructions
🤖 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.go`:
- Around line 416-429: Update the request handling around hostingClusterParam in
hcp_proxy.go to distinguish an absent query parameter from an explicitly empty
one, since Query().Get() collapses both to "". Keep the empty-collection
shortcut in this path only for truly missing hostingCluster on collection
GET/DELETE requests, and let ?hostingCluster= continue through sanitizeProxyName
so it returns HTTP 400. Add a focused test covering the ?hostingCluster= case to
lock in the behavior.
In `@test/e2e/hcp_proxy_test.go`:
- Around line 154-157: The HostedClusterList assertions must validate the full
empty-list payload on both routes. Update the tests at
test/e2e/hcp_proxy_test.go lines 154-157 and 383-389 to decode each response
body, assert kind equals HostedClusterList, and verify items is present as an
empty array rather than accepting missing or null items; also ensure the
APIService route validates this payload instead of only HTTP 200.
- Around line 148-150: Update the e2e cluster proxy request around client.Get to
create a per-request context with a timeout, pass it via an HTTP request instead
of using client.Get directly, and register its cancellation with
ginkgo.DeferCleanup. Preserve the existing proxy URL and response handling while
ensuring each Eventually attempt has a bounded round-trip.
---
Nitpick comments:
In `@pkg/manager/hcp_proxy_test.go`:
- Around line 320-410: Add explicit failure messages to the new assertions in
handleRoute tests so each check states the route contract being verified. Update
the affected assert.Equal, assert.Contains, assert.Empty, and require.NoError
calls in Test_handleRoute_WhenWatchRequested_ItShouldReturn405,
Test_handleRoute_WhenMissingHostingCluster_OnNamedEndpoint_ItShouldReturn400,
Test_handleRoute_WhenMissingHostingCluster_OnCollectionGET_ItShouldReturnEmptyList,
Test_handleRoute_WhenMissingHostingCluster_OnCollectionDELETE_ItShouldReturnEmptyList,
Test_handleRoute_WhenMissingHostingCluster_OnCollectionPOST_ItShouldReturn400,
Test_handleRoute_WhenClusterWideGET_WithoutHostingCluster_ItShouldReturnEmptyList,
and Test_handleRoute_WhenInvalidHostingCluster_OnCollection_ItShouldReturn400 to
include messages that mention the expected route behavior and why it matters.
In `@pkg/manager/hcp_proxy.go`:
- Around line 512-519: Update handleEmptyCollection in hcpProxy so the
json.NewEncoder(w).Encode call no longer discards its error. If the surrounding
handler flow can return an error, propagate it with contextual wrapping from
handleEmptyCollection; otherwise, log the encoding failure through the proxy’s
structured logger with enough context to identify the response path. Keep the
existing HostedClusterList response payload and header setup unchanged.
In `@test/e2e/hcp_proxy_test.go`:
- Around line 151-157: Update the changed Gomega assertions in the e2e route
check to include meaningful failure messages in the hcp_proxy_test flow,
especially around the status code and empty HostedClusterList response. Use the
existing resp, doc, and json.NewDecoder assertions to attach messages that state
the expected route contract (HTTP 200 and an empty items list) so failures
clearly identify what was expected and why the proxy response matters.
🪄 Autofix
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: 17357e0a-cc17-45dd-a40f-b21450caf09c
📒 Files selected for processing (3)
pkg/manager/hcp_proxy.gopkg/manager/hcp_proxy_test.gotest/e2e/hcp_proxy_test.go
🛑 Comments failed to post (3)
pkg/manager/hcp_proxy.go (1)
416-429: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject an explicitly empty
hostingClustervalue.
Query().Get()returns""for both an absent parameter and?hostingCluster=. Therefore, a collection request with?hostingCluster=returns HTTP 200 instead of reachingsanitizeProxyNameand returning HTTP 400.Check parameter presence separately. Add a test for
?hostingCluster=.Proposed fix
- hostingClusterParam := r.URL.Query().Get("hostingCluster") + query := r.URL.Query() + hostingClusterParam := query.Get("hostingCluster") - if hostingClusterParam == "" { + if _, supplied := query["hostingCluster"]; !supplied {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.query := r.URL.Query() hostingClusterParam := query.Get("hostingCluster") // When hostingCluster is completely absent, the Kubernetes namespace // controller may be sending collection-level GET (list) or DELETE // (delete-collection) requests during namespace cleanup. The same // applies to cluster-wide list calls (e.g. "oc get hostedclusters -A"). // Return an empty HostedClusterList so these callers are not blocked. // POST (create) still requires a spoke target → fall through to 400. if _, supplied := query["hostingCluster"]; !supplied { isNamespacedCollection := len(parts) == 3 && parts[0] == "namespaces" && parts[2] == hcpProxyResource isClusterWideList := len(parts) == 1 && parts[0] == hcpProxyResource if (isNamespacedCollection || isClusterWideList) && (r.Method == http.MethodGet || r.Method == http.MethodDelete) { p.handleEmptyCollection(w, r) return🤖 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.go` around lines 416 - 429, Update the request handling around hostingClusterParam in hcp_proxy.go to distinguish an absent query parameter from an explicitly empty one, since Query().Get() collapses both to "". Keep the empty-collection shortcut in this path only for truly missing hostingCluster on collection GET/DELETE requests, and let ?hostingCluster= continue through sanitizeProxyName so it returns HTTP 400. Add a focused test covering the ?hostingCluster= case to lock in the behavior.test/e2e/hcp_proxy_test.go (2)
148-150: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== files ==" git ls-files | rg '(^|/)hcp_proxy_test\.go$|hcp_proxy|proxy' | sed -n '1,120p' echo "== outline ==" ast-grep outline test/e2e/hcp_proxy_test.go --view compact || true echo "== context/todo usage ==" rg -n "context\\.(TODO|WithTimeout|New\\(|Background)|DeferCleanup|Eventually|client\\.Get\\(|\\.Do\\(" test/e2e/hcp_proxy_test.go echo "== relevant lines ==" cat -n test/e2e/hcp_proxy_test.go | sed -n '120,175p;350,405p'Repository: stolostron/hypershift-addon-operator
Length of output: 7141
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== imports and variables ==" cat -n test/e2e/hcp_proxy_test.go | sed -n '1,70p' echo "== context.TODO references with line context ==" rg -n -C 4 "context\.TODO|ctx := context\.TODO\(\)|var ctx|context\.Background" test/e2e/hcp_proxy_test.go echo "== helper definitions for API funcs ==" rg -n -C 6 "func insecureHTTPClient|func proxyURL|func NewKubeClient|func Eventually|eventuallyTimeout|eventuallyInterval" test/e2eRepository: stolostron/hypershift-addon-operator
Length of output: 15619
Set per-request deadlines on the new e2e cluster proxy calls.
client.Getsends requests with no request context, so it uses the client deadline instead of a request deadline. The APIService route callsDo(ctx)wherectxiscontext.TODO()and has no deadline, and the surroundingEventuallyhas no built-in way to cancel a blocking round-trip. Use a timed request context per call and register its cancellation withginkgo.DeferCleanup.🤖 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 `@test/e2e/hcp_proxy_test.go` around lines 148 - 150, Update the e2e cluster proxy request around client.Get to create a per-request context with a timeout, pass it via an HTTP request instead of using client.Get directly, and register its cancellation with ginkgo.DeferCleanup. Preserve the existing proxy URL and response handling while ensuring each Eventually attempt has a bounded round-trip.Sources: Coding guidelines, Path instructions
154-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== locate file ==" git ls-files | rg '(^|/)hcp_proxy_test\.go$' || true echo "== file stats ==" wc -l test/e2e/hcp_proxy_test.go echo "== relevant sections ==" sed -n '130,175p' test/e2e/hcp_proxy_test.go sed -n '360,405p' test/e2e/hcp_proxy_test.go echo "== helper definitions ==" rg -n "eventuallyTimeout|eventuallyInterval|HostedClusterList|hostedclusters|Do\\(ctx\\)" test/e2e/hcp_proxy_test.go echo "== go behavior probe for nil/missing items map lookup ==" python3 - <<'PY' from collections.abc import Mapping cases = [ {'json': b'{"kind":"HostedClusterList"}', 'parsed': {"kind": "HostedClusterList"}}, {'json': b'{"kind":"HostedClusterList","items":null}', 'parsed': {"kind": "HostedClusterList", "items": None}}, {'json': b'{"kind":"HostedClusterList","items":[]}', 'parsed': {"kind": "HostedClusterList", "items": []}}, {'json': b'{"kind":"HostedClusterList","items":["x"]}', 'parsed': {"kind": "HostedClusterList", "items": ["x"]}}, {'json': b'{"kind":"WrongKind","items":[]}', 'parsed': {"kind": "WrongKind", "items": []}}, ] for case in cases: r = case['parsed'] missing = r.get("items") item_type = type(missing).__name__ empty_like = missing is None or (isinstance(missing, list) and len(missing) == 0) print(f"{case['json']} => missing={missing!r}, type={item_type}, present-and-array-and-empty={isinstance(missing, list) and len(missing) == 0}, gomega-like-empty={empty_like}") PYRepository: stolostron/hypershift-addon-operator
Length of output: 6581
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== locate file ==" git ls-files | rg '(^|/)hcp_proxy_test\.go$' || true echo "== file stats ==" wc -l test/e2e/hcp_proxy_test.go echo "== relevant sections ==" sed -n '130,175p' test/e2e/hcp_proxy_test.go sed -n '360,405p' test/e2e/hcp_proxy_test.go echo "== helper definitions ==" rg -n "eventuallyTimeout|eventuallyInterval|HostedClusterList|hostedclusters|Do\\(ctx\\)" test/e2e/hcp_proxy_test.go echo "== Go-like missing map lookup behavior probe ==" python3 - <<'PY' from collections.abc import Mapping cases = [ {'json': b'{"kind":"HostedClusterList"}', 'parsed': {"kind": "HostedClusterList"}}, {'json': b'{"kind":"HostedClusterList","items":null}', 'parsed': {"kind": "HostedClusterList", "items": None}}, {'json': b'{"kind":"HostedClusterList","items":[]}', 'parsed': {"kind": "HostedClusterList", "items": []}}, {'json': b'{"kind":"HostedClusterList","items":["x"]}', 'parsed': {"kind": "HostedClusterList", "items": ["x"]}}, {'json': b'{"kind":"WrongKind","items":[]}', 'parsed': {"kind": "WrongKind", "items": []}}, ] for case in cases: r = case['parsed'] missing = r.get("items") item_type = type(missing).__name__ emptiness_like_gomega_nil_empty = missing is None or (isinstance(missing, list) and len(missing) == 0) precise = isinstance(missing, list) and len(missing) == 0 print(f"{case['json']} => missing={missing!r}, type={item_type}; present-and-array-and-empty={precise}; gomega-like-empty={emptiness_like_gomega_nil_empty}") PYRepository: stolostron/hypershift-addon-operator
Length of output: 6572
Assert the empty
HostedClusterListpayload on both routes.A missing
itemsfield oritems: nullsatisfies the currentdoc["items"] == nil && len == 0check, but the response contract requires an empty array. The APIService check also accepts any HTTP 200 response. Both tests should decode the body and assertkind == "HostedClusterList"with a present, zero-lengthitemsarray.📍 Affects 1 file
test/e2e/hcp_proxy_test.go#L154-L157(this comment)test/e2e/hcp_proxy_test.go#L383-L389🤖 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 `@test/e2e/hcp_proxy_test.go` around lines 154 - 157, The HostedClusterList assertions must validate the full empty-list payload on both routes. Update the tests at test/e2e/hcp_proxy_test.go lines 154-157 and 383-389 to decode each response body, assert kind equals HostedClusterList, and verify items is present as an empty array rather than accepting missing or null items; also ensure the APIService route validates this payload instead of only HTTP 200.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kurwang, yiraeChristineKim 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
Summary
The HCP proxy's
handleRouteunconditionally required thehostingClusterquery 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 thehcp.ocm.io/v1alpha1API group. Additionally,oc get hostedclusters -Afailed with 400 because the cluster-wide list path was not handled.Changes
HostedClusterListfor namespaced and cluster-wide collection GET/DELETE requests whenhostingClusteris absent (semantically correct since the proxy stores no resources locally)hostingCluster— absent allows list/delete-collection, invalid always returns 400hostingClusterstill returns 400 since a create requires a spoke target?watch=truerequests with 405 since the proxy cannot maintain long-lived event streamslistanddeletecollectionto discovery verbs so the namespace controller knows these operations are supportedVerification
Tested on cluster
policy-grc-cp-autoclaims-xl8hp.dev08with imageACM-39570-v4:oc get hostedclusters -Aoc get hostedclusters -n clustersoc get hostedclusters(current ns)oc get hostedclusters -A -o yamloc get hostedclusters -A -o jsonoc delete hostedclusters --all -n clustersoc api-resources | grep hostedclusterhcp.ocm.io/v1alpha1hostingClusterHostedClusterListhostingClusterHostedClusterListoc get hc -A)HostedClusterListhostingClusterhostingClusterhostingCluster(../evil)?watch=true)list,deletecollectionIssue references
Made with Cursor
Summary by CodeRabbit