WIP: OCPBUGS-10431: clear staticPodPath template default when user sets it to empty

[harche]

- What I did

Follow-up to #5724. That PR relaxed validation so a KubeletConfig CR with staticPodPath: "" is accepted for worker and custom pools, but the empty value never reached the rendered kubelet.conf: mergo.Merge(..., mergo.WithOverride) in generateKubeletIgnFiles does not overwrite a destination field with a zero-value source, so the template default /etc/kubernetes/manifests survived the merge.

This change detects an explicit staticPodPath: "" in the raw KubeletConfig (presence check on the raw JSON, since after decoding an explicit empty value is indistinguishable from unset) and clears the template default before merging, mirroring the existing protectKernelDefaults workaround a few lines above.

Also adds a render-path unit test asserting that:

• an explicit {"staticPodPath":""} renders a kubelet.conf without /etc/kubernetes/manifests
• an unset staticPodPath keeps the template default

- How to verify it

1. On a cluster with this change, apply a KubeletConfig CR targeting the worker pool with staticPodPath: ""
2. Check the rendered MachineConfig (oc get mc 99-worker-generated-kubelet -o yaml) or /etc/kubernetes/kubelet.conf on a worker node: the staticPodPath entry should be absent
3. Without this change, the rendered config still contains staticPodPath: /etc/kubernetes/manifests even though the CR is accepted

- Description for the changelog

Fix staticPodPath: "" in a KubeletConfig CR not taking effect in the rendered kubelet configuration.

WIP: pending cluster testing.

Summary by CodeRabbit

• Bug Fixes

  • Preserved an explicitly provided empty staticPodPath so it’s rendered as intended, rather than falling back to the template default.

• Tests

  • Added new test coverage to confirm correct staticPodPath behavior when the value is omitted or explicitly empty, validating handling across supported configuration input formats.

[openshift-merge-bot]

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-robot]

@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 I did

Follow-up to #5724. That PR relaxed validation so a KubeletConfig CR with staticPodPath: "" is accepted for worker and custom pools, but the empty value never reached the rendered kubelet.conf: mergo.Merge(..., mergo.WithOverride) in generateKubeletIgnFiles does not overwrite a destination field with a zero-value source, so the template default /etc/kubernetes/manifests survived the merge.

This change detects an explicit staticPodPath: "" in the raw KubeletConfig (presence check on the raw JSON, since after decoding an explicit empty value is indistinguishable from unset) and clears the template default before merging, mirroring the existing protectKernelDefaults workaround a few lines above.

Also adds a render-path unit test asserting that:

• an explicit {"staticPodPath":""} renders a kubelet.conf without /etc/kubernetes/manifests
• an unset staticPodPath keeps the template default

- How to verify it

1. On a cluster with this change, apply a KubeletConfig CR targeting the worker pool with staticPodPath: ""
2. Check the rendered MachineConfig (oc get mc 99-worker-generated-kubelet -o yaml) or /etc/kubernetes/kubelet.conf on a worker node: the staticPodPath entry should be absent
3. Without this change, the rendered config still contains staticPodPath: /etc/kubernetes/manifests even though the CR is accepted

- Description for the changelog

Fix staticPodPath: "" in a KubeletConfig CR not taking effect in the rendered kubelet configuration.

WIP: pending cluster testing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01F9QZAvsyRgd8KagUdBxaAZ

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.

[openshift-ci]

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

[coderabbitai]

Walkthrough

Kubelet configuration generation now preserves an explicitly empty staticPodPath from raw JSON or YAML input while retaining template defaults when the field is omitted. Tests decode rendered Ignition content and verify both behaviors.

Changes

Static Pod Path Handling

Layer / File(s)	Summary
Detect explicit empty staticPodPath pkg/controller/kubelet-config/helpers.go	Raw kubelet configuration is checked for an explicitly empty staticPodPath, clearing the original value before merging.
Validate rendered configuration pkg/controller/kubelet-config/staticpodpath_render_test.go	A helper decodes rendered Ignition content, and tests verify struct-marshaled, raw JSON, and YAML inputs produce the expected staticPodPath output.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (14 passed)

Check name	Status	Explanation
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	No Ginkgo titles were added; the only new test name is the static Go test function TestStaticPodPathEmptyRender.
Test Structure And Quality	✅ Passed	Unit test is self-contained and matches nearby package style; no cluster resources, waits, or cleanup concerns were introduced.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests or MicroShift-unsupported APIs; the added test is a standard Go unit test with no MicroShift-specific concerns.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The added test is a plain Go unit test in pkg/controller/kubelet-config, not a Ginkgo e2e test, and it has no multi-node/HA assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	Only kubelet-config merge/render logic and tests changed; no pod specs, affinities, replicas, node selectors, or topology assumptions were added.
Ote Binary Stdout Contract	✅ Passed	No new main/init/TestMain/BeforeSuite stdout writes were added; the PR only changes helper logic and test-body code, with no process-level stdout output.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	Only a Go unit test was added; no Ginkgo/e2e test, IPv4-only logic, or external connectivity is present.
No-Weak-Crypto	✅ Passed	Changed files only adjust kubelet config merging/tests; scans found no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons.
Container-Privileges	✅ Passed	No container/K8s manifest changes or privilege flags were introduced; touched files only adjust kubelet config merge logic and tests.
No-Sensitive-Data-In-Logs	✅ Passed	The PR adds no production logging, and the new test messages don't include passwords, tokens, PII, or hostnames.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: clearing the staticPodPath template default when users set it to empty.

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[openshift-ci]

