Merge pull request #1830 from wking/config-filter-hooks

hooks: Add pre-create hooks for runtime-config manipulation

8 files changed, 378 insertions, 17 deletions

diff --git a/docs/libpod.conf.5.md b/docs/libpod.conf.5.md
index c02d247fb..98eb5bece 100644
--- a/docs/libpod.conf.5.md
+++ b/docs/libpod.conf.5.md
@@ -37,7 +37,9 @@ libpod to manage containers.
 
   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`.
+  Podman and libpod currently support an additional `precreate` state which is called before the runtime's `create` operation.  Unlike the other stages, which receive the container state on their standard input, `precreate` hooks receive the proposed runtime configuration on their standard input.  They may alter that configuration as they see fit, and write the altered form to their standard output.
+
+  **WARNING**: the `precreate` hook lets you do powerful things, such as adding additional mounts to the runtime configuration.  That power also makes it easy to break things.  Before reporting libpod errors, try running your container with `precreate` hooks disabled to see if the problem is due to one of your hooks.
 
 **static_dir**=""
   Directory for persistent libpod files (database, etc)
diff --git a/docs/podman.1.md b/docs/podman.1.md
index bde349e6f..a73ebb55e 100644
--- a/docs/podman.1.md
+++ b/docs/podman.1.md
@@ -43,6 +43,10 @@ For the bind-mount conditions, only mounts explicitly requested by the caller vi
 
 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`.
 
+Podman and libpod currently support an additional `precreate` state which is called before the runtime's `create` operation.  Unlike the other stages, which receive the container state on their standard input, `precreate` hooks receive the proposed runtime configuration on their standard input.  They may alter that configuration as they see fit, and write the altered form to their standard output.
+
+**WARNING**: the `precreate` hook lets you do powerful things, such as adding additional mounts to the runtime configuration.  That power also makes it easy to break things.  Before reporting libpod errors, try running your container with `precreate` hooks disabled to see if the problem is due to one of your hooks.
+
 **--log-level**
 
 Log messages above specified level: debug, info, warn, error (default), fatal or panic
diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index cc4c36bc9..69df33bc9 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -1181,6 +1181,7 @@ func (c *Container) saveSpec(spec *spec.Spec) error {
 	return nil
 }
 
+// Warning: precreate hooks may alter 'config' in place.
 func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (extensionStageHooks map[string][]spec.Hook, err error) {
 	var locale string
 	var ok bool
@@ -1209,13 +1210,13 @@ func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (exten
 		}
 	}
 
+	allHooks := make(map[string][]spec.Hook)
 	if c.runtime.config.HooksDir == nil {
 		if rootless.IsRootless() {
 			return nil, nil
 		}
-		allHooks := make(map[string][]spec.Hook)
 		for _, hDir := range []string{hooks.DefaultDir, hooks.OverrideDir} {
-			manager, err := hooks.New(ctx, []string{hDir}, []string{"poststop"}, lang)
+			manager, err := hooks.New(ctx, []string{hDir}, []string{"precreate", "poststop"}, lang)
 			if err != nil {
 				if os.IsNotExist(err) {
 					continue
@@ -1233,19 +1234,32 @@ func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (exten
 				allHooks[i] = hook
 			}
 		}
-		return allHooks, nil
+	} else {
+		manager, err := hooks.New(ctx, c.runtime.config.HooksDir, []string{"precreate", "poststop"}, lang)
+		if err != nil {
+			if os.IsNotExist(err) {
+				logrus.Warnf("Requested OCI hooks directory %q does not exist", c.runtime.config.HooksDir)
+				return nil, nil
+			}
+			return nil, err
+		}
+
+		allHooks, err = manager.Hooks(config, c.Spec().Annotations, len(c.config.UserVolumes) > 0)
+		if err != nil {
+			return nil, err
+		}
 	}
 
-	manager, err := hooks.New(ctx, c.runtime.config.HooksDir, []string{"poststop"}, lang)
+	hookErr, err := exec.RuntimeConfigFilter(ctx, allHooks["precreate"], config, exec.DefaultPostKillTimeout)
 	if err != nil {
-		if os.IsNotExist(err) {
-			logrus.Warnf("Requested OCI hooks directory %q does not exist", c.runtime.config.HooksDir)
-			return nil, nil
+		logrus.Warnf("container %s: precreate hook: %v", c.ID(), err)
+		if hookErr != nil && hookErr != err {
+			logrus.Debugf("container %s: precreate hook (hook error): %v", c.ID(), hookErr)
 		}
 		return nil, err
 	}
 
-	return manager.Hooks(config, c.Spec().Annotations, len(c.config.UserVolumes) > 0)
+	return allHooks, nil
 }
 
 // mount mounts the container's root filesystem
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 0745b7732..c66be1061 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -228,10 +228,6 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 		}
 	}
 
