ocicni: watch bindirs too

Config validation involves finding and execing the config's plugin
to determine the CNI spec versions the plugin binary supports.

If the config is written to disk before or concurrently with the
plugin binary then the validation may race with the binary and
fail to either find it or exec it.

Watch bindirs for changes and resync network config when plugin
binaries change.

Signed-off-by: Dan Williams <[email protected]>

Author: dcbw

pkg/ocicni/ocicni.go

@@ -117,11 +117,12 @@ func (plugin *cniNetworkPlugin) podUnlock(podNetwork PodNetwork) {
 	}
 }
 
-func newWatcher(confDir string) (*fsnotify.Watcher, error) {
-	// Ensure plugin directory exists, because the following monitoring logic
-	// relies on that.
-	if err := os.MkdirAll(confDir, 0755); err != nil {
-		return nil, fmt.Errorf("failed to create directory %q: %v", confDir, err)
+func newWatcher(dirs []string) (*fsnotify.Watcher, error) {
+	// Ensure directories exist because the fsnotify watch logic depends on it
+	for _, dir := range dirs {
+		if err := os.MkdirAll(dir, 0755); err != nil {
+			return nil, fmt.Errorf("failed to create directory %q: %v", dir, err)
+		}
 	}
 
 	watcher, err := fsnotify.NewWatcher()
@@ -135,8 +136,10 @@ func newWatcher(confDir string) (*fsnotify.Watcher, error) {
 		}
 	}()
 
-	if err = watcher.Add(confDir); err != nil {
-		return nil, fmt.Errorf("failed to add watch on %q: %v", confDir, err)
+	for _, dir := range dirs {
+		if err = watcher.Add(dir); err != nil {
+			return nil, fmt.Errorf("failed to add watch on %q: %v", dir, err)
+		}
 	}
 
 	return watcher, nil
@@ -152,8 +155,9 @@ func (plugin *cniNetworkPlugin) monitorConfDir(start *sync.WaitGroup) {
 			logrus.Infof("CNI monitoring event %v", event)
 
 			var defaultDeleted bool
-			createWrite := (event.Op&fsnotify.Create == fsnotify.Create ||
-				event.Op&fsnotify.Write == fsnotify.Write)
+			createWriteRename := (event.Op&fsnotify.Create == fsnotify.Create ||
+				event.Op&fsnotify.Write == fsnotify.Write ||
+				event.Op&fsnotify.Rename > 0)
 			if event.Op&fsnotify.Remove == fsnotify.Remove {
 				// Care about the event if the default network
 				// was just deleted
@@ -163,7 +167,7 @@ func (plugin *cniNetworkPlugin) monitorConfDir(start *sync.WaitGroup) {
 				}
 
 			}
-			if !createWrite && !defaultDeleted {
+			if !createWriteRename && !defaultDeleted {
 				continue
 			}
 
@@ -251,7 +255,7 @@ func initCNI(exec cniinvoke.Exec, cacheDir, defaultNetName string, confDir strin
 	plugin.syncNetworkConfig()
 
 	if useInotify {
-		plugin.watcher, err = newWatcher(plugin.confDir)
+		plugin.watcher, err = newWatcher(append([]string{plugin.confDir}, binDirs...))
 		if err != nil {
 			return nil, err
 		}

pkg/ocicni/ocicni_test.go

@@ -70,6 +70,8 @@ type fakeExec struct {
 	delIndex int
 	chkIndex int
 	plugins  []*fakePlugin
+
+	failFind bool
 }
 
 type TestConf struct {
@@ -167,6 +169,10 @@ func (f *fakeExec) ExecPlugin(ctx context.Context, pluginPath string, stdinData
 
 func (f *fakeExec) FindInPath(plugin string, paths []string) (string, error) {
 	Expect(len(paths)).To(BeNumerically(">", 0))
+
+	if f.failFind {
+		return "", fmt.Errorf("failed to find plugin %q in path %s", plugin, paths)
+	}
 	return filepath.Join(paths[0], plugin), nil
 }
 
@@ -180,6 +186,7 @@ func ensureCIDR(cidr string) *net.IPNet {
 var _ = Describe("ocicni operations", func() {
 	var (
 		tmpDir    string
+		tmpBinDir string
 		cacheDir  string
 		networkNS ns.NetNS
 	)
@@ -188,6 +195,8 @@ var _ = Describe("ocicni operations", func() {
 		var err error
 		tmpDir, err = ioutil.TempDir("", "ocicni_tmp")
 		Expect(err).NotTo(HaveOccurred())
+		tmpBinDir, err = ioutil.TempDir("", "ocicni_tmp_bin")
+		Expect(err).NotTo(HaveOccurred())
 		cacheDir, err = ioutil.TempDir("", "ocicni_cache")
 		Expect(err).NotTo(HaveOccurred())
 
@@ -201,6 +210,8 @@ var _ = Describe("ocicni operations", func() {
 
 		err := os.RemoveAll(tmpDir)
 		Expect(err).NotTo(HaveOccurred())
+		err = os.RemoveAll(tmpBinDir)
+		Expect(err).NotTo(HaveOccurred())
 		err = os.RemoveAll(cacheDir)
 		Expect(err).NotTo(HaveOccurred())
 	})
@@ -247,6 +258,41 @@ var _ = Describe("ocicni operations", func() {
 		ocicni.Shutdown()
 	})
 
+	It("finds an asynchronously written default network configuration whose plugin is written later", func() {
+		fExec := &fakeExec{failFind: true}
+		ocicni, err := initCNI(fExec, "", "test", tmpDir, true, tmpBinDir)
+		Expect(err).NotTo(HaveOccurred())
+
+		_, _, err = writeConfig(tmpDir, "10-test.conf", "test", "myplugin", "0.3.1")
+		Expect(err).NotTo(HaveOccurred())
+		Consistently(ocicni.Status, 5).ShouldNot(Succeed())
+
+		// 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)
+		Expect(err).NotTo(HaveOccurred())
+
+		tmp := ocicni.(*cniNetworkPlugin)
+		Eventually(func() error {
+			net := tmp.getDefaultNetwork()
+			if net == nil {
+				return fmt.Errorf("no default net")
+			}
+			if net.name != "test" {
+				return fmt.Errorf("name not test")
+			}
+			if len(net.config.Plugins) == 0 {
+				return fmt.Errorf("no plugins")
+			}
+			if net.config.Plugins[0].Network.Type != "myplugin" {
+				return fmt.Errorf("wrong plugin type")
+			}
+			return nil
+		}, 10).Should(Succeed())
+
+		ocicni.Shutdown()
+	})
+
 	It("finds and refinds an asynchronously written default network configuration", func() {
 		ocicni, err := initCNI(&fakeExec{}, "", "test", tmpDir, true, "/opt/cni/bin")
 		Expect(err).NotTo(HaveOccurred())