Skip to content

Conversation

@zeeke
Copy link
Contributor

@zeeke zeeke commented Nov 17, 2025

MultusNetworkPolicy implementation default to util.GetHostname to know the name of the node it is running on and filter pods accordingly. In some cases, that information might be not accurate, leading to a wrong behavior of the controller.

Use downward api to populate the --hostname argument of the multus network policy controller.

Link: https://github.com/kubernetes/component-helpers/blob/v0.34.2/node/util/hostname.go#L28

@coderabbitai
Copy link

coderabbitai bot commented Nov 17, 2025

Walkthrough

The multus-networkpolicy DaemonSet container now receives the node name at runtime: an environment variable K8S_NODE_NAME is set from spec.nodeName via a fieldRef, and the container args include a --hostname-override=$(K8S_NODE_NAME) flag to pass that per-node value to the process.

Changes

Cohort / File(s) Summary
Kubernetes manifest configuration
bindata/network/multus-networkpolicy/multus-networkpolicy.yaml
Adds environment variable K8S_NODE_NAME sourced from spec.nodeName via fieldRef and updates container args to include --hostname-override=$(K8S_NODE_NAME) for per-node hostname injection.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify the fieldRef path spec.nodeName is supported by the target Kubernetes API version.
  • Confirm the --hostname-override=$(K8S_NODE_NAME) substitution works with the container entrypoint/argument parsing.
✨ Finishing touches
🧪 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 a4d1b97 and c2853f0.

📒 Files selected for processing (1)
  • bindata/network/multus-networkpolicy/multus-networkpolicy.yaml (1 hunks)
🧰 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/network/multus-networkpolicy/multus-networkpolicy.yaml
🔇 Additional comments (1)
bindata/network/multus-networkpolicy/multus-networkpolicy.yaml (1)

44-49: Correctly implements hostname override using Downward API.

The implementation properly:

  • Uses the correct Kubernetes variable substitution syntax $(K8S_NODE_NAME) in the --hostname-override flag (line 44)
  • Defines the environment variable with fieldRef pointing to spec.nodeName (lines 46–49), ensuring each pod receives its actual node name at runtime
  • Maintains the flag name --hostname-override as required by the multus-networkpolicy controller

This replaces the potentially inaccurate util.GetHostname() with a reliable per-node value sourced from the Downward API, which is the correct pattern for obtaining node metadata in Kubernetes.


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

@openshift-ci openshift-ci bot requested review from miheer and pperiyasamy November 17, 2025 17:45
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 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 fda7a9f and c54a54b.

📒 Files selected for processing (1)
  • bindata/network/multus-networkpolicy/multus-networkpolicy.yaml (1 hunks)
🧰 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/network/multus-networkpolicy/multus-networkpolicy.yaml

@zeeke
Copy link
Contributor Author

zeeke commented Nov 18, 2025

/retest

2 similar comments
@zeeke
Copy link
Contributor Author

zeeke commented Nov 20, 2025

/retest

@zeeke
Copy link
Contributor Author

zeeke commented Nov 20, 2025

/retest

@zeeke
Copy link
Contributor Author

zeeke commented Nov 20, 2025

@pliurh @LionelJouin @bpickard22 please, take a look when you have time.

@zeeke
Copy link
Contributor Author

zeeke commented Dec 1, 2025

@pliurh @kyrtapz @LionelJouin any chance to get a review here?

@pliurh
Copy link
Contributor

pliurh commented Dec 4, 2025

The commit message needs to be updated; the flag name is --hostname-override.

MultusNetworkPolicy implementation default to `util.GetHostname`
to know the name of the node it is running on and filter pods accordingly.
In some cases, that information might be not accurate, leading to a
wrong behavior of the controller.

Use downward api to populate the `--hostname-override` argument of
the multus network policy controller.

Link: https://github.com/kubernetes/component-helpers/blob/v0.34.2/node/util/hostname.go#L28
Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
@zeeke
Copy link
Contributor Author

zeeke commented Dec 4, 2025

The commit message needs to be updated; the flag name is --hostname-override.

Thanks for noticing. just fixed

@zeeke
Copy link
Contributor Author

zeeke commented Dec 12, 2025

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 12, 2025

@zeeke: 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/security c2853f0 link false /test security

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.

@pliurh
Copy link
Contributor

pliurh commented Dec 15, 2025

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 15, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pliurh, zeeke

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2025
@zeeke
Copy link
Contributor Author

zeeke commented Dec 18, 2025

/check-required-labels

@zeeke zeeke changed the title mnp: Set hostname using downward API CNF-21001: mnp: Set hostname using downward API Dec 19, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 19, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 19, 2025

@zeeke: This pull request references CNF-21001 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

MultusNetworkPolicy implementation default to util.GetHostname to know the name of the node it is running on and filter pods accordingly. In some cases, that information might be not accurate, leading to a wrong behavior of the controller.

Use downward api to populate the --hostname argument of the multus network policy controller.

Link: https://github.com/kubernetes/component-helpers/blob/v0.34.2/node/util/hostname.go#L28

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.

@zeeke
Copy link
Contributor Author

zeeke commented Dec 19, 2025

/jira refresh
/verified by CI

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 19, 2025

@zeeke: This pull request references CNF-21001 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

/jira refresh
/verified by CI

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.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants