adding templates needed for hcp proxy to work - #3716
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (7)
WalkthroughChangesHCP proxy integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant APIService
participant ProxyService
participant ManagerDeployment
APIService->>ProxyService: Route hcp.ocm.io/v1alpha1 requests on port 443
ProxyService->>ManagerDeployment: Forward TCP traffic to port 9443
ManagerDeployment->>ManagerDeployment: Use mounted hcp-proxy TLS secret
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-deployment.yaml (2)
68-76: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce non-root execution.
runAsNonRootandrunAsUserremain commented out, so the container identity is determined by the image default. SetrunAsNonRoot: trueand use a compatible non-root UID.As per path instructions, Kubernetes/OpenShift workloads must set
securityContext: runAsNonRoot.🤖 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/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-deployment.yaml` around lines 68 - 76, Update the deployment’s securityContext to enforce non-root execution by enabling runAsNonRoot: true and setting runAsUser to a compatible non-root UID, rather than leaving both fields commented out. Preserve the existing conditional seccompProfile configuration.Source: Path instructions
127-130: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAdd CPU and memory limits.
This container defines requests only. Add both CPU and memory limits to prevent unbounded resource consumption.
As per path instructions, resource limits are required on every container.
🤖 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/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-deployment.yaml` around lines 127 - 130, Update the container resource specification under resources in the hypershift-addon-manager deployment to add both cpu and memory limits alongside the existing requests. Use appropriate bounded values consistent with the chart’s resource conventions, ensuring every container defines CPU and memory limits.Source: Path instructions
pkg/templates/rbac_gen.go (1)
1-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the required Apache 2.0 copyright header.
This Go file has no copyright/license header before the generated-code marker.
As per path instructions,
**/*.go: “Copyright header required: Apache 2.0 with "Red Hat, Inc." or "stolostron contributors".”🤖 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/templates/rbac_gen.go` around lines 1 - 3, Add the required Apache 2.0 copyright header naming “Red Hat, Inc.” or “stolostron contributors” before the generated-code marker in the generated Go file, while preserving the existing “Code generated by go generate; DO NOT EDIT.” notice.Source: 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/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-clusterrole.yaml`:
- Line 4: Define one shared DNS-safe, lowercase, hyphenated name template for
the HCP proxy RBAC objects, then reuse it for the ClusterRole name,
ClusterRoleBinding metadata.name, and roleRef.name. Update both
hypershift-addon-hcp-proxy-clusterrole.yaml:4 and
hypershift-addon-hcp-proxy-clusterrolebinding.yaml:4-8; preserve the existing
object relationships while eliminating colons and uppercase characters.
- Around line 21-23: The RBAC roles currently grant impersonation on bare
userextras instead of concrete subresources. Update the source RBAC annotation
and generated output in
pkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-clusterrole.yaml
(lines 21-23), config/rbac/role.yaml (lines 624-629), and
pkg/templates/rbac_gen.go (line 258) so every affected role lists each supported
userextras/<key> resource explicitly, including userextras/scopes, and
regenerate the generated files from the updated source.
In
`@pkg/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-deployment.yaml`:
- Around line 61-64: Update the hypershift-addon-manager deployment container
around the hcp-proxy-tls volume to require the TLS Secret instead of allowing an
absent mount. Add HTTPS liveness/readiness probes targeting the proxy’s actual
health endpoint on port 9443, and enforce non-root execution through the
container security context so API aggregation only receives ready TLS-backed
traffic.
---
Outside diff comments:
In
`@pkg/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-deployment.yaml`:
- Around line 68-76: Update the deployment’s securityContext to enforce non-root
execution by enabling runAsNonRoot: true and setting runAsUser to a compatible
non-root UID, rather than leaving both fields commented out. Preserve the
existing conditional seccompProfile configuration.
- Around line 127-130: Update the container resource specification under
resources in the hypershift-addon-manager deployment to add both cpu and memory
limits alongside the existing requests. Use appropriate bounded values
consistent with the chart’s resource conventions, ensuring every container
defines CPU and memory limits.
In `@pkg/templates/rbac_gen.go`:
- Around line 1-3: Add the required Apache 2.0 copyright header naming “Red Hat,
Inc.” or “stolostron contributors” before the generated-code marker in the
generated Go file, while preserving the existing “Code generated by go generate;
DO NOT EDIT.” notice.
🪄 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: 1c6ac871-155b-4508-82b8-12daa17d4785
📒 Files selected for processing (7)
config/rbac/role.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-apiservice.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-clusterrole.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-clusterrolebinding.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-service.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-deployment.yamlpkg/templates/rbac_gen.go
|
@kurwang @yiraeChristineKim There are some coderabbit comments that need to be addressed. |
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)
config/rbac/role.yaml (1)
13-16: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLimit wildcard access on Services and ServiceAccounts.
config/rbac/role.yamlgrants*verbs on coreservicesandserviceaccounts, which opens the operator to arbitrary service-routing and service-identity changes across the cluster. The RBAC generation source references narrower annotations for these resources; keep the generated/committed rules to the minimal verbs actually required and regenerate the RBAC manifests from//go:generate go run pkg/templates/rbac.go.Also applies to: 79-84
🤖 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 `@config/rbac/role.yaml` around lines 13 - 16, Update the RBAC generation source and committed rules for core services and serviceaccounts to use the narrow verbs required by their annotations instead of wildcard access. Regenerate config/rbac/role.yaml using the existing pkg/templates/rbac.go go:generate command, ensuring both affected resource rule blocks retain only the minimal required verbs.Source: 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/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-clusterrolebinding.yaml`:
- Around line 4-8: Quote the Helm template expressions used for the RBAC names
in both metadata.name and roleRef.name within the hypershift-addon-hcp-proxy
ClusterRoleBinding, preserving the existing colon-separated rendered values as
valid YAML strings.
---
Outside diff comments:
In `@config/rbac/role.yaml`:
- Around line 13-16: Update the RBAC generation source and committed rules for
core services and serviceaccounts to use the narrow verbs required by their
annotations instead of wildcard access. Regenerate config/rbac/role.yaml using
the existing pkg/templates/rbac.go go:generate command, ensuring both affected
resource rule blocks retain only the minimal required verbs.
🪄 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: 21eb1513-a687-48bd-b3fe-ed92a63d248c
📒 Files selected for processing (7)
config/rbac/role.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-apiservice.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-clusterrole.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-clusterrolebinding.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-service.yamlpkg/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-deployment.yamlpkg/templates/rbac_gen.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/templates/charts/toggle/hypershift/templates/hypershift-addon-hcp-proxy-apiservice.yaml
- pkg/templates/charts/toggle/hypershift/templates/hypershift-addon-manager-deployment.yaml
|
@dislbenn |
Signed-off-by: kurwang <kurwang@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dislbenn, 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 |
3 participants
Description
Add hub-side HCP proxy infrastructure resources to backplane-operator so the hypershift-addon-manager can serve the
hcp.ocm.io/v1alpha1aggregated API for creating/managing HostedClusters from the hub.Related Issue
ACM-37269
Companion PR: stolostron/hypershift-addon-operator#760
Changes Made
New Templates (
pkg/templates/charts/toggle/hypershift/templates/)hypershift-addon-hcp-proxy-service.yaml— Service (port 443 → targetPort 9443) withservice-caannotation for automatic TLS cert generationhypershift-addon-hcp-proxy-apiservice.yaml— Registersv1alpha1.hcp.ocm.iowith kube-apiserver aggregation, routing requests to the HCP proxy Servicehypershift-addon-hcp-proxy-clusterrole.yaml— Dedicated ClusterRole granting:clusterview.open-cluster-management.io/userpermissionsGET (probe auth API + check caller's admin bindings)authentication.k8s.io/userextrasimpersonationoperator.open-cluster-management.io/multiclusterhubsread accesscluster.open-cluster-management.io/managedclustersread access (spoke health checks)config.openshift.io/apiserversread access (TLS profile compliance)hypershift-addon-hcp-proxy-clusterrolebinding.yaml— Binds the above ClusterRole to thehypershift-addon-manager-saServiceAccountModified Templates
hypershift-addon-manager-deployment.yaml— Added:containerPort: 9443(namedhcp-proxy) for the proxy listener/etc/hcp-proxy/tls(readOnly) for the service-ca-generated TLS certhypershift-addon-hcp-proxy-tlsSecret (optional: true for non-OpenShift fallback)Generated Files
pkg/templates/rbac_gen.go— Regenerated viago generateto include HCP proxy RBAC markersconfig/rbac/role.yaml— Regenerated viamake manifests(controller-gen v0.19.0) so the backplane-operator itself has permission to manage the new resourcesScreenshots (if applicable)
N/A — infrastructure/RBAC changes only.
Checklist
Additional Notes
pkg/manager/hcp_proxy.go), merged in PR chore(deps): update konflux references to 2be7c9c (backplane-2.4) #760. This PR provides the infrastructure (Service, APIService, RBAC, Deployment wiring) that backplane-operator is responsible for provisioning.ocm-proxyserverandclusterview.controller-genwas upgraded from stale v0.15.0 → v0.19.0 (matching whatCONTROLLER_TOOLS_VERSIONalready specified in the Makefile) to fix incompatibility with k8s v0.35.x vendored dependencies.Reviewers
/cc @yiraeChristineKim
Definition of Done
Summary by CodeRabbit
hcp.ocm.io.9443.