From 89430ffe65636b45cab834c94f176c1dc1d8a167 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 10 May 2018 11:06:53 -0700 Subject: hooks: Order injection by collated JSON filename We also considered ordering with sort.Strings, but Matthew rejected that because it uses a byte-by-byte UTF-8 comparison [1] which would fail many language-specific conventions [2]. There's some more discussion of the localeToLanguage mapping in [3]. Currently language.Parse does not handle either 'C' or 'POSIX', returning: und, language: tag is not well-formed for both. [1]: https://github.com/projectatomic/libpod/pull/686#issuecomment-387914358 [2]: https://en.wikipedia.org/wiki/Alphabetical_order#Language-specific_conventions [3]: https://github.com/golang/go/issues/25340 Signed-off-by: W. Trevor King Closes: #686 Approved by: mheon --- pkg/hooks/README.md | 8 ++++++ pkg/hooks/hooks.go | 65 +++++++++++++++++++++++++++++++++++++++-------- pkg/hooks/hooks_test.go | 49 ++++++++++++++++++++++++++++++----- pkg/hooks/monitor_test.go | 16 ++++++++++-- 4 files changed, 119 insertions(+), 19 deletions(-) (limited to 'pkg') diff --git a/pkg/hooks/README.md b/pkg/hooks/README.md index e47000a09..0d7037222 100644 --- a/pkg/hooks/README.md +++ b/pkg/hooks/README.md @@ -23,6 +23,10 @@ For both `crio` and `podman`, hooks are read from `/usr/share/containers/oci/hoo For `crio`, hook JSON is also read from `/etc/containers/oci/hooks.d/*.json`. If files of with the same name exist in both directories, the one in `/etc/containers/oci/hooks.d` takes precedence. +Hooks MUST be injected in the JSON filename case- and width-insensitive collation order. +Collation order depends on your locale, as set by [`LC_ALL`][LC_ALL], [`LC_COLLATE`][LC_COLLATE], or [`LANG`][LANG] (in order of decreasing precedence). +For example, in the [POSIX locale][LC_COLLATE-POSIX], a matching hook defined in `01-my-hook.json` would be injected before matching hooks defined in `02-another-hook.json` and `01-UPPERCASE.json`. + Each JSON file should contain an object with the following properties: ### 1.0.0 Hook Schema @@ -160,6 +164,10 @@ $ cat /etc/containers/oci/hooks.d/osystemd-hook.json ``` [JSON]: https://tools.ietf.org/html/rfc8259 +[LANG]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_02 +[LC_ALL]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_02 +[LC_COLLATE]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap07.html#tag_07_03_02 +[LC_COLLATE-POSIX]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap07.html#tag_07_03_02_06 [nvidia-container-runtime-hook]: https://github.com/NVIDIA/nvidia-container-runtime/tree/master/hook/nvidia-container-runtime-hook [oci-systemd-hook]: https://github.com/projectatomic/oci-systemd-hook [oci-umount]: https://github.com/projectatomic/oci-umount diff --git a/pkg/hooks/hooks.go b/pkg/hooks/hooks.go index f079dd0f7..337d6e3e2 100644 --- a/pkg/hooks/hooks.go +++ b/pkg/hooks/hooks.go @@ -10,6 +10,8 @@ import ( rspec "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" current "github.com/projectatomic/libpod/pkg/hooks/1.0.0" + "golang.org/x/text/collate" + "golang.org/x/text/language" ) // Version is the current hook configuration version. @@ -26,18 +28,27 @@ 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 } +type namedHook struct { + name string + hook *current.Hook +} + +type namedHooks []*namedHook + // New creates a new hook manager. Directories are ordered by // increasing preference (hook configurations in later directories // override configurations with the same filename from earlier // directories). -func New(ctx context.Context, directories []string) (manager *Manager, err error) { +func New(ctx context.Context, directories []string, lang language.Tag) (manager *Manager, err error) { manager = &Manager{ hooks: map[string]*current.Hook{}, directories: directories, + language: lang, } for _, dir := range directories { @@ -50,29 +61,48 @@ func New(ctx context.Context, directories []string) (manager *Manager, err error return manager, nil } -// 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) { +// filenames returns sorted hook entries. +func (m *Manager) namedHooks() (hooks []*namedHook) { m.lock.Lock() defer m.lock.Unlock() + + hooks = make([]*namedHook, len(m.hooks)) + i := 0 for name, hook := range m.hooks { - match, err := hook.When.Match(config, annotations, hasBindMounts) + hooks[i] = &namedHook{ + name: name, + hook: hook, + } + i++ + } + + return hooks +} + +// 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) { + hooks := m.namedHooks() + collator := collate.New(m.language, collate.IgnoreCase, collate.IgnoreWidth) + collator.Sort(namedHooks(hooks)) + for _, namedHook := range hooks { + match, err := namedHook.hook.When.Match(config, annotations, hasBindMounts) if err != nil { - return errors.Wrapf(err, "matching hook %q", name) + return errors.Wrapf(err, "matching hook %q", namedHook.name) } if match { if config.Hooks == nil { config.Hooks = &rspec.Hooks{} } - for _, stage := range hook.Stages { + for _, stage := range namedHook.hook.Stages { switch stage { case "prestart": - config.Hooks.Prestart = append(config.Hooks.Prestart, hook.Hook) + config.Hooks.Prestart = append(config.Hooks.Prestart, namedHook.hook.Hook) case "poststart": - config.Hooks.Poststart = append(config.Hooks.Poststart, hook.Hook) + config.Hooks.Poststart = append(config.Hooks.Poststart, namedHook.hook.Hook) case "poststop": - config.Hooks.Poststop = append(config.Hooks.Poststop, hook.Hook) + config.Hooks.Poststop = append(config.Hooks.Poststop, namedHook.hook.Hook) default: - return fmt.Errorf("hook %q: unknown stage %q", name, stage) + return fmt.Errorf("hook %q: unknown stage %q", namedHook.name, stage) } } } @@ -102,3 +132,18 @@ func (m *Manager) add(path string) (err error) { m.hooks[filepath.Base(path)] = hook return nil } + +// Len is part of the collate.Lister interface. +func (hooks namedHooks) Len() int { + return len(hooks) +} + +// Swap is part of the collate.Lister interface. +func (hooks namedHooks) Swap(i, j int) { + hooks[i], hooks[j] = hooks[j], hooks[i] +} + +// Bytes is part of the collate.Lister interface. +func (hooks namedHooks) Bytes(i int) []byte { + return []byte(hooks[i].name) +} diff --git a/pkg/hooks/hooks_test.go b/pkg/hooks/hooks_test.go index 109f6b046..9da2ec8b0 100644 --- a/pkg/hooks/hooks_test.go +++ b/pkg/hooks/hooks_test.go @@ -12,6 +12,7 @@ import ( rspec "github.com/opencontainers/runtime-spec/specs-go" current "github.com/projectatomic/libpod/pkg/hooks/1.0.0" "github.com/stretchr/testify/assert" + "golang.org/x/text/language" ) // path is the path to an example hook executable. @@ -26,13 +27,28 @@ func TestGoodNew(t *testing.T) { } defer os.RemoveAll(dir) - jsonPath := filepath.Join(dir, "a.json") - err = ioutil.WriteFile(jsonPath, []byte(fmt.Sprintf("{\"version\": \"1.0.0\", \"hook\": {\"path\": \"%s\"}, \"when\": {\"always\": true}, \"stages\": [\"prestart\", \"poststart\", \"poststop\"]}", path)), 0644) + for i, name := range []string{ + "01-my-hook.json", + "01-UPPERCASE.json", + "02-another-hook.json", + } { + jsonPath := filepath.Join(dir, name) + var extraStages string + if i == 0 { + extraStages = ", \"poststart\", \"poststop\"" + } + 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) + if err != nil { + t.Fatal(err) + } + } + + lang, err := language.Parse("und-u-va-posix") if err != nil { t.Fatal(err) } - manager, err := New(ctx, []string{dir}) + manager, err := New(ctx, []string{dir}, lang) if err != nil { t.Fatal(err) } @@ -43,20 +59,34 @@ func TestGoodNew(t *testing.T) { t.Fatal(err) } + one := 1 + two := 2 + three := 3 assert.Equal(t, &rspec.Hooks{ Prestart: []rspec.Hook{ { - Path: path, + Path: path, + Timeout: &one, + }, + { + Path: path, + Timeout: &two, + }, + { + Path: path, + Timeout: &three, }, }, Poststart: []rspec.Hook{ { - Path: path, + Path: path, + Timeout: &one, }, }, Poststop: []rspec.Hook{ { - Path: path, + Path: path, + Timeout: &one, }, }, }, config.Hooks) @@ -77,7 +107,12 @@ func TestBadNew(t *testing.T) { t.Fatal(err) } - _, err = New(ctx, []string{dir}) + lang, err := language.Parse("und-u-va-posix") + if err != nil { + t.Fatal(err) + } + + _, err = New(ctx, []string{dir}, lang) if err == nil { t.Fatal("unexpected success") } diff --git a/pkg/hooks/monitor_test.go b/pkg/hooks/monitor_test.go index 9cbde1986..3887b2072 100644 --- a/pkg/hooks/monitor_test.go +++ b/pkg/hooks/monitor_test.go @@ -11,6 +11,7 @@ import ( rspec "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" + "golang.org/x/text/language" ) func TestMonitorGood(t *testing.T) { @@ -21,7 +22,12 @@ func TestMonitorGood(t *testing.T) { } defer os.RemoveAll(dir) - manager, err := New(ctx, []string{dir}) + lang, err := language.Parse("und-u-va-posix") + if err != nil { + t.Fatal(err) + } + + manager, err := New(ctx, []string{dir}, lang) if err != nil { t.Fatal(err) } @@ -114,7 +120,13 @@ func TestMonitorGood(t *testing.T) { func TestMonitorBadWatcher(t *testing.T) { ctx := context.Background() - manager, err := New(ctx, []string{}) + + lang, err := language.Parse("und-u-va-posix") + if err != nil { + t.Fatal(err) + } + + manager, err := New(ctx, []string{}, lang) if err != nil { t.Fatal(err) } -- cgit v1.2.3-54-g00ecf