Merge pull request #2501 from mtrmac/fixed-hook-order

RFC: Make hooks sort order locale-independent

4 files changed, 13 insertions, 61 deletions

diff --git a/pkg/hooks/docs/oci-hooks.5.md b/pkg/hooks/docs/oci-hooks.5.md
index 493e4f157..c876dd2f8 100644
--- a/pkg/hooks/docs/oci-hooks.5.md
+++ b/pkg/hooks/docs/oci-hooks.5.md
@@ -23,10 +23,9 @@ If multiple directories are configured, a JSON filename in a preferred directory
 
 Tools consuming this format may also opt to monitor the hook directries for changes, in which case they will notice additions, changes, and removals to JSON files without needing to be restarted or otherwise signaled.  When the tool monitors multiple hooks directories, the precedence discussed in the previous paragraph still applies.  For example, if a consuming tool watches for hooks in `/etc/containers/oci/hooks.d` and `/usr/share/containers/oci/hooks.d` (in order of decreasing precedence), then writing a new hook definition to `/etc/containers/oci/hooks.d/01-my-hook.json` will mask the hook previously loaded from `/usr/share/containers/oci/hooks.d/01-my-hook.json`.  Subsequent changes to `/usr/share/containers/oci/hooks.d/01-my-hook.json` will have no effect on the consuming tool as long as `/etc/containers/oci/hooks.d/01-my-hook.json` exists.  Removing `/etc/containers/oci/hooks.d/01-my-hook.json` will reload the hook from `/usr/share/containers/oci/hooks.d/01-my-hook.json`.
 
-Hooks are injected in the JSON filename case- and width-insensitive collation order.
-Collation order depends on your locale, as set by `LC_ALL`, `LC_COLLATE`, or `LANG` (in order of decreasing precedence).
-For more information, see `locale(7)`.
-For example, in the POSIX locale, 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`.
+Hooks are injected in the order obtained by sorting the JSON file names, after converting them to lower case, based on their Unicode code points.
+For example, 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`.
+It is strongly recommended to make the sort oder unambiguous depending on an ASCII-only prefix (like the `01`/`02` above).
 
 Each JSON file should contain an object with one of the following schemas.
 
diff --git a/pkg/hooks/hooks.go b/pkg/hooks/hooks.go
index 8a44a4982..5ed028b95 100644
--- a/pkg/hooks/hooks.go
+++ b/pkg/hooks/hooks.go
@@ -5,14 +5,14 @@ import (
 	"context"
 	"fmt"
 	"path/filepath"
+	"sort"
+	"strings"
 	"sync"
 
 	current "github.com/containers/libpod/pkg/hooks/1.0.0"
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
-	"golang.org/x/text/collate"
-	"golang.org/x/text/language"
 )
 
 // Version is the current hook configuration version.
@@ -31,7 +31,6 @@ type Manager struct {
 	hooks           map[string]*current.Hook
 	directories     []string
 	extensionStages []string
-	language        language.Tag
 	lock            sync.Mutex
 }
 
@@ -40,8 +39,6 @@ type namedHook struct {
 	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
@@ -51,12 +48,11 @@ type namedHooks []*namedHook
 // those specified in the OCI Runtime Specification and to control
 // OCI-defined stages instead of delagating to the OCI runtime.  See
 // Hooks() for more information.
-func New(ctx context.Context, directories []string, extensionStages []string, lang language.Tag) (manager *Manager, err error) {
+func New(ctx context.Context, directories []string, extensionStages []string) (manager *Manager, err error) {
 	manager = &Manager{
 		hooks:           map[string]*current.Hook{},
 		directories:     directories,
 		extensionStages: extensionStages,
-		language:        lang,
 	}
 
 	for _, dir := range directories {
@@ -94,15 +90,14 @@ func (m *Manager) namedHooks() (hooks []*namedHook) {
 // extensionStageHooks.  This takes precedence over their inclusion in
 // the OCI configuration.  For example:
 //
-//   manager, err := New(ctx, []string{DefaultDir}, []string{"poststop"}, lang)
+//   manager, err := New(ctx, []string{DefaultDir}, []string{"poststop"})
 //   extensionStageHooks, err := manager.Hooks(config, annotations, hasBindMounts)
 //
 // will have any matching post-stop hooks in extensionStageHooks and
 // will not insert them into config.Hooks.Poststop.
 func (m *Manager) Hooks(config *rspec.Spec, annotations map[string]string, hasBindMounts bool) (extensionStageHooks map[string][]rspec.Hook, err error) {
 	hooks := m.namedHooks()
-	collator := collate.New(m.language, collate.IgnoreCase, collate.IgnoreWidth)
-	collator.Sort(namedHooks(hooks))
+	sort.Slice(hooks, func(i, j int) bool { return strings.ToLower(hooks[i].name) < strings.ToLower(hooks[j].name) })
 	localStages := map[string]bool{} // stages destined for extensionStageHooks
 	for _, stage := range m.extensionStages {
 		localStages[stage] = true
@@ -166,18 +161,3 @@ 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 2925a99e4..02cde6d64 100644
--- a/pkg/hooks/hooks_test.go
+++ b/pkg/hooks/hooks_test.go
@@ -12,7 +12,6 @@ import (
 	current "github.com/containers/libpod/pkg/hooks/1.0.0"
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
-	"golang.org/x/text/language"
 )
 
 // path is the path to an example hook executable.
@@ -43,12 +42,7 @@ func TestGoodNew(t *testing.T) {
 		}
 	}
 
-	lang, err := language.Parse("und-u-va-posix")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	manager, err := New(ctx, []string{dir}, []string{}, lang)
+	manager, err := New(ctx, []string{dir}, []string{})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -110,12 +104,7 @@ func TestBadNew(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	lang, err := language.Parse("und-u-va-posix")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	_, err = New(ctx, []string{dir}, []string{}, lang)
+	_, err = New(ctx, []string{dir}, []string{})
 	if err == nil {
 		t.Fatal("unexpected success")
 	}
diff --git a/pkg/hooks/monitor_test.go b/pkg/hooks/monitor_test.go
index b3af4bb43..31d7f9e39 100644
--- a/pkg/hooks/monitor_test.go
+++ b/pkg/hooks/monitor_test.go
@@ -11,7 +11,6 @@ import (
 
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
-	"golang.org/x/text/language"
 )
 
 func TestMonitorOneDirGood(t *testing.T) {
@@ -22,12 +21,7 @@ func TestMonitorOneDirGood(t *testing.T) {
 	}
 	defer os.RemoveAll(dir)
 
-	lang, err := language.Parse("und-u-va-posix")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	manager, err := New(ctx, []string{dir}, []string{}, lang)
+	manager, err := New(ctx, []string{dir}, []string{})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -132,12 +126,7 @@ func TestMonitorTwoDirGood(t *testing.T) {
 	}
 	defer os.RemoveAll(fallbackDir)
 
-	lang, err := language.Parse("und-u-va-posix")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	manager, err := New(ctx, []string{fallbackDir, primaryDir}, []string{}, lang)
+	manager, err := New(ctx, []string{fallbackDir, primaryDir}, []string{})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -312,12 +301,7 @@ func TestMonitorTwoDirGood(t *testing.T) {
 func TestMonitorBadWatcher(t *testing.T) {
 	ctx := context.Background()
 
-	lang, err := language.Parse("und-u-va-posix")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	manager, err := New(ctx, []string{}, []string{}, lang)
+	manager, err := New(ctx, []string{}, []string{})
 	if err != nil {
 		t.Fatal(err)
 	}