Skip to content

Read and write CNI file only once #2115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aauren merged 3 commits into from
Jul 6, 2026
+64 −78
Merged

Read and write CNI file only once #2115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a077a81
Don't export cniNetworkConfig struct fields
twz123 Jul 1, 2026
f6dc14f
Export cniNetworkConfig
twz123 Jul 1, 2026
89c5637
Read and write CNI file only once
twz123 Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 37 additions & 52 deletions pkg/controllers/routing/network_routes_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,10 @@ func (nrc *NetworkRoutingController) Run(
stopCh <-chan struct{},
wg *sync.WaitGroup,
) {
var err error
if nrc.enableCNI {
nrc.updateCNIConfig()
}
mtu, cniNetConf := nrc.initCNIConfig()

klog.V(1).Info("Populating ipsets.")
err = nrc.syncNodeIPSets()
err := nrc.syncNodeIPSets()
if err != nil {
klog.Errorf("Failed initial ipset setup: %s", err)
}
Expand Down Expand Up @@ -289,48 +286,33 @@ func (nrc *NetworkRoutingController) Run(
}
}

if nrc.autoMTU {
mtu, err := nrc.krNode.GetNodeMTU()
if mtu > 0 {
klog.Infof("Setting MTU of kube-bridge interface to: %d", mtu)
err = netlink.LinkSetMTU(kubeBridgeIf, mtu)
if err != nil {
klog.Errorf(
"Failed to find MTU for node IP: %s for intelligently setting the kube-bridge MTU "+
"due to %s.",
nrc.krNode.GetPrimaryNodeIP(),
"Failed to set MTU for kube-bridge interface due to: %s (kubeBridgeIf: %#v, mtu: %v)",
err.Error(),
kubeBridgeIf,
mtu,
)
}
if mtu > 0 {
klog.Infof("Setting MTU of kube-bridge interface to: %d", mtu)
err = netlink.LinkSetMTU(kubeBridgeIf, mtu)
if err != nil {
klog.Errorf(
"Failed to set MTU for kube-bridge interface due to: %s (kubeBridgeIf: %#v, mtu: %v)",
err.Error(),
kubeBridgeIf,
mtu,
// Update the CNI config to include the effective MTU, if any.
if currentMTU := kubeBridgeIf.Attrs().MTU; currentMTU > 0 && cniNetConf != nil && currentMTU != mtu {
klog.Warningf(
"Updating CNI config with current MTU for kube-bridge: %d",
currentMTU,
)
// need to correct kuberouter.conf because autoConfigureMTU() may have set an invalid value!
currentMTU := kubeBridgeIf.Attrs().MTU
if currentMTU > 0 && currentMTU != mtu {
klog.Warningf(
"Updating config file with current MTU for kube-bridge: %d",
currentMTU,
)
cniNetConf, err := utils.NewCNINetworkConfig(nrc.cniConfFile)
if err == nil {
cniNetConf.SetMTU(currentMTU)
if err = cniNetConf.WriteCNIConfig(); err != nil {
klog.Errorf("Failed to update CNI config file due to: %v", err)
}
} else {
klog.Errorf("Failed to load CNI config file to reset MTU due to: %v", err)
}
}
cniNetConf.SetMTU(currentMTU)
}
} else {
klog.Infof("Not setting MTU of kube-bridge interface")
}
}

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

@greptile-apps greptile-apps Bot Jul 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Fix in Claude Code Fix in Codex

@twz123 twz123 Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@twz123 twz123 Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See #2116.


// enable netfilter for the bridge
func() {
ctx, cancel := context.WithTimeout(context.Background(), maxModprobeTimeout)
Expand Down Expand Up @@ -460,11 +442,23 @@ func (nrc *NetworkRoutingController) Run(
}
}

func (nrc *NetworkRoutingController) updateCNIConfig() {
func (nrc *NetworkRoutingController) initCNIConfig() (mtu int, _ *utils.CNINetworkConfig) {
if nrc.autoMTU {
if nodeMTU, err := nrc.krNode.GetNodeMTU(); err != nil {
klog.Errorf("Not setting MTU of kube-bridge interface: cannot determine the node's MTU: %v", err)
} else {
mtu = nodeMTU
}
}

if !nrc.enableCNI {
return mtu, nil
}

// Parse the existing IPAM CIDRs from the CNI conf file
cniNetConf, err := utils.NewCNINetworkConfig(nrc.cniConfFile)
if err != nil {
klog.Errorf("failed to parse CNI Config: %v", err)
klog.Fatalf("failed to parse CNI Config: %v", err)
}

// Insert any IPv4 CIDRs that are missing from the IPAM configuration in the CNI
Expand All @@ -483,20 +477,11 @@ func (nrc *NetworkRoutingController) updateCNIConfig() {
}
}

if nrc.autoMTU {
// Get the MTU by looking at the node's interface that is associated with the primary IP of the cluster
mtu, err := nrc.krNode.GetNodeMTU()
if err != nil {
klog.Fatalf("failed to generate MTU: %v", err)
}

if mtu > 0 {
cniNetConf.SetMTU(mtu)
}

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

func (nrc *NetworkRoutingController) watchBgpUpdates() {
Expand Down
50 changes: 25 additions & 25 deletions pkg/utils/cni.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ import (
"strings"
)

type cniNetworkConfig struct {
FilePath string
Conf *Conf
ConfList *ConfList
type CNINetworkConfig struct {
filePath string
conf *Conf
confList *ConfList
}

func NewCNINetworkConfig(cniConfFilePath string) (*cniNetworkConfig, error) {
cniNetConf := cniNetworkConfig{
FilePath: cniConfFilePath,
func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) {
cniNetConf := CNINetworkConfig{
filePath: cniConfFilePath,
}

cniFileBytes, err := os.ReadFile(cniConfFilePath)
Expand All @@ -35,7 +35,7 @@ func NewCNINetworkConfig(cniConfFilePath string) (*cniNetworkConfig, error) {
if len(confList.Plugins) == 0 {
return nil, fmt.Errorf("CNI config list %s has no plugins", cniConfFilePath)
}
cniNetConf.ConfList = confList
cniNetConf.confList = confList
} else {
// If we're working with a conf setup
conf := new(Conf)
Expand All @@ -46,7 +46,7 @@ func NewCNINetworkConfig(cniConfFilePath string) (*cniNetworkConfig, error) {
if conf.Type == "" {
return nil, fmt.Errorf("error load CNI config, file appears to have no type: %s", cniConfFilePath)
}
cniNetConf.Conf = conf
cniNetConf.conf = conf
}

if err = cniNetConf.consolidateSubnets(); err != nil {
Expand All @@ -59,7 +59,7 @@ func NewCNINetworkConfig(cniConfFilePath string) (*cniNetworkConfig, error) {
// consolidateSubnets Many people still define the legacy single subnet variation of the IPAM plugin instead of the
// newer ranges variation. To account for this and make parsing simpler, we do the same thing that the official IPAM
// config loader does and collapse them into ranges.
func (c *cniNetworkConfig) consolidateSubnets() error {
func (c *CNINetworkConfig) consolidateSubnets() error {
brPlug := c.getBridgePlugin()
if brPlug.IPAM.Subnet != "" {
err := c.InsertPodCIDRIntoIPAM(brPlug.IPAM.Subnet)
Expand All @@ -75,12 +75,12 @@ func (c *cniNetworkConfig) consolidateSubnets() error {

// IsConfList checks to see if this CNI configuration is a *.conflist file or if it is a *.conf file. Returns true for
// *.conflist, returns false for anything else.
func (c *cniNetworkConfig) IsConfList() bool {
return strings.HasSuffix(strings.ToLower(c.FilePath), ".conflist")
func (c *CNINetworkConfig) IsConfList() bool {
return strings.HasSuffix(strings.ToLower(c.filePath), ".conflist")
}

// getPodCIDRsMapFromCNISpec gets pod CIDR allocated to the node as a map from CNI spec file and returns it
func (c *cniNetworkConfig) getPodCIDRsMapFromCNISpec() (map[string]*net.IPNet, error) {
func (c *CNINetworkConfig) getPodCIDRsMapFromCNISpec() (map[string]*net.IPNet, error) {
podCIDRs := make(map[string]*net.IPNet)

ipamConfig := c.getBridgePlugin().IPAM
Expand All @@ -93,7 +93,7 @@ func (c *cniNetworkConfig) getPodCIDRsMapFromCNISpec() (map[string]*net.IPNet, e
_, netCIDR, err := net.ParseCIDR(item.Subnet)
if err != nil {
return nil, fmt.Errorf("unable to parse CIDR '%s' contained in CNI: %s",
item.Subnet, c.FilePath)
item.Subnet, c.filePath)
}
podCIDRs[netCIDR.String()] = netCIDR
}
Expand All @@ -105,7 +105,7 @@ func (c *cniNetworkConfig) getPodCIDRsMapFromCNISpec() (map[string]*net.IPNet, e
}

// GetPodCIDRsFromCNISpec gets pod CIDR allocated to the node from CNI spec file and returns it
func (c *cniNetworkConfig) GetPodCIDRsFromCNISpec() ([]*net.IPNet, error) {
func (c *CNINetworkConfig) GetPodCIDRsFromCNISpec() ([]*net.IPNet, error) {
podCIDRMap, err := c.getPodCIDRsMapFromCNISpec()
if err != nil {
return nil, err
Expand All @@ -118,21 +118,21 @@ func (c *cniNetworkConfig) GetPodCIDRsFromCNISpec() ([]*net.IPNet, error) {
}

// getBridgePlugin get the bridge plugin configuration out of the cniNetworkConfig in a consistent manner
func (c *cniNetworkConfig) getBridgePlugin() *Conf {
if c.ConfList != nil {
for _, conf := range c.ConfList.Plugins {
func (c *CNINetworkConfig) getBridgePlugin() *Conf {
if c.confList != nil {
for _, conf := range c.confList.Plugins {
if conf.Type == "bridge" {
return conf
}
}
}
return c.Conf
return c.conf
}

// InsertPodCIDRIntoIPAM insert a new cidr into the CNI file. If the CIDR already exists in the CNI ranges, then
// operation is a noop. Throws an error if either the passed cidr cannot be parsed or if there is a problem with the
// CIDRs already in the CNI config.
func (c *cniNetworkConfig) InsertPodCIDRIntoIPAM(cidr string) error {
func (c *CNINetworkConfig) InsertPodCIDRIntoIPAM(cidr string) error {
ipamConfig := c.getBridgePlugin().IPAM

// This should have already been sanitized by the GetPodCIDR* functions before it comes to us, but you can never be
Expand All @@ -158,27 +158,27 @@ func (c *cniNetworkConfig) InsertPodCIDRIntoIPAM(cidr string) error {
return nil
}

func (c *cniNetworkConfig) SetMTU(mtu int) {
func (c *CNINetworkConfig) SetMTU(mtu int) {
brPlugin := c.getBridgePlugin()
brPlugin.MTU = float64(mtu)
}

func (c *cniNetworkConfig) WriteCNIConfig() error {
func (c *CNINetworkConfig) WriteCNIConfig() error {
var cniBytes []byte
var err error
if c.IsConfList() {
cniBytes, err = json.Marshal(c.ConfList)
cniBytes, err = json.Marshal(c.confList)
if err != nil {
return fmt.Errorf("unable to marshal CNI ConfList: %w", err)
}
} else {
cniBytes, err = json.Marshal(c.Conf)
cniBytes, err = json.Marshal(c.conf)
if err != nil {
return fmt.Errorf("unable to marshal CNI Conf: %w", err)
}
}

err = os.WriteFile(c.FilePath, cniBytes, 0644)
err = os.WriteFile(c.filePath, cniBytes, 0644)
if err != nil {
return fmt.Errorf("failed to write into CNI conf file: %w", err)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/utils/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,8 @@ func (n *LocalKRNode) GetNodeMTU() (int, error) {
}
}
}
return 0, errors.New("failed to find interface with specified node IP")

return 0, fmt.Errorf("primary node IP %s is not assigned to any interface", n.PrimaryIP)
}

// GetNodeIPAddrs returns all of the node's IP addresses (whether internal or external) as defined by the Kubernetes
Expand Down