feat: add HCP proxy CreateRequest to create HostedClusters from the hub - #760
Merged
openshift-merge-bot[bot] merged 2 commits intoJul 22, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a hub-side HCP proxy API for HostedCluster and NodePool management, central TLS-profile handling, local Kind E2E automation, deployment manifests, developer tooling, documentation, and supporting dependency updates. ChangesHCP proxy
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Hub API
participant HCP Proxy
participant Cluster Proxy
participant Spoke API
Client->>Hub API: Call hcp.ocm.io/v1alpha1
Hub API->>HCP Proxy: Route APIService request
HCP Proxy->>Cluster Proxy: Forward validated impersonated request
Cluster Proxy->>Spoke API: Access HostedCluster or NodePool
Spoke API-->>HCP Proxy: Return resource result
HCP Proxy-->>Client: Return API response
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (6 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 |
yiraeChristineKim
force-pushed
the
ACM-37268
branch
from
July 20, 2026 17:06
fa39073 to
cd8bb7a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (2)
.work/jira/solve/spec-OCPBUGS-89352.md (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the shell script directly.
make fmtandmake vetdo not validate the Bash syntax or confirm that the obsolete binary path was removed. Addbash -n, ShellCheck if used by the repository, and a targeted search or canary execution to the verification steps.🤖 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 @.work/jira/solve/spec-OCPBUGS-89352.md around lines 37 - 40, Update the Verification section to validate the shell script directly: add bash -n, run the repository’s ShellCheck command if applicable, and include a targeted search or canary execution confirming the obsolete binary path is absent.README.md (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify a language for fenced code blocks.
Several fenced code blocks are missing a language identifier, which triggers
markdownlint-cli2(MD040) warnings.
README.md#L68-L68: appendtextorbashto the opening fence (e.g., ````text`).docs/management/from-hub-cli.md#L11-L11: appendtextto the opening fence.docs/management/from-hub-cli.md#L53-L53: appendtextto the opening fence.🤖 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 `@README.md` at line 68, Specify a language on each affected fenced code block: append text or bash to the opening fence in README.md at lines 68-68, and append text to the opening fences in docs/management/from-hub-cli.md at lines 11-11 and 53-53. No other content changes are needed.Source: Linters/SAST tools
🤖 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 @.cursor/rules/backplane-operator-sync.mdc:
- Line 17: Replace the developer-specific absolute checkout path in the rule
file with a repository-relative or clearly generic placeholder reference to the
backplane-operator repository, removing the embedded username and local
filesystem location.
In @.cursor/rules/tls-compliance.mdc:
- Around line 40-43: Update the NewTLSConfigFromProfile example to bind its
second return value as unsupported rather than err, then handle the
len(unsupported) > 0 case with the appropriate failure behavior. Remove the
invalid error-based check so the documented signature matches the actual API and
compiles.
In @.github/workflows/e2e-hcp-proxy.yaml:
- Around line 24-28: Add a job-level permissions block to e2e-hcp-proxy with all
GITHUB_TOKEN permissions denied by default and only contents read access
enabled, since the HCP Proxy E2E job only needs to read repository contents.
- Around line 30-36: Update the workflow steps using actions/checkout and
actions/setup-go to reference immutable full commit SHAs instead of version
tags, preserving their current action versions. Add persist-credentials: false
to the checkout step while leaving the existing setup-go configuration
unchanged.
- Around line 42-44: Remove the KUBECONFIG override from the e2e step, or set it
using the runner’s $HOME shell variable within the run command rather than
env.HOME. Preserve the existing make test-e2e-hcp-proxy invocation and
KIND_CLUSTER_NAME argument.
In `@go.mod`:
- Line 35: Update the k8s.io/kube-aggregator dependency in go.mod from v0.34.2
to v0.35.2, matching the versions used by the other k8s.io modules. Then rerun
make vendor to refresh vendored dependencies.
In `@Makefile`:
- Line 249: Update the namespace creation command to use $(KUBECTL) for the
piped apply invocation as well as the create invocation, preserving support for
KUBECTL overrides consistently with the surrounding target.
- Around line 215-218: Update the e2e-hcp-proxy-full target so kind-delete
always runs after test-e2e-hcp-proxy, including when the test fails. Capture the
test command’s exit status, perform cleanup unconditionally, then return the
original test status.
In `@pkg/manager/hcp_proxy_test.go`:
- Line 1: Prepend the repository-standard Apache 2.0 copyright header,
attributing “Red Hat, Inc.” or “stolostron contributors,” to
pkg/manager/hcp_proxy_test.go at lines 1-1 and test/e2e/hcp_proxy_test.go at
lines 1-1, before each package declaration.
In `@pkg/manager/hcp_proxy.go`:
- Around line 839-843: Update the NodePool creation loop around createOnSpoke so
failed creations are not reported as created: on error, skip appending that
NodePool to createdNodePools, or accumulate the failure and return an
appropriate partial-failure response instead of the current success response.
Preserve logging and successful NodePool reporting.
- Around line 604-619: Update the probe error handling in the hub
permission-check flow around hubDynClient and
hubDynClient.Resource(gvr).Resource errors: retain the non-fatal return only for
the specific unregistered-API NotFound case, and propagate every other probe
error instead of logging and returning nil, so authorization fails closed.
- Line 1: Add the required Apache 2.0 copyright header at the top of the new
hcp_proxy.go file, before the package declaration, including either “Red Hat,
Inc.” or “stolostron contributors” as required by the repository path
instructions.
- Around line 878-884: The NodePool deletion loop in the surrounding manager
flow leaks HTTP response bodies and ignores request-construction errors. Update
the code around hcpClient.Do(req) to handle http.NewRequestWithContext errors
before sending, and close every successful response body after the delete
request; preserve the existing iteration and deletion behavior.
- Around line 152-169: Update the TLS configuration in the HCP proxy server
setup to require and verify client certificates before accepting X-Remote-*
identity headers. Wire tlsCfg.ClientAuth and tlsCfg.ClientCAs to the
requestheader-client-ca bundle using the existing certificate-loading
configuration, while preserving the current server certificate and handler
setup.
In `@test/e2e/addon-manager-deployment.yaml`:
- Around line 76-81: Update the container definition’s securityContext to
include runAsNonRoot: true, add CPU and memory resource limits, and configure
liveness and readiness probes for the existing proxy endpoints on port 8443:
/healthz and /readyz respectively. Keep the existing privilege-escalation and
read-only filesystem settings.
In `@test/e2e/hcp_proxy_test.go`:
- Around line 299-317: Replace the tautological matcher in the Eventually call
within the hostedclusters proxy test with a callback that performs the REST
request and asserts statusCode is not http.StatusOK, returning the assertion
result for retry evaluation. Remove the separate post-Eventually status
assertion so polling continues until the proxy responds with a non-200 status.
- Around line 209-213: Update the discovery response assertions in the
handleDiscovery test to expect two resources, then verify both entries include
hostedclusters and hostedclusters/resources. Preserve the existing
hostedclusters assertion and add an assertion for the published subresource.
---
Nitpick comments:
In @.work/jira/solve/spec-OCPBUGS-89352.md:
- Around line 37-40: Update the Verification section to validate the shell
script directly: add bash -n, run the repository’s ShellCheck command if
applicable, and include a targeted search or canary execution confirming the
obsolete binary path is absent.
In `@README.md`:
- Line 68: Specify a language on each affected fenced code block: append text or
bash to the opening fence in README.md at lines 68-68, and append text to the
opening fences in docs/management/from-hub-cli.md at lines 11-11 and 53-53. No
other content changes are needed.
🪄 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: Enterprise
Run ID: f1a66eb0-a500-4764-a6c1-eeb61f283581
📥 Commits
Reviewing files that changed from the base of the PR and between 1895b4e and fa3907350df1807b3d19eb083177da2740afdc6f.
⛔ Files ignored due to path filters (1)
.cursor/debug-8d39ff.logis excluded by!**/*.log
📒 Files selected for processing (20)
.cursor/rules/backplane-operator-sync.mdc.cursor/rules/tls-compliance.mdc.github/workflows/e2e-hcp-proxy.yaml.vscode/launch.json.work/jira/solve/spec-OCPBUGS-89352.mdCLAUDE.mdMakefileREADME.mddocs/README.mddocs/management/from-hub-cli.mdgo.modpkg/manager/discovery_config_controller.gopkg/manager/hcp_proxy.gopkg/manager/hcp_proxy_test.gopkg/manager/manager.gopkg/manager/manifests/hcp-proxy/apiservice.yamlpkg/manager/manifests/hcp-proxy/rbac.yamlpkg/manager/manifests/hcp-proxy/service.yamltest/e2e/addon-manager-deployment.yamltest/e2e/hcp_proxy_test.go
yiraeChristineKim
force-pushed
the
ACM-37268
branch
2 times, most recently
from
July 20, 2026 17:56
03546d7 to
3f623f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/manager/hcp_proxy.go (1)
146-154: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
X-Remote-*identity is still trusted without client-cert verification.The server enables only the serving cert;
ClientAuth/ClientCAsare not wired to therequestheader-client-cabundle. Because the Service fronts this backend on443 → 9443, any in-cluster workload that can reach the ClusterIP can setX-Remote-User/X-Remote-Group(consumed bywhoIsTheCallerat Line 500) and be impersonated for both the hub permission check and spoke writes. The added comment documents the risk but does not close it — the aggregation guarantee only holds for traffic that actually transits kube-apiserver, not for direct in-cluster connections.Gate
X-Remote-*on verified client certs against the requestheader CA (or otherwise ensure the port is only reachable via the aggregator).🤖 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 146 - 154, Update the TLS configuration in the hcp proxy setup around tlsConfigFn and tlsCfg to require verified client certificates using the requestheader-client-ca bundle, wiring ClientAuth and ClientCAs appropriately before serving requests. Ensure whoIsTheCaller only trusts X-Remote-* identity headers after this verification, or otherwise restrict direct backend access so the service cannot be reached without aggregator authentication.
🧹 Nitpick comments (3)
pkg/manager/hcp_proxy_test.go (1)
1494-1503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixed
time.Sleep(200ms)can flake under CI load.
StartHCPProxybinds asynchronously and the test relies on a hard-coded sleep before cancelling. On a slow runner the server may not be listening yet, making the graceful-shutdown assertion nondeterministic. Prefer polling readiness (e.g. dial the bound address until it accepts) before cancel, or expose the listener address so the test can wait deterministically.🤖 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 1494 - 1503, Replace the fixed time.Sleep in the StartHCPProxy shutdown test with deterministic readiness polling: wait until the TLS server has successfully bound and accepts a connection before calling cancel. Use the server’s bound address or an exposed listener readiness signal, retain the existing timeout failure behavior, and then perform the current errCh graceful-shutdown assertion.Makefile (1)
156-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHarden
ensure-kinddownload to matchensure-clusteradm.Two robustness gaps versus
ensure-clusteradm: (1)$(GOBIN)is not created, socurl -sSLo "$(GOBIN)/kind"fails if the directory doesn't exist; (2)curllacks-f, so an HTTP error response is written to thekindbinary and thenchmod +x'd, causing confusing downstream failures.🔧 Proposed fix
`@if` ! command -v kind >/dev/null 2>&1 && [ ! -f "$(GOBIN)/kind" ]; then \ OS=$$(uname -s | tr '[:upper:]' '[:lower:]'); \ ARCH=$$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/'); \ echo "Installing kind $(KIND_VERSION) for $$OS/$$ARCH into $(GOBIN)..."; \ - curl -sSLo "$(GOBIN)/kind" \ + mkdir -p "$(GOBIN)"; \ + curl -fsSLo "$(GOBIN)/kind" \ "https://kind.sigs.k8s.io/dl/$(KIND_VERSION)/kind-$$OS-$$ARCH"; \ chmod +x "$(GOBIN)/kind"; \ fi🤖 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 `@Makefile` around lines 156 - 166, Harden the ensure-kind target by creating $(GOBIN) before downloading and adding curl’s fail-on-HTTP-error option to the kind download command. Keep the existing OS/architecture detection, installation, chmod, and version verification flow unchanged.test/e2e/hcp_proxy_test.go (1)
284-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
cfg.
cfgfromutil.NewKubeConfig()is never used — the discovery poll goes throughkubeClient.Discovery(), and the only reference is the_ = cfgno-op inside the closure. Drop the fetch and the discard.♻️ Proposed cleanup
ginkgo.By("Waiting for hcp.ocm.io to appear in server API groups") - cfg, err := util.NewKubeConfig() - gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Eventually(func() bool { groups, _, err := kubeClient.Discovery().ServerGroupsAndResources() if err != nil { return false } for _, g := range groups { if g.Name == hcpProxyAPIGroup { return true } } - _ = cfg return false🤖 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 284 - 297, Remove the unused cfg initialization and discard from the test setup around kubeClient.Discovery().ServerGroupsAndResources(); delete the util.NewKubeConfig() call and the _ = cfg statement, preserving the existing discovery polling behavior.
🤖 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 `@docs/management/from-hub-cli.md`:
- Around line 149-151: Update the fenced CLI example containing `hcp from-hub
create <platform> [flags]` to specify the Bash language identifier, using a bash
fence while preserving the command unchanged.
In `@pkg/manager/hcp_proxy.go`:
- Around line 1009-1049: Handle every http.NewRequestWithContext error in the
affected handler, including the namespace, HostedCluster, and NodePool request
construction near the existing request flow. Check each construction result
before calling hcpClient.Do, return an appropriate client error response, and
wrap the underlying error with fmt.Errorf using contextual messages rather than
discarding it.
---
Duplicate comments:
In `@pkg/manager/hcp_proxy.go`:
- Around line 146-154: Update the TLS configuration in the hcp proxy setup
around tlsConfigFn and tlsCfg to require verified client certificates using the
requestheader-client-ca bundle, wiring ClientAuth and ClientCAs appropriately
before serving requests. Ensure whoIsTheCaller only trusts X-Remote-* identity
headers after this verification, or otherwise restrict direct backend access so
the service cannot be reached without aggregator authentication.
---
Nitpick comments:
In `@Makefile`:
- Around line 156-166: Harden the ensure-kind target by creating $(GOBIN) before
downloading and adding curl’s fail-on-HTTP-error option to the kind download
command. Keep the existing OS/architecture detection, installation, chmod, and
version verification flow unchanged.
In `@pkg/manager/hcp_proxy_test.go`:
- Around line 1494-1503: Replace the fixed time.Sleep in the StartHCPProxy
shutdown test with deterministic readiness polling: wait until the TLS server
has successfully bound and accepts a connection before calling cancel. Use the
server’s bound address or an exposed listener readiness signal, retain the
existing timeout failure behavior, and then perform the current errCh
graceful-shutdown assertion.
In `@test/e2e/hcp_proxy_test.go`:
- Around line 284-297: Remove the unused cfg initialization and discard from the
test setup around kubeClient.Discovery().ServerGroupsAndResources(); delete the
util.NewKubeConfig() call and the _ = cfg statement, preserving the existing
discovery polling behavior.
🪄 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: Enterprise
Run ID: a5dbdf4e-3d3f-4d5f-ae22-651742874f29
📥 Commits
Reviewing files that changed from the base of the PR and between fa3907350df1807b3d19eb083177da2740afdc6f and 26174dc801ceb8787b7d1d388ea8fed8740b147a.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.cursor/rules/backplane-operator-sync.mdc.cursor/rules/tls-compliance.mdc.github/workflows/e2e-hcp-proxy.yaml.vscode/launch.jsonCLAUDE.mdDockerfileMakefileREADME.mddocs/README.mddocs/management/from-hub-cli.mdgo.modpkg/agent/agent.gopkg/manager/discovery_config_controller.gopkg/manager/hcp_proxy.gopkg/manager/hcp_proxy_test.gopkg/manager/manager.gotest/e2e/addon-manager-deployment.yamltest/e2e/hcp_proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- .vscode/launch.json
- .cursor/rules/backplane-operator-sync.mdc
- docs/README.md
- .github/workflows/e2e-hcp-proxy.yaml
- CLAUDE.md
- README.md
- pkg/manager/discovery_config_controller.go
- pkg/manager/manager.go
Comment on lines
+149
to
+151
| ``` | ||
| hcp from-hub create <platform> [flags] | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced block language.
The code fence at Line 149 lacks a language identifier, triggering Markdownlint MD040. Use ```bash for this CLI example.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 149-149: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/management/from-hub-cli.md` around lines 149 - 151, Update the fenced
CLI example containing `hcp from-hub create <platform> [flags]` to specify the
Bash language identifier, using a bash fence while preserving the command
unchanged.
Source: Linters/SAST tools
yiraeChristineKim
force-pushed
the
ACM-37268
branch
2 times, most recently
from
July 20, 2026 19:08
a38e0f8 to
f403dc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/manager/manager.go (1)
263-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fetchTLSProfileOrDefaultnever returns a non-nil error.The
errorreturn is dead — the fetch failure is already handled via the Intermediate fallback. Consider dropping the return value (and the caller'sif err != nilat Lines 156-159) so the signature reflects that this cannot fail.🤖 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/manager.go` around lines 263 - 274, Remove the unused error return from fetchTLSProfileOrDefault, since fetch failures already fall back to the Intermediate TLS profile. Update its callers, including the handling around the caller at Lines 156-159, to use the returned TLSProfileSpec directly and eliminate the dead if err != nil branch while preserving the existing fallback behavior.
🤖 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 `@docs/management/hcp-proxy-local-dev.md`:
- Around line 126-136: Update the “Identity header reference” documentation to
state that the hub permission gate is enforced on ACM/MCE hubs but skipped when
the clusterview API is unavailable, including kind/non-ACM local development.
Preserve the existing permission-check description for ACM/MCE environments.
In `@hack/install_ocm.sh`:
- Around line 34-40: Suppress shell tracing around the credential-parsing and
error-reporting logic in install_ocm.sh: disable xtrace before the hub_token
assignment and related join.sh parsing, validation, and cat join.sh output, then
restore the prior tracing state afterward. Ensure neither the hub token nor
join.sh contents appear in CI logs while preserving the existing validation and
exit behavior.
In `@pkg/manager/manager.go`:
- Around line 180-187: Update the synchronous EnableHypershiftCLIDownload call
in the manager startup flow to pass managerCtx instead of the parent ctx, so its
retry loop observes cancelManager() and stops during a graceful restart.
Preserve the existing error logging and subsequent managerCtx wait behavior.
---
Nitpick comments:
In `@pkg/manager/manager.go`:
- Around line 263-274: Remove the unused error return from
fetchTLSProfileOrDefault, since fetch failures already fall back to the
Intermediate TLS profile. Update its callers, including the handling around the
caller at Lines 156-159, to use the returned TLSProfileSpec directly and
eliminate the dead if err != nil branch while preserving the existing fallback
behavior.
🪄 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: Enterprise
Run ID: 2f3ca871-f0c2-486f-a229-84e15c95e665
📥 Commits
Reviewing files that changed from the base of the PR and between 26174dc801ceb8787b7d1d388ea8fed8740b147a and 24cff63601385f9e01d7ab94902e22e0018df1d9.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
.cursor/rules/backplane-operator-sync.mdc.cursor/rules/tls-compliance.mdc.github/workflows/e2e-hcp-proxy.yaml.vscode/launch.jsonCLAUDE.mdDockerfileMakefileREADME.mddocs/README.mddocs/management/from-hub-cli.mddocs/management/hcp-proxy-local-dev.mdgo.modhack/install_ocm.shpkg/agent/agent.gopkg/manager/hcp_proxy.gopkg/manager/hcp_proxy_test.gopkg/manager/manager.goquickstart/README.mdtest/e2e/addon-manager-deployment.yamltest/e2e/hcp_proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- Dockerfile
- .vscode/launch.json
- pkg/agent/agent.go
- .cursor/rules/backplane-operator-sync.mdc
- go.mod
- .github/workflows/e2e-hcp-proxy.yaml
- .cursor/rules/tls-compliance.mdc
- CLAUDE.md
- pkg/manager/hcp_proxy_test.go
Comment on lines
+126
to
+136
| ## 4. Identity header reference | ||
|
|
||
| | Header | Example value | Notes | | ||
| |--------|--------------|-------| | ||
| | `X-Remote-User` | `kube:admin` | Must match a user with `managedcluster:admin` binding for the `hostingCluster` | | ||
| | `X-Remote-Group` | `system:cluster-admins` | One or more groups; used for spoke impersonation | | ||
|
|
||
| The proxy enforces two permission gates: | ||
|
|
||
| 1. **Hub gate** — `GET clusterview.open-cluster-management.io/v1alpha1/userpermissions/managedcluster:admin` | ||
| under the caller's identity; request is denied if the target spoke is not in the bindings. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the kind/non-ACM permission-check exception.
The proxy skips the hub permission check when the clusterview API is unavailable, but this section says the caller must always have a managedcluster:admin binding and that the hub gate always denies otherwise. Clarify that this gate is enforced on ACM/MCE hubs and skipped for kind/non-ACM local development.
Suggested wording
-The proxy enforces two permission gates:
+On ACM/MCE hubs, the proxy enforces two permission gates. On kind or other
+non-ACM hubs where the clusterview API is unavailable, the hub gate is skipped:📝 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.
Suggested change
| ## 4. Identity header reference | |
| | Header | Example value | Notes | | |
| |--------|--------------|-------| | |
| | `X-Remote-User` | `kube:admin` | Must match a user with `managedcluster:admin` binding for the `hostingCluster` | | |
| | `X-Remote-Group` | `system:cluster-admins` | One or more groups; used for spoke impersonation | | |
| The proxy enforces two permission gates: | |
| 1. **Hub gate** — `GET clusterview.open-cluster-management.io/v1alpha1/userpermissions/managedcluster:admin` | |
| under the caller's identity; request is denied if the target spoke is not in the bindings. | |
| ## 4. Identity header reference | |
| | Header | Example value | Notes | | |
| |--------|--------------|-------| | |
| | `X-Remote-User` | `kube:admin` | Must match a user with `managedcluster:admin` binding for the `hostingCluster` | | |
| | `X-Remote-Group` | `system:cluster-admins` | One or more groups; used for spoke impersonation | | |
| On ACM/MCE hubs, the proxy enforces two permission gates. On kind or other | |
| non-ACM hubs where the clusterview API is unavailable, the hub gate is skipped: | |
| 1. **Hub gate** — `GET clusterview.open-cluster-management.io/v1alpha1/userpermissions/managedcluster:admin` | |
| under the caller's identity; request is denied if the target spoke is not in the bindings. |
🤖 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 `@docs/management/hcp-proxy-local-dev.md` around lines 126 - 136, Update the
“Identity header reference” documentation to state that the hub permission gate
is enforced on ACM/MCE hubs but skipped when the clusterview API is unavailable,
including kind/non-ACM local development. Preserve the existing permission-check
description for ACM/MCE environments.
Comment on lines
+34
to
+40
| hub_apiserver=$(grep -oE -- '--hub-apiserver[[:space:]]+[^[:space:]]+' join.sh | awk '{print $2}' | head -1) | ||
| if [[ -z "${hub_token}" || -z "${hub_apiserver}" ]]; then | ||
| echo "ERROR: failed to parse hub token/apiserver from join.sh" >&2 | ||
| cat join.sh >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Hub join token is leaked into CI logs by set -xv.
With xtrace enabled globally (Line 9), the hub_token=$(...) assignment and cat join.sh echo the bearer token to the job output. Suppress tracing around credential handling.
Proposed fix
+{ set +x; } 2>/dev/null
hub_token=$(grep -oE -- '--hub-token[[:space:]]+[^[:space:]]+' join.sh | awk '{print $2}' | head -1)
hub_apiserver=$(grep -oE -- '--hub-apiserver[[:space:]]+[^[:space:]]+' join.sh | awk '{print $2}' | head -1)
if [[ -z "${hub_token}" || -z "${hub_apiserver}" ]]; then
echo "ERROR: failed to parse hub token/apiserver from join.sh" >&2
- cat join.sh >&2
exit 1
fi
+set -x📝 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.
Suggested change
| hub_token=$(grep -oE -- '--hub-token[[:space:]]+[^[:space:]]+' join.sh | awk '{print $2}' | head -1) | |
| hub_apiserver=$(grep -oE -- '--hub-apiserver[[:space:]]+[^[:space:]]+' join.sh | awk '{print $2}' | head -1) | |
| if [[ -z "${hub_token}" || -z "${hub_apiserver}" ]]; then | |
| echo "ERROR: failed to parse hub token/apiserver from join.sh" >&2 | |
| cat join.sh >&2 | |
| exit 1 | |
| fi | |
| { set +x; } 2>/dev/null | |
| hub_token=$(grep -oE -- '--hub-token[[:space:]]+[^[:space:]]+' join.sh | awk '{print $2}' | head -1) | |
| hub_apiserver=$(grep -oE -- '--hub-apiserver[[:space:]]+[^[:space:]]+' join.sh | awk '{print $2}' | head -1) | |
| if [[ -z "${hub_token}" || -z "${hub_apiserver}" ]]; then | |
| echo "ERROR: failed to parse hub token/apiserver from join.sh" >&2 | |
| exit 1 | |
| fi | |
| set -x |
🤖 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 `@hack/install_ocm.sh` around lines 34 - 40, Suppress shell tracing around the
credential-parsing and error-reporting logic in install_ocm.sh: disable xtrace
before the hub_token assignment and related join.sh parsing, validation, and cat
join.sh output, then restore the prior tracing state afterward. Ensure neither
the hub token nor join.sh contents appear in CI logs while preserving the
existing validation and exit behavior.
yiraeChristineKim
force-pushed
the
ACM-37268
branch
2 times, most recently
from
July 21, 2026 04:03
9cd28dc to
cefed1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CLAUDE.md`:
- Around line 81-84: Update the documented TLS profile fallback around
FetchAPIServerTLSProfile and GetTLSProfileSpec so it never discards the fallback
error. Handle the returned error explicitly, wrapping it with contextual
information and returning or propagating it, or reference a helper that
validates and returns the fallback profile.
In `@test/e2e/hcp_proxy_test.go`:
- Around line 94-153: Add descriptive failure messages to the Gomega Expect
assertions in the affected It blocks, covering request errors, response status
codes, response body content, JSON decoding, document kinds and names, resource
counts and names, and the missing-hostingCluster status. Keep each message
concise and explain the expected behavior and its relevance, matching the
message style used near the later assertions.
🪄 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: Enterprise
Run ID: 54f1d763-e282-4c90-96b3-d7cfa773fd8c
📥 Commits
Reviewing files that changed from the base of the PR and between bd6f2bad3c34eccc543caaf3d19fa8f4f8f11cb4 and 84e55167644474b64a5d4116cc5804d20301f4c3.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
.cursor/rules/backplane-operator-sync.mdc.cursor/rules/tls-compliance.mdc.dockerignore.github/workflows/e2e-hcp-proxy.yaml.vscode/launch.jsonCLAUDE.mdDockerfile.e2eMakefileREADME.mddocs/README.mddocs/management/from-hub-cli.mddocs/management/hcp-proxy-local-dev.mdgo.modhack/install_cluster_proxy.shhack/install_ocm.shpkg/agent/agent.gopkg/manager/hcp_proxy.gopkg/manager/hcp_proxy_test.gopkg/manager/manager.goquickstart/README.mdtest/e2e/addon-manager-deployment.yamltest/e2e/e2e_suite_test.gotest/e2e/hcp_proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- .dockerignore
- .vscode/launch.json
- Dockerfile.e2e
- quickstart/README.md
- README.md
- .github/workflows/e2e-hcp-proxy.yaml
- docs/README.md
- test/e2e/e2e_suite_test.go
- .cursor/rules/tls-compliance.mdc
- go.mod
- pkg/agent/agent.go
- hack/install_ocm.sh
- docs/management/hcp-proxy-local-dev.md
- pkg/manager/manager.go
- .cursor/rules/backplane-operator-sync.mdc
- pkg/manager/hcp_proxy_test.go
Comment on lines
+81
to
+84
| profileSpec, err := tlspkg.FetchAPIServerTLSProfile(ctx, hubClient) | ||
| if err != nil { // fallback for kind/non-OpenShift clusters | ||
| profileSpec, _ = tlspkg.GetTLSProfileSpec(nil) // Intermediate (TLS 1.2+) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not document an ignored fallback error.
The example discards GetTLSProfileSpec’s error with _, which conflicts with the repository rule to never ignore Go error returns. Show explicit handling or delegate to a helper that validates and returns the fallback profile.
As per path instructions, Go code must never ignore error returns and should wrap errors with context.
Proposed documentation fix
if err != nil { // fallback for kind/non-OpenShift clusters
- profileSpec, _ = tlspkg.GetTLSProfileSpec(nil) // Intermediate (TLS 1.2+)
+ fallback, fallbackErr := tlspkg.GetTLSProfileSpec(nil)
+ if fallbackErr != nil {
+ return fmt.Errorf("get Intermediate TLS profile: %w", fallbackErr)
+ }
+ profileSpec = fallback
}📝 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.
Suggested change
| profileSpec, err := tlspkg.FetchAPIServerTLSProfile(ctx, hubClient) | |
| if err != nil { // fallback for kind/non-OpenShift clusters | |
| profileSpec, _ = tlspkg.GetTLSProfileSpec(nil) // Intermediate (TLS 1.2+) | |
| } | |
| profileSpec, err := tlspkg.FetchAPIServerTLSProfile(ctx, hubClient) | |
| if err != nil { // fallback for kind/non-OpenShift clusters | |
| fallback, fallbackErr := tlspkg.GetTLSProfileSpec(nil) | |
| if fallbackErr != nil { | |
| return fmt.Errorf("get Intermediate TLS profile: %w", fallbackErr) | |
| } | |
| profileSpec = fallback | |
| } |
🤖 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 `@CLAUDE.md` around lines 81 - 84, Update the documented TLS profile fallback
around FetchAPIServerTLSProfile and GetTLSProfileSpec so it never discards the
fallback error. Handle the returned error explicitly, wrapping it with
contextual information and returning or propagating it, or reference a helper
that validates and returns the fallback profile.
Source: Path instructions
kurwang
reviewed
Jul 21, 2026
kurwang
reviewed
Jul 21, 2026
kurwang
reviewed
Jul 21, 2026
kurwang
reviewed
Jul 21, 2026
kurwang
reviewed
Jul 21, 2026
kurwang
left a comment
kurwang
left a comment
Contributor
There was a problem hiding this comment.
added some comments this is an awesome pr
yiraeChristineKim
force-pushed
the
ACM-37268
branch
from
July 22, 2026 15:14
eeb0f39 to
860f96a
Compare
Implement the hub-side HCP proxy so platform engineers can create a HostedCluster (plus NodePools and Secrets) on a managed hosting cluster using hub credentials, without a per-cluster kubeconfig (ACM-37268). Creation order: Namespace → Secrets → HostedCluster → NodePool(s) via cluster-proxy, with managedcluster:admin auth, ManagedCluster health checks, duplicate-name handling, DNS-1123 path sanitization, and hcp.ocm.io/created-via labels. Includes unit/e2e coverage and TLS profile compliance for the proxy server. Signed-off-by: yiraeChristineKim <yikim@redhat.com> Co-Authored-By: Cursor <cursoragent@cursor.com>
yiraeChristineKim
force-pushed
the
ACM-37268
branch
from
July 22, 2026 15:42
860f96a to
f193a1f
Compare
Avoid a race where kubectl wait fails with NotFound when placement has not yet created the ManagedClusterAddOn after OCM join. Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
[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 |
13 tasks
This was referenced Jul 27, 2026
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CreateRequestso platform engineers can create a HostedCluster (plus NodePools and Secrets) on a managed hosting cluster using hub credentials, without a per-cluster kubeconfig (ACM-37268).managedcluster:adminauth, ManagedCluster health checks, duplicate-name (409) handling, andhcp.ocm.io/created-via=hcp-from-hublabels.Test plan
handleCreate(auth failure, ManagedCluster unhealthy, duplicate create) pass403when caller lacksmanagedcluster:adminon the target hosting clusterhcp.ocm.io/created-viaandhcp.ocm.io/hostedclusterlabelsMade with Cursor
Summary by CodeRabbit
New Features
Documentation
hcp from-huband developing/testing the HCP Proxy locally.Bug Fixes