From b8407eab4821a922dfd753e01493eacb3d8dd511 Mon Sep 17 00:00:00 2001 From: Jorik Jonker Date: Wed, 5 Apr 2023 16:54:39 +0200 Subject: [PATCH 1/2] fix: improve error handling The control loop currently segfaults when none of the config files was found. This commit makes it error, providing the user with useful feedback. Also, L260 returned a `nil` disguised as a var named `err`, which could be done more explicit. Signed-off-by: Jorik Jonker --- pkg/config/config.go | 16 +++++++++++++--- pkg/config/config_test.go | 7 +++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index dd29ab32e..685f2cbbb 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -256,11 +256,11 @@ func GetFlatIPAM(isControlLoop bool, IPAM *types.IPAMConfig, extraConfigPaths .. } foundflatfile = confpath - return flatipam, foundflatfile, err + return flatipam, foundflatfile, nil } } - var err error - return flatipam, foundflatfile, err + + return flatipam, foundflatfile, NewConfigFileNotFoundError() } func handleEnvArgs(n *types.Net, numV6 int, numV4 int, args types.IPAMEnvArgs) (int, int, error) { @@ -370,6 +370,16 @@ func (e *InvalidPluginError) Error() string { return fmt.Sprintf("only interested in networks whose IPAM type is 'whereabouts'. This one was: %s", e.ipamType) } +type ConfigFileNotFoundError struct{} + +func NewConfigFileNotFoundError() *ConfigFileNotFoundError { + return &ConfigFileNotFoundError{} +} + +func (e *ConfigFileNotFoundError) Error() string { + return "config file not found" +} + func storageError() error { return fmt.Errorf("you have not configured the storage engine (looks like you're using an invalid `kubernetes.kubeconfig` parameter in your config)") } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 54b8f6838..cea648d36 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -8,6 +8,8 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + + "github.com/k8snetworkplumbingwg/whereabouts/pkg/types" ) func TestAllocate(t *testing.T) { @@ -49,6 +51,11 @@ var _ = Describe("Allocation operations", func() { }) + It("throws an error when no flat-files are found", func() { + _, _, err := GetFlatIPAM(true, &types.IPAMConfig{}) + Expect(err).To(MatchError(NewConfigFileNotFoundError())) + }) + It("can load a global flat-file config", func() { globalconf := `{ From 55b24e2899cc6591033752aba30b486b59e42091 Mon Sep 17 00:00:00 2001 From: Jorik Jonker Date: Mon, 1 May 2023 13:35:24 +0200 Subject: [PATCH 2/2] fix: refactor tests to have a config file It is required per my previous commit, which seemed to break a couple of tests. Signed-off-by: Jorik Jonker --- cmd/whereabouts_test.go | 83 +++++++++++++++++++++++++++++++-------- pkg/config/config_test.go | 54 +++++++++++++++++++++---- 2 files changed, 112 insertions(+), 25 deletions(-) diff --git a/cmd/whereabouts_test.go b/cmd/whereabouts_test.go index 7aefbff4f..62ff4d7f5 100644 --- a/cmd/whereabouts_test.go +++ b/cmd/whereabouts_test.go @@ -7,6 +7,7 @@ import ( "io/fs" "net" "os" + "path/filepath" "strings" "testing" @@ -307,7 +308,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) k8sClient = newK8sIPAM( @@ -379,7 +382,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) k8sClient = newK8sIPAM( @@ -448,7 +453,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) k8sClient = newK8sIPAM( @@ -528,7 +535,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) k8sClient = newK8sIPAM( @@ -617,7 +626,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) k8sClient = newK8sIPAM( @@ -681,7 +692,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).To(HaveLen(2)) k8sClient = newK8sIPAM( @@ -746,7 +759,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).To(HaveLen(2)) k8sClient = newK8sIPAM( @@ -809,7 +824,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) k8sClient = newK8sIPAM( @@ -875,7 +892,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) k8sClient = newK8sIPAM( @@ -934,7 +953,9 @@ var _ = Describe("Whereabouts operations", func() { } }`, backend) - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) wbClient := *kubernetes.NewKubernetesClient( @@ -1031,7 +1052,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) wbClient := *kubernetes.NewKubernetesClient( @@ -1083,7 +1106,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName), "") + secondConfPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(confsecond), 0755)).To(Succeed()) + secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName), secondConfPath) Expect(err).NotTo(HaveOccurred()) // Allocate the IP @@ -1149,7 +1174,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) wbClient := *kubernetes.NewKubernetesClient( @@ -1201,7 +1228,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName), "") + secondConfPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(confsecond), 0755)).To(Succeed()) + secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName), secondConfPath) Expect(err).NotTo(HaveOccurred()) // Allocate the IP @@ -1268,7 +1297,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + ipamConf, cniVersion, err := config.LoadIPAMConfig([]byte(conf), cniArgs(podNamespace, podName), confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConf.IPRanges).NotTo(BeEmpty()) wbClient := *kubernetes.NewKubernetesClient( @@ -1320,7 +1351,9 @@ var _ = Describe("Whereabouts operations", func() { Args: cniArgs(podNamespace, podName), } - secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName), "") + secondConfPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(secondConfPath, []byte(confsecond), 0755)).To(Succeed()) + secondIPAMConf, secondCNIVersion, err := config.LoadIPAMConfig([]byte(confsecond), cniArgs(podNamespace, podName), secondConfPath) Expect(err).NotTo(HaveOccurred()) // Allocate the IP @@ -1415,10 +1448,26 @@ func ipamConfig(podName string, namespace string, ipRange string, gw string, kub return nil } - ipamConfWithDefaults, _, err := config.LoadIPAMConfig(bytes, cniArgs(namespace, podName), "") + tmpDir, err := os.MkdirTemp("", "whereabouts") if err != nil { return nil } + confPath := filepath.Join(tmpDir, "wherebouts.conf") + err = os.WriteFile(confPath, bytes, 0755) + if err != nil { + return nil + } + + ipamConfWithDefaults, _, err := config.LoadIPAMConfig(bytes, cniArgs(namespace, podName), confPath) + if err != nil { + return nil + } + + err = os.RemoveAll(tmpDir) + if err != nil { + return nil + } + return ipamConfWithDefaults } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index cea648d36..994d408e1 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -4,6 +4,7 @@ import ( "fmt" "net" "os" + "path/filepath" "testing" . "github.com/onsi/ginkgo" @@ -18,6 +19,18 @@ func TestAllocate(t *testing.T) { } var _ = Describe("Allocation operations", func() { + var tmpDir string + + BeforeEach(func() { + var err error + tmpDir, err = os.MkdirTemp("", "whereabouts") + Expect(err).NotTo(HaveOccurred()) + }) + + AfterEach(func() { + Expect(os.RemoveAll(tmpDir)).To(Succeed()) + }) + It("can load a basic config", func() { conf := `{ @@ -37,7 +50,10 @@ var _ = Describe("Allocation operations", func() { } }` - ipamconfig, _, err := LoadIPAMConfig([]byte(conf), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + + ipamconfig, _, err := LoadIPAMConfig([]byte(conf), "", confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamconfig.LogLevel).To(Equal("debug")) Expect(ipamconfig.LogFile).To(Equal("/tmp/whereabouts.log")) @@ -167,7 +183,10 @@ var _ = Describe("Allocation operations", func() { ] }` - ipamconfig, err := LoadIPAMConfiguration([]byte(conf), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + + ipamconfig, err := LoadIPAMConfiguration([]byte(conf), "", confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamconfig.LogLevel).To(Equal("debug")) Expect(ipamconfig.LogFile).To(Equal("/tmp/whereabouts.log")) @@ -214,7 +233,10 @@ var _ = Describe("Allocation operations", func() { } }` - ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + + ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "", confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConfig.IPRanges[0].Range).To(Equal("192.168.1.0/24")) Expect(ipamConfig.IPRanges[0].RangeStart).To(Equal(net.ParseIP("192.168.1.5"))) @@ -239,7 +261,10 @@ var _ = Describe("Allocation operations", func() { } }` - ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + + ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "", confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConfig.IPRanges[0].Range).To(Equal("192.168.1.0/24")) Expect(ipamConfig.IPRanges[0].RangeStart).To(Equal(net.ParseIP("192.168.1.0"))) @@ -264,7 +289,10 @@ var _ = Describe("Allocation operations", func() { } }` - ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + + ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "", confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConfig.IPRanges[0].Range).To(Equal("192.168.1.0/24")) Expect(ipamConfig.IPRanges[0].RangeStart).To(Equal(net.ParseIP("192.168.1.44"))) @@ -290,7 +318,10 @@ var _ = Describe("Allocation operations", func() { } }` - ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + + ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "", confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConfig.IPRanges[0].Range).To(Equal("192.168.1.0/24")) Expect(ipamConfig.IPRanges[0].RangeStart).To(Equal(net.ParseIP("192.168.1.44"))) @@ -318,7 +349,10 @@ var _ = Describe("Allocation operations", func() { } }` - ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "") + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(conf), 0755)).To(Succeed()) + + ipamConfig, _, err := LoadIPAMConfig([]byte(conf), "", confPath) Expect(err).NotTo(HaveOccurred()) Expect(ipamConfig.IPRanges[0].Range).To(Equal("192.168.1.0/24")) Expect(ipamConfig.IPRanges[0].RangeStart).To(Equal(net.ParseIP("192.168.1.44"))) @@ -340,7 +374,11 @@ var _ = Describe("Allocation operations", func() { "gateway": "192.168.10.1" } }` - _, _, err := LoadIPAMConfig([]byte(invalidConf), "") + + confPath := filepath.Join(tmpDir, "whereabouts.conf") + Expect(os.WriteFile(confPath, []byte(invalidConf), 0755)).To(Succeed()) + + _, _, err := LoadIPAMConfig([]byte(invalidConf), "", confPath) Expect(err).To(MatchError("invalid range start for CIDR 192.168.2.16/28: 192.168.1.5")) })