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

libpod/container_internal: Deprecate implicit hook directories

10 files changed, 73 insertions, 59 deletions

diff --git a/cmd/podman/libpodruntime/runtime.go b/cmd/podman/libpodruntime/runtime.go
index a4b3581be..f69eaf3a4 100644
--- a/cmd/podman/libpodruntime/runtime.go
+++ b/cmd/podman/libpodruntime/runtime.go
@@ -90,8 +90,8 @@ func GetRuntimeWithStorageOpts(c *cli.Context, storageOpts *storage.StoreOptions
 	if c.GlobalIsSet("default-mounts-file") {
 		options = append(options, libpod.WithDefaultMountsFile(c.GlobalString("default-mounts-file")))
 	}
-	if c.GlobalIsSet("hooks-dir-path") {
-		options = append(options, libpod.WithHooksDir(c.GlobalString("hooks-dir-path")))
+	if c.GlobalIsSet("hooks-dir") {
+		options = append(options, libpod.WithHooksDir(c.GlobalStringSlice("hooks-dir")...))
 	}
 
 	// TODO flag to set CNI plugins dir?
diff --git a/cmd/podman/main.go b/cmd/podman/main.go
index 6be192593..bcae04575 100644
--- a/cmd/podman/main.go
+++ b/cmd/podman/main.go
@@ -8,7 +8,6 @@ import (
 	"syscall"
 
 	"github.com/containers/libpod/libpod"
-	"github.com/containers/libpod/pkg/hooks"
 	_ "github.com/containers/libpod/pkg/hooks/0.1.0"
 	"github.com/containers/libpod/pkg/rootless"
 	"github.com/containers/libpod/version"
@@ -206,11 +205,9 @@ func main() {
 			Usage:  "path to default mounts file",
 			Hidden: true,
 		},
-		cli.StringFlag{
-			Name:   "hooks-dir-path",
-			Usage:  "set the OCI hooks directory path",
-			Value:  hooks.DefaultDir,
-			Hidden: true,
+		cli.StringSliceFlag{
+			Name:  "hooks-dir",
+			Usage: "set the OCI hooks directory path (may be set multiple times)",
 		},
 		cli.IntFlag{
 			Name:   "max-workers",
diff --git a/docs/libpod.conf.5.md b/docs/libpod.conf.5.md
index 198e927ee..d63baeb88 100644
--- a/docs/libpod.conf.5.md
+++ b/docs/libpod.conf.5.md
@@ -24,6 +24,18 @@ libpod to manage containers.
 **cgroup_manager**=""
   Specify the CGroup Manager to use; valid values are "systemd" and "cgroupfs"
 
+**hooks_dir**=["*path*", ...]
+
+  Each `*.json` file in the path configures a hook for Podman containers.  For more details on the syntax of the JSON files and the semantics of hook injection, see `oci-hooks(5)`.  Podman and libpod currently support both the 1.0.0 and 0.1.0 hook schemas, although the 0.1.0 schema is deprecated.
+
+  Paths listed later in the array higher precedence (`oci-hooks(5)` discusses directory precedence).
+
+  For the annotation conditions, libpod uses any annotations set in the generated OCI configuration.
+
+  For the bind-mount conditions, only mounts explicitly requested by the caller via `--volume` are considered.  Bind mounts that libpod inserts by default (e.g. `/dev/shm`) are not considered.
+
+  If `hooks_dir` is unset for root callers, Podman and libpod will currently default to `/usr/share/containers/oci/hooks.d` and `/etc/containers/oci/hooks.d` in order of increasing precedence.  Using these defaults is deprecated, and callers should migrate to explicitly setting `hooks_dir`.
+
 **static_dir**=""
   Directory for persistent libpod files (database, etc)
   By default this will be configured relative to where containers/storage
diff --git a/docs/podman.1.md b/docs/podman.1.md
index b7433d850..bde349e6f 100644
--- a/docs/podman.1.md
+++ b/docs/podman.1.md
@@ -31,6 +31,18 @@ CGroup manager to use for container cgroups. Supported values are cgroupfs or sy
 
 Path to where the cpu performance results should be written
 
+**--hooks-dir**=**path**
+
+Each `*.json` file in the path configures a hook for Podman containers.  For more details on the syntax of the JSON files and the semantics of hook injection, see `oci-hooks(5)`.  Podman and libpod currently support both the 1.0.0 and 0.1.0 hook schemas, although the 0.1.0 schema is deprecated.
+
+This option may be set multiple times; paths from later options have higher precedence (`oci-hooks(5)` discusses directory precedence).
+
+For the annotation conditions, libpod uses any annotations set in the generated OCI configuration.
+
+For the bind-mount conditions, only mounts explicitly requested by the caller via `--volume` are considered.  Bind mounts that libpod inserts by default (e.g. `/dev/shm`) are not considered.
+
+If `--hooks-dir` is unset for root callers, Podman and libpod will currently default to `/usr/share/containers/oci/hooks.d` and `/etc/containers/oci/hooks.d` in order of increasing precedence.  Using these defaults is deprecated, and callers should migrate to explicitly setting `--hooks-dir`.
+
 **--log-level**
 
 Log messages above specified level: debug, info, warn, error (default), fatal or panic
@@ -161,18 +173,6 @@ the exit codes follow the `chroot` standard, see below:
 
     The mounts.conf file specifies volume mount directories that are automatically mounted inside containers when executing the `podman run` or `podman start` commands. When Podman runs in rootless mode, the file `$HOME/.config/containers/mounts.conf` is also used. Please refer to containers-mounts.conf(5) for further details.
 
-**OCI hooks JSON** (`/etc/containers/oci/hooks.d/*.json`, `/usr/share/containers/oci/hooks.d/*.json`)
-
-    Each `*.json` file in `/etc/containers/oci/hooks.d` and `/usr/share/containers/oci/hooks.d` configures a hook for Podman containers, with `/etc/containers/oci/hooks.d` having higher precedence.  For more details on the syntax of the JSON files and the semantics of hook injection, see `oci-hooks(5)`.
-
-    Podman and libpod currently support both the 1.0.0 and 0.1.0 hook schemas, although the 0.1.0 schema is deprecated.
-
-    For the annotation conditions, libpod uses any annotations set in the generated OCI configuration.
-
-    For the bind-mount conditions, only mounts explicitly requested by the caller via `--volume` are considered.  Bind mounts that libpod inserts by default (e.g. `/dev/shm`) are not considered.
-
-    Hooks are not used when running in rootless mode.
-
 **policy.json** (`/etc/containers/policy.json`)
 
     Signature verification policy files are used to specify policy, e.g. trusted keys, applicable when deciding whether to accept an image, or individual signatures of that image, as valid.
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]
diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go
index aaee7fa53..4a9bd4e46 100644
--- a/test/e2e/run_test.go
+++ b/test/e2e/run_test.go
@@ -336,7 +336,7 @@ var _ = Describe("Podman run", func() {
 		hooksDir := tempdir + "/hooks"
 		os.Mkdir(hooksDir, 0755)
 		fileutils.CopyFile("hooks/hooks.json", hooksDir)
-		os.Setenv("HOOK_OPTION", fmt.Sprintf("--hooks-dir-path=%s", hooksDir))
+		os.Setenv("HOOK_OPTION", fmt.Sprintf("--hooks-dir=%s", hooksDir))
 		os.Remove(hcheck)
 		session := podmanTest.Podman([]string{"run", ALPINE, "ls"})
 		session.Wait(10)