libpod/container_internal: Deprecate implicit hook directories

Part of the motivation for 800eb863 (Hooks supports two directories,
process default and override, 2018-09-17, #1487) was [1]:

> We only use this for override. The reason this was caught is people
> are trying to get hooks to work with CoreOS. You are not allowed to
> write to /usr/share... on CoreOS, so they wanted podman to also look
> at /etc, where users and third parties can write.

But we'd also been disabling hooks completely for rootless users.  And
even for root users, the override logic was tricky when folks actually
had content in both directories.  For example, if you wanted to
disable a hook from the default directory, you'd have to add a no-op
hook to the override directory.

Also, the previous implementation failed to handle the case where
there hooks defined in the override directory but the default
directory did not exist:

  $ podman version
  Version:       0.11.2-dev
  Go Version:    go1.10.3
  Git Commit:    "6df7409cb5a41c710164c42ed35e33b28f3f7214"
  Built:         Sun Dec  2 21:30:06 2018
  OS/Arch:       linux/amd64
  $ ls -l /etc/containers/oci/hooks.d/test.json
  -rw-r--r--. 1 root root 184 Dec  2 16:27 /etc/containers/oci/hooks.d/test.json
  $ podman --log-level=debug run --rm docker.io/library/alpine echo 'successful container' 2>&1 | grep -i hook
  time="2018-12-02T21:31:19-08:00" level=debug msg="reading hooks from /usr/share/containers/oci/hooks.d"
  time="2018-12-02T21:31:19-08:00" level=warning msg="failed to load hooks: {}%!(EXTRA *os.PathError=open /usr/share/containers/oci/hooks.d: no such file or directory)"

With this commit:

  $ podman --log-level=debug run --rm docker.io/library/alpine echo 'successful container' 2>&1 | grep -i hook
  time="2018-12-02T21:33:07-08:00" level=debug msg="reading hooks from /usr/share/containers/oci/hooks.d"
  time="2018-12-02T21:33:07-08:00" level=debug msg="reading hooks from /etc/containers/oci/hooks.d"
  time="2018-12-02T21:33:07-08:00" level=debug msg="added hook /etc/containers/oci/hooks.d/test.json"
  time="2018-12-02T21:33:07-08:00" level=debug msg="hook test.json matched; adding to stages [prestart]"
  time="2018-12-02T21:33:07-08:00" level=warning msg="implicit hook directories are deprecated; set --hooks-dir="/etc/containers/oci/hooks.d" explicitly to continue to load hooks from this directory"
  time="2018-12-02T21:33:07-08:00" level=error msg="container create failed: container_linux.go:336: starting container process caused "process_linux.go:399: container init caused \"process_linux.go:382: running prestart hook 0 caused \\\"error running hook: exit status 1, stdout: , stderr: oh, noes!\\\\n\\\"\""

(I'd setup the hook to error out).  You can see that it's silenly
ignoring the ENOENT for /usr/share/containers/oci/hooks.d and
continuing on to load hooks from /etc/containers/oci/hooks.d.

When it loads the hook, it also logs a warning-level message
suggesting that callers explicitly configure their hook directories.
That will help consumers migrate, so we can drop the implicit hook
directories in some future release.  When folks *do* explicitly
configure hook directories (via the newly-public --hooks-dir and
hooks_dir options), we error out if they're missing:

  $ podman --hooks-dir /does/not/exist run --rm docker.io/library/alpine echo 'successful container'
  error setting up OCI Hooks: open /does/not/exist: no such file or directory

I've dropped the trailing "path" from the old, hidden --hooks-dir-path
and hooks_dir_path because I think "dir(ectory)" is already enough
context for "we expect a path argument".  I consider this name change
non-breaking because the old forms were undocumented.

Coming back to rootless users, I've enabled hooks now.  I expect they
were previously disabled because users had no way to avoid
/usr/share/containers/oci/hooks.d which might contain hooks that
required root permissions.  But now rootless users will have to
explicitly configure hook directories, and since their default config
is from ~/.config/containers/libpod.conf, it's a misconfiguration if
it contains hooks_dir entries which point at directories with hooks
that require root access.  We error out so they can fix their
libpod.conf.

[1]: https://github.com/containers/libpod/pull/1487#discussion_r218149355

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

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]