Read and write CNI file only once by twz123 · Pull Request #2115 · cloudnativelabs/kube-router

twz123 · 2026-07-02T14:31:56Z

What type of PR is this?

cleanup
feature

What this PR does / why we need it:

Retain the parsed CNI config throughout NRC's Run method. Postpone writing the CNI file until the kube-bridge is set up. By this time, router will have collected all the required information, including the correct MTU. This ensures that the node's MTU is read only once and that the CNI configuration is read, parsed, and written only once.

Note that kube-router will no longer terminate when the node MTU cannot be determined and both --enable-cni and --auto-mtu are true. Instead, an error is logged, and the MTU won't be set. This makes the behavior consistent with when --enable-cni is false. Moreover, a malformed CNI file will cause kube-router to terminate with a proper message, instead of panicking due to a nil pointer access.

Which issue(s) this PR is related to:

Was AI used during the creation of this PR?

Yes, for local code review.

What, if any, amount of integration testing was done with this change in a Kubernetes environment?

None yet. I'll probably trigger a k0s CI run, but that requires me to build a custom kube-router OCI image first.

Does this PR introduce a breaking change?

I hope not. There is a difference in behavior, as described above.

NONE

Anything else the reviewer should know that wasn't already covered?

I'm currently trying to tackle the race window between when the init container puts the CNI template on the node's file system and when it actually becomes usable after kube-router has filled in the missing parts. This is an intermediate step.

That race window triggers all sorts of fun stuff 😅, like this:

bridge: reliably pin the MAC on a bridge that lost its ports containernetworking/plugins#1265

The rest of the codebase only uses the exported methods, and doesn't need to access those fields. Unexporting them makes it easier to reason about it. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>

It's used outside the utils package, so it makes sense to have a type that's nameable outside this package. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>

Retain the parsed CNI config throughout NRC's Run method. Postpone writing the CNI file until the kube-bridge is set up. By this time, router will have collected all the required information, including the correct MTU. This ensures that the node's MTU is read only once and that the CNI configuration is read, parsed, and written only once. Note that kube-router will no longer terminate when the node MTU cannot be determined and both --enable-cni and --auto-mtu are true. Instead, an error is logged, and the MTU won't be set. This makes the behavior consistent with when --enable-cni is false. Moreover, a malformed CNI file will cause kube-router to terminate with a proper message, instead of panicking due to a nil pointer access. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>

greptile-apps · 2026-07-02T14:40:23Z

Greptile Summary

This PR refactors the CNI/MTU initialization in NetworkRoutingController.Run to read the CNI config and node MTU exactly once, then defer writing the CNI file until after kube-bridge is set up. It also intentionally softens the fatal error on MTU lookup failure (changing it from klog.Fatalf to klog.Errorf) and upgrades the CNI parse failure from a silent nil-dereference panic to a proper klog.Fatalf.

updateCNIConfig() is replaced by initCNIConfig(), which returns (mtu, *CNINetworkConfig) and is called once at the top of Run; WriteCNIConfig is now called after the bridge is configured.
CNINetworkConfig is exported (with private fields) so the routing package can hold the parsed config across the lifecycle of Run.
GetNodeMTU's error message now includes the primary IP that was not found, aiding debugging.

Confidence Score: 4/5

Safe to merge; the changes are a clean refactoring with documented intentional behavior changes and no regressions on the critical paths.

The refactoring correctly defers CNI writes, eliminates duplicate MTU reads, and converts a nil-dereference panic on CNI parse failure into a clean termination. The only issues are a pre-existing copy-paste comment error carried over into the new function and a minor edge case where the CNI file could reflect a desired MTU that was never actually applied to the bridge.

pkg/controllers/routing/network_routes_controller.go — specifically the kube-bridge nil-interface handling around the MTU block and the unconditional CNI write that follows.

Important Files Changed