[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 isabella-janssen 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:

• OWNERS

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

pkg/controller/kubelet-config/helpers.go (1)

6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove unused import.

If you apply the suggested fix below to use yaml.NewYAMLOrJSONDecoder instead of json.Unmarshal, this encoding/json import will become unused and can be removed.

♻️ Proposed refactor

-	"encoding/json"

🤖 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/controller/kubelet-config/helpers.go` at line 6, Remove the unused
encoding/json import from the imports in helpers.go after replacing
json.Unmarshal with yaml.NewYAMLOrJSONDecoder.

pkg/controller/kubelet-config/staticpodpath_render_test.go (1)

69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Avoid reusing a mutable configuration object without copying.

generateKubeletIgnFiles mutates the originalKubeConfig object through mergo.Merge and direct field assignments. The subsequent original2 := originalKubeConfig.DeepCopy() at line 86 copies the already-mutated state from this first call. To ensure the second test case runs against a pristine configuration, deep copy originalKubeConfig before passing it to the first function call.

♻️ Proposed fix

-	kubeletIgn, _, _, err := generateKubeletIgnFiles(kc, originalKubeConfig)
+	kubeletIgn, _, _, err := generateKubeletIgnFiles(kc, originalKubeConfig.DeepCopy())

🤖 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/controller/kubelet-config/staticpodpath_render_test.go` at line 69,
Deep-copy originalKubeConfig before the first generateKubeletIgnFiles call so
that function receives an isolated mutable configuration. Preserve the existing
originalKubeConfig object for the later original2 copy, ensuring both test cases
start from pristine state.

🤖 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/controller/kubelet-config/helpers.go`:
- Around line 556-561: Update the raw kubelet configuration parsing near
DecodeKubeletConfig to use yaml.NewYAMLOrJSONDecoder, preserving support for
both YAML and JSON payloads. Handle and explicitly return the decoder error
instead of silently skipping the staticPodPath override, while retaining the
existing override behavior for an explicitly empty value.

---

Nitpick comments:
In `@pkg/controller/kubelet-config/helpers.go`:
- Line 6: Remove the unused encoding/json import from the imports in helpers.go
after replacing json.Unmarshal with yaml.NewYAMLOrJSONDecoder.

In `@pkg/controller/kubelet-config/staticpodpath_render_test.go`:
- Line 69: Deep-copy originalKubeConfig before the first generateKubeletIgnFiles
call so that function receives an isolated mutable configuration. Preserve the
existing originalKubeConfig object for the later original2 copy, ensuring both
test cases start from pristine state.

🪄 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: b8b438f0-eb46-4464-9b89-23b7534cbe3b

📥 Commits

Reviewing files that changed from the base of the PR and between cb061e1 and af14db4.

📒 Files selected for processing (2)

• pkg/controller/kubelet-config/helpers.go
• pkg/controller/kubelet-config/staticpodpath_render_test.go

[harche]

Folding this into #6325 so the staticPodPath work is reviewable as one unit — the mergo empty-value fix only makes sense together with moving the CRI-O metrics proxy off static pods. Closing in favor of #6325.

[openshift-ci-robot]

@harche: This pull request references Jira Issue OCPBUGS-10431. The bug has been updated to no longer refer to the pull request using the external bug tracker.

Details

In response to this:

- What I did

Follow-up to #5724. That PR relaxed validation so a KubeletConfig CR with staticPodPath: "" is accepted for worker and custom pools, but the empty value never reached the rendered kubelet.conf: mergo.Merge(..., mergo.WithOverride) in generateKubeletIgnFiles does not overwrite a destination field with a zero-value source, so the template default /etc/kubernetes/manifests survived the merge.

This change detects an explicit staticPodPath: "" in the raw KubeletConfig (presence check on the raw JSON, since after decoding an explicit empty value is indistinguishable from unset) and clears the template default before merging, mirroring the existing protectKernelDefaults workaround a few lines above.

Also adds a render-path unit test asserting that:

• an explicit {"staticPodPath":""} renders a kubelet.conf without /etc/kubernetes/manifests
• an unset staticPodPath keeps the template default

- How to verify it

1. On a cluster with this change, apply a KubeletConfig CR targeting the worker pool with staticPodPath: ""
2. Check the rendered MachineConfig (oc get mc 99-worker-generated-kubelet -o yaml) or /etc/kubernetes/kubelet.conf on a worker node: the staticPodPath entry should be absent
3. Without this change, the rendered config still contains staticPodPath: /etc/kubernetes/manifests even though the CR is accepted

- Description for the changelog

Fix staticPodPath: "" in a KubeletConfig CR not taking effect in the rendered kubelet configuration.

WIP: pending cluster testing.

Summary by CodeRabbit

• Bug Fixes

• Preserved an explicitly provided empty staticPodPath so it’s rendered as intended, rather than falling back to the template default.

• Tests

• Added new test coverage to confirm correct staticPodPath behavior when the value is omitted or explicitly empty, validating handling across supported configuration input formats.

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.