Skip to content

hooks: Add package support for extension stages #758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

hooks: Add package support for extension stages #758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions libpod/container_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1324,7 +1324,7 @@ func (c *Container) setupOCIHooks(ctx context.Context, g *generate.Generator) er
}
}

manager, err := hooks.New(ctx, []string{c.runtime.config.HooksDir}, lang)
manager, err := hooks.New(ctx, []string{c.runtime.config.HooksDir}, []string{}, lang)
if err != nil {
if c.runtime.config.HooksDirNotExistFatal || !os.IsNotExist(err) {
return err
Expand All @@ -1333,5 +1333,6 @@ func (c *Container) setupOCIHooks(ctx context.Context, g *generate.Generator) er
return nil
}

return manager.Hooks(g.Spec(), c.Spec().Annotations, len(c.config.UserVolumes) > 0)
_, err = manager.Hooks(g.Spec(), c.Spec().Annotations, len(c.config.UserVolumes) > 0)
return err
}
6 changes: 5 additions & 1 deletion pkg/hooks/1.0.0/hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func Read(content []byte) (hook *Hook, err error) {
}

// Validate performs load-time hook validation.
func (hook *Hook) Validate() (err error) {
func (hook *Hook) Validate(extensionStages []string) (err error) {
if hook == nil {
return errors.New("nil hook")
}
Expand Down Expand Up @@ -68,6 +68,10 @@ func (hook *Hook) Validate() (err error) {
}

validStages := map[string]bool{"prestart": true, "poststart": true, "poststop": true}
for _, stage := range extensionStages {
validStages[stage] = true
}

for _, stage := range hook.Stages {
if !validStages[stage] {
return fmt.Errorf("unknown stage %q", stage)
Expand Down
34 changes: 24 additions & 10 deletions pkg/hooks/1.0.0/hook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ func TestGoodValidate(t *testing.T) {
},
Stages: []string{"prestart"},
}
err := hook.Validate()
err := hook.Validate([]string{})
if err != nil {
t.Fatal(err)
}
}

func TestNilValidation(t *testing.T) {
var hook *Hook
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
Expand All @@ -68,7 +68,7 @@ func TestNilValidation(t *testing.T) {

func TestWrongVersion(t *testing.T) {
hook := Hook{Version: "0.1.0"}
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
Expand All @@ -80,7 +80,7 @@ func TestNoHookPath(t *testing.T) {
Version: "1.0.0",
Hook: rspec.Hook{},
}
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
Expand All @@ -94,7 +94,7 @@ func TestUnknownHookPath(t *testing.T) {
Path: filepath.Join("does", "not", "exist"),
},
}
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
Expand All @@ -111,7 +111,7 @@ func TestNoStages(t *testing.T) {
Path: path,
},
}
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
Expand All @@ -126,13 +126,27 @@ func TestInvalidStage(t *testing.T) {
},
Stages: []string{"does-not-exist"},
}
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
assert.Regexp(t, "^unknown stage \"does-not-exist\"$", err.Error())
}

func TestExtensionStage(t *testing.T) {
hook := Hook{
Version: "1.0.0",
Hook: rspec.Hook{
Path: path,
},
Stages: []string{"prestart", "b"},
}
err := hook.Validate([]string{"a", "b", "c"})
Copy link
Member

@rhatdan rhatdan May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Do on one line
if err := ...

Copy link
Contributor Author

@wking wking May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Do on one line
if err := ...

Is there a way to do this automatically (e.g. with gofmt)? I usually use separate lines, and there are already many instances of the separate-line approach in the hooks package:

$ git describe --tags
v0.5.2-3-g07253fc
$ git grep -B1 'if err != nil' pkg/hooks
pkg/hooks/0.1.0/hook_test.go-   hook, err := read([]byte("{\"hook\": \"/a/b/c\", \"stages\": [\"prestart\"], \"cmds\": [\"sh\"]}"))
pkg/hooks/0.1.0/hook_test.go:   if err != nil {
--
pkg/hooks/0.1.0/hook_test.go-   hook, err := read([]byte("{\"hook\": \"/a/b/c\", \"arguments\": [\"d\", \"e\"], \"stages\": [\"prestart\"], \
"cmds\": [\"sh\"]}"))
pkg/hooks/0.1.0/hook_test.go:   if err != nil {
--
...
--
pkg/hooks/1.0.0/hook_test.go-   err := hook.Validate()
pkg/hooks/1.0.0/hook_test.go:   if err != nil {
--
...
--
pkg/hooks/hooks.go-             err = ReadDir(dir, manager.hooks)
pkg/hooks/hooks.go:             if err != nil {
--
...
--
pkg/hooks/hooks_test.go-                err = ioutil.WriteFile(jsonPath, []byte(fmt.Sprintf("{\"version\": \"1.0.0\", \"hook\": {\"path\": \"%s\", \"timeout\": %d}, \"when\": {\"always\": true}, \"stages\": [\"prestart\"%s]}", path, i+1, extraStages)), 0644)
pkg/hooks/hooks_test.go:                if err != nil {
--
...
--
--
pkg/hooks/hooks_test.go-        err = manager.Hooks(config, map[string]string{}, false)
pkg/hooks/hooks_test.go:        if err != nil {
--
...
--
pkg/hooks/monitor.go-//   err := <-sync // block until writers are established
pkg/hooks/monitor.go://   if err != nil {
--
...
--
pkg/hooks/monitor_test.go-      err = <-sync
pkg/hooks/monitor_test.go:      if err != nil {
--
...

It looks like I've only gone with the one-line approach twice:

$ git grep 'if .*:=' pkg/hooks
pkg/hooks/1.0.0/hook.go:        if _, err := os.Stat(hook.Hook.Path); err != nil {
pkg/hooks/read.go:      if err := json.Unmarshal(content, &ver); err != nil {

I can go whichever way you like for any lines you like. Changing this one line manually is easy, but I'm not sure how you feel about the other cases.

Copy link
Member

@rhatdan rhatdan May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know of any gofmt call to do it. But when I look at the go code in hooks directory I see all instances where err is created by itself to use the single line.

grep "err.*:=" -r pkg/hooks/
pkg/hooks/hooks.go:	raw, err := ioutil.ReadFile(hookPath)
pkg/hooks/hooks.go:	if err := json.Unmarshal(raw, &hook); err != nil {
pkg/hooks/hooks.go:	if _, err := os.Stat(hook.Hook); err != nil {
pkg/hooks/hooks.go:	if _, err := os.Stat(hooksPath); err != nil {
pkg/hooks/hooks.go:	files, err := ioutil.ReadDir(hooksPath)
pkg/hooks/hooks.go:		hook, err := readHook(filepath.Join(hooksPath, file.Name()))
pkg/hooks/hooks.go:	if err := readHooks(hooksPath, hooksMap); err != nil {
pkg/hooks/hooks.go:		if err := readHooks(OverrideHooksDir, hooksMap); err != nil {

Copy link
Contributor Author

@wking wking May 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when I look at the go code in hooks directory I see all instances where err is created by itself to use the single line.

grep "err.*:=" -r pkg/hooks/
...
pkg/hooks/hooks.go:	if err := json.Unmarshal(raw, &hook); err != nil {
...

It looks like you're looking at a version from before 68eb128 (#686), which moved the Unmarshal call from pkg/hooks/hooks.go into other places:

$ git show 68eb128fb0a | grep '^diff\|json\.Unmarshal' | grep -B1 Unmarshal
diff --git a/pkg/hooks/0.1.0/hook.go b/pkg/hooks/0.1.0/hook.go
+       if err = json.Unmarshal(content, &raw); err != nil {
--
diff --git a/pkg/hooks/1.0.0/hook.go b/pkg/hooks/1.0.0/hook.go
+       if err = json.Unmarshal(content, &hook); err != nil {
--
diff --git a/pkg/hooks/hooks.go b/pkg/hooks/hooks.go
-       if err := json.Unmarshal(raw, &hook); err != nil {
--
diff --git a/pkg/hooks/read.go b/pkg/hooks/read.go
+       if err := json.Unmarshal(content, &ver); err != nil {

My grep above is still what I get with the current master (69a6cb2). And 68eb128 added a bunch of examples with the two-line approach. Most of those additions were *_test.go files, but there were some others which show up in my earlier grep.

Anyhow, I'm fine translating any of these to the one-line approach but identifying them is hard. Do you also want me to adjust master entries like this to:

if _, err := Read(filepath.Join("does", "not", "exist.json")); err == nil {
	t.Fatal("unexpected success")
} else {
	assert.Regexp(t, "^open does/not/exist.json: no such file or directory$", err.Error())
	if !os.IsNotExist(err) {
		t.Fatal("opaque wrapping for not-exist errors")
	}
}

I'm not sure where you see the tighter-scoping benefit of the one-line approach balancing out the readability drop from the more compact approach. For that TestUnknownPath example, the above if/else would be the entire test function, so gains from tighter scoping would be very small.

Or should I just be adjusting entries already being touched by this PR?

Copy link
Contributor Author

@wking wking May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of a line touched in this PR that could be one-lined if you like.

Copy link
Member

@rhatdan rhatdan May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I guess I was looking at an older pool. I guess we can allow this in and then I will open a PR to fix the non single like ones.

if err != nil {
t.Fatal(err)
}
}

func TestInvalidAnnotationKey(t *testing.T) {
hook := Hook{
Version: "1.0.0",
Expand All @@ -146,7 +160,7 @@ func TestInvalidAnnotationKey(t *testing.T) {
},
Stages: []string{"prestart"},
}
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
Expand All @@ -166,7 +180,7 @@ func TestInvalidAnnotationValue(t *testing.T) {
},
Stages: []string{"prestart"},
}
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
Expand All @@ -184,7 +198,7 @@ func TestInvalidCommand(t *testing.T) {
},
Stages: []string{"prestart"},
}
err := hook.Validate()
err := hook.Validate([]string{})
if err == nil {
t.Fatal("unexpected success")
}
Expand Down
7 changes: 2 additions & 5 deletions pkg/hooks/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ Each JSON file should contain an object with the following properties:
Entries MUST be [POSIX extended regular expressions][POSIX-ERE].
* **`hasBindMounts`** (OPTIONAL, boolean) If `hasBindMounts` is true and the caller requested host-to-container bind mounts (beyond those that CRI-O or libpod use by default), this condition matches.
* **`stages`** (REQUIRED, array of strings) Stages when the hook MUST be injected.
Entries MUST be chosen from the 1.0.1 OCI Runtime Specification [hook stages][spec-hooks].
Entries MUST be chosen from the 1.0.1 OCI Runtime Specification [hook stages][spec-hooks] or from extention stages supported by the package consumer.
Copy link
Contributor Author

@wking wking May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being strict here helps catch typos like pre-start (where the user likely intended prestart). But it's going to make sharing a single /usr/share/containers/oci/hooks.d difficult. Do we expect all tools consuming that directory to always agree on the set of extension stages? Do we expect tools to add additional directories for extension stages not yet adopted across the whole consumer community? Do we want to provide a way for tools to allow unrecognized extension stages?

Copy link
Member

@mheon mheon May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm content to say that all users of a single hooks.d should share the same extension stages - simplest solution for now. We can make further changes later if this proves inadequate.

Copy link
Contributor Author

@wking wking May 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm content to say that all users of a single hooks.d should share the same extension stages - simplest solution for now.

So what directory should I change to when I add postexit support for #730? Does the extensionStages config need to be per-directory to avoid breaking users who don't need postexit but have existing content in /usr/share/containers/oci/hooks.d?


If *all* of the conditions set in `when` match, then the `hook` MUST be injected for the stages set in `stages`.

Expand Down Expand Up @@ -114,10 +114,7 @@ Previous versions of CRI-O and libpod supported the 0.1.0 hook schema:
The injected hook's [`args`][spec-hooks] is `hook` with `arguments` appended.
* **`stages`** (REQUIRED, array of strings) Stages when the hook MUST be injected.
`stage` is an allowed synonym for this property, but you MUST NOT set both `stages` and `stage`.
Entries MUST be chosen from:
* **`prestart`**, to inject [pre-start][].
* **`poststart`**, to inject [post-start][].
* **`poststop`**, to inject [post-stop][].
Entries MUST be chosen from the 1.0.1 OCI Runtime Specification [hook stages][spec-hooks] or from extention stages supported by the package consumer.
* **`cmds`** (OPTIONAL, array of strings) The hook MUST be injected if the configured [`process.args[0]`][spec-process] matches an entry.
`cmd` is an allowed synonym for this property, but you MUST NOT set both `cmds` and `cmd`.
Entries MUST be [POSIX extended regular expressions][POSIX-ERE].
Expand Down
41 changes: 27 additions & 14 deletions pkg/hooks/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,11 @@ const (

// Manager provides an opaque interface for managing CRI-O hooks.
type Manager struct {
hooks map[string]*current.Hook
language language.Tag
directories []string
lock sync.Mutex
hooks map[string]*current.Hook
directories []string
extensionStages []string
language language.Tag
lock sync.Mutex
}

type namedHook struct {
Expand All @@ -44,15 +45,16 @@ type namedHooks []*namedHook
// increasing preference (hook configurations in later directories
// override configurations with the same filename from earlier
// directories).
func New(ctx context.Context, directories []string, lang language.Tag) (manager *Manager, err error) {
func New(ctx context.Context, directories []string, extensionStages []string, lang language.Tag) (manager *Manager, err error) {
manager = &Manager{
hooks: map[string]*current.Hook{},
directories: directories,
language: lang,
hooks: map[string]*current.Hook{},
directories: directories,
extensionStages: extensionStages,
language: lang,
}

for _, dir := range directories {
err = ReadDir(dir, manager.hooks)
err = ReadDir(dir, manager.extensionStages, manager.hooks)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -80,14 +82,18 @@ func (m *Manager) namedHooks() (hooks []*namedHook) {
}

// Hooks injects OCI runtime hooks for a given container configuration.
func (m *Manager) Hooks(config *rspec.Spec, annotations map[string]string, hasBindMounts bool) (err error) {
func (m *Manager) Hooks(config *rspec.Spec, annotations map[string]string, hasBindMounts bool) (extensionStages map[string][]rspec.Hook, err error) {
hooks := m.namedHooks()
collator := collate.New(m.language, collate.IgnoreCase, collate.IgnoreWidth)
collator.Sort(namedHooks(hooks))
validStages := map[string]bool{} // beyond the OCI stages
for _, stage := range m.extensionStages {
validStages[stage] = true
}
for _, namedHook := range hooks {
match, err := namedHook.hook.When.Match(config, annotations, hasBindMounts)
if err != nil {
return errors.Wrapf(err, "matching hook %q", namedHook.name)
return extensionStages, errors.Wrapf(err, "matching hook %q", namedHook.name)
}
if match {
if config.Hooks == nil {
Expand All @@ -102,12 +108,19 @@ func (m *Manager) Hooks(config *rspec.Spec, annotations map[string]string, hasBi
case "poststop":
config.Hooks.Poststop = append(config.Hooks.Poststop, namedHook.hook.Hook)
default:
return fmt.Errorf("hook %q: unknown stage %q", namedHook.name, stage)
if !validStages[stage] {
return extensionStages, fmt.Errorf("hook %q: unknown stage %q", namedHook.name, stage)
}
if extensionStages == nil {
extensionStages = map[string][]rspec.Hook{}
}
extensionStages[stage] = append(extensionStages[stage], namedHook.hook.Hook)
}
}
}
}
return nil

return extensionStages, nil
}

// remove remove a hook by name.
Expand All @@ -125,7 +138,7 @@ func (m *Manager) remove(hook string) (ok bool) {
func (m *Manager) add(path string) (err error) {
m.lock.Lock()
defer m.lock.Unlock()
hook, err := Read(path)
hook, err := Read(path, m.extensionStages)
if err != nil {
return err
}
Expand Down
65 changes: 60 additions & 5 deletions pkg/hooks/hooks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,13 @@ func TestGoodNew(t *testing.T) {
t.Fatal(err)
}

manager, err := New(ctx, []string{dir}, lang)
manager, err := New(ctx, []string{dir}, []string{}, lang)
if err != nil {
t.Fatal(err)
}

config := &rspec.Spec{}
err = manager.Hooks(config, map[string]string{}, false)
extensionStages, err := manager.Hooks(config, map[string]string{}, false)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -90,6 +90,9 @@ func TestGoodNew(t *testing.T) {
},
},
}, config.Hooks)

var nilExtensionStages map[string][]rspec.Hook
assert.Equal(t, nilExtensionStages, extensionStages)
}

func TestBadNew(t *testing.T) {
Expand All @@ -112,7 +115,7 @@ func TestBadNew(t *testing.T) {
t.Fatal(err)
}

_, err = New(ctx, []string{dir}, lang)
_, err = New(ctx, []string{dir}, []string{}, lang)
if err == nil {
t.Fatal("unexpected success")
}
Expand All @@ -139,11 +142,14 @@ func TestBrokenMatch(t *testing.T) {
Args: []string{"/bin/sh"},
},
}
err := manager.Hooks(config, map[string]string{}, false)
extensionStages, err := manager.Hooks(config, map[string]string{}, false)
if err == nil {
t.Fatal("unexpected success")
}
assert.Regexp(t, "^matching hook \"a\\.json\": command: error parsing regexp: .*", err.Error())

var nilExtensionStages map[string][]rspec.Hook
assert.Equal(t, nilExtensionStages, extensionStages)
}

func TestInvalidStage(t *testing.T) {
Expand All @@ -162,11 +168,60 @@ func TestInvalidStage(t *testing.T) {
},
},
}
err := manager.Hooks(&rspec.Spec{}, map[string]string{}, false)
extensionStages, err := manager.Hooks(&rspec.Spec{}, map[string]string{}, false)
if err == nil {
t.Fatal("unexpected success")
}
assert.Regexp(t, "^hook \"a\\.json\": unknown stage \"does-not-exist\"$", err.Error())

var nilExtensionStages map[string][]rspec.Hook
assert.Equal(t, nilExtensionStages, extensionStages)
}

func TestExtensionStage(t *testing.T) {
always := true
manager := Manager{
hooks: map[string]*current.Hook{
"a.json": {
Version: current.Version,
Hook: rspec.Hook{
Path: "/a/b/c",
},
When: current.When{
Always: &always,
},
Stages: []string{"prestart", "a", "b"},
},
},
extensionStages: []string{"a", "b", "c"},
}

config := &rspec.Spec{}
extensionStages, err := manager.Hooks(config, map[string]string{}, false)
if err != nil {
t.Fatal(err)
}

assert.Equal(t, &rspec.Hooks{
Prestart: []rspec.Hook{
{
Path: "/a/b/c",
},
},
}, config.Hooks)

assert.Equal(t, map[string][]rspec.Hook{
"a": {
{
Path: "/a/b/c",
},
},
"b": {
{
Path: "/a/b/c",
},
},
}, extensionStages)
}

func init() {
Expand Down
Loading