Skip to content

OCPBUGS-10431: move CRI-O metrics proxy from static pod to MCD daemonset - #6325

Open
harche wants to merge 1 commit into
openshift:mainfrom
harche:crio-metrics-proxy-via-mcd
Open

OCPBUGS-10431: move CRI-O metrics proxy from static pod to MCD daemonset#6325
harche wants to merge 1 commit into
openshift:mainfrom
harche:crio-metrics-proxy-via-mcd

Conversation

@harche

@harche harche commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

What this does

Moves the kube-rbac-proxy-crio CRI-O metrics proxy off static pods and into the existing machine-config-daemon daemonset 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 staticPodPath on worker pools for STIG V-242397 (context in OCPBUGS-10431): removing staticPodPath kills the proxy, port 9637 goes dead, up{job="crio"} drops to 0, and TargetDown fires.

How

  • New kube-rbac-proxy-crio container in the MCD daemonset. The MCD is already hostNetwork: true, so it binds node:9637 and forwards to CRI-O's loopback-only metrics endpoint at 127.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.
  • Deleted the static pod templates (criometricsproxy.yaml + criometricsproxy-config.yaml for master/worker/arbiter). MCD removes the on-host files when the rendered config drops them.
  • Static authorizer config moves to a kube-rbac-proxy-crio ConfigMap 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-crio Service now selects the machine-config-daemon pods.

Upgrade ordering

syncMachineConfigDaemon waits 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.yaml to disappear and for the port to be released, then execs the proxy. Fresh installs pass both checks immediately. The container also declares no ports: — 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)

  • Verified on two live 4.22.6 clusters (upgrade path; full details in the test-results comment below): daemonset rolled out Ready-but-waiting with all 6 static pods still present (no rollout deadlock), handoff completed as nodes updated, up{job="crio"} == 1 on all nodes throughout, no TargetDown. Also caught & fixed a hostPort admission conflict (now folded into the single squashed commit).
  • Verified staticPodPath: "" on the worker pool: KubeletConfig reports Success, staticPodPath absent from kubelet.conf on all workers, port 9637 still served by the MCD sidecar, CRI-O metrics uninterrupted on all 6 nodes.
  • QE: test/extended-priv getAllKubeProxyPod looks for kube-rbac-proxy-crio-* mirror pods; it tolerates finding none but the TLS-profile test loses that coverage until repointed at the MCD sidecar

Summary by CodeRabbit

  • New Features
    • Integrated the CRI-O metrics proxy into the node’s machine configuration daemon as a DaemonSet container serving the secure metrics endpoint.
    • Added a dedicated ConfigMap to manage Prometheus metrics access authorization.
  • Bug Fixes
    • Updated the metrics proxy service selector to align with the machine configuration daemon.
    • Improved proxy startup reliability by coordinating legacy removal, port readiness, and kubelet TLS material availability.
    • Fixed kubelet ignition rendering when staticPodPath is explicitly set to "".
  • Tests
    • Added coverage for staticPodPath: "" rendering behavior.
  • Chores
    • Removed legacy kube-rbac-proxy CRI-O manifest templates and static-pod definitions.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 21, 2026
@openshift-ci

openshift-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 21, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@harche: This pull request references Jira Issue OCPBUGS-10431, which is invalid:

  • expected the bug to be in one of the following states: NEW, ASSIGNED, POST, but it is ON_QA instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

What this does

Moves the kube-rbac-proxy-crio CRI-O metrics proxy off static pods and into the existing machine-config-daemon daemonset 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 staticPodPath on worker pools for STIG V-242397 (context in OCPBUGS-10431): removing staticPodPath kills the proxy, port 9637 goes dead, up{job="crio"} drops to 0, and TargetDown fires.

How

  • New kube-rbac-proxy-crio container in the MCD daemonset. The MCD is already hostNetwork: true, so it binds node:9637 and forwards to CRI-O's loopback-only metrics endpoint at 127.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.
  • Deleted the static pod templates (criometricsproxy.yaml + criometricsproxy-config.yaml for master/worker/arbiter). MCD removes the on-host files when the rendered config drops them.
  • Static authorizer config moves to a kube-rbac-proxy-crio ConfigMap 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-crio Service now selects the machine-config-daemon pods.

Upgrade ordering

syncMachineConfigDaemon waits 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.yaml to disappear and for the port to be released, then execs the proxy. Fresh installs pass both checks immediately.

