OCPBUGS-98724: egress-router: Propagate macvlan master and mode to NAD config

[Ultimate-etamitlU]

Why

The EgressRouter controller sets network_interfaces render data but the NAD template never references it. The egress-router-cni plugin sees no interfaceArgs in the CNI config, so it auto-detects master as the default route interface (br-ex on OVN-Kubernetes nodes) and defaults mode to bridge — ignoring user-specified spec.networkInterface.macvlan.master and spec.networkInterface.macvlan.mode.

What

• Replace unused network_interfaces render data with explicit MacvlanMaster and MacvlanMode values in the controller
• Add interfaceType and interfaceArgs fields to the NAD template, with conditional master inclusion
• Lowercase the mode to match CNI plugin expectation (API: Bridge → CNI: bridge)
• When master is not specified in the CR, behavior is unchanged — the CNI plugin continues to auto-detect

Testing / How to verify it

Automated

• Unit tests added covering: master propagation, master omission when empty, mode lowercasing for all 4 macvlan modes (Bridge/Private/VEPA/Passthru), and AllowedDestinations JSON generation
• make test-unit passes locally
• CI lanes: ci/prow/e2e-gcp-ovn, ci/prow/e2e-aws-ovn-serial-* cover EgressRouter functionality

Manual verification

• Deploy EgressRouter CR with explicit master set to a VLAN subinterface — verify NAD contains correct interfaceArgs.master
• Deploy EgressRouter CR without master — verify CNI plugin still auto-detects (backward compat)
• Verify egress traffic flows through the specified parent interface

User impact

EgressRouter CRs with spec.networkInterface.macvlan.master set will now correctly use the specified parent interface instead of silently falling back to br-ex.

Upgrade / Rollback

• On upgrade: existing EgressRouter NADs will be re-rendered with the new interfaceArgs fields on next reconciliation. CRs without explicit master are unaffected.
• On rollback: NADs revert to omitting interfaceArgs; CNI plugin auto-detects as before.

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

@Ultimate-etamitlU: This pull request references Jira Issue OCPBUGS-98724, which is invalid:

• expected the bug to target the "5.0.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:

Summary

• The EgressRouter controller set network_interfaces render data but the NAD template never referenced it, so the egress-router-cni plugin always auto-detected master as the default route interface (br-ex) and defaulted mode to bridge, ignoring user-specified values
• Add interfaceType and interfaceArgs fields to the NAD template and wire MacvlanMaster/MacvlanMode through the controller render data
• When master is not specified in the CR, behavior is unchanged — the CNI plugin continues to auto-detect

Test plan

• Unit tests pass (make test-unit) — master propagation, master omission, mode lowercasing for all 4 macvlan modes
• Deploy EgressRouter CR with explicit master set to a VLAN subinterface — verify NAD contains correct interfaceArgs.master
• Deploy EgressRouter CR without master — verify CNI plugin still auto-detects (backward compat)
• Verify egress traffic flows through the specified parent interface

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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: e04c2504-b2e3-404c-8f81-be8c1db1bcb8

📥 Commits

Reviewing files that changed from the base of the PR and between 44a14ff and b91136e.

📒 Files selected for processing (3)

• bindata/egress-router/000-nad.yaml
• pkg/controller/egress_router/egress_router_controller.go
• pkg/controller/egress_router/egress_router_controller_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

• bindata/egress-router/000-nad.yaml
• pkg/controller/egress_router/egress_router_controller.go
• pkg/controller/egress_router/egress_router_controller_test.go

---

Summary by CodeRabbit

• New Features
  • Egress router network attachments now support configurable MACVLAN interface modes.
  • MACVLAN master interfaces can be specified when required and are omitted when not configured.
  • MACVLAN mode values are normalized for consistent configuration.
• Bug Fixes
  • Improved generation of allowed destination rules, including protocol and optional target-port details.
• Tests
  • Added coverage for MACVLAN settings and destination rule rendering.

Walkthrough

The egress-router controller passes explicit MACVLAN mode and master values to the NAD template. The template renders CNI interface configuration with conditional master handling and lowercase mode output. Tests validate the rendered configuration and destination JSON.

Changes

MACVLAN NAD rendering

Layer / File(s)	Summary
Render MACVLAN configuration bindata/egress-router/000-nad.yaml, pkg/controller/egress_router/egress_router_controller.go	The controller supplies MACVLAN master and lowercase mode values, and the NAD template renders interfaceType and conditional interfaceArgs.
Validate rendered NAD output pkg/controller/egress_router/egress_router_controller_test.go	Rendering helpers and tests verify master inclusion or omission, lowercase modes, interface type, and allowed-destination JSON formatting.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: arghosh93, kyrtapz

---

Caution

Pre-merge checks failed

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

• Ignore

❌ Failed checks (3 errors, 1 warning)

