OCPBUGS-77404: bugfix: mount /etc/containers so image controllers respect cluster-wide image registry configurations#439
Conversation
…de image registry configurations Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
openshift-ci-robot
added
jira/severity-moderate
|
@everettraven: This pull request references Jira Issue OCPBUGS-77404, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughDeployment and operator changes: the controller-manager now requests the ChangesController manager hostaccess and related wiring
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
| path: tls-ca-bundle.pem | ||
| - emptyDir: {} | ||
| name: tmp | ||
| - hostPath: |
There was a problem hiding this comment.
Open Question: Does this automatically handle pull secrets for private registries as well?
There was a problem hiding this comment.
It doesn't, but I've opened openshift/openshift-controller-manager#447 to help with this.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
bindata/assets/openshift-controller-manager/deploy.yaml (2)
60-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd resource limits for memory and CPU.
The coding guidelines require resource limits on every container. Currently, only resource requests are defined. Without limits, the controller-manager pod could consume unbounded resources, potentially impacting control plane stability.
📊 Proposed fix to add resource limits
resources: requests: memory: 100Mi cpu: 100m + limits: + memory: 200Mi + cpu: 200mNote: Adjust limit values based on observed resource usage patterns.
🤖 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 `@bindata/assets/openshift-controller-manager/deploy.yaml` around lines 60 - 63, The resources block currently only sets requests (memory: 100Mi, cpu: 100m); add a limits section alongside requests to enforce upper bounds (e.g., limits: memory: 200Mi, cpu: 200m or other values tuned to observed usage) so the openshift-controller-manager container cannot consume unbounded resources; update the same resources mapping (the resources key for the controller-manager container) to include a limits subsection with both memory and cpu entries.Source: Coding guidelines
49-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit
runAsNonRoot: trueto the container securityContext.While the
hostaccessSCC may enforce non-root execution, the manifest should explicitly declarerunAsNonRoot: truefor defense in depth and to match the PR's stated goal of "preventing running the container as root." Without this setting, the security guarantee depends entirely on the SCC, which could change.🔒 Proposed fix to add explicit runAsNonRoot
- name: controller-manager securityContext: + runAsNonRoot: true readOnlyRootFilesystem: true allowPrivilegeEscalation: false capabilities:🤖 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 `@bindata/assets/openshift-controller-manager/deploy.yaml` around lines 49 - 54, The container's securityContext is missing an explicit runAsNonRoot setting; update the container spec's securityContext (the existing securityContext block that contains readOnlyRootFilesystem, allowPrivilegeEscalation, and capabilities.drop) to add runAsNonRoot: true so the pod manifest explicitly prevents running as UID 0 (defense in depth) while preserving the other keys (readOnlyRootFilesystem, allowPrivilegeEscalation, capabilities.drop).Source: Coding guidelines
🤖 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 `@bindata/assets/openshift-controller-manager/deploy.yaml`:
- Line 36: The Deployment currently sets openshift.io/required-scc: hostaccess
which is too permissive—either replace it with the restricted-v3 SCC or wire a
purpose-built custom SCC and document why elevated host access is required;
additionally, in the podSpec/container securityContext ensure runAsNonRoot: true
is set, explicitly set automountServiceAccountToken: false at the podSpec level,
and add resources.limits for the controller container to complement existing
resources.requests so limits are enforced.
---
Outside diff comments:
In `@bindata/assets/openshift-controller-manager/deploy.yaml`:
- Around line 60-63: The resources block currently only sets requests (memory:
100Mi, cpu: 100m); add a limits section alongside requests to enforce upper
bounds (e.g., limits: memory: 200Mi, cpu: 200m or other values tuned to observed
usage) so the openshift-controller-manager container cannot consume unbounded
resources; update the same resources mapping (the resources key for the
controller-manager container) to include a limits subsection with both memory
and cpu entries.
- Around line 49-54: The container's securityContext is missing an explicit
runAsNonRoot setting; update the container spec's securityContext (the existing
securityContext block that contains readOnlyRootFilesystem,
allowPrivilegeEscalation, and capabilities.drop) to add runAsNonRoot: true so
the pod manifest explicitly prevents running as UID 0 (defense in depth) while
preserving the other keys (readOnlyRootFilesystem, allowPrivilegeEscalation,
capabilities.drop).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0f6912d5-6a19-4d54-a6d9-64aca0dbe28d
📒 Files selected for processing (1)
bindata/assets/openshift-controller-manager/deploy.yaml
| annotations: | ||
| target.workload.openshift.io/management: '{"effect": "PreferredDuringScheduling"}' | ||
| openshift.io/required-scc: restricted-v3 | ||
| openshift.io/required-scc: hostaccess |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing custom SCC definitions in the repository
rg -n "kind:\s*SecurityContextConstraints" -A 20Repository: openshift/cluster-openshift-controller-manager-operator
Length of output: 81
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="bindata/assets/openshift-controller-manager/deploy.yaml"
echo "== File size =="
wc -l "$FILE"
echo "== SCC annotation occurrences (context) =="
rg -n "openshift\.io/required-scc|required-scc" "$FILE" -n
echo "== runAsNonRoot occurrences =="
rg -n "runAsNonRoot" "$FILE" || true
echo "== resources.limits occurrences =="
rg -n "resources:|limits:" "$FILE"
echo "== Extract: container securityContext/resources/volumes around likely sections =="
# show around where the reviewer cited lines (roughly); adjust if file differs
sed -n '1,130p' "$FILE" | nl -ba | sed -n '1,130p'
sed -n '90,180p' "$FILE" | nl -ba | sed -n '1,120p'
echo "== hostPath volume /etc/containers mount check =="
rg -n "etc/containers|hostPath|readOnly" "$FILE"
echo "== Anti-affinity / topology-related scheduling constraints =="
rg -n "antiAffinity|requiredDuringSchedulingIgnoredDuringExecution|topologyKey|hostname" "$FILE" || trueRepository: openshift/cluster-openshift-controller-manager-operator
Length of output: 499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for any custom SCC definitions/usage in repo =="
rg -n "SecurityContextConstraints" -S . || true
rg -n "hostaccess|openshift\.io/required-scc" -S . || true
rg -n "SecurityContextConstraint" -S . || trueRepository: openshift/cluster-openshift-controller-manager-operator
Length of output: 19745
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="bindata/assets/openshift-controller-manager/deploy.yaml"
echo "== Extract around SCC (lines 1-60) =="
awk 'NR<=60{print NR ":" $0}' "$FILE"
echo "== Extract around resources (55-95) =="
awk 'NR>=55 && NR<=95{print NR ":" $0}' "$FILE"
echo "== Extract around hostPath/volumes (95-140) =="
awk 'NR>=95 && NR<=140{print NR ":" $0}' "$FILE"
echo "== Extract last lines (141-200) =="
awk 'NR>=141 && NR<=200{print NR ":" $0}' "$FILE"
echo "== Field presence checks =="
for pat in \
"runAsNonRoot" \
"allowPrivilegeEscalation" \
"readOnlyRootFilesystem" \
"capabilities:" \
"automountServiceAccountToken" \
"limits:" \
"livenessProbe" \
"readinessProbe" \
"hostPID" \
"hostNetwork" \
"hostIPC" \
"privileged" \
"antiAffinity" \
"topologyKey"; do
echo "-- $pat"
rg -n "$pat" "$FILE" || true
done
echo "== Search for custom SCC manifests outside vendor =="
rg -n --glob '!vendor/**' "kind:\s*SecurityContextConstraints" -S . || trueRepository: openshift/cluster-openshift-controller-manager-operator
Length of output: 6288
Major security hardening gaps in controller-manager OpenShift Deployment
openshift.io/required-scc: hostaccess
hostaccessis less restricted thanrestricted-v3and doesn’t align with the guideline requiring a restricted/custom-scoped SCC—either justify the need or wire a custom SCC.- Missing
runAsNonRoot: trueat pod/containersecurityContext(defense-in-depth). - Missing
resources.limits(onlyresources.requestsare set). automountServiceAccountTokenis not explicitly set tofalse.
🤖 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 `@bindata/assets/openshift-controller-manager/deploy.yaml` at line 36, The
Deployment currently sets openshift.io/required-scc: hostaccess which is too
permissive—either replace it with the restricted-v3 SCC or wire a purpose-built
custom SCC and document why elevated host access is required; additionally, in
the podSpec/container securityContext ensure runAsNonRoot: true is set,
explicitly set automountServiceAccountToken: false at the podSpec level, and add
resources.limits for the controller container to complement existing
resources.requests so limits are enforced.
Source: Coding guidelines
by syncing the global pull-secret to the openshift-controller-manager namespace mounted as a volume on the openshift-controller-manager pods and setting the REGISTRY_AUTH_FILE environment variable to the mounted file. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
There was a problem hiding this comment.
I think we need to remove this as e2e-upgrade reports
event happened 36 times, something is wrong: namespace/openshift-controller-manager hmsg/827f4eb8d4 replicaset/controller-manager-99bbc9cc8 - reason/FailedCreate Error creating: pods "controller-manager-99bbc9cc8-" is forbidden: unable to validate against any security context constraint: [metadata.annotations[seccomp.security.alpha.kubernetes.io/pod]: Forbidden: seccomp may not be set, metadata.annotations[container.seccomp.security.alpha.kubernetes.io/controller-manager]: Forbidden: seccomp may not be set] (21:59:34Z) result=reject }
|
@bpalmer The changes in general look fine to me, the seccomp concern is the only one I have. Could you take a look at that and let me know if this is an issue? I dont see any mention https://docs.redhat.com/en/documentation/openshift_container_platform/4.22/html/authentication_and_authorization/managing-pod-security-policies#default-sccs_configuring-internal-oauth about it not being allowed. However it says privileged containers cannot use |
…ment Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/sync_openshiftcontrollermanager_v311_00_test.go (1)
752-755: ⚡ Quick winAssert the new mount contract alongside
REGISTRY_AUTH_FILE.These updates only verify the env var. The feature also depends on the matching secret volume/volumeMount for
/var/run/secrets/image-auth/auth.jsonand the/etc/containershost mount, so those regressions would still pass this suite.This follows the PR objective to mount both the global pull-secret and
/etc/containersinto the deployment.Also applies to: 788-791, 819-822
🤖 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/operator/sync_openshiftcontrollermanager_v311_00_test.go` around lines 752 - 755, The test currently only asserts the REGISTRY_AUTH_FILE env var; also assert that the deployment spec includes a Volume referencing the global pull-secret (a Secret volume whose SecretName equals the cluster pull secret) and a corresponding VolumeMount mounting that secret to /var/run/secrets/image-auth/auth.json, and assert a HostPath (or projected host) Volume and VolumeMount for /etc/containers so the host config is mounted into the container; update the assertions around REGISTRY_AUTH_FILE (and the other similar blocks) to check for a Volume with the secret name, a VolumeMount with MountPath "/var/run/secrets/image-auth" (or the exact file mount), and a Volume/VolumeMount that mounts "/etc/containers" from the host into the pod spec to ensure both secret and host mounts are present.
🤖 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 `@bindata/assets/openshift-controller-manager/scc-rolebinding.yaml`:
- Around line 7-9: The RoleBinding's roleRef is missing the apiGroup; update the
RoleBinding where roleRef.name is
"system:openshift-controller-manager:hostaccess-role" (the hostaccess
RoleBinding) to include roleRef.apiGroup: rbac.authorization.k8s.io so roleRef
has keys kind, name, and apiGroup like other RoleBindings (e.g.,
route-controller-manager-leader-rolebinding); modify the manifest defining
roleRef to add the apiGroup field accordingly.
---
Nitpick comments:
In `@pkg/operator/sync_openshiftcontrollermanager_v311_00_test.go`:
- Around line 752-755: The test currently only asserts the REGISTRY_AUTH_FILE
env var; also assert that the deployment spec includes a Volume referencing the
global pull-secret (a Secret volume whose SecretName equals the cluster pull
secret) and a corresponding VolumeMount mounting that secret to
/var/run/secrets/image-auth/auth.json, and assert a HostPath (or projected host)
Volume and VolumeMount for /etc/containers so the host config is mounted into
the container; update the assertions around REGISTRY_AUTH_FILE (and the other
similar blocks) to check for a Volume with the secret name, a VolumeMount with
MountPath "/var/run/secrets/image-auth" (or the exact file mount), and a
Volume/VolumeMount that mounts "/etc/containers" from the host into the pod spec
to ensure both secret and host mounts are present.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1df28a42-a6e6-4738-bd20-49f2979e7e19
📒 Files selected for processing (5)
bindata/assets/openshift-controller-manager/deploy.yamlbindata/assets/openshift-controller-manager/scc-role.yamlbindata/assets/openshift-controller-manager/scc-rolebinding.yamlpkg/operator/starter.gopkg/operator/sync_openshiftcontrollermanager_v311_00_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- bindata/assets/openshift-controller-manager/deploy.yaml
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@everettraven: This pull request references Jira Issue OCPBUGS-77404, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jitsingh@redhat.com), skipping review request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@everettraven: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
This last failure has a really odd failure. It says the Going to see if maybe this was a fluke. /retest-required |
3 participants
Description
Fixes OCPBUGS-77404 by updating the
openshift-controller-managerdeployment to mount the/etc/containersdirectory from the host that the image controllers using the containers/image package will read for registry configurations.The Machine Configuration Operator is responsible for syncing cluster configurations through the image APIs to this directory on the nodes.
In order to do this, the SCC needed to be adjusted to one that allowed host path mounting. To limit the changes as much as possible I updated the SCC to
hostaccessfromrestricted-v3so that we can mount the host path but prevent running the container asroot, maintaining the user namespace security boundary.Summary by CodeRabbit