-	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
 	if c.config.Rootfs == "" && c.config.ImageVolumes {
 		if err := c.addLocalVolumes(ctx, &g, execUser); err != nil {
@@ -384,6 +380,12 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 		logrus.Debugf("set root propagation to %q", rootPropagation)
 		g.SetLinuxRootPropagation(rootPropagation)
 	}
+
+	// Warning: precreate hooks may alter g.Config in place.
+	if c.state.ExtensionStageHooks, err = c.setupOCIHooks(ctx, g.Config); err != nil {
+		return nil, errors.Wrapf(err, "error setting up OCI Hooks")
+	}
+
 	return g.Config, nil
 }
 
diff --git a/pkg/hooks/exec/exec.go b/pkg/hooks/exec/exec.go
index 94469b1d2..0dd091561 100644
--- a/pkg/hooks/exec/exec.go
+++ b/pkg/hooks/exec/exec.go
@@ -10,6 +10,7 @@ import (
 	"time"
 
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
 )
 
 // DefaultPostKillTimeout is the recommended default post-kill timeout.
@@ -42,7 +43,11 @@ func Run(ctx context.Context, hook *rspec.Hook, state []byte, stdout io.Writer,
 	}
 	exit := make(chan error, 1)
 	go func() {
-		exit <- cmd.Wait()
+		err := cmd.Wait()
+		if err != nil {
+			err = errors.Wrapf(err, "executing %v", cmd.Args)
+		}
+		exit <- err
 	}()
 
 	select {
diff --git a/pkg/hooks/exec/exec_test.go b/pkg/hooks/exec/exec_test.go
index 62e45ff3a..7aac315cb 100644
--- a/pkg/hooks/exec/exec_test.go
+++ b/pkg/hooks/exec/exec_test.go
@@ -163,14 +163,14 @@ func TestRunCancel(t *testing.T) {
 			name:              "context timeout",
 			contextTimeout:    time.Duration(1) * time.Second,
 			expectedStdout:    "waiting\n",
-			expectedHookError: "^signal: killed$",
+			expectedHookError: "^executing \\[sh -c echo waiting; sleep 2; echo done]: signal: killed$",
 			expectedRunError:  context.DeadlineExceeded,
 		},
 		{
 			name:              "hook timeout",
 			hookTimeout:       &one,
 			expectedStdout:    "waiting\n",
-			expectedHookError: "^signal: killed$",
+			expectedHookError: "^executing \\[sh -c echo waiting; sleep 2; echo done]: signal: killed$",
 			expectedRunError:  context.DeadlineExceeded,
 		},
 	} {
@@ -207,7 +207,7 @@ func TestRunKillTimeout(t *testing.T) {
 	}
 	hookErr, err := Run(ctx, hook, []byte("{}"), nil, nil, time.Duration(0))
 	assert.Equal(t, context.DeadlineExceeded, err)
-	assert.Regexp(t, "^(failed to reap process within 0s of the kill signal|signal: killed)$", hookErr)
+	assert.Regexp(t, "^(failed to reap process within 0s of the kill signal|executing \\[sh -c sleep 1]: signal: killed)$", hookErr)
 }
 
 func init() {
diff --git a/pkg/hooks/exec/runtimeconfigfilter.go b/pkg/hooks/exec/runtimeconfigfilter.go
new file mode 100644
index 000000000..c6971f680
--- /dev/null
+++ b/pkg/hooks/exec/runtimeconfigfilter.go
@@ -0,0 +1,68 @@
+package exec
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"reflect"
+	"time"
+
+	"github.com/davecgh/go-spew/spew"
+	spec "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
+	"github.com/pmezard/go-difflib/difflib"
+	"github.com/sirupsen/logrus"
+)
+
+var spewConfig = spew.ConfigState{
+	Indent:                  " ",
+	DisablePointerAddresses: true,
+	DisableCapacities:       true,
+	SortKeys:                true,
+}
+
+// RuntimeConfigFilter calls a series of hooks.  But instead of
+// passing container state on their standard input,
+// RuntimeConfigFilter passes the proposed runtime configuration (and
+// reads back a possibly-altered form from their standard output).
+func RuntimeConfigFilter(ctx context.Context, hooks []spec.Hook, config *spec.Spec, postKillTimeout time.Duration) (hookErr, err error) {
+	data, err := json.Marshal(config)
+	for i, hook := range hooks {
+		var stdout bytes.Buffer
+		hookErr, err = Run(ctx, &hook, data, &stdout, nil, postKillTimeout)
+		if err != nil {
+			return hookErr, err
+		}
+
+		data = stdout.Bytes()
+		var newConfig spec.Spec
+		err = json.Unmarshal(data, &newConfig)
+		if err != nil {
+			logrus.Debugf("invalid JSON from config-filter hook %d:\n%s", i, string(data))
+			return nil, errors.Wrapf(err, "unmarshal output from config-filter hook %d", i)
+		}
+
+		if !reflect.DeepEqual(config, &newConfig) {
+			old := spewConfig.Sdump(config)
+			new := spewConfig.Sdump(&newConfig)
+			diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{
+				A:        difflib.SplitLines(old),
+				B:        difflib.SplitLines(new),
+				FromFile: "Old",
+				FromDate: "",
+				ToFile:   "New",
+				ToDate:   "",
+				Context:  1,
+			})
+			if err == nil {
+				logrus.Debugf("precreate hook %d made configuration changes:\n%s", i, diff)
+			} else {
+				logrus.Warnf("precreate hook %d made configuration changes, but we could not compute a diff: %v", i, err)
+			}
+		}
+
+		*config = newConfig
+	}
+
+	return nil, nil
+}
diff --git a/pkg/hooks/exec/runtimeconfigfilter_test.go b/pkg/hooks/exec/runtimeconfigfilter_test.go
new file mode 100644
index 000000000..52d590d14
--- /dev/null
+++ b/pkg/hooks/exec/runtimeconfigfilter_test.go
@@ -0,0 +1,266 @@
+package exec
+
+import (
+	"context"
+	"encoding/json"
+	"os"
+	"testing"
+	"time"
+
+	spec "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
+)
+
+func pointerInt(value int) *int {
+	return &value
+}
+
+func pointerUInt32(value uint32) *uint32 {
+	return &value
+}
+
+func pointerFileMode(value os.FileMode) *os.FileMode {
+	return &value
+}
+
+func TestRuntimeConfigFilter(t *testing.T) {
+	unexpectedEndOfJSONInput := json.Unmarshal([]byte("{\n"), nil)
+
+	for _, test := range []struct {
+		name              string
+		contextTimeout    time.Duration
+		hooks             []spec.Hook
+		input             *spec.Spec
+		expected          *spec.Spec
+		expectedHookError string
+		expectedRunError  error
+	}{
+		{
+			name: "no-op",
+			hooks: []spec.Hook{
+				{
+					Path: path,
+					Args: []string{"sh", "-c", "cat"},
+				},
+			},
+			input: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+			},
+			expected: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+			},
+		},
+		{
+			name: "device injection",
+			hooks: []spec.Hook{
+				{
+					Path: path,
+					Args: []string{"sh", "-c", `sed 's|\("gid":0}\)|\1,{"path": "/dev/sda","type":"b","major":8,"minor":0,"fileMode":384,"uid":0,"gid":0}|'`},
+				},
+			},
+			input: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+				Linux: &spec.Linux{
+					Devices: []spec.LinuxDevice{
+						{
+							Path:     "/dev/fuse",
+							Type:     "c",
+							Major:    10,
+							Minor:    229,
+							FileMode: pointerFileMode(0600),
+							UID:      pointerUInt32(0),
+							GID:      pointerUInt32(0),
+						},
+					},
+				},
+			},
+			expected: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+				Linux: &spec.Linux{
+					Devices: []spec.LinuxDevice{
+						{
+							Path:     "/dev/fuse",
+							Type:     "c",
+							Major:    10,
+							Minor:    229,
+							FileMode: pointerFileMode(0600),
+							UID:      pointerUInt32(0),
+							GID:      pointerUInt32(0),
+						},
+						{
+							Path:     "/dev/sda",
+							Type:     "b",
+							Major:    8,
+							Minor:    0,
+							FileMode: pointerFileMode(0600),
+							UID:      pointerUInt32(0),
+							GID:      pointerUInt32(0),
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "chaining",
+			hooks: []spec.Hook{
+				{
+					Path: path,
+					Args: []string{"sh", "-c", `sed 's|\("gid":0}\)|\1,{"path": "/dev/sda","type":"b","major":8,"minor":0,"fileMode":384,"uid":0,"gid":0}|'`},
+				},
+				{
+					Path: path,
+					Args: []string{"sh", "-c", `sed 's|/dev/sda|/dev/sdb|'`},
+				},
+			},
+			input: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+				Linux: &spec.Linux{
+					Devices: []spec.LinuxDevice{
+						{
+							Path:     "/dev/fuse",
+							Type:     "c",
+							Major:    10,
+							Minor:    229,
+							FileMode: pointerFileMode(0600),
+							UID:      pointerUInt32(0),
+							GID:      pointerUInt32(0),
+						},
+					},
+				},
+			},
+			expected: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+				Linux: &spec.Linux{
+					Devices: []spec.LinuxDevice{
+						{
+							Path:     "/dev/fuse",
+							Type:     "c",
+							Major:    10,
+							Minor:    229,
+							FileMode: pointerFileMode(0600),
+							UID:      pointerUInt32(0),
+							GID:      pointerUInt32(0),
+						},
+						{
+							Path:     "/dev/sdb",
+							Type:     "b",
+							Major:    8,
+							Minor:    0,
+							FileMode: pointerFileMode(0600),
+							UID:      pointerUInt32(0),
+							GID:      pointerUInt32(0),
+						},
+					},
+				},
+			},
+		},
+		{
+			name:           "context timeout",
+			contextTimeout: time.Duration(1) * time.Second,
+			hooks: []spec.Hook{
+				{
+					Path: path,
+					Args: []string{"sh", "-c", "sleep 2"},
+				},
+			},
+			input: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+			},
+			expected: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+			},
+			expectedHookError: "^executing \\[sh -c sleep 2]: signal: killed$",
+			expectedRunError:  context.DeadlineExceeded,
+		},
+		{
+			name: "hook timeout",
+			hooks: []spec.Hook{
+				{
+					Path:    path,
+					Args:    []string{"sh", "-c", "sleep 2"},
+					Timeout: pointerInt(1),
+				},
+			},
+			input: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+			},
+			expected: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+			},
+			expectedHookError: "^executing \\[sh -c sleep 2]: signal: killed$",
+			expectedRunError:  context.DeadlineExceeded,
+		},
+		{
+			name: "invalid JSON",
+			hooks: []spec.Hook{
+				{
+					Path: path,
+					Args: []string{"sh", "-c", "echo '{'"},
+				},
+			},
+			input: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+			},
+			expected: &spec.Spec{
+				Version: "1.0.0",
+				Root: &spec.Root{
+					Path: "rootfs",
+				},
+			},
+			expectedRunError: unexpectedEndOfJSONInput,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			ctx := context.Background()
+			if test.contextTimeout > 0 {
+				var cancel context.CancelFunc
+				ctx, cancel = context.WithTimeout(ctx, test.contextTimeout)
+				defer cancel()
+			}
+			hookErr, err := RuntimeConfigFilter(ctx, test.hooks, test.input, DefaultPostKillTimeout)
+			assert.Equal(t, test.expectedRunError, errors.Cause(err))
+			if test.expectedHookError == "" {
+				if hookErr != nil {
+					t.Fatal(hookErr)
+				}
+			} else {
+				assert.Regexp(t, test.expectedHookError, hookErr.Error())
+			}
+			assert.Equal(t, test.expected, test.input)
+		})
+	}
+}