From 1c8ce85dc3424290e0969179201142860086d26b Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 10:31:23 +0200 Subject: [PATCH 1/9] chore: Use temp dir facility from Go's test framework in the CNI config test Signed-off-by: Tom Wieczorek --- pkg/utils/cni_test.go | 62 +++++++++++-------------------------------- 1 file changed, 16 insertions(+), 46 deletions(-) diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index c65d1c5bf..2f04e47df 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -5,11 +5,12 @@ import ( "errors" "fmt" "os" - "path" + "path/filepath" "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func getConfList() []byte { @@ -374,13 +375,10 @@ func TestNewCNINetworkConfig(t *testing.T) { } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - file, tmpDir, err := createFile(testcase.content, testcase.filename) - if err != nil { - t.Fatalf("Failed to create temporary CNI config file: %v", err) - } - defer os.RemoveAll(tmpDir) + confFilePath := filepath.Join(t.TempDir(), testcase.filename) + require.NoError(t, os.WriteFile(confFilePath, testcase.content, 0600)) - cni, err := NewCNINetworkConfig(file.Name()) + cni, err := NewCNINetworkConfig(confFilePath) if err != nil { if testcase.err == nil { assert.Fail(t, "if error from NewCNINetworkConfig is not nil, the testcase shouldn't be "+ @@ -468,13 +466,10 @@ func TestCniNetworkConfig_GetPodCIDRsFromCNISpec(t *testing.T) { } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - file, tmpDir, err := createFile(testcase.content, testcase.filename) - if err != nil { - t.Fatalf("Failed to create temporary CNI config file: %v", err) - } - defer os.RemoveAll(tmpDir) + confFilePath := filepath.Join(t.TempDir(), testcase.filename) + require.NoError(t, os.WriteFile(confFilePath, testcase.content, 0600)) - cni, err := NewCNINetworkConfig(file.Name()) + cni, err := NewCNINetworkConfig(confFilePath) assert.Equal(t, testcase.err, err) if err != nil { return @@ -578,13 +573,10 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - file, tmpDir, err := createFile(testcase.content, testcase.filename) - if err != nil { - t.Fatalf("Failed to create temporary CNI config file: %v", err) - } - defer os.RemoveAll(tmpDir) + confFilePath := filepath.Join(t.TempDir(), testcase.filename) + require.NoError(t, os.WriteFile(confFilePath, testcase.content, 0600)) - cni, err := NewCNINetworkConfig(file.Name()) + cni, err := NewCNINetworkConfig(confFilePath) if err != nil { assert.Fail(t, "err should always be nil when calling NewCNINetworkConfig for this suite") } @@ -665,13 +657,10 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - file, tmpDir, err := createFile(testcase.content, testcase.filename) - if err != nil { - t.Fatalf("Failed to create temporary CNI config file: %v", err) - } - defer os.RemoveAll(tmpDir) + confFilePath := filepath.Join(t.TempDir(), testcase.filename) + require.NoError(t, os.WriteFile(confFilePath, testcase.content, 0600)) - cni, err := NewCNINetworkConfig(file.Name()) + cni, err := NewCNINetworkConfig(confFilePath) if err != nil { assert.Fail(t, "err should always be nil when calling NewCNINetworkConfig for this suite") } @@ -690,7 +679,7 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { // Read the CNI directly to ensure that subnet is really removed (which wouldn't be detected upon // re-initialization of NewCNINetworkConfig below because of how it treats subnets - cniFileBytes, err := os.ReadFile(file.Name()) + cniFileBytes, err := os.ReadFile(confFilePath) if err != nil { t.Fatalf("we should be able to read the CNI file we just wrote to") } @@ -721,7 +710,7 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { assert.Emptyf(t, brPlug.IPAM.Subnet, "upon calling WriteCNIConfig() subnet should ALWAYS be blank "+ "because it should have been consolidated with ranges") - cni, err = NewCNINetworkConfig(file.Name()) + cni, err = NewCNINetworkConfig(confFilePath) if err != nil { assert.Fail(t, "err should always be nil when calling NewCNINetworkConfig for this suite") } @@ -753,22 +742,3 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { }) } } - -func createFile(content []byte, filename string) (*os.File, string, error) { - dir, err := os.MkdirTemp("", "kube-router-cni-test") - if err != nil { - return nil, "", fmt.Errorf("cannot create tmpdir: %v", err) - } - fullPath := path.Join(dir, filename) - file, err := os.Create(fullPath) - if err != nil { - return nil, "", fmt.Errorf("cannot create file: %v", err) - } - - if _, err = file.Write(content); err != nil { - return nil, "", fmt.Errorf("cannot write to file: %v", err) - } - - fmt.Println("File is ", file.Name()) - return file, dir, nil -} From 640809b9ef1c08fb51b4ae6ae2571b059fd47f3d Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 10:34:47 +0200 Subject: [PATCH 2/9] chore: Replace tabs with spaces in test CNI configs Signed-off-by: Tom Wieczorek --- pkg/utils/cni_test.go | 60 +++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index 2f04e47df..cc1583313 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -46,22 +46,22 @@ func getConfListWithRanges() []byte { "ipam":{ "ranges": [ [ - { + { "subnet":"10.242.0.0/24" - }, - { + }, + { "subnet":"10.242.1.0/24" - } + } ], [ - { + { "subnet":"10.242.2.0/24" - }, - { + }, + { "subnet":"10.242.3.0/24" - } + } ] - ], + ], "subnet": "10.242.4.0/24", "type":"host-local" }, @@ -86,22 +86,22 @@ func getConfListWithDuplicateRanges() []byte { "ipam":{ "ranges": [ [ - { + { "subnet":"10.242.0.0/24" - }, - { + }, + { "subnet":"10.242.1.0/24" - } + } ], [ - { + { "subnet":"10.242.2.0/24" - }, - { + }, + { "subnet":"10.242.3.0/24" - } + } ] - ], + ], "subnet": "10.242.0.0/24", "type":"host-local" }, @@ -126,22 +126,22 @@ func getConfListWithIPv6DuplicateRanges() []byte { "ipam":{ "ranges": [ [ - { + { "subnet":"10.242.0.0/24" - }, - { + }, + { "subnet":"10.242.1.0/24" - } + } ], [ - { + { "subnet":"10.242.2.0/24" - }, - { + }, + { "subnet":"2001:db8:42:2::/64" - } + } ] - ], + ], "subnet": "2001:db8:42:2::/64", "type":"host-local" }, @@ -191,7 +191,7 @@ func getConf() []byte { "name":"mynet", "bridge":"kube-bridge", "ipam":{ - "type":"host-local", + "type":"host-local", "subnet": "10.242.0.0/24" }, "isDefaultGateway":true, @@ -208,7 +208,7 @@ func getConfWithNoSubnet() []byte { "name":"mynet", "bridge":"kube-bridge", "ipam":{ - "type":"host-local" + "type":"host-local" }, "isDefaultGateway":true, "name":"kubernetes", @@ -224,7 +224,7 @@ func getConfWithNoType() []byte { "name":"mynet", "bridge":"kube-bridge", "ipam":{ - "type":"host-local" + "type":"host-local" }, "isDefaultGateway":true, "name":"kubernetes" From bffa4a3c9f53e5963a18aae286985ce0abc538b6 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 11:26:03 +0200 Subject: [PATCH 3/9] chore: Introduce cniConfContent for CNI config tests This couples the content to its type, and eliminates quite a few extra fields from the test tables. Signed-off-by: Tom Wieczorek --- pkg/utils/cni_test.go | 329 ++++++++++++++++++------------------------ 1 file changed, 140 insertions(+), 189 deletions(-) diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index cc1583313..22463c2e4 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -13,9 +13,25 @@ import ( "github.com/stretchr/testify/require" ) -func getConfList() []byte { - return []byte(` -{ +const ( + cniConfTestFileName = "10-kuberouter.conf" + cniConfListTestFileName = "10-kuberouter.conflist" +) + +type cniConfContent struct { + isConfList bool + bytes []byte +} + +func (c *cniConfContent) fileName() string { + if c.isConfList { + return cniConfListTestFileName + } + return cniConfTestFileName +} + +func getConfList() cniConfContent { + return cniConfContent{true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -32,12 +48,11 @@ func getConfList() []byte { } ] } -`) +`)} } -func getConfListWithRanges() []byte { - return []byte(` -{ +func getConfListWithRanges() cniConfContent { + return cniConfContent{true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -72,12 +87,11 @@ func getConfListWithRanges() []byte { } ] } -`) +`)} } -func getConfListWithDuplicateRanges() []byte { - return []byte(` -{ +func getConfListWithDuplicateRanges() cniConfContent { + return cniConfContent{true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -112,12 +126,11 @@ func getConfListWithDuplicateRanges() []byte { } ] } -`) +`)} } -func getConfListWithIPv6DuplicateRanges() []byte { - return []byte(` -{ +func getConfListWithIPv6DuplicateRanges() cniConfContent { + return cniConfContent{true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -152,12 +165,11 @@ func getConfListWithIPv6DuplicateRanges() []byte { } ] } -`) +`)} } -func getConfListWithNoSubnet() []byte { - return []byte(` -{ +func getConfListWithNoSubnet() cniConfContent { + return cniConfContent{true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -172,38 +184,34 @@ func getConfListWithNoSubnet() []byte { } ] } -`) +`)} } -func getConfListWithNoPlugins() []byte { - return []byte(` -{ +func getConfListWithNoPlugins() cniConfContent { + return cniConfContent{true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet" -} -`) +}`)} } -func getConf() []byte { - return []byte(` -{ +func getConf() cniConfContent { + return cniConfContent{false, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "bridge":"kube-bridge", "ipam":{ - "type":"host-local", + "type":"host-local", "subnet": "10.242.0.0/24" }, "isDefaultGateway":true, "name":"kubernetes", "type":"bridge" } -`) +`)} } -func getConfWithNoSubnet() []byte { - return []byte(` -{ +func getConfWithNoSubnet() cniConfContent { + return cniConfContent{false, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "bridge":"kube-bridge", @@ -214,12 +222,11 @@ func getConfWithNoSubnet() []byte { "name":"kubernetes", "type":"bridge" } -`) +`)} } -func getConfWithNoType() []byte { - return []byte(` -{ +func getConfWithNoType() cniConfContent { + return cniConfContent{false, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "bridge":"kube-bridge", @@ -229,7 +236,7 @@ func getConfWithNoType() []byte { "isDefaultGateway":true, "name":"kubernetes" } -`) +`)} } func TestMarshalUnmarshalRestoration(t *testing.T) { @@ -237,146 +244,128 @@ func TestMarshalUnmarshalRestoration(t *testing.T) { before := getConfList() cl := new(ConfList) - err := json.Unmarshal(before, cl) + err := json.Unmarshal(before.bytes, cl) if err != nil { - t.Fatalf("wasn't able to unmarshal JSON in test: %s", before) + t.Fatalf("wasn't able to unmarshal JSON in test: %s", before.bytes) } after, err := json.MarshalIndent(cl, "", " ") if err != nil { - t.Fatalf("wasn't able to marshal JSON in test: %s", before) + t.Fatalf("wasn't able to marshal JSON in test: %s", before.bytes) } - assert.JSONEqf(t, string(before), string(after), + assert.JSONEqf(t, string(before.bytes), string(after), "JSON is not equal!\nBefore:\n%s\nAfter:\n%s\n", before, after) }) t.Run("Ensure ConfListWithRange is parsed and unparsed properly", func(t *testing.T) { before := getConfListWithRanges() cl := new(ConfList) - err := json.Unmarshal(before, cl) + err := json.Unmarshal(before.bytes, cl) if err != nil { - t.Fatalf("wasn't able to unmarshal JSON in test: %s", before) + t.Fatalf("wasn't able to unmarshal JSON in test: %s", before.bytes) } after, err := json.MarshalIndent(cl, "", " ") if err != nil { - t.Fatalf("wasn't able to marshal JSON in test: %s", before) + t.Fatalf("wasn't able to marshal JSON in test: %s", before.bytes) } - assert.JSONEqf(t, string(before), string(after), + assert.JSONEqf(t, string(before.bytes), string(after), "JSON is not equal!\nBefore:\n%s\nAfter:\n%s\n", before, after) }) t.Run("Ensure ConfListWithNoSubnet is parsed and unparsed properly", func(t *testing.T) { before := getConfListWithNoSubnet() cl := new(ConfList) - err := json.Unmarshal(before, cl) + err := json.Unmarshal(before.bytes, cl) if err != nil { - t.Fatalf("wasn't able to unmarshal JSON in test: %s", before) + t.Fatalf("wasn't able to unmarshal JSON in test: %s", before.bytes) } after, err := json.MarshalIndent(cl, "", " ") if err != nil { - t.Fatalf("wasn't able to marshal JSON in test: %s", before) + t.Fatalf("wasn't able to marshal JSON in test: %s", before.bytes) } - assert.JSONEqf(t, string(before), string(after), + assert.JSONEqf(t, string(before.bytes), string(after), "JSON is not equal!\nBefore:\n%s\nAfter:\n%s\n", before, after) }) t.Run("Ensure ConfWithNoSubnet is parsed and unparsed properly", func(t *testing.T) { before := getConfWithNoSubnet() c := new(Conf) - err := json.Unmarshal(before, c) + err := json.Unmarshal(before.bytes, c) if err != nil { - t.Fatalf("wasn't able to unmarshal JSON in test: %s", before) + t.Fatalf("wasn't able to unmarshal JSON in test: %s", before.bytes) } after, err := json.MarshalIndent(c, "", " ") if err != nil { - t.Fatalf("wasn't able to marshal JSON in test: %s", before) + t.Fatalf("wasn't able to marshal JSON in test: %s", before.bytes) } - assert.JSONEqf(t, string(before), string(after), + assert.JSONEqf(t, string(before.bytes), string(after), "JSON is not equal!\nBefore:\n%s\nAfter:\n%s\n", before, after) }) } func TestNewCNINetworkConfig(t *testing.T) { testcases := []struct { - name string - filename string - isConfList bool - content []byte - err error - ranges []string + name string + content cniConfContent + err error + ranges []string }{ { - name: "Attempt reading from conf", - filename: "10-kuberouter.conf", - isConfList: false, - content: getConf(), - err: nil, + name: "Attempt reading from conf", + content: getConf(), + err: nil, }, { - name: "Attempt reading from conflist", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfList(), - err: nil, + name: "Attempt reading from conflist", + content: getConfList(), + err: nil, }, { - name: "Ensure error upon reading from conf with no type", - filename: "10-kuberouter.conf", - isConfList: false, - content: getConfWithNoType(), - err: errors.New("error load CNI config, file appears to have no type: "), + name: "Ensure error upon reading from conf with no type", + content: getConfWithNoType(), + err: errors.New("error load CNI config, file appears to have no type: "), }, { - name: "Ensure error upon reading from conflist with no plugins", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithNoPlugins(), - err: errors.New("CNI config list "), + name: "Ensure error upon reading from conflist with no plugins", + content: getConfListWithNoPlugins(), + err: errors.New("CNI config list "), }, { - name: "Ensure conf subnet get consolidated into ranges when only subnet exists", - filename: "10-kuberouter.conf", - isConfList: false, - content: getConf(), - err: nil, - ranges: []string{"10.242.0.0/24"}, + name: "Ensure conf subnet get consolidated into ranges when only subnet exists", + content: getConf(), + err: nil, + ranges: []string{"10.242.0.0/24"}, }, { - name: "Ensure conflist subnet get consolidated into ranges when only subnet exists", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfList(), - err: nil, - ranges: []string{"10.242.0.0/24"}, + name: "Ensure conflist subnet get consolidated into ranges when only subnet exists", + content: getConfList(), + err: nil, + ranges: []string{"10.242.0.0/24"}, }, { - name: "Ensure conflist subnets get consolidated with ranges when both exist", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithRanges(), - err: nil, - ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24"}, + name: "Ensure conflist subnets get consolidated with ranges when both exist", + content: getConfListWithRanges(), + err: nil, + ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24"}, }, { - name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithDuplicateRanges(), - err: nil, - ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, + name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", + content: getConfListWithDuplicateRanges(), + err: nil, + ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, }, } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - confFilePath := filepath.Join(t.TempDir(), testcase.filename) - require.NoError(t, os.WriteFile(confFilePath, testcase.content, 0600)) + confFilePath := filepath.Join(t.TempDir(), testcase.content.fileName()) + require.NoError(t, os.WriteFile(confFilePath, testcase.content.bytes, 0600)) cni, err := NewCNINetworkConfig(confFilePath) if err != nil { @@ -388,7 +377,7 @@ func TestNewCNINetworkConfig(t *testing.T) { return } - assert.Equal(t, testcase.isConfList, cni.IsConfList()) + assert.Equal(t, testcase.content.isConfList, cni.IsConfList()) if testcase.ranges != nil { assert.Emptyf(t, cni.getBridgePlugin().IPAM.Subnet, @@ -416,58 +405,46 @@ func TestNewCNINetworkConfig(t *testing.T) { func TestCniNetworkConfig_GetPodCIDRsFromCNISpec(t *testing.T) { testcases := []struct { - name string - filename string - isConfList bool - content []byte - err error - ranges []string + name string + content cniConfContent + err error + ranges []string }{ { - name: "Ensure conf subnet get consolidated into ranges when only subnet exists", - filename: "10-kuberouter.conf", - isConfList: false, - content: getConf(), - err: nil, - ranges: []string{"10.242.0.0/24"}, + name: "Ensure conf subnet get consolidated into ranges when only subnet exists", + content: getConf(), + err: nil, + ranges: []string{"10.242.0.0/24"}, }, { - name: "Ensure conflist subnet get consolidated into ranges when only subnet exists", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfList(), - err: nil, - ranges: []string{"10.242.0.0/24"}, + name: "Ensure conflist subnet get consolidated into ranges when only subnet exists", + content: getConfList(), + err: nil, + ranges: []string{"10.242.0.0/24"}, }, { - name: "Ensure conflist subnets get consolidated with ranges when both exist", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithRanges(), - err: nil, - ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24"}, + name: "Ensure conflist subnets get consolidated with ranges when both exist", + content: getConfListWithRanges(), + err: nil, + ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24"}, }, { - name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithDuplicateRanges(), - err: nil, - ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, + name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", + content: getConfListWithDuplicateRanges(), + err: nil, + ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, }, { - name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithIPv6DuplicateRanges(), - err: nil, - ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "2001:db8:42:2::/64"}, + name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", + content: getConfListWithIPv6DuplicateRanges(), + err: nil, + ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "2001:db8:42:2::/64"}, }, } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - confFilePath := filepath.Join(t.TempDir(), testcase.filename) - require.NoError(t, os.WriteFile(confFilePath, testcase.content, 0600)) + confFilePath := filepath.Join(t.TempDir(), testcase.content.fileName()) + require.NoError(t, os.WriteFile(confFilePath, testcase.content.bytes, 0600)) cni, err := NewCNINetworkConfig(confFilePath) assert.Equal(t, testcase.err, err) @@ -475,7 +452,7 @@ func TestCniNetworkConfig_GetPodCIDRsFromCNISpec(t *testing.T) { return } - assert.Equal(t, testcase.isConfList, cni.IsConfList()) + assert.Equal(t, testcase.content.isConfList, cni.IsConfList()) if testcase.ranges != nil { assert.Emptyf(t, cni.getBridgePlugin().IPAM.Subnet, @@ -506,17 +483,13 @@ func TestCniNetworkConfig_GetPodCIDRsFromCNISpec(t *testing.T) { func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { testcases := []struct { name string - filename string - isConfList bool - content []byte + content cniConfContent err error ranges []string insertRanges []string }{ { name: "Ensure passed CIDR is properly inserted into a CNI conf with no subnets defined", - filename: "10-kuberouter.conf", - isConfList: false, content: getConfWithNoSubnet(), err: nil, ranges: []string{"10.242.0.0/24"}, @@ -524,8 +497,6 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { }, { name: "Ensure multiple CIDRs are properly inserted into a CNI conf with no subnets defined", - filename: "10-kuberouter.conflist", - isConfList: true, content: getConfListWithNoSubnet(), err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24"}, @@ -534,37 +505,29 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { { name: "Ensure multiple IPv4 & IPv6 CIDRs are properly inserted into a CNI conf with no subnets" + "defined", - filename: "10-kuberouter.conflist", - isConfList: true, content: getConfListWithNoSubnet(), err: nil, ranges: []string{"10.242.0.0/24", "2001:db8:42:2::/64"}, insertRanges: []string{"10.242.0.0/24", "2001:db8:42:2::/64"}, }, { - name: "Ensure that new subnets are inserted into a conflist with existing ranges", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithRanges(), - err: nil, + name: "Ensure that new subnets are inserted into a conflist with existing ranges", + content: getConfListWithRanges(), + err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24", "10.242.5.0/24", "10.242.6.0/24"}, insertRanges: []string{"10.242.5.0/24", "10.242.6.0/24"}, }, { name: "Ensure duplicates are not inserted without error", - filename: "10-kuberouter.conflist", - isConfList: true, content: getConfListWithDuplicateRanges(), err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24"}, insertRanges: []string{"10.242.4.0/24"}, }, { - name: "Ensure error is thrown for bad cidr", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithDuplicateRanges(), + name: "Ensure error is thrown for bad cidr", + content: getConfListWithDuplicateRanges(), err: fmt.Errorf("unable to parse input cidr: %s - %s", "10.242.4.0", "invalid CIDR address: 10.242.4.0"), ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, @@ -573,8 +536,8 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - confFilePath := filepath.Join(t.TempDir(), testcase.filename) - require.NoError(t, os.WriteFile(confFilePath, testcase.content, 0600)) + confFilePath := filepath.Join(t.TempDir(), testcase.content.fileName()) + require.NoError(t, os.WriteFile(confFilePath, testcase.content.bytes, 0600)) cni, err := NewCNINetworkConfig(confFilePath) if err != nil { @@ -603,24 +566,18 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { testcases := []struct { name string - filename string - isConfList bool - content []byte + content cniConfContent err error ranges []string insertRanges []string }{ { - name: "Ensure written file is the same as read file when no ranges were inserted", - filename: "10-kuberouter.conf", - isConfList: false, - content: getConfWithNoSubnet(), - err: nil, + name: "Ensure written file is the same as read file when no ranges were inserted", + content: getConfWithNoSubnet(), + err: nil, }, { name: "Ensure written conf file contains single subnet", - filename: "10-kuberouter.conf", - isConfList: false, content: getConf(), err: nil, ranges: []string{"10.242.0.0/24"}, @@ -628,8 +585,6 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { }, { name: "Ensure written conflist file contains multiple subnets", - filename: "10-kuberouter.conflist", - isConfList: true, content: getConfListWithNoSubnet(), err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24"}, @@ -637,19 +592,15 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { }, { name: "Ensure written conflist file has IPv4 & IPv6 CIDRs properly inserted", - filename: "10-kuberouter.conflist", - isConfList: true, content: getConfListWithNoSubnet(), err: nil, ranges: []string{"10.242.0.0/24", "2001:db8:42:2::/64"}, insertRanges: []string{"10.242.0.0/24", "2001:db8:42:2::/64"}, }, { - name: "Ensure that conflist file has multiple subnets written when ranges already exist", - filename: "10-kuberouter.conflist", - isConfList: true, - content: getConfListWithRanges(), - err: nil, + name: "Ensure that conflist file has multiple subnets written when ranges already exist", + content: getConfListWithRanges(), + err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24", "10.242.5.0/24", "10.242.6.0/24"}, insertRanges: []string{"10.242.5.0/24", "10.242.6.0/24"}, @@ -657,8 +608,8 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - confFilePath := filepath.Join(t.TempDir(), testcase.filename) - require.NoError(t, os.WriteFile(confFilePath, testcase.content, 0600)) + confFilePath := filepath.Join(t.TempDir(), testcase.content.fileName()) + require.NoError(t, os.WriteFile(confFilePath, testcase.content.bytes, 0600)) cni, err := NewCNINetworkConfig(confFilePath) if err != nil { From ecb67909010f72b7785f758f30d8eb5477a52ce2 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 11:38:32 +0200 Subject: [PATCH 4/9] chore: Rollup the CNI config marshalling roundtrip test Signed-off-by: Tom Wieczorek --- pkg/utils/cni_test.go | 119 +++++++++++++++--------------------------- 1 file changed, 42 insertions(+), 77 deletions(-) diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index 22463c2e4..12bc0b7cc 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -19,6 +19,7 @@ const ( ) type cniConfContent struct { + name string isConfList bool bytes []byte } @@ -31,7 +32,7 @@ func (c *cniConfContent) fileName() string { } func getConfList() cniConfContent { - return cniConfContent{true, []byte(`{ + return cniConfContent{"confList", true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -52,7 +53,7 @@ func getConfList() cniConfContent { } func getConfListWithRanges() cniConfContent { - return cniConfContent{true, []byte(`{ + return cniConfContent{"confListWithRanges", true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -91,7 +92,7 @@ func getConfListWithRanges() cniConfContent { } func getConfListWithDuplicateRanges() cniConfContent { - return cniConfContent{true, []byte(`{ + return cniConfContent{"confListWithDuplicateRanges", true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -130,7 +131,7 @@ func getConfListWithDuplicateRanges() cniConfContent { } func getConfListWithIPv6DuplicateRanges() cniConfContent { - return cniConfContent{true, []byte(`{ + return cniConfContent{"confListWithIPv6DuplicateRanges", true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -169,7 +170,7 @@ func getConfListWithIPv6DuplicateRanges() cniConfContent { } func getConfListWithNoSubnet() cniConfContent { - return cniConfContent{true, []byte(`{ + return cniConfContent{"confListWithNoSubnet", true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "plugins":[ @@ -188,14 +189,14 @@ func getConfListWithNoSubnet() cniConfContent { } func getConfListWithNoPlugins() cniConfContent { - return cniConfContent{true, []byte(`{ + return cniConfContent{"confListWithNoPlugins", true, []byte(`{ "cniVersion":"0.3.0", "name":"mynet" }`)} } func getConf() cniConfContent { - return cniConfContent{false, []byte(`{ + return cniConfContent{"conf", false, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "bridge":"kube-bridge", @@ -211,7 +212,7 @@ func getConf() cniConfContent { } func getConfWithNoSubnet() cniConfContent { - return cniConfContent{false, []byte(`{ + return cniConfContent{"confWithNoSubnet", false, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "bridge":"kube-bridge", @@ -226,7 +227,7 @@ func getConfWithNoSubnet() cniConfContent { } func getConfWithNoType() cniConfContent { - return cniConfContent{false, []byte(`{ + return cniConfContent{"confWithNoType", false, []byte(`{ "cniVersion":"0.3.0", "name":"mynet", "bridge":"kube-bridge", @@ -239,75 +240,39 @@ func getConfWithNoType() cniConfContent { `)} } +func allCNIConfContents() []cniConfContent { + return []cniConfContent{ + getConfList(), + getConfListWithRanges(), + getConfListWithDuplicateRanges(), + getConfListWithIPv6DuplicateRanges(), + getConfListWithNoSubnet(), + getConfListWithNoPlugins(), + getConf(), + getConfWithNoSubnet(), + getConfWithNoType(), + } +} + func TestMarshalUnmarshalRestoration(t *testing.T) { - t.Run("Ensure ConfList is parsed and unparsed properly", func(t *testing.T) { - before := getConfList() - cl := new(ConfList) - - err := json.Unmarshal(before.bytes, cl) - if err != nil { - t.Fatalf("wasn't able to unmarshal JSON in test: %s", before.bytes) - } - - after, err := json.MarshalIndent(cl, "", " ") - if err != nil { - t.Fatalf("wasn't able to marshal JSON in test: %s", before.bytes) - } - - assert.JSONEqf(t, string(before.bytes), string(after), - "JSON is not equal!\nBefore:\n%s\nAfter:\n%s\n", before, after) - }) - t.Run("Ensure ConfListWithRange is parsed and unparsed properly", func(t *testing.T) { - before := getConfListWithRanges() - cl := new(ConfList) - - err := json.Unmarshal(before.bytes, cl) - if err != nil { - t.Fatalf("wasn't able to unmarshal JSON in test: %s", before.bytes) - } - - after, err := json.MarshalIndent(cl, "", " ") - if err != nil { - t.Fatalf("wasn't able to marshal JSON in test: %s", before.bytes) - } - - assert.JSONEqf(t, string(before.bytes), string(after), - "JSON is not equal!\nBefore:\n%s\nAfter:\n%s\n", before, after) - }) - t.Run("Ensure ConfListWithNoSubnet is parsed and unparsed properly", func(t *testing.T) { - before := getConfListWithNoSubnet() - cl := new(ConfList) - - err := json.Unmarshal(before.bytes, cl) - if err != nil { - t.Fatalf("wasn't able to unmarshal JSON in test: %s", before.bytes) - } - - after, err := json.MarshalIndent(cl, "", " ") - if err != nil { - t.Fatalf("wasn't able to marshal JSON in test: %s", before.bytes) - } - - assert.JSONEqf(t, string(before.bytes), string(after), - "JSON is not equal!\nBefore:\n%s\nAfter:\n%s\n", before, after) - }) - t.Run("Ensure ConfWithNoSubnet is parsed and unparsed properly", func(t *testing.T) { - before := getConfWithNoSubnet() - c := new(Conf) - - err := json.Unmarshal(before.bytes, c) - if err != nil { - t.Fatalf("wasn't able to unmarshal JSON in test: %s", before.bytes) - } - - after, err := json.MarshalIndent(c, "", " ") - if err != nil { - t.Fatalf("wasn't able to marshal JSON in test: %s", before.bytes) - } - - assert.JSONEqf(t, string(before.bytes), string(after), - "JSON is not equal!\nBefore:\n%s\nAfter:\n%s\n", before, after) - }) + for _, content := range allCNIConfContents() { + t.Run(content.name, func(t *testing.T) { + t.Parallel() + + var obj any + if content.isConfList { + obj = new(ConfList) + } else { + obj = new(Conf) + } + + require.NoError(t, json.Unmarshal(content.bytes, obj)) + after, err := json.Marshal(obj) + require.NoError(t, err) + + assert.JSONEq(t, string(content.bytes), string(after)) + }) + } } func TestNewCNINetworkConfig(t *testing.T) { From cb11e1c590ed4d46d3b6822fbf670fb0ea346503 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 12:39:02 +0200 Subject: [PATCH 5/9] chore: Deprecate CNINetworkConfig's IsConfList The check if the config needs to be parsed as conflist or as conf can be done once during struct initialization. The struct can then simply check if the confList field is non-nil. That's an implementation detail to support (un-)marshalling and shouldn't be relevant to struct users. Signed-off-by: Tom Wieczorek --- pkg/utils/cni.go | 11 ++++++----- pkg/utils/cni_test.go | 14 +++++++++++--- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/pkg/utils/cni.go b/pkg/utils/cni.go index a93f9e724..673e72f52 100644 --- a/pkg/utils/cni.go +++ b/pkg/utils/cni.go @@ -26,7 +26,7 @@ func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) { } // If we're working with a conflist setup - if cniNetConf.IsConfList() { + if strings.HasSuffix(strings.ToLower(cniConfFilePath), ".conflist") { confList := new(ConfList) err = json.Unmarshal(cniFileBytes, confList) if err != nil { @@ -73,10 +73,11 @@ func (c *CNINetworkConfig) consolidateSubnets() error { return nil } -// 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. +// IsConfList checks to see if this CNI configuration is a conflist or a conf. +// +// Deprecated: This is internal state. func (c *CNINetworkConfig) IsConfList() bool { - return strings.HasSuffix(strings.ToLower(c.filePath), ".conflist") + return c.confList != nil } // getPodCIDRsMapFromCNISpec gets pod CIDR allocated to the node as a map from CNI spec file and returns it @@ -166,7 +167,7 @@ func (c *CNINetworkConfig) SetMTU(mtu int) { func (c *CNINetworkConfig) WriteCNIConfig() error { var cniBytes []byte var err error - if c.IsConfList() { + if c.confList != nil { cniBytes, err = json.Marshal(c.confList) if err != nil { return fmt.Errorf("unable to marshal CNI ConfList: %w", err) diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index 12bc0b7cc..99703d0d8 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -342,7 +342,11 @@ func TestNewCNINetworkConfig(t *testing.T) { return } - assert.Equal(t, testcase.content.isConfList, cni.IsConfList()) + if testcase.content.isConfList { + assert.NotNilf(t, cni.confList, "Expected a conflist for %s", testcase.content.name) + } else { + assert.Nilf(t, cni.confList, "Didn't expect a conflist for %s", testcase.content.name) + } if testcase.ranges != nil { assert.Emptyf(t, cni.getBridgePlugin().IPAM.Subnet, @@ -417,7 +421,11 @@ func TestCniNetworkConfig_GetPodCIDRsFromCNISpec(t *testing.T) { return } - assert.Equal(t, testcase.content.isConfList, cni.IsConfList()) + if testcase.content.isConfList { + assert.NotNilf(t, cni.confList, "Expected a conflist for %s", testcase.content.name) + } else { + assert.Nilf(t, cni.confList, "Didn't expect a conflist for %s", testcase.content.name) + } if testcase.ranges != nil { assert.Emptyf(t, cni.getBridgePlugin().IPAM.Subnet, @@ -600,7 +608,7 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { t.Fatalf("we should be able to read the CNI file we just wrote to") } var brPlug *Conf - if cni.IsConfList() { + if testcase.content.isConfList { cl := new(ConfList) err = json.Unmarshal(cniFileBytes, cl) if err != nil { From edd88523fa03211841378767d27a9b91d8354214 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 13:34:03 +0200 Subject: [PATCH 6/9] chore: Use error strings in CNI config table tests The tests were asserting the error messages only, so no need to wrap them into errors. Also, remove the messages completely from those table tests in which they were nil for every contained test case. Signed-off-by: Tom Wieczorek --- pkg/utils/cni_test.go | 67 ++++++++++++------------------------------- 1 file changed, 19 insertions(+), 48 deletions(-) diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index 99703d0d8..2a1ddecec 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -2,11 +2,9 @@ package utils import ( "encoding/json" - "errors" "fmt" "os" "path/filepath" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -279,51 +277,45 @@ func TestNewCNINetworkConfig(t *testing.T) { testcases := []struct { name string content cniConfContent - err error + err string ranges []string }{ { name: "Attempt reading from conf", content: getConf(), - err: nil, }, { name: "Attempt reading from conflist", content: getConfList(), - err: nil, }, { name: "Ensure error upon reading from conf with no type", content: getConfWithNoType(), - err: errors.New("error load CNI config, file appears to have no type: "), + err: "error load CNI config, file appears to have no type: ", }, { name: "Ensure error upon reading from conflist with no plugins", content: getConfListWithNoPlugins(), - err: errors.New("CNI config list "), + err: "CNI config list ", }, { name: "Ensure conf subnet get consolidated into ranges when only subnet exists", content: getConf(), - err: nil, ranges: []string{"10.242.0.0/24"}, }, { name: "Ensure conflist subnet get consolidated into ranges when only subnet exists", content: getConfList(), - err: nil, ranges: []string{"10.242.0.0/24"}, }, { name: "Ensure conflist subnets get consolidated with ranges when both exist", content: getConfListWithRanges(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24"}, }, { name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", content: getConfListWithDuplicateRanges(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, }, } @@ -333,14 +325,13 @@ func TestNewCNINetworkConfig(t *testing.T) { require.NoError(t, os.WriteFile(confFilePath, testcase.content.bytes, 0600)) cni, err := NewCNINetworkConfig(confFilePath) - if err != nil { - if testcase.err == nil { - assert.Fail(t, "if error from NewCNINetworkConfig is not nil, the testcase shouldn't be "+ - "nil either") - } - assert.True(t, strings.HasPrefix(err.Error(), testcase.err.Error())) + if testcase.err != "" { + assert.Nil(t, cni) + assert.ErrorContains(t, err, testcase.err) return } + require.NoError(t, err) + require.NotNil(t, cni) if testcase.content.isConfList { assert.NotNilf(t, cni.confList, "Expected a conflist for %s", testcase.content.name) @@ -376,37 +367,31 @@ func TestCniNetworkConfig_GetPodCIDRsFromCNISpec(t *testing.T) { testcases := []struct { name string content cniConfContent - err error ranges []string }{ { name: "Ensure conf subnet get consolidated into ranges when only subnet exists", content: getConf(), - err: nil, ranges: []string{"10.242.0.0/24"}, }, { name: "Ensure conflist subnet get consolidated into ranges when only subnet exists", content: getConfList(), - err: nil, ranges: []string{"10.242.0.0/24"}, }, { name: "Ensure conflist subnets get consolidated with ranges when both exist", content: getConfListWithRanges(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24"}, }, { name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", content: getConfListWithDuplicateRanges(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, }, { name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", content: getConfListWithIPv6DuplicateRanges(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "2001:db8:42:2::/64"}, }, } @@ -416,10 +401,8 @@ func TestCniNetworkConfig_GetPodCIDRsFromCNISpec(t *testing.T) { require.NoError(t, os.WriteFile(confFilePath, testcase.content.bytes, 0600)) cni, err := NewCNINetworkConfig(confFilePath) - assert.Equal(t, testcase.err, err) - if err != nil { - return - } + require.NoError(t, err) + require.NotNil(t, cni) if testcase.content.isConfList { assert.NotNilf(t, cni.confList, "Expected a conflist for %s", testcase.content.name) @@ -457,21 +440,19 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { testcases := []struct { name string content cniConfContent - err error + err string ranges []string insertRanges []string }{ { name: "Ensure passed CIDR is properly inserted into a CNI conf with no subnets defined", content: getConfWithNoSubnet(), - err: nil, ranges: []string{"10.242.0.0/24"}, insertRanges: []string{"10.242.0.0/24"}, }, { name: "Ensure multiple CIDRs are properly inserted into a CNI conf with no subnets defined", content: getConfListWithNoSubnet(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24"}, insertRanges: []string{"10.242.0.0/24", "10.242.1.0/24"}, }, @@ -479,14 +460,12 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { name: "Ensure multiple IPv4 & IPv6 CIDRs are properly inserted into a CNI conf with no subnets" + "defined", content: getConfListWithNoSubnet(), - err: nil, ranges: []string{"10.242.0.0/24", "2001:db8:42:2::/64"}, insertRanges: []string{"10.242.0.0/24", "2001:db8:42:2::/64"}, }, { name: "Ensure that new subnets are inserted into a conflist with existing ranges", content: getConfListWithRanges(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24", "10.242.5.0/24", "10.242.6.0/24"}, insertRanges: []string{"10.242.5.0/24", "10.242.6.0/24"}, @@ -494,14 +473,13 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { { name: "Ensure duplicates are not inserted without error", content: getConfListWithDuplicateRanges(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24"}, insertRanges: []string{"10.242.4.0/24"}, }, { name: "Ensure error is thrown for bad cidr", content: getConfListWithDuplicateRanges(), - err: fmt.Errorf("unable to parse input cidr: %s - %s", "10.242.4.0", + err: fmt.Sprintf("unable to parse input cidr: %s - %s", "10.242.4.0", "invalid CIDR address: 10.242.4.0"), ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, insertRanges: []string{"10.242.4.0"}, @@ -519,10 +497,10 @@ func TestCniNetworkConfig_InsertPodCIDRIntoIPAM(t *testing.T) { for _, cidr := range testcase.insertRanges { err = cni.InsertPodCIDRIntoIPAM(cidr) - if testcase.err != nil { - assert.EqualError(t, err, testcase.err.Error()) + if testcase.err != "" { + assert.EqualError(t, err, testcase.err) } else { - assert.NoError(t, err) + assert.NoErrorf(t, err, "While inserting %s", cidr) } } @@ -540,40 +518,34 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { testcases := []struct { name string content cniConfContent - err error ranges []string insertRanges []string }{ { name: "Ensure written file is the same as read file when no ranges were inserted", content: getConfWithNoSubnet(), - err: nil, }, { name: "Ensure written conf file contains single subnet", content: getConf(), - err: nil, ranges: []string{"10.242.0.0/24"}, insertRanges: []string{"10.242.0.0/24"}, }, { name: "Ensure written conflist file contains multiple subnets", content: getConfListWithNoSubnet(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24"}, insertRanges: []string{"10.242.0.0/24", "10.242.1.0/24"}, }, { name: "Ensure written conflist file has IPv4 & IPv6 CIDRs properly inserted", content: getConfListWithNoSubnet(), - err: nil, ranges: []string{"10.242.0.0/24", "2001:db8:42:2::/64"}, insertRanges: []string{"10.242.0.0/24", "2001:db8:42:2::/64"}, }, { name: "Ensure that conflist file has multiple subnets written when ranges already exist", content: getConfListWithRanges(), - err: nil, ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24", "10.242.4.0/24", "10.242.5.0/24", "10.242.6.0/24"}, insertRanges: []string{"10.242.5.0/24", "10.242.6.0/24"}, @@ -591,8 +563,8 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { if testcase.insertRanges != nil { for _, cidr := range testcase.insertRanges { - err = cni.InsertPodCIDRIntoIPAM(cidr) - assert.Equal(t, testcase.err, err) + err := cni.InsertPodCIDRIntoIPAM(cidr) + require.NoError(t, err) } } @@ -635,9 +607,8 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { "because it should have been consolidated with ranges") cni, err = NewCNINetworkConfig(confFilePath) - if err != nil { - assert.Fail(t, "err should always be nil when calling NewCNINetworkConfig for this suite") - } + require.NoError(t, err) + require.NotNil(t, cni) if testcase.ranges != nil { assert.Emptyf(t, cni.getBridgePlugin().IPAM.Subnet, From b0fe337138992fde863beeba15fd033459d13aee Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 13:42:17 +0200 Subject: [PATCH 7/9] chore: Use getPodCIDRsMapFromCNISpec in CNI Config tests The different test cases assert on an expected set of subnets, but all used their own variants of pulling those subnets out of the CNINetworkConfig struct. Consolidate this by replacing those assertions with a one that checks for the correctness of the keys returned by getPodCIDRsMapFromCNISpec. That is much more compact, and has the same outcome. Signed-off-by: Tom Wieczorek --- pkg/utils/cni_test.go | 79 +++++++------------------------------------ 1 file changed, 13 insertions(+), 66 deletions(-) diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index 2a1ddecec..9a7e20eda 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -3,8 +3,10 @@ package utils import ( "encoding/json" "fmt" + "maps" "os" "path/filepath" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -283,10 +285,12 @@ func TestNewCNINetworkConfig(t *testing.T) { { name: "Attempt reading from conf", content: getConf(), + ranges: []string{"10.242.0.0/24"}, }, { name: "Attempt reading from conflist", content: getConfList(), + ranges: []string{"10.242.0.0/24"}, }, { name: "Ensure error upon reading from conf with no type", @@ -339,26 +343,9 @@ func TestNewCNINetworkConfig(t *testing.T) { assert.Nilf(t, cni.confList, "Didn't expect a conflist for %s", testcase.content.name) } - if testcase.ranges != nil { - assert.Emptyf(t, cni.getBridgePlugin().IPAM.Subnet, - "subnet of cniNetworkConfig should always be empty because it should be consolidated with "+ - "ranges upon creation") - - foundSubnets := make(map[string]any, 0) - for _, rangeSet := range cni.getBridgePlugin().IPAM.Ranges { - for _, rangeSubnet := range rangeSet { - foundSubnets[rangeSubnet.Subnet] = struct{}{} - } - } - - assert.Len(t, foundSubnets, len(testcase.ranges)) - - for _, subnet := range testcase.ranges { - _, found := foundSubnets[subnet] - assert.Truef(t, found, "subnet %s from testcase should have been found in the ranges inside "+ - "cniNetworkConfig", subnet) - } - } + podCIDRs, err := cni.getPodCIDRsMapFromCNISpec() + require.NoError(t, err) + assert.ElementsMatch(t, testcase.ranges, slices.Collect(maps.Keys(podCIDRs))) }) } } @@ -410,28 +397,9 @@ func TestCniNetworkConfig_GetPodCIDRsFromCNISpec(t *testing.T) { assert.Nilf(t, cni.confList, "Didn't expect a conflist for %s", testcase.content.name) } - if testcase.ranges != nil { - assert.Emptyf(t, cni.getBridgePlugin().IPAM.Subnet, - "subnet of cniNetworkConfig should always be empty because it should be consolidated with "+ - "ranges upon creation") - - foundSubnets, err := cni.GetPodCIDRsFromCNISpec() - - assert.Nil(t, err, "err should be nil at this point") - - assert.Len(t, foundSubnets, len(testcase.ranges)) - - for _, subnet := range testcase.ranges { - found := false - for _, foundSubnet := range foundSubnets { - if subnet == foundSubnet.String() { - found = true - } - } - assert.Truef(t, found, "subnet %s from testcase should have been found in the ranges inside "+ - "cniNetworkConfig", subnet) - } - } + podCIDRs, err := cni.getPodCIDRsMapFromCNISpec() + require.NoError(t, err) + assert.ElementsMatch(t, testcase.ranges, slices.Collect(maps.Keys(podCIDRs))) }) } } @@ -610,30 +578,9 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { require.NoError(t, err) require.NotNil(t, cni) - if testcase.ranges != nil { - assert.Emptyf(t, cni.getBridgePlugin().IPAM.Subnet, - "subnet of cniNetworkConfig should always be empty because it should be consolidated with "+ - "ranges upon creation") - - foundSubnets := make(map[string]any, 0) - for _, rangeSet := range cni.getBridgePlugin().IPAM.Ranges { - for _, rangeSubnet := range rangeSet { - foundSubnets[rangeSubnet.Subnet] = struct{}{} - } - } - - assert.Len(t, foundSubnets, len(testcase.ranges)) - - for _, subnet := range testcase.ranges { - _, found := foundSubnets[subnet] - assert.Truef(t, found, "subnet %s from testcase should have been found in the ranges inside "+ - "cniNetworkConfig", subnet) - } - } else { - assert.Emptyf(t, cni.getBridgePlugin().IPAM.Ranges, - "testcase ranges was nil, the subnets re-read from the CNI file after writing should have "+ - "been empty also") - } + podCIDRs, err := cni.getPodCIDRsMapFromCNISpec() + require.NoError(t, err) + assert.ElementsMatch(t, testcase.ranges, slices.Collect(maps.Keys(podCIDRs))) }) } } From 67fa3462c12472164cc3b02e63dc5c60b482e279 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 13:59:30 +0200 Subject: [PATCH 8/9] fix: Reject CNI conflists without a bridge plugin Those would've caused a panic later on. Instead of looking up the bridge plugin config from the conflist on the fly, do it when the struct is initialized, and store the pointer in the conf field. This allows for the removal of getBridgePlugin. Also harden the config parsing against some null JSON fields. Signed-off-by: Tom Wieczorek --- pkg/utils/cni.go | 38 ++++++++++++++-------------------- pkg/utils/cni_test.go | 47 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 25 deletions(-) diff --git a/pkg/utils/cni.go b/pkg/utils/cni.go index 673e72f52..2db1e54de 100644 --- a/pkg/utils/cni.go +++ b/pkg/utils/cni.go @@ -32,8 +32,14 @@ func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) { if err != nil { return nil, fmt.Errorf("failed to load CNI conflist file: %w", err) } - if len(confList.Plugins) == 0 { - return nil, fmt.Errorf("CNI config list %s has no plugins", cniConfFilePath) + for _, conf := range confList.Plugins { + if conf != nil && conf.Type == "bridge" { + cniNetConf.conf = conf + break + } + } + if cniNetConf.conf == nil { + return nil, fmt.Errorf("CNI config list %s has no bridge plugin", cniConfFilePath) } cniNetConf.confList = confList } else { @@ -60,14 +66,13 @@ func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) { // 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 { - brPlug := c.getBridgePlugin() - if brPlug.IPAM.Subnet != "" { - err := c.InsertPodCIDRIntoIPAM(brPlug.IPAM.Subnet) + if c.conf.IPAM.Subnet != "" { + err := c.InsertPodCIDRIntoIPAM(c.conf.IPAM.Subnet) if err != nil { return err } - brPlug.IPAM.Subnet = "" - delete(brPlug.IPAM.raw, "subnet") + c.conf.IPAM.Subnet = "" + delete(c.conf.IPAM.raw, "subnet") } return nil @@ -84,7 +89,7 @@ func (c *CNINetworkConfig) IsConfList() bool { func (c *CNINetworkConfig) getPodCIDRsMapFromCNISpec() (map[string]*net.IPNet, error) { podCIDRs := make(map[string]*net.IPNet) - ipamConfig := c.getBridgePlugin().IPAM + ipamConfig := c.conf.IPAM // Parse ranges from ipamConfig if ipamConfig != nil && len(ipamConfig.Ranges) > 0 { @@ -118,23 +123,11 @@ func (c *CNINetworkConfig) GetPodCIDRsFromCNISpec() ([]*net.IPNet, error) { return podCIDRs, nil } -// 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 { - if conf.Type == "bridge" { - return 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 { - ipamConfig := c.getBridgePlugin().IPAM + ipamConfig := c.conf.IPAM // This should have already been sanitized by the GetPodCIDR* functions before it comes to us, but you can never be // too safe... @@ -160,8 +153,7 @@ func (c *CNINetworkConfig) InsertPodCIDRIntoIPAM(cidr string) error { } func (c *CNINetworkConfig) SetMTU(mtu int) { - brPlugin := c.getBridgePlugin() - brPlugin.MTU = float64(mtu) + c.conf.MTU = float64(mtu) } func (c *CNINetworkConfig) WriteCNIConfig() error { diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index 9a7e20eda..149422b74 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -31,6 +31,10 @@ func (c *cniConfContent) fileName() string { return cniConfTestFileName } +func getNullConfList() cniConfContent { + return cniConfContent{"nullConfList", true, []byte(`null`)} +} + func getConfList() cniConfContent { return cniConfContent{"confList", true, []byte(`{ "cniVersion":"0.3.0", @@ -195,6 +199,27 @@ func getConfListWithNoPlugins() cniConfContent { }`)} } +func getConfListWithNullPlugin() cniConfContent { + return cniConfContent{"getConfListWithNullPlugin", true, []byte(`{ + "cniVersion": "1.1.0", + "name":"mynet", + "plugins":[null] +}`)} +} + +func getConfListWithoutBridgePlugin() cniConfContent { + return cniConfContent{"confListWithoutBridgePlugin", true, []byte(`{ + "cniVersion": "1.1.0", + "name":"mynet", + "plugins":[ + { + "type": "portmap", + "capabilities": {"portMappings": true} + } + ] +}`)} +} + func getConf() cniConfContent { return cniConfContent{"conf", false, []byte(`{ "cniVersion":"0.3.0", @@ -242,12 +267,15 @@ func getConfWithNoType() cniConfContent { func allCNIConfContents() []cniConfContent { return []cniConfContent{ + getNullConfList(), getConfList(), getConfListWithRanges(), getConfListWithDuplicateRanges(), getConfListWithIPv6DuplicateRanges(), getConfListWithNoSubnet(), getConfListWithNoPlugins(), + getConfListWithNullPlugin(), + getConfListWithoutBridgePlugin(), getConf(), getConfWithNoSubnet(), getConfWithNoType(), @@ -285,12 +313,17 @@ func TestNewCNINetworkConfig(t *testing.T) { { name: "Attempt reading from conf", content: getConf(), - ranges: []string{"10.242.0.0/24"}, + ranges: []string{"10.242.0.0/24"}, + }, + { + name: "Rejects null conflists", + content: getNullConfList(), + err: "10-kuberouter.conflist has no bridge plugin", }, { name: "Attempt reading from conflist", content: getConfList(), - ranges: []string{"10.242.0.0/24"}, + ranges: []string{"10.242.0.0/24"}, }, { name: "Ensure error upon reading from conf with no type", @@ -322,6 +355,16 @@ func TestNewCNINetworkConfig(t *testing.T) { content: getConfListWithDuplicateRanges(), ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "10.242.3.0/24"}, }, + { + name: "Doesn't panic on conflists with null plugins", + content: getConfListWithNullPlugin(), + err: "10-kuberouter.conflist has no bridge plugin", + }, + { + name: "Rejects conflists without bridge plugin", + content: getConfListWithoutBridgePlugin(), + err: "10-kuberouter.conflist has no bridge plugin", + }, } for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { From af56fd3af52429c9abcd94f193eb0706477179f4 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Wed, 29 Jul 2026 15:19:36 +0200 Subject: [PATCH 9/9] feat: Support bootstrapping CNI configs from a template file A CNI configuration file that's copied into the CNI configuration directory by an init container is already visible to the container runtime before kube-router has completed it, so on a freshly booted node, pod sandbox creation may transiently fail with errors like "no IP ranges specified" until kube-router has filled in the missing parts. To avoid this, introduce CNI configuration bootstrapping via a template file. If the KUBE_ROUTER_CNI_CONF_TEMPLATE_FILE environment variable is not empty, kube-router reads the config from the template instead, and writes the completed config to the usual CNI config file path, which doesn't need to exist beforehand. The template file's content is interpreted according to the CNI config file name. The template's own file name has no significance. As a result, the container runtime won't pick up an incomplete configuration and try to set up pod networking too early. Since the config is re-derived from the pristine template on every start, the written config is wholly owned by kube-router. Signed-off-by: Tom Wieczorek --- .../generic-kuberouter-cni-conf-template.yaml | 217 ++++++++++++++++++ docs/user-guide.md | 120 +++++++--- .../routing/network_routes_controller.go | 22 +- pkg/utils/cni.go | 21 +- pkg/utils/cni_test.go | 61 ++++- 5 files changed, 391 insertions(+), 50 deletions(-) create mode 100644 daemonset/generic-kuberouter-cni-conf-template.yaml diff --git a/daemonset/generic-kuberouter-cni-conf-template.yaml b/daemonset/generic-kuberouter-cni-conf-template.yaml new file mode 100644 index 000000000..0619b16b3 --- /dev/null +++ b/daemonset/generic-kuberouter-cni-conf-template.yaml @@ -0,0 +1,217 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: kube-router-cfg + namespace: kube-system + labels: + tier: node + k8s-app: kube-router +data: + cni-conf.json: | + { + "cniVersion": "0.3.0", + "name": "mynet", + "plugins": [ + { + "name": "kubernetes", + "type": "bridge", + "bridge": "kube-bridge", + "isDefaultGateway": true, + "ipam": { + "type": "host-local" + } + } + ] + } + +--- +apiVersion: apps/v1 +kind: DaemonSet +metadata: + labels: + k8s-app: kube-router + tier: node + name: kube-router + namespace: kube-system +spec: + selector: + matchLabels: + k8s-app: kube-router + tier: node + template: + metadata: + labels: + k8s-app: kube-router + tier: node + spec: + priorityClassName: system-node-critical + serviceAccountName: kube-router + initContainers: + - name: install-cni + image: docker.io/cloudnativelabs/kube-router + imagePullPolicy: Always + command: + - /bin/sh + - -ec + - | + [ -f /etc/cni/net.d/10-kuberouter.conflist ] || { + rm -f /etc/cni/net.d/*.conf + rm -f /etc/cni/net.d/*.conflist + } + /usr/local/bin/cni-install + volumeMounts: + - name: cni-conf-dir + mountPath: /etc/cni/net.d + - name: host-opt + mountPath: /opt + containers: + - name: kube-router + image: docker.io/cloudnativelabs/kube-router + imagePullPolicy: Always + args: + - --run-router=true + - --run-firewall=true + - --run-service-proxy=false + - --bgp-graceful-restart=true + env: + - name: NODE_NAME + valueFrom: + fieldRef: + fieldPath: spec.nodeName + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: KUBE_ROUTER_CNI_CONF_FILE + value: /etc/cni/net.d/10-kuberouter.conflist + - name: KUBE_ROUTER_CNI_CONF_TEMPLATE_FILE + value: /etc/kube-router/cni-conf.json + livenessProbe: + httpGet: + path: /healthz + port: 20244 + initialDelaySeconds: 10 + periodSeconds: 3 + resources: + requests: + cpu: 250m + memory: 250Mi + securityContext: + privileged: true + volumeMounts: + - name: lib-modules + mountPath: /lib/modules + readOnly: true + - name: cni-conf-dir + mountPath: /etc/cni/net.d + - name: kube-router-cfg + mountPath: /etc/kube-router + readOnly: true + - name: xtables-lock + mountPath: /run/xtables.lock + readOnly: false + hostNetwork: true + hostPID: true + tolerations: + - effect: NoSchedule + operator: Exists + - key: CriticalAddonsOnly + operator: Exists + - effect: NoExecute + operator: Exists + volumes: + - name: lib-modules + hostPath: + path: /lib/modules + - name: cni-conf-dir + hostPath: + path: /etc/cni/net.d + - name: kube-router-cfg + configMap: + name: kube-router-cfg + - name: xtables-lock + hostPath: + path: /run/xtables.lock + type: FileOrCreate + - name: host-opt + hostPath: + path: /opt + +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: kube-router + namespace: kube-system + +--- +kind: ClusterRole +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: kube-router + namespace: kube-system +rules: +- apiGroups: + - "" + resources: + - namespaces + - pods + - services + - nodes + - endpoints + verbs: + - list + - get + - watch +- apiGroups: + - networking.k8s.io + resources: + - networkpolicies + verbs: + - list + - get + - watch +- apiGroups: + - extensions + resources: + - networkpolicies + verbs: + - get + - list + - watch +- apiGroups: + - coordination.k8s.io + resources: + - leases + verbs: + - get + - create + - update +- apiGroups: + - "" + resources: + - services/status + verbs: + - update +- apiGroups: + - discovery.k8s.io + resources: + - endpointslices + verbs: + - get + - list + - watch + +--- +kind: ClusterRoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: kube-router +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: kube-router +subjects: +- kind: ServiceAccount + name: kube-router + namespace: kube-system diff --git a/docs/user-guide.md b/docs/user-guide.md index df1b9573b..028b4cb18 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -35,7 +35,9 @@ - [Hairpin Mode Example](#hairpin-mode-example) - [SNATing Service Traffic](#snating-service-traffic) - [Load balancing Scheduling Algorithms](#load-balancing-scheduling-algorithms) -- [HostPort support](#hostport-support) +- [CNI Configuration](#cni-configuration) + - [CNI Configuration Templates](#cni-configuration-templates) + - [HostPort support](#hostport-support) - [IPVS Graceful termination support](#ipvs-graceful-termination-support) - [MTU](#mtu) - [BGP configuration](#bgp-configuration) @@ -578,46 +580,92 @@ $ kubectl annotate service my-service "kube-router.io/service.schedflags=flag-2" $ kubectl annotate service my-service "kube-router.io/service.schedflags=flag-1,flag-2" ``` -## HostPort support - -If you would like to use `HostPort` functionality below changes are required in the manifest. - -- By default kube-router assumes CNI conf file to be `/etc/cni/net.d/10-kuberouter.conf`. Add an environment variable -`KUBE_ROUTER_CNI_CONF_FILE` to kube-router manifest and set it to `/etc/cni/net.d/10-kuberouter.conflist` - -- Modify `kube-router-cfg` ConfigMap with CNI config that supports `portmap` as additional plug-in - -```json - { - "cniVersion":"0.3.0", - "name":"mynet", - "plugins":[ - { - "name":"kubernetes", - "type":"bridge", - "bridge":"kube-bridge", - "isDefaultGateway":true, - "ipam":{ - "type":"host-local" - } - }, - { - "type":"portmap", - "capabilities":{ - "snat":true, - "portMappings":true - } - } - ] - } -``` - -- Update init container command to create `/etc/cni/net.d/10-kuberouter.conflist` file +## CNI Configuration + +kube-router provides pod networking by means of the standard [`bridge`][cni-bridge] and [`host-local`][cni-host-local] +[containernetworking plugins], optionally accompanied by others like [`portmap`][cni-portmap]. The kube-router Docker +image ships these plugins, and the [example daemonsets](../daemonset/) install them onto the node in an init container. +kube-router's own contribution is the routing of pod traffic between nodes and the management of the plugins' +configuration file, filling in the parts that are node-specific and only known at runtime. The latter is what +`--enable-cni` controls, which is enabled by default. Disable it to use kube-router's features alongside another CNI +provider. On startup, kube-router + +- inserts the node's pod CIDRs into the `host-local` IPAM ranges of the `bridge` plugin, as allocated to the node by + the kube-controller-manager, or as set via the `kube-router.io/pod-cidrs` annotation +- and sets the `bridge` plugin's MTU, if `--auto-mtu` is enabled (see [MTU](#mtu)). + +The completed configuration is written back once the `kube-bridge` interface has been set up. The configuration is read +from and written to `/etc/cni/net.d/10-kuberouter.conf` by default; the location can be changed via the +`KUBE_ROUTER_CNI_CONF_FILE` environment variable. A file name ending in `.conflist` is treated as a [configuration +list][cni-config-format], i.e. a chain of multiple plugins, any other name as a single plugin configuration. + +The configuration file must exist when kube-router starts. The [example daemonsets](../daemonset/) ship it in a +`kube-router-cfg` ConfigMap and use an init container to copy it into the CNI configuration directory. + +### CNI Configuration Templates + +A configuration file that's copied into the CNI configuration directory by an init container is already visible to the +container runtime before kube-router has completed it, so on a freshly booted node, pod sandbox creation may transiently +fail with errors like "no IP ranges specified" until kube-router has filled in the missing parts. + +To avoid this, the `KUBE_ROUTER_CNI_CONF_TEMPLATE_FILE` environment variable can point kube-router to a potentially +incomplete CNI configuration outside of the CNI configuration directory, e.g. a ConfigMap mounted at +`/etc/kube-router/cni-conf.json`. kube-router will then read the configuration from the template instead, and write the +completed configuration to `KUBE_ROUTER_CNI_CONF_FILE`, which doesn't need to exist beforehand. The template is +interpreted according to the file name given in `KUBE_ROUTER_CNI_CONF_FILE`, i.e. as a configuration list if that name +ends in `.conflist`. The template's own file name has no significance. As a result, the container runtime won't pick up +an incomplete configuration and try to set up pod networking too early. Since the configuration is re-derived from the +pristine template on every start, the written configuration is wholly owned by kube-router. + +For an example manifest, please look at the [CNI configuration template manifest]. + +### HostPort support + +If you would like to use `HostPort` functionality, the following changes are required in the manifest: + +- Since the `portmap` plugin needs to be chained after the `bridge` plugin, the CNI configuration has to be a config + list: set `KUBE_ROUTER_CNI_CONF_FILE` to `/etc/cni/net.d/10-kuberouter.conflist` (see above) +- Modify the `kube-router-cfg` ConfigMap with a CNI configuration that supports `portmap` as an additional plug-in: + + ```json + { + "cniVersion":"0.3.0", + "name":"mynet", + "plugins":[ + { + "name":"kubernetes", + "type":"bridge", + "bridge":"kube-bridge", + "isDefaultGateway":true, + "ipam":{ + "type":"host-local" + } + }, + { + "type":"portmap", + "capabilities":{ + "snat":true, + "portMappings":true + } + } + ] + } + ``` + +- Update the init container command to create `/etc/cni/net.d/10-kuberouter.conflist`, or point + `KUBE_ROUTER_CNI_CONF_TEMPLATE_FILE` at the ConfigMap mount instead - Restart the container runtime For an e.g manifest please look at [manifest](../daemonset/kubeadm-kuberouter-all-features-hostport.yaml) with necessary changes required for `HostPort` functionality. +[cni-bridge]: https://www.cni.dev/plugins/v1.1/main/bridge/ +[cni-host-local]: https://www.cni.dev/plugins/v1.1/ipam/host-local/ +[containernetworking plugins]: https://github.com/containernetworking/plugins +[cni-portmap]: https://www.cni.dev/plugins/v1.1/meta/portmap/ +[cni-config-format]: https://www.cni.dev/docs/spec/#configuration-format +[CNI configuration template manifest]: ../daemonset/generic-kuberouter-cni-conf-template.yaml + ## IPVS Graceful termination support We support experimental graceful termination of IPVS destinations. When possible, the pod's diff --git a/pkg/controllers/routing/network_routes_controller.go b/pkg/controllers/routing/network_routes_controller.go index 28da08fbf..5f4462ca0 100644 --- a/pkg/controllers/routing/network_routes_controller.go +++ b/pkg/controllers/routing/network_routes_controller.go @@ -146,6 +146,7 @@ type NetworkRoutingController struct { bgpRRServer bool bgpClusterID string cniConfFile string + cniConfTemplateFile string disableSrcDstCheck bool initSrcDstCheckDone atomic.Bool ec2IamAuthorized atomic.Bool @@ -407,10 +408,10 @@ func (nrc *NetworkRoutingController) initCNIConfig() (mtu int, _ *utils.CNINetwo return mtu, nil } - // Parse the existing IPAM CIDRs from the CNI conf file - cniNetConf, err := utils.NewCNINetworkConfig(nrc.cniConfFile) + // Parse the existing IPAM CIDRs from the CNI conf (template) file + cniNetConf, err := utils.NewCNINetworkConfigFromTemplate(nrc.cniConfFile, nrc.cniConfTemplateFile) if err != nil { - klog.Fatalf("failed to parse CNI Config: %v", err) + klog.Fatalf("failed to load CNI configuration: %v", err) } // Insert any IPv4 CIDRs that are missing from the IPAM configuration in the CNI @@ -1301,9 +1302,18 @@ func NewNetworkRoutingController(clientset kubernetes.Interface, if nrc.cniConfFile == "" { nrc.cniConfFile = "/etc/cni/net.d/10-kuberouter.conf" } - // #nosec G703: cniConfFile is not untrusted user input - if _, err := os.Stat(nrc.cniConfFile); os.IsNotExist(err) { - return nil, errors.New("CNI conf file " + nrc.cniConfFile + " does not exist.") + nrc.cniConfTemplateFile = os.Getenv("KUBE_ROUTER_CNI_CONF_TEMPLATE_FILE") + if nrc.cniConfTemplateFile != "" { + // The CNI conf file will be written from the template, so only the template needs to exist. + // #nosec G703: cniConfTemplateFile is not untrusted user input + if _, err := os.Stat(nrc.cniConfTemplateFile); os.IsNotExist(err) { + return nil, errors.New("CNI conf template file " + nrc.cniConfTemplateFile + " does not exist.") + } + } else { + // #nosec G703: cniConfFile is not untrusted user input + if _, err := os.Stat(nrc.cniConfFile); os.IsNotExist(err) { + return nil, errors.New("CNI conf file " + nrc.cniConfFile + " does not exist.") + } } } diff --git a/pkg/utils/cni.go b/pkg/utils/cni.go index 2db1e54de..dd4d94631 100644 --- a/pkg/utils/cni.go +++ b/pkg/utils/cni.go @@ -16,13 +16,22 @@ type CNINetworkConfig struct { } func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) { + return NewCNINetworkConfigFromTemplate(cniConfFilePath, "") +} + +func NewCNINetworkConfigFromTemplate(cniConfFilePath, cniConfTemplateFilePath string) (*CNINetworkConfig, error) { cniNetConf := CNINetworkConfig{ filePath: cniConfFilePath, } - cniFileBytes, err := os.ReadFile(cniConfFilePath) + fileDesc, srcFilePath := "configuration file", cniConfFilePath + if cniConfTemplateFilePath != "" { + fileDesc, srcFilePath = "configuration template file", cniConfTemplateFilePath + } + + cniFileBytes, err := os.ReadFile(srcFilePath) if err != nil { - return nil, fmt.Errorf("error reading %s: %w", cniConfFilePath, err) + return nil, fmt.Errorf("failed to read %s: %w", fileDesc, err) } // If we're working with a conflist setup @@ -30,7 +39,7 @@ func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) { confList := new(ConfList) err = json.Unmarshal(cniFileBytes, confList) if err != nil { - return nil, fmt.Errorf("failed to load CNI conflist file: %w", err) + return nil, fmt.Errorf("failed to parse %s %s as configuration list: %w", fileDesc, srcFilePath, err) } for _, conf := range confList.Plugins { if conf != nil && conf.Type == "bridge" { @@ -39,7 +48,7 @@ func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) { } } if cniNetConf.conf == nil { - return nil, fmt.Errorf("CNI config list %s has no bridge plugin", cniConfFilePath) + return nil, fmt.Errorf("configuration list in %s %s has no bridge plugin", fileDesc, srcFilePath) } cniNetConf.confList = confList } else { @@ -47,10 +56,10 @@ func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) { conf := new(Conf) err = json.Unmarshal(cniFileBytes, conf) if err != nil { - return nil, fmt.Errorf("failed to load CNI conf file: %w", err) + return nil, fmt.Errorf("failed to parse %s %s: %w", fileDesc, srcFilePath, err) } if conf.Type == "" { - return nil, fmt.Errorf("error load CNI config, file appears to have no type: %s", cniConfFilePath) + return nil, fmt.Errorf("configuration in %s %s has no type", fileDesc, srcFilePath) } cniNetConf.conf = conf } diff --git a/pkg/utils/cni_test.go b/pkg/utils/cni_test.go index 149422b74..557a71d32 100644 --- a/pkg/utils/cni_test.go +++ b/pkg/utils/cni_test.go @@ -328,12 +328,12 @@ func TestNewCNINetworkConfig(t *testing.T) { { name: "Ensure error upon reading from conf with no type", content: getConfWithNoType(), - err: "error load CNI config, file appears to have no type: ", + err: "10-kuberouter.conf has no type", }, { name: "Ensure error upon reading from conflist with no plugins", content: getConfListWithNoPlugins(), - err: "CNI config list ", + err: "10-kuberouter.conflist has no bridge plugin", }, { name: "Ensure conf subnet get consolidated into ranges when only subnet exists", @@ -627,3 +627,60 @@ func TestCniNetworkConfig_WriteCNIConfig(t *testing.T) { }) } } + +func TestNewCNINetworkConfig_Template(t *testing.T) { + t.Run("Ensure template is parsed according to the CNI conf file name", func(t *testing.T) { + tmpDir := t.TempDir() + templateFilePath := filepath.Join(tmpDir, "cni-conf.json") + require.NoError(t, os.WriteFile(templateFilePath, getConfList().bytes, 0600)) + + cni, err := NewCNINetworkConfigFromTemplate(filepath.Join(tmpDir, cniConfListTestFileName), templateFilePath) + require.NoError(t, err) + require.NotNil(t, cni) + + assert.NotNil(t, cni.confList, "The CNI conf file name, not the template file name, should determine "+ + "if the config is a conflist") + }) + + t.Run("Ensure CNI conf file is written from the template", func(t *testing.T) { + tmpDir := t.TempDir() + templateFilePath := filepath.Join(tmpDir, "cni-conf.json") + require.NoError(t, os.WriteFile(templateFilePath, getConfListWithNoSubnet().bytes, 0600)) + templateFileInfo, err := os.Stat(templateFilePath) + require.NoError(t, err) + + cniConfFilePath := filepath.Join(tmpDir, cniConfListTestFileName) + cni, err := NewCNINetworkConfigFromTemplate(cniConfFilePath, templateFilePath) + require.NoError(t, err) + require.NotNil(t, cni) + + require.NoError(t, cni.InsertPodCIDRIntoIPAM("10.242.0.0/24")) + require.NoError(t, cni.WriteCNIConfig()) + + writtenCNI, err := NewCNINetworkConfig(cniConfFilePath) + require.NoError(t, err) + require.NotNil(t, writtenCNI) + + podCIDRs, err := writtenCNI.getPodCIDRsMapFromCNISpec() + require.NoError(t, err) + assert.ElementsMatch(t, []string{"10.242.0.0/24"}, slices.Collect(maps.Keys(podCIDRs))) + + if newTemplateFileInfo, err := os.Stat(templateFilePath); assert.NoError(t, err) { + assert.True(t, templateFileInfo.Size() == newTemplateFileInfo.Size() && + templateFileInfo.ModTime().Equal(newTemplateFileInfo.ModTime()), + "The template file should remain untouched") + } + + entries, err := os.ReadDir(tmpDir) + require.NoError(t, err) + assert.Lenf(t, entries, 2, "No other files besides the template and the CNI conf file should be left "+ + "behind in the CNI conf dir") + }) + + t.Run("Ensure a missing template file is an error", func(t *testing.T) { + tmpDir := t.TempDir() + _, err := NewCNINetworkConfigFromTemplate( + filepath.Join(tmpDir, cniConfListTestFileName), filepath.Join(tmpDir, "cni-conf.json")) + assert.ErrorIs(t, err, os.ErrNotExist) + }) +}