Check name	Status	Explanation	Resolution
Title check	❌ Error	The title is relevant and imperative, but it exceeds the 72-character limit.	Shorten it to under 72 characters while keeping the component prefix and imperative verb, e.g. 'egress-router: Propagate macvlan master to NAD config'.
Pr Quality	❌ Error	The PR is well scoped and has Why/What/Testing/impact sections, but the visible PR text lacks the required bug/issue reference.	Add the Jira/bug ID (e.g. OCPBUGS-98724) to the PR title or description; the rest of the description can stay as-is.
E2e Tests For Feature Changes	❌ Error	The PR fixes user-facing macvlan behavior in pkg/ but adds no test/e2e/ changes.	Add or modify e2e coverage under test/e2e/ for this behavior change, or document why e2e is infeasible in the PR and justify the override.
Go And Test Code Quality	⚠️ Warning	ensureEgressRouter returns bare err twice after render/apply failures, omitting context in non-trivial production code.	Wrap those returns with context, e.g. return fmt.Errorf("render egress-router manifests: %w", err) and return fmt.Errorf("apply egress-router object: %w", err).

✅ Passed checks (20 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description directly matches the change and explains the macvlan and NAD updates.
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.
Commit Message Quality	✅ Passed	Single commit is a cohesive egress-router change with a concise scoped subject, explanatory body, and no merge commits.
Unit Tests For Go Changes	✅ Passed	Production Go and bindata template changes are accompanied by a new *_test.go file covering the behavior.
Rbac Least Privilege	✅ Passed	Only bindata/egress-router/000-nad.yaml changed; no ClusterRole/Role rules were added or modified.
Docs For Feature And Behavior Changes	✅ Passed	Bug fix only: it propagates macvlan master/mode into the NAD template, and no docs/ files were changed.
Stale Project Docs And Config	✅ Passed	Only implementation files changed; no docs/**, AGENTS.md, ARCHITECTURE.md, or .coderabbit.yaml paths/refs became stale.
Ai-Generated Code Smell	✅ Passed	PASS: The added tests are targeted to the macvlan/NAD changes, use a small table-driven suite, and I found no comment slop or AI/tool references.
Stable And Deterministic Test Names	✅ Passed	No Ginkgo titles were added; all test names are static, and the only subtest name uses fixed macvlan enum strings.
Test Structure And Quality	✅ Passed	PASS: These are plain unit tests, not Ginkgo; they use isolated helpers/TestMain, no cluster ops, and each test targets one behavior.
Microshift Test Compatibility	✅ Passed	The added tests are plain Go unit tests, not Ginkgo e2e tests, and they don't invoke MicroShift-unsupported OpenShift APIs or guarded cluster features.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	PASS: The added tests are plain Go unit tests, not Ginkgo e2e tests, and they only render manifests in-memory without any node/topology assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	Only NAD rendering and tests changed; no nodeSelector, affinity, topologySpreadConstraints, replicas, or PDB logic was added.
Ote Binary Stdout Contract	✅ Passed	PASS: The new TestMain only sets manifestDir and exits; no stdout writes were added in TestMain/init/suite setup, and the NAD/template changes are non-I/O.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the new tests are plain Go unit tests and make no external network calls.
No-Weak-Crypto	✅ Passed	Touched egress-router files only add macvlan template/data plumbing and tests; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons found.
Container-Privileges	✅ Passed	Changed files only add macvlan config to a NAD and controller/test data; no privileged, hostPID/Network/IPC, SYS_ADMIN, root, or allowPrivilegeEscalation settings were introduced.
No-Sensitive-Data-In-Logs	✅ Passed	No new logs expose secrets/PII; changes only add template fields and tests, while existing klog messages are generic lifecycle/error logs.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

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.12.2)

level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v1.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope

... [truncated 17377 characters] ...

red in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n"

---

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: Ultimate-etamitlU
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

[openshift-ci]

Hi @Ultimate-etamitlU. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[coderabbitai]

🧹 Nitpick comments (1)

pkg/controller/egress_router/egress_router_controller_test.go (1)

56-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace custom unstructuredNestedString with unstructured.NestedString.

This function reimplements unstructured.NestedString from k8s.io/apimachinery/pkg/apis/meta/v1/unstructured. Prefer using the well-known library function to reduce boilerplate and align with standard Kubernetes ecosystem patterns.

♻️ Proposed refactor

First, add the import for the unstructured package at the top of the file:

import (
	// ...
	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

Then, remove this custom function entirely and update its call site on line 41:

-			configStr, found, err := unstructuredNestedString(obj.Object, "spec", "config")
+			configStr, found, err := unstructured.NestedString(obj.Object, "spec", "config")

🤖 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/egress_router/egress_router_controller_test.go` around lines
56 - 77, Remove the custom unstructuredNestedString helper and import
k8s.io/apimachinery/pkg/apis/meta/v1/unstructured. Update its call site to use
unstructured.NestedString directly, preserving the existing nested-field lookup
behavior.

🤖 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.

Nitpick comments:
In `@pkg/controller/egress_router/egress_router_controller_test.go`:
- Around line 56-77: Remove the custom unstructuredNestedString helper and
import k8s.io/apimachinery/pkg/apis/meta/v1/unstructured. Update its call site
to use unstructured.NestedString directly, preserving the existing nested-field
lookup behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c6496fbe-1351-4b39-bcd9-66a25cc52aae

📥 Commits

Reviewing files that changed from the base of the PR and between 76e94f2 and 44a14ff.

📒 Files selected for processing (3)

• bindata/egress-router/000-nad.yaml
• pkg/controller/egress_router/egress_router_controller.go
• pkg/controller/egress_router/egress_router_controller_test.go

[Ultimate-etamitlU]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.