Merge pull request #1920 from wking/explicit-hooks-dirs

libpod/container_internal: Deprecate implicit hook directories

5 files changed, 43 insertions, 38 deletions

diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index e31a8099c..934ad7a22 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -1168,10 +1168,6 @@ func (c *Container) saveSpec(spec *spec.Spec) error {
 }
 
 func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (extensionStageHooks map[string][]spec.Hook, err error) {
-	if len(c.runtime.config.HooksDir) == 0 {
-		return nil, nil
-	}
-
 	var locale string
 	var ok bool
 	for _, envVar := range []string{
@@ -1199,25 +1195,39 @@ func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (exten
 		}
 	}
 
-	allHooks := make(map[string][]spec.Hook)
-	for _, hDir := range c.runtime.config.HooksDir {
-		manager, err := hooks.New(ctx, []string{hDir}, []string{"poststop"}, lang)
-		if err != nil {
-			if c.runtime.config.HooksDirNotExistFatal || !os.IsNotExist(err) {
-				return nil, err
-			}
-			logrus.Warnf("failed to load hooks: %q", err)
+	if c.runtime.config.HooksDir == nil {
+		if rootless.IsRootless() {
 			return nil, nil
 		}
-		hooks, err := manager.Hooks(config, c.Spec().Annotations, len(c.config.UserVolumes) > 0)
-		if err != nil {
-			return nil, err
-		}
-		for i, hook := range hooks {
-			allHooks[i] = hook
+		allHooks := make(map[string][]spec.Hook)
+		for _, hDir := range []string{hooks.DefaultDir, hooks.OverrideDir} {
+			manager, err := hooks.New(ctx, []string{hDir}, []string{"poststop"}, lang)
+			if err != nil {
+				if os.IsNotExist(err) {
+					continue
+				}
+				return nil, err
+			}
+			hooks, err := manager.Hooks(config, c.Spec().Annotations, len(c.config.UserVolumes) > 0)
+			if err != nil {
+				return nil, err
+			}
+			if len(hooks) > 0 || config.Hooks != nil {
+				logrus.Warnf("implicit hook directories are deprecated; set --hooks-dir=%q explicitly to continue to load hooks from this directory", hDir)
+			}
+			for i, hook := range hooks {
+				allHooks[i] = hook
+			}
 		}
+		return allHooks, nil
 	}
-	return allHooks, nil
+
+	manager, err := hooks.New(ctx, c.runtime.config.HooksDir, []string{"poststop"}, lang)
+	if err != nil {
+		return nil, err
+	}
+
+	return manager.Hooks(config, c.Spec().Annotations, len(c.config.UserVolumes) > 0)
 }
 
 // mount mounts the container's root filesystem
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 8861d7728..780bf5279 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -224,10 +224,8 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 		}
 	}
 
-	if !rootless.IsRootless() {
-		if c.state.ExtensionStageHooks, err = c.setupOCIHooks(ctx, g.Config); err != nil {
-			return nil, errors.Wrapf(err, "error setting up OCI Hooks")
-		}
+	if c.state.ExtensionStageHooks, err = c.setupOCIHooks(ctx, g.Config); err != nil {
+		return nil, errors.Wrapf(err, "error setting up OCI Hooks")
 	}
 
 	// Bind builtin image volumes
diff --git a/libpod/options.go b/libpod/options.go
index 7f4e3ac6b..e1d0b5007 100644
--- a/libpod/options.go
+++ b/libpod/options.go
@@ -179,21 +179,20 @@ func WithStaticDir(dir string) RuntimeOption {
 	}
 }
 
-// WithHooksDir sets the directory to look for OCI runtime hooks config.
-// Note we are not saving this in database, since this is really just for used
-// for testing.
-func WithHooksDir(hooksDir string) RuntimeOption {
+// WithHooksDir sets the directories to look for OCI runtime hook configuration.
+func WithHooksDir(hooksDirs ...string) RuntimeOption {
 	return func(rt *Runtime) error {
 		if rt.valid {
 			return ErrRuntimeFinalized
 		}
 
-		if hooksDir == "" {
-			return errors.Wrap(ErrInvalidArg, "empty-string hook directories are not supported")
+		for _, hooksDir := range hooksDirs {
+			if hooksDir == "" {
+				return errors.Wrap(ErrInvalidArg, "empty-string hook directories are not supported")
+			}
 		}
 
-		rt.config.HooksDir = []string{hooksDir}
-		rt.config.HooksDirNotExistFatal = true
+		rt.config.HooksDir = hooksDirs
 		return nil
 	}
 }
diff --git a/libpod/runtime.go b/libpod/runtime.go
index 9feae03fc..e043715b1 100644
--- a/libpod/runtime.go
+++ b/libpod/runtime.go
@@ -12,7 +12,6 @@ import (
 	"github.com/containers/image/types"
 	"github.com/containers/libpod/libpod/image"
 	"github.com/containers/libpod/pkg/firewall"
-	"github.com/containers/libpod/pkg/hooks"
 	sysreg "github.com/containers/libpod/pkg/registries"
 	"github.com/containers/libpod/pkg/rootless"
 	"github.com/containers/libpod/pkg/util"
@@ -141,11 +140,11 @@ type RuntimeConfig struct {
 	// CNIDefaultNetwork is the network name of the default CNI network
 	// to attach pods to
 	CNIDefaultNetwork string `toml:"cni_default_network,omitempty"`
-	// HooksDir Path to the directory containing hooks configuration files
+	// HooksDir holds paths to the directories containing hooks
+	// configuration files. When the same filename is present in in
+	// multiple directories, the file in the directory listed last in
+	// this slice takes precedence.
 	HooksDir []string `toml:"hooks_dir"`
-	// HooksDirNotExistFatal switches between fatal errors and non-fatal
-	// warnings if the configured HooksDir does not exist.
-	HooksDirNotExistFatal bool `toml:"hooks_dir_not_exist_fatal"`
 	// DefaultMountsFile is the path to the default mounts file for testing
 	// purposes only
 	DefaultMountsFile string `toml:"-"`
@@ -203,7 +202,6 @@ var (
 			"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
 		},
 		CgroupManager:         SystemdCgroupsManager,
-		HooksDir:              []string{hooks.DefaultDir, hooks.OverrideDir},
 		StaticDir:             filepath.Join(storage.DefaultStoreOptions.GraphRoot, "libpod"),
 		TmpDir:                "",
 		MaxLogSize:            -1,
diff --git a/libpod/testdata/config.toml b/libpod/testdata/config.toml
index e19d36017..1d78f2083 100644
--- a/libpod/testdata/config.toml
+++ b/libpod/testdata/config.toml
@@ -14,7 +14,7 @@
     seccomp_profile = "/etc/crio/seccomp.json"
     apparmor_profile = "crio-default"
     cgroup_manager = "cgroupfs"
-    hooks_dir_path = "/usr/share/containers/oci/hooks.d"
+    hooks_dir = ["/usr/share/containers/oci/hooks.d"]
     pids_limit = 2048
     container_exits_dir = "/var/run/podman/exits"
   [crio.image]