Filename	Overview
pkg/controllers/routing/network_routes_controller.go	Refactors CNI/MTU initialization into a new initCNIConfig helper; defers WriteCNIConfig until after kube-bridge is set up; eliminates double MTU lookup. Contains a pre-existing copy-paste comment error on the IPv6 loop carried over from updateCNIConfig.
pkg/utils/cni.go	Exports CNINetworkConfig type (previously cniNetworkConfig) while keeping struct fields private; no logic changes, straightforward visibility refactor.
pkg/utils/node.go	Improves GetNodeMTU error message to include the primary IP that was not found; no logic changes.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Run
    participant initCNIConfig
    participant GetNodeMTU
    participant NewCNINetworkConfig
    participant netlink
    participant WriteCNIConfig

    Run->>initCNIConfig: call at startup
    alt "autoMTU == true"
        initCNIConfig->>GetNodeMTU: get node MTU
        alt error
            GetNodeMTU-->>initCNIConfig: error
            initCNIConfig->>initCNIConfig: klog.Errorf (mtu stays 0)
        else success
            GetNodeMTU-->>initCNIConfig: "mtu > 0"
            initCNIConfig->>initCNIConfig: "mtu = nodeMTU"
        end
    end
    alt "enableCNI == true"
        initCNIConfig->>NewCNINetworkConfig: parse CNI file
        alt parse error
            NewCNINetworkConfig-->>initCNIConfig: error
            initCNIConfig->>initCNIConfig: klog.Fatalf (terminate)
        else success
            NewCNINetworkConfig-->>initCNIConfig: cniNetConf
            initCNIConfig->>initCNIConfig: InsertPodCIDRs
            alt "mtu > 0"
                initCNIConfig->>initCNIConfig: cniNetConf.SetMTU(mtu)
            end
        end
    end
    initCNIConfig-->>Run: (mtu, cniNetConf)

    Run->>Run: syncNodeIPSets, overlays, egress...
    Run->>netlink: create/find kube-bridge
    netlink-->>Run: kubeBridgeIf

    alt "mtu > 0"
        Run->>netlink: LinkSetMTU(kubeBridgeIf, mtu)
        alt "SetMTU error and currentMTU != mtu"
            Run->>Run: cniNetConf.SetMTU(currentMTU)
        end
    end

    alt "cniNetConf != nil"
        Run->>WriteCNIConfig: write once, after bridge is set up
    end

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Run
    participant initCNIConfig
    participant GetNodeMTU
    participant NewCNINetworkConfig
    participant netlink
    participant WriteCNIConfig

    Run->>initCNIConfig: call at startup
    alt "autoMTU == true"
        initCNIConfig->>GetNodeMTU: get node MTU
        alt error
            GetNodeMTU-->>initCNIConfig: error
            initCNIConfig->>initCNIConfig: klog.Errorf (mtu stays 0)
        else success
            GetNodeMTU-->>initCNIConfig: "mtu > 0"
            initCNIConfig->>initCNIConfig: "mtu = nodeMTU"
        end
    end
    alt "enableCNI == true"
        initCNIConfig->>NewCNINetworkConfig: parse CNI file
        alt parse error
            NewCNINetworkConfig-->>initCNIConfig: error
            initCNIConfig->>initCNIConfig: klog.Fatalf (terminate)
        else success
            NewCNINetworkConfig-->>initCNIConfig: cniNetConf
            initCNIConfig->>initCNIConfig: InsertPodCIDRs
            alt "mtu > 0"
                initCNIConfig->>initCNIConfig: cniNetConf.SetMTU(mtu)
            end
        end
    end
    initCNIConfig-->>Run: (mtu, cniNetConf)

    Run->>Run: syncNodeIPSets, overlays, egress...
    Run->>netlink: create/find kube-bridge
    netlink-->>Run: kubeBridgeIf

    alt "mtu > 0"
        Run->>netlink: LinkSetMTU(kubeBridgeIf, mtu)
        alt "SetMTU error and currentMTU != mtu"
            Run->>Run: cniNetConf.SetMTU(currentMTU)
        end
    end

    alt "cniNetConf != nil"
        Run->>WriteCNIConfig: write once, after bridge is set up
    end