Still TODO (WIP)

🤖 Generated with Claude Code

https://claude.ai/code/session_01ChGPtsm6ZxReyA3oZjq79o

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.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The CRI-O metrics proxy moves into the machine-config-daemon DaemonSet, with synchronized authorization, updated Service routing, and removal of standalone templates. Kubelet rendering also preserves explicitly empty staticPodPath values across typed, JSON, and YAML inputs.

Changes

CRI-O metrics proxy migration

Layer / File(s) Summary
Proxy authorization configuration and synchronization
manifests/machineconfigdaemon/kube-rbac-proxy-crio-config.yaml, pkg/operator/sync.go
Adds the proxy authorization ConfigMap and applies it through syncMachineConfigDaemon.
DaemonSet proxy runtime and Service routing
manifests/machineconfigdaemon/daemonset.yaml, install/0000_80_machine-config_00_service.yaml
Adds the CRI-O proxy container with startup waits, TLS configuration, hostPath and ConfigMap mounts, and changes the Service selector to target machine-config-daemon pods.
Standalone proxy template removal
templates/arbiter/.../criometricsproxy*, templates/master/.../criometricsproxy*, templates/worker/.../criometricsproxy*
Removes standalone CRI-O proxy Pods and generated proxy configuration from kubelet templates.

Kubelet staticPodPath handling

Layer / File(s) Summary
Explicit empty value preservation and rendering tests
pkg/controller/kubelet-config/helpers.go, pkg/controller/kubelet-config/staticpodpath_render_test.go
Preserves raw staticPodPath: "" during configuration merging and tests typed, JSON, and YAML rendering behavior.

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
Loading

