lib/container_server: Deprecate implicit hook directories

The previous approach was breaking when the default hook directory was missing [1]: crio[15828]: level=debug msg="reading hooks from /usr/share/containers/oci/hooks.d" crio[15828]: level=debug msg="added hook /usr/share/containers/oci/hooks.d/test.json" crio[15828]: level=debug msg="reading hooks from /etc/containers/oci/hooks.d" crio[15828]: level=warning msg="failed to load hooks: open /etc/containers/oci/hooks.d: no such file or directory" With this commit, I've deprecated the implicit defaults in favor of having users explicitly configure their hook directories (either via --hooks-dir on the command line or hooks_dir in their config file). This is similar to the recent containers/podman@a4b483c8 (libpod/container_internal: Deprecate implicit hook directories, 2018-12-02, containers/podman#1920). Different from the libpod implementation is that CRI-O is only loading the hook manager at this point, so we don't know if the hook directories actually have hooks that will apply to future container (or even if they have hooks at all). I've just warned about their existence if the directories exist, to help poke folks into migrating to explicitly-configured directories. Another difference is that we get a single manager, so we need to ensure the default directories will work before feeding them in. I'm now stat'ing them to check, which will address [1]. But that's racy, since the directory could exist at stat time but be gone by the time the manager tries to load it. I'm ok with the race, because that entire code path is deprecated anyway. There's no race for the "user explicitly asked me to load /whatever and now it's gone" case. Now that the default case is pre-checking, I've tightened 50d4993 (lib: Use libpod's hooks package, 2018-04-07, #1517)'s load-failure warning down to a fatal error. I've dropped the trailing "path" from the old, hidden --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 form was undocumented. I've also changed hooks_dir_path to hooks_dir and changed it from a string to a []string. That is potentially a breaking change because hooks_dir_path became publicly documented in 50539c3 (crio.conf(5): update manpage to the latest state, 2018-08-22, #1749). But it's only been a few months since it landed, and making that setting public was only one of a few changes it had made. So I'm reasonably optimistic that there aren't (m)any users explicitly setting hooks_dir_path yet, and haven't gone through the trouble of continuing to support the old setting. [1]: #1942 (comment) Signed-off-by: W. Trevor King <[email protected]>
cri-o · Dec 4, 2018 · 461af28 · 461af28
1 parent 8740dd5
commit 461af28
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 48 deletions.
diff --git a/cmd/crio/config.go b/cmd/crio/config.go
@@ -179,8 +179,9 @@ default_sysctls = [
 additional_devices = [
 {{ range $device := .AdditionalDevices}}{{ printf "\t%q, \n" $device}}{{ end }}]
 
-# Path to the OCI hooks directory for automatically executed hooks.
-hooks_dir_path = "{{ .HooksDirPath }}"
+# Path to OCI hooks directories for automatically executed hooks.
+hooks_dir = [
+{{ range $hooksDir := .HooksDir }}{{ printf "\t%q\n" $hooksDir}}{{ end }}]
 
 # List of default mounts for each container. **Deprecated:** this option will
 # be removed in future versions in favor of default_mounts_file.

diff --git a/cmd/crio/main.go b/cmd/crio/main.go
@@ -15,7 +15,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/containers/libpod/pkg/hooks"
 	_ "github.com/containers/libpod/pkg/hooks/0.1.0"
 	"github.com/containers/libpod/pkg/spec"
 	"github.com/containers/storage/pkg/reexec"
@@ -226,8 +225,8 @@ func mergeConfig(config *server.Config, ctx *cli.Context) error {
 	if ctx.GlobalIsSet("cgroup-manager") {
 		config.CgroupManager = ctx.GlobalString("cgroup-manager")
 	}
-	if ctx.GlobalIsSet("hooks-dir-path") {
-		config.HooksDirPath = ctx.GlobalString("hooks-dir-path")
+	if ctx.GlobalIsSet("hooks-dir") {
+		config.HooksDir = ctx.GlobalStringSlice("hooks-dir")
 	}
 	if ctx.GlobalIsSet("default-mounts") {
 		config.DefaultMounts = ctx.GlobalStringSlice("default-mounts")
@@ -477,11 +476,9 @@ func main() {
 			Value: string(lib.ImageVolumesMkdir),
 			Usage: "image volume handling ('mkdir', 'bind', or 'ignore')",
 		},
-		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.StringSliceFlag{
 			Name:  "default-mounts",

diff --git a/docs/crio.8.md b/docs/crio.8.md
@@ -19,6 +19,7 @@ crio
 [--enable-metrics]
 [--gid-mappings=[value]]
 [--help|-h]
+[--hooks-dir=[value]]
 [--insecure-registry=[value]]
 [--listen=[value]]
 [--log=[value]]
@@ -82,6 +83,18 @@ crio [GLOBAL OPTIONS] config [OPTIONS]
 
 **--help, -h**: Print usage statement
 
+**--hooks-dir**=["*path*", ...]
+
+Each `*.json` file in the path configures a hook for CRI-O containers.  For more details on the syntax of the JSON files and the semantics of hook injection, see `oci-hooks(5)`.  CRI-O 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, CRI-O uses the Kubernetes annotations, which are a subset of the annotations passed to the OCI runtime.  For example, `io.kubernetes.cri-o.Volumes` is part of the OCI runtime configuration annotations, but it is not part of the Kubernetes annotations being matched for hooks.
+
+For the bind-mount conditions, only mounts explicitly requested by Kubernetes configuration are considered.  Bind mounts that CRI-O inserts by default (e.g. `/dev/shm`) are not considered.
+
+If `hooks_dir` is unset, CRI-O 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`.
+
 **--insecure-registry=**: Enable insecure registry communication, i.e., enable un-encrypted and/or untrusted communication.
 
 1. List of insecure registries can contain an element with CIDR notation to specify a whole subnet.
@@ -154,16 +167,6 @@ it later with **--config**. Global options will modify the output.
 **crio.conf** (`/etc/crio/crio.conf`)
   `cri-o` configuration file for all of the available command-line options for the crio(8) program, but in a TOML format that can be more easily modified and versioned.
 
-**hook 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 CRI-O containers, with `/etc/containers/oci/hooks.d` having higher precedence.  `crio(8)` monitors the hook directories for changes, so there is no need to restart the server after adjusting the hook configuration.  For more details on the syntax of the JSON files and the semantics of hook injection, see `oci-hooks(5)`.
-
-  CRI-O currently supports both the 1.0.0 and 0.1.0 hook schemas, although the 0.1.0 schema is deprecated.
-
-  For the annotation conditions, CRI-O uses the Kubernetes annotations, which are a subset of the annotations passed to the OCI runtime.  For example, io.kubernetes.cri-o.Volumes is part of the OCI runtime configuration annotations, but it is not part of the Kubernetes annotations being matched for hooks.
-
-  For the bind-mount conditions, only mounts explicitly requested by Kubernetes configuration are considered.  Bind mounts that CRI-O inserts by default (e.g. `/dev/shm`) are not considered.
-
 **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/docs/crio.conf.5.md b/docs/crio.conf.5.md
@@ -132,8 +132,16 @@ The `crio.runtime` table contains settings pertaining to the OCI runtime used an
 **additional_devices**=[]
   List of additional devices. If it is empty or commented out, only the devices defined in the container json file by the user/kube will be added.
 
-**hooks_dir_path**="/usr/share/containers/oci/hooks.d"
-  Path to the OCI hooks directory for automatically executed hooks.
+**hooks_dir**=["*path*", ...]
+  Each `*.json` file in the path configures a hook for CRI-O containers.  For more details on the syntax of the JSON files and the semantics of hook injection, see `oci-hooks(5)`.  CRI-O 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, CRI-O uses the Kubernetes annotations, which are a subset of the annotations passed to the OCI runtime.  For example, `io.kubernetes.cri-o.Volumes` is part of the OCI runtime configuration annotations, but it is not part of the Kubernetes annotations being matched for hooks.
+
+  For the bind-mount conditions, only mounts explicitly requested by Kubernetes configuration are considered.  Bind mounts that CRI-O inserts by default (e.g. `/dev/shm`) are not considered.
+
+  If `hooks_dir` is unset, CRI-O 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`.
 
 **default_mounts**=[]
   List of default mounts for each container. **Deprecated:** this option will be removed in future versions in favor of `default_mounts_file`.

diff --git a/lib/config.go b/lib/config.go
@@ -7,7 +7,6 @@ import (
 	"github.com/BurntSushi/toml"
 	"github.com/containers/image/pkg/sysregistries"
 	"github.com/containers/image/types"
-	"github.com/containers/libpod/pkg/hooks"
 	"github.com/containers/storage"
 	"github.com/kubernetes-sigs/cri-o/oci"
 )
@@ -181,8 +180,11 @@ type RuntimeConfig struct {
 	// handle cgroups for containers.
 	CgroupManager string `toml:"cgroup_manager"`
 
-	// HooksDirPath location of oci hooks config files
-	HooksDirPath string `toml:"hooks_dir_path"`
+	// 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"`
 
 	// DefaultMounts is the list of mounts to be mounted for each container
 	// The format of each mount is "host-path:container-path"
@@ -384,7 +386,6 @@ func DefaultConfig() *Config {
 			PidsLimit:                DefaultPidsLimit,
 			ContainerExitsDir:        containerExitsDir,
 			ContainerAttachSocketDir: oci.ContainerAttachSocketDir,
-			HooksDirPath:             hooks.DefaultDir,
 			LogSizeMax:               DefaultLogSizeMax,
 			DefaultCapabilities:      DefaultCapabilities,
 			LogLevel:                 "error",

diff --git a/lib/container_server.go b/lib/container_server.go
@@ -145,22 +145,6 @@ func New(ctx context.Context, config *Config) (*ContainerServer, error) {
 		lock = new(sync.Mutex)
 	}
 
-	hookDirectories := []string{}
-	missingHookDirectoryFatal := false
-
-	// If hooks directory is set in config use it
-	if config.HooksDirPath != "" {
-		hookDirectories = append(hookDirectories, config.HooksDirPath)
-
-		// If user overrode default hooks, this means it is in a test, so don't
-		// use OverrideHooksDirPath
-		if config.HooksDirPath == hooks.DefaultDir {
-			hookDirectories = append(hookDirectories, hooks.OverrideDir)
-		} else {
-			missingHookDirectoryFatal = true
-		}
-	}
-
 	var locale string
 	var ok bool
 	for _, envVar := range []string{
@@ -188,12 +172,20 @@ func New(ctx context.Context, config *Config) (*ContainerServer, error) {
 		}
 	}
 
+	hookDirectories := config.HooksDir
+	if config.HooksDir == nil {
+		for _, hooksDir := range []string{hooks.DefaultDir, hooks.OverrideDir} {
+			_, err = os.Stat(hooksDir)
+			if err == nil {
+				hookDirectories = append(hookDirectories, hooksDir)
+				logrus.Warnf("implicit hook directories are deprecated; set --hooks-dir=%q explicitly to continue to load hooks from this directory", hooksDir)
+			}
+		}
+	}
+
 	hooks, err := hooks.New(ctx, hookDirectories, []string{}, lang)
 	if err != nil {
-		if missingHookDirectoryFatal || !os.IsNotExist(err) {
-			return nil, err
-		}
-		logrus.Warnf("failed to load hooks: %v", err)
+		return nil, err
 	}
 
 	return &ContainerServer{

diff --git a/lib/testdata/config.toml b/lib/testdata/config.toml
@@ -12,7 +12,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"
     ctr_stop_timeout = 10

diff --git a/test/helpers.bash b/test/helpers.bash
@@ -74,7 +74,7 @@ export REGISTRIES_CONFIG_PATH="$INTEGRATION_ROOT/registries.conf"
 # Setup default hooks dir
 HOOKSDIR=$TESTDIR/hooks
 mkdir ${HOOKSDIR}
-HOOKS_OPTS="--hooks-dir-path=$HOOKSDIR"
+HOOKS_OPTS="--hooks-dir=$HOOKSDIR"
 
 # Setup default mounts using deprecated --default-mounts flag
 # should be removed, once the flag is removed