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 <wking@tremily.us>

Closes: #686
Approved by: mheon

4 files changed, 119 insertions, 19 deletions

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)
 	}