OCPBUGS-65631: Use dedicated service accounts for multus pods

[yboaron]

Fix multus and cni-sysctl-allowlist-ds to use dedicated service accounts instead of default.

[openshift-ci-robot]

@yboaron: This pull request references Jira Issue OCPBUGS-65631, which is invalid:

• expected the bug to target the "4.21.0" version, but no target version was set

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:

Fix multus and cni-sysctl-allowlist-ds to use dedicated service accounts instead of default.

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]

Walkthrough

The changes add and manage serviceAccountName configurations in DaemonSet manifests. One modification adds serviceAccountName: multus-ancillary-tools to the allowlist DaemonSet pod template spec, while another unconditionally applies serviceAccountName: multus to the multus DaemonSet by removing its conditional wrapper. A test is added to verify consistent serviceAccountName assignment.

Changes

Cohort / File(s)	Summary
DaemonSet manifest configuration bindata/allowlist/daemonset/daemonset.yaml, bindata/network/multus/multus.yaml	Added serviceAccountName: multus-ancillary-tools field to allowlist DaemonSet pod template spec. Removed conditional {{ if not .NETWORK_NODE_IDENTITY_ENABLE }} wrapper from multus DaemonSet, making serviceAccountName: multus unconditional.
Multus test suite pkg/network/multus_test.go	Added TestMultusServiceAccountAlwaysSet() test function verifying serviceAccountName is consistently set to "multus" with and without Node Identity enabled. Added findDaemonSet() helper function to locate DaemonSet within rendered objects. Updated imports to include unstructured utilities.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Review focus areas:
  • Verify the conditional removal in multus.yaml does not introduce unintended behavior changes in existing deployments
  • Confirm test coverage correctly validates serviceAccountName in both Node Identity scenarios
  • Ensure the helper function correctly locates and returns the intended DaemonSet object

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 41be31e and 2785ae1.

📒 Files selected for processing (3)

• bindata/allowlist/daemonset/daemonset.yaml (1 hunks)
• bindata/network/multus/multus.yaml (0 hunks)
• pkg/network/multus_test.go (2 hunks)

💤 Files with no reviewable changes (1)

• bindata/network/multus/multus.yaml

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• bindata/allowlist/daemonset/daemonset.yaml
• pkg/network/multus_test.go

🧬 Code graph analysis (1)

pkg/network/multus_test.go (1)

pkg/apply/apply.go (1)

• Object (22-25)

🔇 Additional comments (4)

bindata/allowlist/daemonset/daemonset.yaml (1)

21-21: LGTM! Security improvement by using dedicated service account.

Adding a dedicated service account instead of using the default is a security best practice, following the principle of least privilege.

pkg/network/multus_test.go (3)

9-9: LGTM! Necessary import for unstructured field access.

The import is required for accessing nested fields in the DaemonSet spec using NestedString.

---

60-99: LGTM! Comprehensive test coverage for service account assignment.

The test thoroughly verifies that the Multus DaemonSet has its serviceAccountName set to "multus" in both scenarios (with and without node identity enabled), ensuring the PR objective is met.

---

101-108: LGTM! Clean helper function with correct filtering logic.

The helper properly filters DaemonSets by kind, namespace, and name, returning nil when not found.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yboaron
Once this PR has been reviewed and has the lgtm label, please assign kyrtapz 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

[yboaron]

/jira refresh

[openshift-ci-robot]

@yboaron: This pull request references Jira Issue OCPBUGS-65631, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.21.0) matches configured target version for branch (4.21.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (weliang@redhat.com), skipping review request.

Details

In response to this:

/jira refresh

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.

[ehearne-redhat]

/test security

[ehearne-redhat]

/retest

[ehearne-redhat]

/retest

[yboaron]

/retest

[yboaron]

/retest

[openshift-ci]

@yboaron: The following tests 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/security	2785ae1	link	false	/test security
ci/prow/e2e-aws-ovn-windows	2785ae1	link	true	/test e2e-aws-ovn-windows

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.

[yboaron]

It seems that e2e-aws-ovn-window gate is consistently failing [1] ,

cc @ricky-rav
[1]
https://prow.ci.openshift.org/job-history/gs/test-platform-results/pr-logs/directory/pull-ci-openshift-cluster-network-operator-master-e2e-aws-ovn-windows

[ricky-rav]

@yboaron yeah, that job is permafailing, it should be ignored. A maintainer will override it when approving this PR.

[yboaron]

/cc @kyrtapz

[yboaron]

/cc @pliurh

[pliurh]

Why I cannot see that the cni-sysctl-allowlist-ds` is updated?

[yboaron]

Hi @pliurh
cni-sysctl-allowlist-ds DaemonSet is updated in
bindata/allowlist/daemonset/daemonset.yaml [1] file , I added serviceAccountName: multus-ancillary-tools to ensure
it uses a dedicated service account instead of the default one.

[1]
https://github.com/yboaron/cluster-network-operator/blob/2785ae1cd1c4ef2e45279a374a2f06f53b600351/bindata/allowlist/daemonset/daemonset.yaml#L21

[pliurh]

It looks good to me.
@kyrtapz Can you take a look? IIUC, when NETWORK_NODE_IDENTITY_ENABLE=true, Multus won't use the token and cert provided for the SA.