Skip to content

Commit

Permalink
Merge pull request #187 from saschagrunert/cleanups
Browse files Browse the repository at this point in the history 
Apply small cleanups and error check paths
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored Mar 13, 2024
2 parents f870c19 + ab3f6e9 commit d17a01b
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 37 deletions.
37 changes: 19 additions & 18 deletions pkg/ocicni/ocicni.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"net"
"os"
"path"
Expand Down Expand Up @@ -221,6 +220,13 @@ func initCNI(exec cniinvoke.Exec, cacheDir, defaultNetName string, confDir strin
binDirs = []string{DefaultBinDir}
}

if exec == nil {
exec = &cniinvoke.DefaultExec{
RawExec: &cniinvoke.RawExec{Stderr: os.Stderr},
PluginDecoder: cniversion.PluginDecoder{},
}
}

plugin := &cniNetworkPlugin{
cniConfig: libcni.NewCNIConfigWithCacheDir(binDirs, cacheDir, exec),
defaultNetName: netName{
Expand All @@ -239,20 +245,15 @@ func initCNI(exec cniinvoke.Exec, cacheDir, defaultNetName string, confDir strin
cacheDir: cacheDir,
}

if exec == nil {
exec = &cniinvoke.DefaultExec{
RawExec: &cniinvoke.RawExec{Stderr: os.Stderr},
PluginDecoder: cniversion.PluginDecoder{},
}
}

nsm, err := newNSManager()
if err != nil {
return nil, err
}
plugin.nsManager = nsm

plugin.syncNetworkConfig()
if err := plugin.syncNetworkConfig(); err != nil {
logrus.Errorf("CNI sync network config failed: %v", err)
}

if useInotify {
plugin.watcher, err = newWatcher(append([]string{plugin.confDir}, binDirs...))
Expand Down Expand Up @@ -494,7 +495,7 @@ func (plugin *cniNetworkPlugin) forEachNetwork(podNetwork *PodNetwork, fromCache

rt, err := buildCNIRuntimeConf(podNetwork, ifName, podNetwork.RuntimeConfig[network.Name])
if err != nil {
logrus.Errorf("error building CNI runtime config: %v", err)
logrus.Errorf("Error building CNI runtime config: %v", err)
return err
}

Expand All @@ -503,8 +504,8 @@ func (plugin *cniNetworkPlugin) forEachNetwork(podNetwork *PodNetwork, fromCache
var newRt *libcni.RuntimeConf
cniNet, newRt, err = plugin.loadNetworkFromCache(network.Name, rt)
if err != nil {
logrus.Errorf("error loading cached network config: %v", err)
logrus.Warningf("falling back to loading from existing plugins on disk")
logrus.Errorf("Error loading cached network config: %v", err)
logrus.Warningf("Falling back to loading from existing plugins on disk")
} else {
// Use the updated RuntimeConf
rt = newRt
Expand Down Expand Up @@ -580,7 +581,7 @@ func (plugin *cniNetworkPlugin) getCachedNetworkInfo(containerID string) ([]NetA
}

dirPath := filepath.Join(cacheDir, "results")
entries, err := ioutil.ReadDir(dirPath)
entries, err := os.ReadDir(dirPath)
if err != nil {
return nil, err
}
Expand All @@ -600,9 +601,9 @@ func (plugin *cniNetworkPlugin) getCachedNetworkInfo(containerID string) ([]NetA
}

cacheFile := filepath.Join(dirPath, fname)
bytes, err := ioutil.ReadFile(cacheFile)
bytes, err := os.ReadFile(cacheFile)
if err != nil {
logrus.Errorf("failed to read CNI cache file %s: %v", cacheFile, err)
logrus.Errorf("Failed to read CNI cache file %s: %v", cacheFile, err)
continue
}

Expand All @@ -614,11 +615,11 @@ func (plugin *cniNetworkPlugin) getCachedNetworkInfo(containerID string) ([]NetA
}{}

if err := json.Unmarshal(bytes, &cachedInfo); err != nil {
logrus.Errorf("failed to unmarshal CNI cache file %s: %v", cacheFile, err)
logrus.Errorf("Failed to unmarshal CNI cache file %s: %v", cacheFile, err)
continue
}
if cachedInfo.Kind != libcni.CNICacheV1 {
logrus.Warningf("unknown CNI cache file %s kind %q", cacheFile, cachedInfo.Kind)
logrus.Warningf("Unknown CNI cache file %s kind %q", cacheFile, cachedInfo.Kind)
continue
}
if cachedInfo.ContainerID != containerID {
Expand All @@ -629,7 +630,7 @@ func (plugin *cniNetworkPlugin) getCachedNetworkInfo(containerID string) ([]NetA
continue
}
if cachedInfo.IfName == "" || cachedInfo.NetName == "" {
logrus.Warningf("missing CNI cache file %s ifname %q or netname %q", cacheFile, cachedInfo.IfName, cachedInfo.NetName)
logrus.Warningf("Missing CNI cache file %s ifname %q or netname %q", cacheFile, cachedInfo.IfName, cachedInfo.NetName)
continue
}

Expand Down
40 changes: 21 additions & 19 deletions pkg/ocicni/ocicni_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"encoding/base64"
"encoding/json"
"fmt"
"io/ioutil"
"net"
"os"
"path/filepath"
Expand All @@ -32,7 +31,7 @@ func writeConfig(dir, fileName, netName, plugin string, version string) (string,
"type": "%s",
"cniVersion": "%s"
}`, netName, plugin, version)
return conf, confPath, ioutil.WriteFile(confPath, []byte(conf), 0644)
return conf, confPath, os.WriteFile(confPath, []byte(conf), 0644)
}

func writeCacheFile(dir, containerID, netName, ifname, config string) {
Expand All @@ -52,7 +51,7 @@ func writeCacheFile(dir, containerID, netName, ifname, config string) {
Expect(err).NotTo(HaveOccurred())

filePath := filepath.Join(dirName, fmt.Sprintf("%s-%s-%s", netName, containerID, ifname))
err = ioutil.WriteFile(filePath, []byte(cachedData), 0644)
err = os.WriteFile(filePath, []byte(cachedData), 0644)
Expect(err).NotTo(HaveOccurred())
}

Expand Down Expand Up @@ -146,7 +145,10 @@ func (f *fakeExec) ExecPlugin(ctx context.Context, pluginPath string, stdinData
// SetUpPod We only care about a few fields
testConf := &TestConf{}
err = json.Unmarshal(stdinData, &testConf)
Expect(err).NotTo(HaveOccurred())

testData, err := json.Marshal(testConf)
Expect(err).NotTo(HaveOccurred())
if plugin.expectedConf != "" {
Expect(string(testData)).To(MatchJSON(plugin.expectedConf))
}
Expand Down Expand Up @@ -193,11 +195,11 @@ var _ = Describe("ocicni operations", func() {

BeforeEach(func() {
var err error
tmpDir, err = ioutil.TempDir("", "ocicni_tmp")
tmpDir, err = os.MkdirTemp("", "ocicni_tmp")
Expect(err).NotTo(HaveOccurred())
tmpBinDir, err = ioutil.TempDir("", "ocicni_tmp_bin")
tmpBinDir, err = os.MkdirTemp("", "ocicni_tmp_bin")
Expect(err).NotTo(HaveOccurred())
cacheDir, err = ioutil.TempDir("", "ocicni_cache")
cacheDir, err = os.MkdirTemp("", "ocicni_cache")
Expect(err).NotTo(HaveOccurred())

networkNS, err = testutils.NewNS()
Expand Down Expand Up @@ -233,7 +235,7 @@ var _ = Describe("ocicni operations", func() {
Expect(len(net.config.Plugins)).To(BeNumerically(">", 0))
Expect(net.config.Plugins[0].Network.Type).To(Equal("myplugin"))

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("finds an asynchronously written default network configuration", func() {
Expand All @@ -255,7 +257,7 @@ var _ = Describe("ocicni operations", func() {
Expect(len(net.config.Plugins)).To(BeNumerically(">", 0))
Expect(net.config.Plugins[0].Network.Type).To(Equal("myplugin"))

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("finds an asynchronously written default network configuration whose plugin is written later", func() {
Expand All @@ -269,7 +271,7 @@ var _ = Describe("ocicni operations", func() {

// Write a file in the bindir to trigger the fsnotify code to resync
fExec.failFind = false
err = ioutil.WriteFile(filepath.Join(tmpBinDir, "myplugin"), []byte("adsfasdfsafd"), 0755)
err = os.WriteFile(filepath.Join(tmpBinDir, "myplugin"), []byte("adsfasdfsafd"), 0755)
Expect(err).NotTo(HaveOccurred())

tmp := ocicni.(*cniNetworkPlugin)
Expand All @@ -290,7 +292,7 @@ var _ = Describe("ocicni operations", func() {
return nil
}, 10).Should(Succeed())

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("should monitor the net conf dir for changes when default network is not specified", func() {
Expand Down Expand Up @@ -321,7 +323,7 @@ var _ = Describe("ocicni operations", func() {
Expect(len(net.config.Plugins)).To(BeNumerically(">", 0))
Expect(net.config.Plugins[0].Network.Type).To(Equal("testplugin"))

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("should monitor the net conf dir for changes when default network is specified", func() {
Expand Down Expand Up @@ -352,7 +354,7 @@ var _ = Describe("ocicni operations", func() {
Expect(len(net.config.Plugins)).To(BeNumerically(">", 0))
Expect(net.config.Plugins[0].Network.Type).To(Equal("testplugin"))

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("finds and refinds an asynchronously written default network configuration", func() {
Expand Down Expand Up @@ -382,7 +384,7 @@ var _ = Describe("ocicni operations", func() {
Expect(err).NotTo(HaveOccurred())
Eventually(ocicni.Status, 5).Should(Succeed())

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("finds an ASCIIbetically first network configuration as default real-time if given no default network name", func() {
Expand Down Expand Up @@ -414,7 +416,7 @@ var _ = Describe("ocicni operations", func() {
Expect(len(net.config.Plugins)).To(BeNumerically(">", 0))
Expect(net.config.Plugins[0].Network.Type).To(Equal("myplugin"))

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("returns correct default network from loadNetworks()", func() {
Expand Down Expand Up @@ -638,7 +640,7 @@ var _ = Describe("ocicni operations", func() {
Expect(err).NotTo(HaveOccurred())
Expect(fake.delIndex).To(Equal(len(fake.plugins)))

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("sets up and tears down a pod using specified networks", func() {
Expand Down Expand Up @@ -715,7 +717,7 @@ var _ = Describe("ocicni operations", func() {
Expect(err).NotTo(HaveOccurred())
Expect(fake.delIndex).To(Equal(len(fake.plugins)))

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("sets up and tears down a pod using specified v4 networks", func() {
Expand Down Expand Up @@ -801,7 +803,7 @@ var _ = Describe("ocicni operations", func() {
Expect(err).NotTo(HaveOccurred())
Expect(fake.delIndex).To(Equal(len(fake.plugins)))

ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

Context("when tearing down a pod using cached info", func() {
Expand Down Expand Up @@ -855,7 +857,7 @@ var _ = Describe("ocicni operations", func() {
})

AfterEach(func() {
ocicni.Shutdown()
Expect(ocicni.Shutdown()).NotTo(HaveOccurred())
})

It("uses the specified networks", func() {
Expand Down Expand Up @@ -910,7 +912,7 @@ var _ = Describe("ocicni operations", func() {

ocicni, err := initCNI(fake, cacheDir, defaultNetName, tmpDir, true, "/opt/cni/bin")
Expect(err).NotTo(HaveOccurred())
defer ocicni.Shutdown()
defer Expect(ocicni.Shutdown()).NotTo(HaveOccurred())

podNet := PodNetwork{
Name: "pod1",
Expand Down

0 comments on commit d17a01b

Please sign in to comment.