OCPBUGS-10431: move CRI-O metrics proxy from static pod to MCD daemonset - #6325
OCPBUGS-10431: move CRI-O metrics proxy from static pod to MCD daemonset#6325harche wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/valid-reference
|
@harche: This pull request references Jira Issue OCPBUGS-10431, 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. |
|
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:
WalkthroughThe CRI-O metrics proxy moves into the ChangesCRI-O metrics proxy migration
Kubelet staticPodPath handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MachineConfigDaemon
participant kube-rbac-proxy-crio
participant Kubelet
participant CRIOMetrics
MachineConfigDaemon->>kube-rbac-proxy-crio: Start proxy container
kube-rbac-proxy-crio->>kube-rbac-proxy-crio: Wait for legacy manifest removal and port 9637
kube-rbac-proxy-crio->>Kubelet: Read kubelet TLS material
kube-rbac-proxy-crio->>CRIOMetrics: Proxy secured metrics from 127.0.0.1:9537
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 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
🤖 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 `@manifests/machineconfigdaemon/daemonset.yaml`:
- Around line 88-93: Define both liveness and readiness probes for the
kube-rbac-proxy-crio container, using its crio-metrics port and the expected
health endpoint. Ensure readiness remains false while startup is blocked waiting
for the static pod manifest removal or port release, so upgrade failures are not
reported as healthy.
- Around line 126-129: Update the container resources block in the machineconfig
daemon workload to add both CPU and memory limits alongside the existing
requests. Use appropriate limit values consistent with the manifest’s resource
policy, while preserving the current request settings.
- Around line 124-125: Harden the machine-config-daemon metrics-proxy
container’s securityContext by removing privileged: true, enabling
readOnlyRootFilesystem and allowPrivilegeEscalation: false, and dropping all
Linux capabilities. Evaluate and enable runAsNonRoot: true if the proxy can
still read the kubelet certificates; otherwise retain only the restrictions that
preserve certificate access.
🪄 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: c3b9af61-5566-4e70-81fc-8dd2392dbde1
📒 Files selected for processing (10)
install/0000_80_machine-config_00_service.yamlmanifests/machineconfigdaemon/daemonset.yamlmanifests/machineconfigdaemon/kube-rbac-proxy-crio-config.yamlpkg/operator/sync.gotemplates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy-config.yamltemplates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy.yamltemplates/master/01-master-kubelet/_base/files/criometricsproxy-config.yamltemplates/master/01-master-kubelet/_base/files/criometricsproxy.yamltemplates/worker/01-worker-kubelet/_base/files/criometricsproxy-config.yamltemplates/worker/01-worker-kubelet/_base/files/criometricsproxy.yaml
💤 Files with no reviewable changes (6)
- templates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy.yaml
- templates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy-config.yaml
- templates/master/01-master-kubelet/_base/files/criometricsproxy.yaml
- templates/master/01-master-kubelet/_base/files/criometricsproxy-config.yaml
- templates/worker/01-worker-kubelet/_base/files/criometricsproxy.yaml
- templates/worker/01-worker-kubelet/_base/files/criometricsproxy-config.yaml
| securityContext: | ||
| privileged: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Drop privileged: true and enforce restrictive security context.
As per path instructions and coding guidelines, containers must not use privileged: true without justification. Additionally, configure readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, drop all capabilities, and evaluate if runAsNonRoot: true can be applied without breaking access to the kubelet certificates.
Since this container only proxies metrics and reads TLS files, it likely does not need full privileged access like the main machine-config-daemon container.
🛡️ Proposed fix to harden the security context
securityContext:
- privileged: true
+ privileged: false
+ readOnlyRootFilesystem: true
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop:
+ - ALL📝 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.
| securityContext: | |
| privileged: true | |
| securityContext: | |
| privileged: false | |
| readOnlyRootFilesystem: true | |
| allowPrivilegeEscalation: false | |
| capabilities: | |
| drop: | |
| - ALL |
🤖 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 `@manifests/machineconfigdaemon/daemonset.yaml` around lines 124 - 125, Harden
the machine-config-daemon metrics-proxy container’s securityContext by removing
privileged: true, enabling readOnlyRootFilesystem and allowPrivilegeEscalation:
false, and dropping all Linux capabilities. Evaluate and enable runAsNonRoot:
true if the proxy can still read the kubelet certificates; otherwise retain only
the restrictions that preserve certificate access.
Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
privileged: true is required here: the container reads /var/lib/kubelet/pki/kubelet-server-current.pem (root-owned, 0600) and /etc/kubernetes/kubelet-ca.crt from hostPath mounts. On RHCOS an unprivileged container runs as container_t under SELinux and cannot read kubelet-labeled host files regardless of UID — privileged grants spc_t, which is why both containers of the static pod this replaces were privileged. runAsNonRoot is impossible for the same reason (0600 root-owned key). The existing 9001 kube-rbac-proxy sidecar is unprivileged only because it mounts a Secret and ConfigMap, not hostPaths. This preserves the exact security posture of the static pod, and the pod is already annotated openshift.io/required-scc: privileged.
There was a problem hiding this comment.
@harche, agreed—thanks for the detailed RHCOS/SELinux context. The proxy requires privileged spc_t access to the kubelet-labeled, root-owned hostPath TLS key, and runAsNonRoot would be incompatible with the required 0600 key access. Since this preserves the existing static pod’s security posture within an already privileged SCC pod, the hardening recommendation does not apply here. Withdrawing this finding.
✏️ Learnings added
Learnt from: harche
Repo: openshift/machine-config-operator PR: 6325
File: manifests/machineconfigdaemon/daemonset.yaml:124-125
Timestamp: 2026-07-21T16:57:02.640Z
Learning: In `manifests/machineconfigdaemon/daemonset.yaml`, the `kube-rbac-proxy-crio` container must remain privileged because it reads the root-owned (0600) `/var/lib/kubelet/pki/kubelet-server-current.pem` and `/etc/kubernetes/kubelet-ca.crt` through hostPath mounts. On RHCOS, an unprivileged container runs under SELinux `container_t` and cannot read kubelet-labeled host files regardless of UID; `privileged: true` grants the required `spc_t` domain. `runAsNonRoot` is incompatible with access to the kubelet private key. This retains the security posture of the privileged static pod the container replaces.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@harche would you mind trying to manually ask for spc_t in the container, rather than give privileged? it is a slightly narrower security grant and may be safer in some cases
There was a problem hiding this comment.
Good call. I tried it on a live 4.22 cluster-bot cluster, and spc_t alone turned out to be necessary but not sufficient. The sidecar went into CrashLoopBackOff with:
error loading certificate: open /var/lib/kubelet/pki/kubelet-server-current.pem: permission denied
The failure is DAC, not SELinux: the kube-rbac-proxy image runs as uid 65534 by default, and the kubelet serving cert is root-owned 0600. privileged: true had been masking this, since privileged containers get the full capability set including CAP_DAC_OVERRIDE even as non-root. That is how the old static pod (also uid 65534 plus privileged) read the cert.
What works, and what's now in the PR:
securityContext:
runAsUser: 0
seLinuxOptions:
type: spc_tRoot satisfies DAC on the 0600 cert, and spc_t satisfies SELinux on the kubelet-labeled hostPaths. Compared to privileged this drops the full capability grant and device access, and keeps the default seccomp profile. So it lands where you were aiming: same functionality, narrower grant.
Verified on the live cluster: the daemonset rolls out cleanly, the sidecar reads the cert and serves on :9637 (ps shows system_u:system_r:spc_t:s0 running as root), and up{job="crio"} == 1 on all 6 nodes.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
can you add CAP_DAC_OVERRIDE instead of giving root? that may be technically safer, though maybe practically not that much more
There was a problem hiding this comment.
Tried it on a live cluster, and interestingly capabilities.add: ["DAC_OVERRIDE"] on the image's default non-root user does not work. Same crash:
error loading certificate: open /var/lib/kubelet/pki/kubelet-server-current.pem: permission denied
The reason: at every execve() the kernel recomputes a process's effective capabilities, and for a non-root process the only way an added capability survives that recomputation is the ambient capability set (see capabilities(7), "Transformation of capabilities during execve"). Kubernetes has no way to set ambient capabilities (kubernetes#56374, KEP-2763), so the added DAC_OVERRIDE is gone by the time the proxy runs. privileged hid this for the old static pod because privileged containers get every capability set populated, ambient included.
Since the cert is root-owned, uid 0 passes the DAC check as the file owner, needing no capabilities at all. So the PR now carries:
securityContext:
runAsUser: 0
capabilities:
drop: ["ALL"]
seLinuxOptions:
type: spc_tVerified on the cluster: all sidecars serving, up{job="crio"} == 1 on all 6 nodes, and /proc/<pid>/status for the proxy shows CapEff, CapPrm and CapBnd all zero. The process is root in name but holds an empty bounding set, which is a narrower grant than non-root plus DAC_OVERRIDE would have been: it cannot read other users' files beyond what DAC ownership allows, cannot chown, cannot kill, cannot bind low ports. SELinux confinement aside, the only elevation left is spc_t itself.
5e89e4c to
b3a4b37
Compare
Live cluster test resultsValidated end-to-end on two separate cluster-bot 4.22.6 clusters (AWS, 3 masters + 3 workers). The second run tested this PR's exact head (all three commits combined). TL;DR: upgrade handoff from static pod to MCD sidecar completes with zero scrape gap ( Round 1 — upgrade path + hostPort admission bug foundDeployed as a dev image over a running 4.22.6 cluster (CVO scaled down, MCO deployment +
Round 2 — fresh cluster, this PR's exact headRepeated the full flow on a second cluster with this branch (daemonset move + hostPort fix + mergo empty-value fix folded in from #6319).
Runtime-level teardown audit (all 6 nodes)Verified via
Workers: 🤖 Generated with Claude Code |
16a6dc3 to
30f0bdb
Compare
harche
changed the title
|
monitoring CI jobs. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
30f0bdb to
d4a8f0e
Compare
|
/retest-required |
|
/test e2e-gcp-op-part1 e2e-gcp-op-part2 e2e-gcp-op-ocl-part1 e2e-gcp-op-ocl-part2 e2e-gcp-op-single-node e2e-aws-ovn e2e-aws-ovn-upgrade e2e-hypershift |
|
/test e2e-hypershift |
|
/test e2e-aws-ovn-upgrade |
|
/test e2e-hypershift |
|
/test e2e-aws-ovn |
|
/test e2e-aws-ovn-upgrade |
|
/hold cancel /cc @haircommander |
openshift-ci
Bot
removed
the
do-not-merge/hold
d4a8f0e to
f62d77d
Compare
f62d77d to
cffbb42
Compare
|
/retest-required |
|
/lgtm thanks for driving this! |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haircommander, harche 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 |
|
/retest-required |
|
@harche: all tests passed! 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. |
Thanks @haircommander /cc @yuqi-zhang |
3 participants
What this does
Moves the
kube-rbac-proxy-crioCRI-O metrics proxy off static pods and into the existingmachine-config-daemondaemonset as an additional container.The static pod (added in #4175 / OCPNODE-2098) is the only static pod running on every node role, which blocks disabling
staticPodPathon worker pools for STIG V-242397 (context in OCPBUGS-10431): removingstaticPodPathkills the proxy, port 9637 goes dead,up{job="crio"}drops to 0, and TargetDown fires.How
kube-rbac-proxy-criocontainer in the MCD daemonset. The MCD is alreadyhostNetwork: true, so it bindsnode:9637and forwards to CRI-O's loopback-only metrics endpoint at127.0.0.1:9537, serving the kubelet certificate (kubelet-server-current.pem) and validating clients against the kubelet CA — the cluster-monitoring-operator kubelet ServiceMonitor scrape chain (job="crio") is unchanged.criometricsproxy.yaml+criometricsproxy-config.yamlfor master/worker/arbiter). MCD removes the on-host files when the rendered config drops them.kube-rbac-proxy-crioConfigMap instead of a MachineConfig-written host file; the proxy uses in-cluster SA credentials (MCD SA already has tokenreview/SAR permissions) instead of the kubelet kubeconfig.kube-rbac-proxy-crioService now selects themachine-config-daemonpods.Upgrade ordering
syncMachineConfigDaemonwaits for the daemonset rollout before syncing the node configs that delete the static pod manifest. A naive bind on 9637 would crashloop against the still-running static pod and deadlock the upgrade. The new container instead waits for/etc/kubernetes/manifests/criometricsproxy.yamlto disappear and for the port to be released, then execs the proxy. Fresh installs pass both checks immediately. The container also declares noports:— with hostNetwork a declared containerPort acts as a hostPort, and the kubelet would reject the pod at admission while the legacy static pod still declares 9637.This PR also includes the #6319 change (mergo empty-value fix: clear the staticPodPath template default when the user sets it to empty), squashed into the single commit so the whole staticPodPath story is reviewable in one place; #6319 is closed in favor of this PR.
Still TODO (WIP)
up{job="crio"} == 1on all nodes throughout, no TargetDown. Also caught & fixed a hostPort admission conflict (now folded into the single squashed commit).staticPodPath: ""on the worker pool: KubeletConfig reports Success,staticPodPathabsent fromkubelet.confon all workers, port 9637 still served by the MCD sidecar, CRI-O metrics uninterrupted on all 6 nodes.test/extended-privgetAllKubeProxyPodlooks forkube-rbac-proxy-crio-*mirror pods; it tolerates finding none but the TLS-profile test loses that coverage until repointed at the MCD sidecarSummary by CodeRabbit
staticPodPathis explicitly set to"".staticPodPath: ""rendering behavior.