Improved hooks monitoring

...to work for specific edge cases with a simpler solution.
Re-reads hooks directories after any changes are detected by the watchers.
Added monitoring test for adding a different invalid hook to primary directory.
Some issues with prior code:
- ReadDir would stop when it encounters an invalid hook, rather than registering an error but continuing to read the valid hook.
- Wouldn’t account for Rename and Chmod events.
- After doing a mv of the hooks file instead of rm, it would still think the hooks file is in the directory, but it has been moved to another location.
- If a hook file was renamed, it would register the renamed file as a separate hook and not delete the original, so it would then execute the hook twice - once for the renamed file, and once for the original name which it did not delete.

Signed-off-by: samc24 <sam.chaturvedi24@gmail.com>

4 files changed, 37 insertions, 72 deletions

diff --git a/pkg/hooks/hooks.go b/pkg/hooks/hooks.go
index 5ed028b95..b962ffa5c 100644
--- a/pkg/hooks/hooks.go
+++ b/pkg/hooks/hooks.go
@@ -4,7 +4,6 @@ package hooks
 import (
 	"context"
 	"fmt"
-	"path/filepath"
 	"sort"
 	"strings"
 	"sync"
@@ -138,26 +137,3 @@ func (m *Manager) Hooks(config *rspec.Spec, annotations map[string]string, hasBi
 
 	return extensionStageHooks, nil
 }
-
-// remove remove a hook by name.
-func (m *Manager) remove(hook string) (ok bool) {
-	m.lock.Lock()
-	defer m.lock.Unlock()
-	_, ok = m.hooks[hook]
-	if ok {
-		delete(m.hooks, hook)
-	}
-	return ok
-}
-
-// add adds a hook by path
-func (m *Manager) add(path string) (err error) {
-	m.lock.Lock()
-	defer m.lock.Unlock()
-	hook, err := Read(path, m.extensionStages)
-	if err != nil {
-		return err
-	}
-	m.hooks[filepath.Base(path)] = hook
-	return nil
-}
diff --git a/pkg/hooks/monitor.go b/pkg/hooks/monitor.go
index febe3483f..c50b321f2 100644
--- a/pkg/hooks/monitor.go
+++ b/pkg/hooks/monitor.go
@@ -2,9 +2,8 @@ package hooks
 
 import (
 	"context"
-	"os"
-	"path/filepath"
 
+	current "github.com/containers/libpod/pkg/hooks/1.0.0"
 	"github.com/fsnotify/fsnotify"
 	"github.com/sirupsen/logrus"
 )
@@ -49,47 +48,11 @@ func (m *Manager) Monitor(ctx context.Context, sync chan<- error) {
 	for {
 		select {
 		case event := <-watcher.Events:
-			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)
-					}
-				}
-			} 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)
-					}
+			m.hooks = make(map[string]*current.Hook)
+			for _, dir := range m.directories {
+				err = ReadDir(dir, m.extensionStages, m.hooks)
+				if err != nil {
+					logrus.Errorf("failed loading hooks for %s: %v", event.Name, err)
 				}
 			}
 		case <-ctx.Done():
diff --git a/pkg/hooks/monitor_test.go b/pkg/hooks/monitor_test.go
index 31d7f9e39..dc67eaf83 100644
--- a/pkg/hooks/monitor_test.go
+++ b/pkg/hooks/monitor_test.go
@@ -226,7 +226,28 @@ func TestMonitorTwoDirGood(t *testing.T) {
 		assert.Equal(t, primaryInjected, config.Hooks) // masked by primary
 	})
 
-	t.Run("bad-primary-addition", func(t *testing.T) {
+	primaryPath2 := filepath.Join(primaryDir, "0a.json") //0a because it will be before a.json alphabetically
+
+	t.Run("bad-primary-new-addition", func(t *testing.T) {
+		err = ioutil.WriteFile(primaryPath2, []byte("{\"version\": \"-1\"}"), 0644)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
+
+		config := &rspec.Spec{}
+		fmt.Println("expected: ", config.Hooks)
+		expected := primaryInjected // 0a.json is bad, a.json is still good
+		_, err = manager.Hooks(config, map[string]string{}, false)
+		fmt.Println("actual: ", config.Hooks)
+		if err != nil {
+			t.Fatal(err)
+		}
+		assert.Equal(t, expected, config.Hooks)
+	})
+
+	t.Run("bad-primary-same-addition", func(t *testing.T) {
 		err = ioutil.WriteFile(primaryPath, []byte("{\"version\": \"-1\"}"), 0644)
 		if err != nil {
 			t.Fatal(err)
@@ -235,7 +256,7 @@ func TestMonitorTwoDirGood(t *testing.T) {
 		time.Sleep(100 * time.Millisecond) // wait for monitor to notice
 
 		config := &rspec.Spec{}
-		expected := config.Hooks
+		expected := fallbackInjected
 		_, err = manager.Hooks(config, map[string]string{}, false)
 		if err != nil {
 			t.Fatal(err)
diff --git a/pkg/hooks/read.go b/pkg/hooks/read.go
index d3995a0be..560ff1899 100644
--- a/pkg/hooks/read.go
+++ b/pkg/hooks/read.go
@@ -67,7 +67,7 @@ func ReadDir(path string, extensionStages []string, hooks map[string]*current.Ho
 	if err != nil {
 		return err
 	}
-
+	res := err
 	for _, file := range files {
 		filePath := filepath.Join(path, file.Name())
 		hook, err := Read(filePath, extensionStages)
@@ -80,12 +80,17 @@ func ReadDir(path string, extensionStages []string, hooks map[string]*current.Ho
 					continue
 				}
 			}
-			return err
+			if res == nil {
+				res = err
+			} else {
+				res = errors.Wrapf(res, "%v", err)
+			}
+			continue
 		}
 		hooks[file.Name()] = hook
 		logrus.Debugf("added hook %s", filePath)
 	}
-	return nil
+	return res
 }
 
 func init() {