Potentially breaking: Make hooks sort order locale-independent

Don't sort OCI hooks using the locale collation order; it does not
make sense for the same system-wide directory to be interpreted differently
depending on the user's LC_COLLATE setting, and the language-specific
collation order can even change over time.

Besides, the current collation order determination code has never worked
with the most common LC_COLLATE values like en_US.UTF-8.

Ideally, we would like to just order based on Unicode code points
to be reliably stable, but the existing implementation is case-insensitive,
so we are forced to rely on the unicode case mapping tables at least.

(This gives up on canonicalization and width-insensitivity, potentially
breaking users who rely on these previously documented properties.)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

6 files changed, 15 insertions, 154 deletions

diff --git a/libpod/container_internal.go b/libpod/container_internal.go
index 22df36c11..485b43f7d 100644
--- a/libpod/container_internal.go
+++ b/libpod/container_internal.go
@@ -25,7 +25,6 @@ import (
 	opentracing "github.com/opentracing/opentracing-go"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
-	"golang.org/x/text/language"
 	kwait "k8s.io/apimachinery/pkg/util/wait"
 )
 
@@ -34,15 +33,6 @@ const (
 	artifactsDir = "artifacts"
 )
 
-var (
-	// localeToLanguageMap maps from locale values to language tags.
-	localeToLanguageMap = map[string]string{
-		"":      "und-u-va-posix",
-		"c":     "und-u-va-posix",
-		"posix": "und-u-va-posix",
-	}
-)
-
 // rootFsSize gets the size of the container's root filesystem
 // A container FS is split into two parts.  The first is the top layer, a
 // mutable layer, and the rest is the RootFS: the set of immutable layers
@@ -1283,48 +1273,15 @@ func (c *Container) saveSpec(spec *spec.Spec) error {
 	return nil
 }
 
-// localeToLanguage translates POSIX locale strings to BCP 47 language tags.
-func localeToLanguage(locale string) string {
-	locale = strings.Replace(strings.SplitN(locale, ".", 2)[0], "_", "-", 1)
-	langString, ok := localeToLanguageMap[strings.ToLower(locale)]
-	if !ok {
-		langString = locale
-	}
-	return langString
-}
-
 // 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
-	for _, envVar := range []string{
-		"LC_ALL",
-		"LC_COLLATE",
-		"LANG",
-	} {
-		locale, ok = os.LookupEnv(envVar)
-		if ok {
-			break
-		}
-	}
-
-	langString := localeToLanguage(locale)
-	lang, err := language.Parse(langString)
-	if err != nil {
-		logrus.Warnf("failed to parse language %q: %s", langString, err)
-		lang, err = language.Parse("und-u-va-posix")
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	allHooks := make(map[string][]spec.Hook)
 	if c.runtime.config.HooksDir == nil {
 		if rootless.IsRootless() {
 			return nil, nil
 		}
 		for _, hDir := range []string{hooks.DefaultDir, hooks.OverrideDir} {
-			manager, err := hooks.New(ctx, []string{hDir}, []string{"precreate", "poststop"}, lang)
+			manager, err := hooks.New(ctx, []string{hDir}, []string{"precreate", "poststop"})
 			if err != nil {
 				if os.IsNotExist(err) {
 					continue
@@ -1343,7 +1300,7 @@ func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (exten
 			}
 		}
 	} else {
-		manager, err := hooks.New(ctx, c.runtime.config.HooksDir, []string{"precreate", "poststop"}, lang)
+		manager, err := hooks.New(ctx, c.runtime.config.HooksDir, []string{"precreate", "poststop"})
 		if err != nil {
 			if os.IsNotExist(err) {
 				logrus.Warnf("Requested OCI hooks directory %q does not exist", c.runtime.config.HooksDir)
diff --git a/libpod/container_internal_test.go b/libpod/container_internal_test.go
index 1654af929..f1e2b70a7 100644
--- a/libpod/container_internal_test.go
+++ b/libpod/container_internal_test.go
@@ -17,54 +17,6 @@ import (
 // hookPath is the path to an example hook executable.
 var hookPath string
 
-func TestLocaleToLanguage(t *testing.T) {
-	for _, testCase := range []struct {
-		locale   string
-		language string
-	}{
-		{
-			locale:   "",
-			language: "und-u-va-posix",
-		},
-		{
-			locale:   "C",
-			language: "und-u-va-posix",
-		},
-		{
-			locale:   "POSIX",
-			language: "und-u-va-posix",
-		},
-		{
-			locale:   "c",
-			language: "und-u-va-posix",
-		},
-		{
-			locale:   "en",
-			language: "en",
-		},
-		{
-			locale:   "en_US",
-			language: "en-US",
-		},
-		{
-			locale:   "en.UTF-8",
-			language: "en",
-		},
-		{
-			locale:   "en_US.UTF-8",
-			language: "en-US",
-		},
-		{
-			locale:   "does-not-exist",
-			language: "does-not-exist",
-		},
-	} {
-		t.Run(testCase.locale, func(t *testing.T) {
-			assert.Equal(t, testCase.language, localeToLanguage(testCase.locale))
-		})
-	}
-}
-
 func TestPostDeleteHooks(t *testing.T) {
 	ctx := context.Background()
 	dir, err := ioutil.TempDir("", "libpod_test_")
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)
 	}