Comments Outside Diff (1)

pkg/controllers/routing/network_routes_controller.go, line 472-473 (link)

Copy-paste error in the comment: the loop below iterates over IPv6 CIDRs (podIPv6CIDRs), not IPv4. This comment was carried over unchanged from the old updateCNIConfig function.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

_{Reviews (1): Last reviewed commit: "Read and write CNI file only once" | Re-trigger Greptile}

greptile-apps · 2026-07-02T14:40:27Z

+	if cniNetConf != nil {
+		if err := cniNetConf.WriteCNIConfig(); err != nil {
+			klog.Fatalf("failed to write CNI file: %v", err)
+		}
+	}


CNI written even when bridge setup fails

WriteCNIConfig is called unconditionally after the bridge-setup block, even if kubeBridgeIf was never successfully obtained (e.g., both the initial LinkByName and the creation-retry failed with errors). In that case, LinkSetMTU would have been called on a nil interface (likely panicking), but if it somehow returned an error instead, the MTU in the CNI config would still reflect the desired value rather than the actual bridge MTU. This is a pre-existing hazard, but the new unconditional write path makes the CNI file a source of truth that diverges from the actual bridge state on failure.

Indeed, noticed that myself, but decided not to fix it here. I might submit another PR for this.

aauren

Thanks @twz123

aauren · 2026-07-06T01:31:34Z

Just a small nit, in the future please match the repo's conventional commit messages standard

twz123 · 2026-07-07T16:12:12Z

Just a small nit, in the future please match the repo's conventional commit messages standard

Alright 🫡 (I usually don't do conventional commits and try to avoid them unless something or someone requests them 😬)

twz123 added 3 commits July 2, 2026 16:32

Don't export cniNetworkConfig struct fields

a077a81

The rest of the codebase only uses the exported methods, and doesn't need to access those fields. Unexporting them makes it easier to reason about it. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>

Export cniNetworkConfig

f6dc14f

It's used outside the utils package, so it makes sense to have a type that's nameable outside this package. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>

twz123 force-pushed the write-cni-file-once branch from 6473f19 to 89c5637 Compare July 2, 2026 14:33

greptile-apps Bot reviewed Jul 2, 2026

View reviewed changes

twz123 changed the title ~~Write cni file once~~ Read and write CNI file only once Jul 2, 2026

aauren approved these changes Jul 6, 2026

View reviewed changes

aauren merged commit 9f766c7 into cloudnativelabs:master Jul 6, 2026
9 checks passed

twz123 deleted the write-cni-file-once branch July 6, 2026 05:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Read and write CNI file only once - #2115

Read and write CNI file only once#2115
aauren merged 3 commits into
cloudnativelabs:masterfrom
twz123:write-cni-file-once

twz123 commented Jul 2, 2026

Uh oh!

greptile-apps Bot commented Jul 2, 2026 •

edited

Loading

Greptile Summary

Sequence Diagram

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Jul 2, 2026

Uh oh!

twz123 Jul 2, 2026

Uh oh!

twz123 Jul 3, 2026

Uh oh!

aauren left a comment

Uh oh!

Uh oh!

aauren commented Jul 6, 2026

Uh oh!

twz123 commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

twz123 commented Jul 2, 2026

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Was AI used during the creation of this PR?

What, if any, amount of integration testing was done with this change in a Kubernetes environment?

Does this PR introduce a breaking change?

Anything else the reviewer should know that wasn't already covered?

Uh oh!

greptile-apps Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Comments Outside Diff (1)

Uh oh!

greptile-apps Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

twz123 Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

twz123 Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

aauren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aauren commented Jul 6, 2026

Uh oh!

twz123 commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps Bot commented Jul 2, 2026 •

edited

Loading