hooks: Fix monitoring of multiple directories

This isn't an issue with podman, which will only ever use one
directory.  But CRI-O generally uses two directories, and we want to
make sure that changes to the fallback directory are not clobbering
hooks configured in the override directory.  More background in [1].

I've split the handling into a single-directory block and a
multiple-directory block so we don't waste time polling the filesystem
for single-directory removals.

I'm using the single-directory block for the the zero-directory case
as well.  Managers with zero directories should not be receiving
fsnotify events, so I don't think it really matters which block
handles them.  If we want to handle this case robustly (because we're
concerned about something in the hook package adjusted the private
.directories property on the fly?), then we'll probably want to add an
explicit zero-directory block in future work.

[1]: https://github.com/kubernetes-incubator/cri-o/pull/1470

Signed-off-by: W. Trevor King <wking@tremily.us>

Closes: #757
Approved by: rhatdan

3 files changed, 236 insertions, 15 deletions

diff --git a/pkg/hooks/hooks.go b/pkg/hooks/hooks.go
index 6413829ee..66e97d6bd 100644
--- a/pkg/hooks/hooks.go
+++ b/pkg/hooks/hooks.go
@@ -1,4 +1,4 @@
-// Package hooks implements CRI-O's hook handling.
+// Package hooks implements hook configuration and handling for CRI-O and libpod.
 package hooks
 
 import (
diff --git a/pkg/hooks/monitor.go b/pkg/hooks/monitor.go
index ba5e0f246..febe3483f 100644
--- a/pkg/hooks/monitor.go
+++ b/pkg/hooks/monitor.go
@@ -2,6 +2,7 @@ package hooks
 
 import (
 	"context"
+	"os"
 	"path/filepath"
 
 	"github.com/fsnotify/fsnotify"
@@ -11,7 +12,7 @@ import (
 // Monitor dynamically monitors hook directories for additions,
 // updates, and removals.
 //
-// This function write two empty structs to the sync channel: the
+// This function writes two empty structs to the sync channel: the
 // first is written after the watchers are established and the second
 // when this function exits.  The expected usage is:
 //
@@ -48,18 +49,47 @@ func (m *Manager) Monitor(ctx context.Context, sync chan<- error) {
 	for {
 		select {
 		case event := <-watcher.Events:
-			if event.Op&fsnotify.Remove == fsnotify.Remove {
-				ok := m.remove(filepath.Base(event.Name))
-				if ok {
-					logrus.Debugf("removed hook %s", event.Name)
+			filename := filepath.Base(event.Name)
+			if len(m.directories) <= 1 {
+				if event.Op&fsnotify.Remove == fsnotify.Remove {
+					ok := m.remove(filename)
+					if ok {
+						logrus.Debugf("removed hook %s", event.Name)
+					}
+				} else if event.Op&fsnotify.Create == fsnotify.Create || event.Op&fsnotify.Write == fsnotify.Write {
+					err = m.add(event.Name)
+					if err == nil {
+						logrus.Debugf("added hook %s", event.Name)
+					} else if err != ErrNoJSONSuffix {
+						logrus.Errorf("failed to add hook %s: %v", event.Name, err)
+					}
 				}
-			}
-			if event.Op&fsnotify.Create == fsnotify.Create || event.Op&fsnotify.Write == fsnotify.Write {
-				err = m.add(event.Name)
-				if err == nil {
-					logrus.Debugf("added hook %s", event.Name)
-				} else if err != ErrNoJSONSuffix {
-					logrus.Errorf("failed to add hook %s: %v", event.Name, err)
+			} else if event.Op&fsnotify.Create == fsnotify.Create || event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Remove == fsnotify.Remove {
+				err = nil
+				found := false
+				for i := len(m.directories) - 1; i >= 0; i-- {
+					path := filepath.Join(m.directories[i], filename)
+					err = m.add(path)
+					if err == nil {
+						found = true
+						logrus.Debugf("(re)added hook %s (triggered activity on %s)", path, event.Name)
+						break
+					} else if err == ErrNoJSONSuffix {
+						found = true
+						break // this is not going to change for fallback directories
+					} else if os.IsNotExist(err) {
+						continue // move on to the next fallback directory
+					} else {
+						found = true
+						logrus.Errorf("failed to (re)add hook %s (triggered by activity on %s): %v", path, event.Name, err)
+						break
+					}
+				}
+				if (found || event.Op&fsnotify.Remove == fsnotify.Remove) && err != nil {
+					ok := m.remove(filename)
+					if ok {
+						logrus.Debugf("removed hook %s (triggered by activity on %s)", filename, event.Name)
+					}
 				}
 			}
 		case <-ctx.Done():
diff --git a/pkg/hooks/monitor_test.go b/pkg/hooks/monitor_test.go
index 78e8d3fe5..b3af4bb43 100644
--- a/pkg/hooks/monitor_test.go
+++ b/pkg/hooks/monitor_test.go
@@ -14,7 +14,7 @@ import (
 	"golang.org/x/text/language"
 )
 
-func TestMonitorGood(t *testing.T) {
+func TestMonitorOneDirGood(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	dir, err := ioutil.TempDir("", "hooks-test-")
 	if err != nil {
@@ -92,7 +92,7 @@ func TestMonitorGood(t *testing.T) {
 	})
 
 	t.Run("bad-addition", func(t *testing.T) {
-		err = ioutil.WriteFile(jsonPath, []byte("{\"version\": \"-1\"]}"), 0644)
+		err = ioutil.WriteFile(jsonPath, []byte("{\"version\": \"-1\"}"), 0644)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -118,6 +118,197 @@ func TestMonitorGood(t *testing.T) {
 	assert.Equal(t, context.Canceled, err)
 }
 
+func TestMonitorTwoDirGood(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	primaryDir, err := ioutil.TempDir("", "hooks-test-primary-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(primaryDir)
+
+	fallbackDir, err := ioutil.TempDir("", "hooks-test-fallback-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(fallbackDir)
+
+	lang, err := language.Parse("und-u-va-posix")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	manager, err := New(ctx, []string{fallbackDir, primaryDir}, []string{}, lang)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	sync := make(chan error, 2)
+	go manager.Monitor(ctx, sync)
+	err = <-sync
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	fallbackPath := filepath.Join(fallbackDir, "a.json")
+	fallbackJSON := []byte(fmt.Sprintf("{\"version\": \"1.0.0\", \"hook\": {\"path\": \"%s\"}, \"when\": {\"always\": true}, \"stages\": [\"prestart\"]}", path))
+	fallbackInjected := &rspec.Hooks{
+		Prestart: []rspec.Hook{
+			{
+				Path: path,
+			},
+		},
+	}
+
+	t.Run("good-fallback-addition", func(t *testing.T) {
+		err = ioutil.WriteFile(fallbackPath, fallbackJSON, 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		assert.Equal(t, fallbackInjected, config.Hooks)
+	})
+
+	primaryPath := filepath.Join(primaryDir, "a.json")
+	primaryJSON := []byte(fmt.Sprintf("{\"version\": \"1.0.0\", \"hook\": {\"path\": \"%s\", \"timeout\": 1}, \"when\": {\"always\": true}, \"stages\": [\"prestart\"]}", path))
+	one := 1
+	primaryInjected := &rspec.Hooks{
+		Prestart: []rspec.Hook{
+			{
+				Path:    path,
+				Timeout: &one,
+			},
+		},
+	}
+
+	t.Run("good-primary-override", func(t *testing.T) {
+		err = ioutil.WriteFile(primaryPath, primaryJSON, 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		assert.Equal(t, primaryInjected, config.Hooks)
+	})
+
+	t.Run("good-fallback-removal", func(t *testing.T) {
+		err = os.Remove(fallbackPath)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.Equal(t, primaryInjected, config.Hooks) // masked by primary
+	})
+
+	t.Run("good-fallback-restore", func(t *testing.T) {
+		err = ioutil.WriteFile(fallbackPath, fallbackJSON, 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.Equal(t, primaryInjected, config.Hooks) // masked by primary
+	})
+
+	t.Run("bad-primary-addition", func(t *testing.T) {
+		err = ioutil.WriteFile(primaryPath, []byte("{\"version\": \"-1\"}"), 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		expected := config.Hooks
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.Equal(t, expected, config.Hooks)
+	})
+
+	t.Run("good-primary-removal", func(t *testing.T) {
+		err = os.Remove(primaryPath)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.Equal(t, fallbackInjected, config.Hooks)
+	})
+
+	t.Run("good-non-json-addition", func(t *testing.T) {
+		err = ioutil.WriteFile(filepath.Join(fallbackDir, "README"), []byte("Hello"), 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		assert.Equal(t, fallbackInjected, config.Hooks)
+	})
+
+	t.Run("good-fallback-removal", func(t *testing.T) {
+		err = os.Remove(fallbackPath)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		expected := config.Hooks
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.Equal(t, expected, config.Hooks)
+	})
+
+	cancel()
+	err = <-sync
+	assert.Equal(t, context.Canceled, err)
+}
+
 func TestMonitorBadWatcher(t *testing.T) {
 	ctx := context.Background()