Suggested reviewers: djoshy


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Container-Privileges ❌ Error manifests/machineconfigdaemon/daemonset.yaml adds a privileged: true container and sets hostNetwork: true/hostPID: true. Remove unnecessary privilege flags or justify them; prefer least-privilege securityContext and avoid hostPID/hostNetwork unless required.
✅ Passed checks (14 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The only added test is TestStaticPodPathEmptyRender, a static Go test name with no dynamic Ginkgo-style title content.
Test Structure And Quality ✅ Passed PASS: The new test is a pure unit test (no Ginkgo/cluster ops), has no resource lifecycle concerns, and uses explicit failure messages on key checks.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the only test change is a unit test in pkg/controller/kubelet-config, so MicroShift compatibility isn’t implicated.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the only new test is a standard Go unit test with no SNO-sensitive assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed No new topology-specific scheduling rules were added; the CRI-O proxy just joins the existing MCD DaemonSet, which already uses a linux selector and broad tolerations.
Ote Binary Stdout Contract ✅ Passed No process-level stdout writes were added; changed Go files are helper/test only, and targeted scans found no fmt.Print*/klog/log setup in them.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the only new test is a standard unit test, and the diff shows no IPv4-only or external-network assumptions.
No-Weak-Crypto ✅ Passed PASS: The new proxy reuses existing TLS profile ciphers; defaults are Intermediate/Modern, and no md5/sha1/des/rc4/3DES/ECB, custom crypto, or secret comparisons were added.
No-Sensitive-Data-In-Logs ✅ Passed New runtime logs are generic wait messages; the diff adds no secret-bearing log statements or sensitive interpolation.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: moving the CRI-O metrics proxy from a static pod to the MCD DaemonSet.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci

openshift-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: harche
Once this PR has been reviewed and has the lgtm label, please assign proietfb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e4adb96 and 5e89e4c.

📒 Files selected for processing (10)
  • install/0000_80_machine-config_00_service.yaml
  • manifests/machineconfigdaemon/daemonset.yaml
  • manifests/machineconfigdaemon/kube-rbac-proxy-crio-config.yaml
  • pkg/operator/sync.go
  • templates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy-config.yaml
  • templates/arbiter/01-arbiter-kubelet/_base/files/criometricsproxy.yaml
  • templates/master/01-master-kubelet/_base/files/criometricsproxy-config.yaml
  • templates/master/01-master-kubelet/_base/files/criometricsproxy.yaml
  • templates/worker/01-worker-kubelet/_base/files/criometricsproxy-config.yaml
  • templates/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

Comment thread manifests/machineconfigdaemon/daemonset.yaml Outdated
Comment thread manifests/machineconfigdaemon/daemonset.yaml
Comment thread manifests/machineconfigdaemon/daemonset.yaml
@harche

harche commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Live cluster test results

Validated 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 (up{job="crio"} == 1 on all nodes throughout, no TargetDown), and staticPodPath: "" on workers now works without losing CRI-O metrics.

Round 1 — upgrade path + hostPort admission bug found

Deployed as a dev image over a running 4.22.6 cluster (CVO scaled down, MCO deployment + machine-config-operator-images ConfigMap repointed), i.e. the real upgrade scenario with 6 kube-rbac-proxy-crio static pods running.

  • Bug found by this test: the sidecar's containerPort: 9637 declaration acts as a hostPort on a hostNetwork pod, so the kubelet rejected the updated MCD pod at admission (NodePorts status) while the legacy static pod still declared 9637 — fixed by dropping the ports declaration (second commit).
  • After the fix: daemonset rolled out 6/6 with the sidecar Ready-but-waiting while all static pods were still present — confirming the rollout does not deadlock against waitForDaemonsetRollout.
  • As each node applied the rendered config (static pod manifest removed), the sidecar transitioned to Listening securely on :9637.
  • up{job="crio"} == 1 on all 6 nodes at every phase; 0 TargetDown.
  • On-node (SSH): /etc/kubernetes/manifests/ empty, legacy /etc/kubernetes/crio-metrics-proxy.cfg removed, :9637 owned by the kube-rbac-proxy sidecar process.
  • staticPodPath: "" KubeletConfig on workers: Success=True, staticPodPath absent from kubelet.conf on all workers, metrics uninterrupted.
Round 2 — fresh cluster, this PR's exact head

Repeated the full flow on a second cluster with this branch (daemonset move + hostPort fix + mergo empty-value fix folded in from #6319).

  • Same no-deadlock rollout and handoff behavior reproduced.
  • PID→pod proof that the daemonset serves the metrics: the :9637 listener (pid 2736) has cgroup kubepods-burstable-podef897fd9_c462_... matching the UID of daemonset pod machine-config-daemon-b6fb5; crictl confirms the kube-rbac-proxy-crio container belongs to that pod.
  • Direct authenticated scrape of https://localhost:9637/metrics returns 154 container_runtime_crio_* series; unauthenticated requests get HTTP 401; CRI-O's own endpoint stays loopback-only on 127.0.0.1:9537.
  • staticPodPath: "" on workers: staticPodPath gone from kubelet.conf on all 3 workers while :9637 keeps serving on each; up{job="crio"} == 1 on all 6 nodes; 0 TargetDown.
Runtime-level teardown audit (all 6 nodes)

Verified via crictl/ss on every node that the static pods are truly gone, not just absent from the API:

Check Result (every node)
criometricsproxy.yaml on disk absent
CRI-O sandboxes kube-rbac-proxy-crio-* (incl. exited) 0
Containers from those sandboxes (crictl ps -a) 0
Listeners on :9637 exactly 1 (the MCD sidecar)

Workers: /etc/kubernetes/manifests/ completely empty (kubelet no longer watches a static pod dir — the STIG V-242397 end state). Masters: manifests dir contains exactly the four control-plane static pods (etcd, kube-apiserver, kube-controller-manager, kube-scheduler) — platform static pods untouched.

🤖 Generated with Claude Code

@harche
harche force-pushed the crio-metrics-proxy-via-mcd branch 3 times, most recently from 16a6dc3 to 30f0bdb Compare July 23, 2026 14:19
@harche harche changed the title WIP: OCPBUGS-10431: move CRI-O metrics proxy from static pod to MCD daemonset OCPBUGS-10431: move CRI-O metrics proxy from static pod to MCD daemonset Jul 23, 2026
@harche

harche commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

monitoring CI jobs.

/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2026
@harche
harche marked this pull request as ready for review July 23, 2026 15:44
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2026
@harche
harche force-pushed the crio-metrics-proxy-via-mcd branch from 30f0bdb to d4a8f0e Compare July 23, 2026 16:20
@harche

harche commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

/retest-required

@harche

harche commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

/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

@harche

harche commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-hypershift

@harche

harche commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-upgrade

@harche

harche commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-hypershift

@harche

harche commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn

@harche

harche commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-upgrade

@openshift-ci

openshift-ci Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@harche: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ovn-upgrade d4a8f0e link true /test e2e-aws-ovn-upgrade

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants