WIP: Bootstrap hypershift release controller#81750
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
[REHEARSALNOTIFIER] Note: If this PR includes changes to step registry files ( |
WalkthroughAdds HyperShift release configuration, annotation support, generated controller deployments and services, external routing, and namespace-scoped RBAC for release-controller and release-payload-controller. ChangesHyperShift release controller
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as release-controller-hypershift Route
participant Service as release-controller-api-hypershift Service
participant API as release-controller-api-hypershift Deployment
Client->>Route: HTTPS request
Route->>Service: Edge-terminated request
Service->>API: Port 80 to targetPort 8080
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hack/generators/release-controllers/content/hypershift_admin_resources.py (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
namespace: ciinstead of using config value.
release_payload_controller_resources.py's_namespaced_rbac_resourcesusesconfig.rc_deployment_namespacefor the equivalent subject namespace. Hardcoding'ci'here risks drifting if that config value ever changes.♻️ Suggested consistency fix
+def generate_hypershift_admin_resources(gendoc): """Generate RBAC for HyperShift release controller in existing hypershift namespace""" + config = gendoc.context.config # Create RBAC for release controller to manage imagestreams and releasepayloads ... 'subjects': [{ 'kind': 'ServiceAccount', 'name': 'release-controller', - 'namespace': 'ci' + 'namespace': config.rc_deployment_namespace }]🤖 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/generators/release-controllers/content/hypershift_admin_resources.py` around lines 46 - 50, Replace the hardcoded 'ci' subject namespace in the hypershift admin resource definitions with the configured namespace value, matching _namespaced_rbac_resources in release_payload_controller_resources.py by using config.rc_deployment_namespace.
🤖 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 `@hack/generators/release-controllers/content/hypershift_resources.py`:
- Around line 90-186: Add securityContext settings to every container in the pod
templates, including the init container and both regular containers under the
git-sync and release-controller definitions. Set runAsNonRoot: true,
allowPrivilegeEscalation: false, and capabilities.drop: ["ALL"] at container
level, and apply the same hardening to the corresponding deployment variant
referenced by the repeated section.
- Around line 116-146: Add resource specifications to every container in the
generated Kubernetes manifest: the git-sync sidecar’s resources must include
both requests and limits, and the git-sync-init and both controller containers
must define complete requests and limits. Update the container definitions
associated with the git-sync resources and the controller entries to use
appropriate CPU and memory values consistently.
---
Nitpick comments:
In `@hack/generators/release-controllers/content/hypershift_admin_resources.py`:
- Around line 46-50: Replace the hardcoded 'ci' subject namespace in the
hypershift admin resource definitions with the configured namespace value,
matching _namespaced_rbac_resources in release_payload_controller_resources.py
by using config.rc_deployment_namespace.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bd467bd7-2cfa-4bb8-b684-22a163f82d54
📒 Files selected for processing (10)
clusters/app.ci/release-controller/admin_deploy-hypershift-controller.yamlclusters/app.ci/release-controller/deploy-hypershift-controller.yamlclusters/app.ci/release-payload-controller/admin_rbac.yamlcore-services/release-controller/_releases/release-hypershift.jsonhack/annotate.shhack/generators/release-controllers/content/__init__.pyhack/generators/release-controllers/content/hypershift_admin_resources.pyhack/generators/release-controllers/content/hypershift_resources.pyhack/generators/release-controllers/content/release_payload_controller_resources.pyhack/generators/release-controllers/generate-release-controllers.py
| "initContainers": [ | ||
| { | ||
| "name": "git-sync-init", | ||
| "command": ["/git-sync"], | ||
| "args": [ | ||
| "--repo=https://github.com/openshift/release.git", | ||
| "--ref=main", | ||
| "--root=/tmp/git-sync", | ||
| "--one-time=true", | ||
| "--depth=1" | ||
| ], | ||
| "env": [ | ||
| { | ||
| "name": "GIT_SYNC_DEST", | ||
| "value": "release" | ||
| } | ||
| ], | ||
| "image": "quay-proxy.ci.openshift.org/openshift/ci:ci_git-sync_v4.3.0", | ||
| "volumeMounts": [ | ||
| { | ||
| "name": "release", | ||
| "mountPath": "/tmp/git-sync" | ||
| } | ||
| ] | ||
| } | ||
| ], | ||
| "containers": [ | ||
| { | ||
| "name": "git-sync", | ||
| "command": ["/git-sync"], | ||
| "args": [ | ||
| "--repo=https://github.com/openshift/release.git", | ||
| "--ref=main", | ||
| "--period=30s", | ||
| "--root=/tmp/git-sync", | ||
| "--max-failures=3" | ||
| ], | ||
| "env": [ | ||
| { | ||
| "name": "GIT_SYNC_DEST", | ||
| "value": "release" | ||
| } | ||
| ], | ||
| "image": "quay-proxy.ci.openshift.org/openshift/ci:ci_git-sync_v4.3.0", | ||
| "volumeMounts": [ | ||
| { | ||
| "name": "release", | ||
| "mountPath": "/tmp/git-sync" | ||
| } | ||
| ], | ||
| "resources": { | ||
| "requests": { | ||
| "memory": "1Gi", | ||
| "cpu": "0.5", | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "command": [ | ||
| "/usr/bin/release-controller", | ||
| "--release-namespace=hypershift", | ||
| "--prow-config=/etc/config/config.yaml", | ||
| "--supplemental-prow-config-dir=/etc/config", | ||
| "--job-config=/var/repo/release/ci-operator/jobs", | ||
| "--prow-namespace=ci", | ||
| "--job-namespace=ci-release", | ||
| "--tools-image-stream-tag=release-controller-bootstrap:tools", | ||
| "--release-architecture=amd64", | ||
| "-v=4", | ||
| "--manifest-list-mode" | ||
| ], | ||
| 'image': 'quay-proxy.ci.openshift.org/openshift/ci:ci_release-controller_latest', | ||
| 'imagePullPolicy': 'Always', | ||
| "name": "controller", | ||
| "volumeMounts": get_rc_volume_mounts(), | ||
| 'livenessProbe': { | ||
| 'httpGet': { | ||
| 'path': '/healthz', | ||
| 'port': 8081 | ||
| }, | ||
| 'initialDelaySeconds': 3, | ||
| 'periodSeconds': 3, | ||
| }, | ||
| 'readinessProbe': { | ||
| 'httpGet': { | ||
| 'path': '/healthz/ready', | ||
| 'port': 8081 | ||
| }, | ||
| 'initialDelaySeconds': 10, | ||
| 'periodSeconds': 3, | ||
| 'timeoutSeconds': 600, | ||
| }, | ||
| } | ||
| ], | ||
| "serviceAccountName": "release-controller", | ||
| "volumes": get_rc_volumes(context) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing securityContext on all containers (root + privilege escalation risk, per Checkov).
Neither Deployment sets a securityContext at pod or container level. Checkov flags allowPrivilegeEscalation and root-container risk on both. Per path instructions for Kubernetes manifests, containers should set runAsNonRoot, allowPrivilegeEscalation: false, and drop all capabilities.
🛡️ Suggested fix (apply to each container)
{
"command": [
"/usr/bin/release-controller",
...
],
'image': 'quay-proxy.ci.openshift.org/openshift/ci:ci_release-controller_latest',
'imagePullPolicy': 'Always',
"name": "controller",
"volumeMounts": get_rc_volume_mounts(),
+ "securityContext": {
+ "allowPrivilegeEscalation": False,
+ "runAsNonRoot": True,
+ "capabilities": {"drop": ["ALL"]},
+ },Also applies to: 202-257
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 110-110: Do not hardcode temporary file or directory names
Context: "/tmp/git-sync"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 136-136: Do not hardcode temporary file or directory names
Context: "/tmp/git-sync"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 Ruff (0.15.20)
[error] 111-111: Probable insecure usage of temporary file or directory: "/tmp/git-sync"
(S108)
[error] 137-137: Probable insecure usage of temporary file or directory: "/tmp/git-sync"
(S108)
🤖 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/generators/release-controllers/content/hypershift_resources.py` around
lines 90 - 186, Add securityContext settings to every container in the pod
templates, including the init container and both regular containers under the
git-sync and release-controller definitions. Set runAsNonRoot: true,
allowPrivilegeEscalation: false, and capabilities.drop: ["ALL"] at container
level, and apply the same hardening to the corresponding deployment variant
referenced by the repeated section.
Sources: Path instructions, Linters/SAST tools
| "containers": [ | ||
| { | ||
| "name": "git-sync", | ||
| "command": ["/git-sync"], | ||
| "args": [ | ||
| "--repo=https://github.com/openshift/release.git", | ||
| "--ref=main", | ||
| "--period=30s", | ||
| "--root=/tmp/git-sync", | ||
| "--max-failures=3" | ||
| ], | ||
| "env": [ | ||
| { | ||
| "name": "GIT_SYNC_DEST", | ||
| "value": "release" | ||
| } | ||
| ], | ||
| "image": "quay-proxy.ci.openshift.org/openshift/ci:ci_git-sync_v4.3.0", | ||
| "volumeMounts": [ | ||
| { | ||
| "name": "release", | ||
| "mountPath": "/tmp/git-sync" | ||
| } | ||
| ], | ||
| "resources": { | ||
| "requests": { | ||
| "memory": "1Gi", | ||
| "cpu": "0.5", | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No resource limits/requests on most containers.
The git-sync-init initContainer and both controller containers have no resources block at all; the git-sync sidecar only sets requests without limits. Per path instructions, every container in a K8s manifest should have resource limits to avoid noisy-neighbor impact on the shared CI cluster.
♻️ Suggested fix
"name": "controller",
"volumeMounts": get_rc_volume_mounts(),
+ "resources": {
+ "requests": {"cpu": "100m", "memory": "256Mi"},
+ "limits": {"cpu": "1", "memory": "1Gi"},
+ }, "volumeMounts": [
{
"name": "release",
"mountPath": "/tmp/git-sync"
}
- ]
+ ],
+ "resources": {
+ "requests": {"cpu": "50m", "memory": "128Mi"},
+ "limits": {"cpu": "200m", "memory": "256Mi"},
+ }Also applies to: 216-235
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 136-136: Do not hardcode temporary file or directory names
Context: "/tmp/git-sync"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 Ruff (0.15.20)
[error] 137-137: Probable insecure usage of temporary file or directory: "/tmp/git-sync"
(S108)
🤖 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/generators/release-controllers/content/hypershift_resources.py` around
lines 116 - 146, Add resource specifications to every container in the generated
Kubernetes manifest: the git-sync sidecar’s resources must include both requests
and limits, and the git-sync-init and both controller containers must define
complete requests and limits. Update the container definitions associated with
the git-sync resources and the controller entries to use appropriate CPU and
memory values consistently.
Source: Path instructions
1 participant
1st stage in an experiment to determine whether or not we could leverage https://github.com/openshift/release-controller to gate automated promotion of HyperShift Operator builds to the repositories that ARO/ROSA consume from.
This sets up a new release controller deployment (will gives us a separate dashboard for HyperShift) with a very minimal promotion config looking at the existing HyperShift ImageStream on the
app.cicluster (are we ok watching the existing image stream?)I've been trying to work out how to validate whether this works or not and as far as I can tell the only way is to actually deploy it, would prefer some review before trying it on a live cluster so putting here to start that process
Summary by CodeRabbit
app.cicluster, including routing, probes, shared credentials, and Jira integration.hypershiftstable release configuration to gate verified promotion of HyperShift Operator builds.hypershiftnamespace.