From cfe0f5a5b722bcfa2f7028efb024f5a94f5ff4ec Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 11 Dec 2019 08:13:12 -0800 Subject: [PATCH 1/6] Use md5 to generate unique lock names --- pkg/util/lock/lock.go | 12 ++---------- pkg/util/lock/lock_test.go | 19 ++++++++++--------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index 5083fa0d8cb4..d5c945b9d8e2 100644 --- a/pkg/util/lock/lock.go +++ b/pkg/util/lock/lock.go @@ -17,12 +17,12 @@ limitations under the License. package lock import ( + "crypto/md5" "fmt" "io/ioutil" "os" "os/user" "regexp" - "strings" "time" "github.com/golang/glog" @@ -78,13 +78,5 @@ func UserMutexSpec(path string) mutex.Spec { func getMutexNameForPath(path string) string { // juju requires that names match ^[a-zA-Z][a-zA-Z0-9-]*$", and be under 40 chars long. - n := strings.Trim(nonString.ReplaceAllString(path, "-"), "-") - // we need to always guarantee an alphanumeric prefix - prefix := "m" - - // Prefer the last 40 chars, as paths tend get more specific toward the end - if len(n) >= 40 { - return prefix + n[len(n)-39:] - } - return prefix + n + return fmt.Sprintf("m%x", md5.Sum([]byte(path))) } diff --git a/pkg/util/lock/lock_test.go b/pkg/util/lock/lock_test.go index 8bb97d046578..d7d7a22fe90a 100644 --- a/pkg/util/lock/lock_test.go +++ b/pkg/util/lock/lock_test.go @@ -24,45 +24,46 @@ func TestUserMutexSpec(t *testing.T) { var tests = []struct { description string path string - expected string }{ { description: "standard", path: "/foo/bar", - expected: "mfoo-bar-test", }, { description: "deep directory", path: "/foo/bar/baz/bat", - expected: "mfoo-bar-baz-bat-test", }, { description: "underscores", path: "/foo_bar/baz", - expected: "mfoo-bar-baz-test", }, { description: "starts with number", path: "/foo/2bar/baz", - expected: "mfoo-2bar-baz-test", }, { description: "starts with punctuation", path: "/.foo/bar", - expected: "mfoo-bar-test", }, { description: "long filename", path: "/very-very-very-very-very-very-very-very-long/bar", - expected: "m-very-very-very-very-very-long-bar-test", + }, + { + description: "Windows kubeconfig", + path: `C:\Users\admin/.kube/config`, + }, + { + description: "Windows json", + path: `C:\Users\admin\.minikube\profiles\containerd-20191210T212325.7356633-8584\config.json`, }, } for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { got := UserMutexSpec(tc.path) - if got.Name != tc.expected { - t.Errorf("%s mutex name = %q, expected %q", tc.path, got.Name, tc.expected) + if len(got.Name) > 40 { + t.Errorf("%s mutex name is too long", got.Name) } }) } From af4e690837bbd939c38bfafb9e27d7ad424dce61 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 11 Dec 2019 09:14:20 -0800 Subject: [PATCH 2/6] Use sha1 for a larger hash, truncated to 40 chars --- pkg/util/lock/lock.go | 13 ++++++------- pkg/util/lock/lock_test.go | 4 ++-- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index d5c945b9d8e2..6c5eee6ac8b1 100644 --- a/pkg/util/lock/lock.go +++ b/pkg/util/lock/lock.go @@ -17,12 +17,11 @@ limitations under the License. package lock import ( - "crypto/md5" + "crypto/sha1" "fmt" "io/ioutil" "os" "os/user" - "regexp" "time" "github.com/golang/glog" @@ -32,8 +31,6 @@ import ( ) var ( - // nonString is characters to strip from lock names - nonString = regexp.MustCompile(`[\W_]+`) // forceID is a user id for consistent testing forceID = "" ) @@ -64,9 +61,10 @@ func UserMutexSpec(path string) mutex.Spec { id = u.Uid } } - + name := getMutexNameForPath(fmt.Sprintf("%s-%s", path, id)) + glog.Infof("mutex name for %s: %s", path, name) s := mutex.Spec{ - Name: getMutexNameForPath(fmt.Sprintf("%s-%s", path, id)), + Name: name, Clock: clock.WallClock, // Poll the lock twice a second Delay: 500 * time.Millisecond, @@ -78,5 +76,6 @@ func UserMutexSpec(path string) mutex.Spec { func getMutexNameForPath(path string) string { // juju requires that names match ^[a-zA-Z][a-zA-Z0-9-]*$", and be under 40 chars long. - return fmt.Sprintf("m%x", md5.Sum([]byte(path))) + name := fmt.Sprintf("mk%x", sha1.Sum([]byte(path))) + return name[0:40] } diff --git a/pkg/util/lock/lock_test.go b/pkg/util/lock/lock_test.go index d7d7a22fe90a..43656fa94369 100644 --- a/pkg/util/lock/lock_test.go +++ b/pkg/util/lock/lock_test.go @@ -62,8 +62,8 @@ func TestUserMutexSpec(t *testing.T) { for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { got := UserMutexSpec(tc.path) - if len(got.Name) > 40 { - t.Errorf("%s mutex name is too long", got.Name) + if len(got.Name) != 40 { + t.Errorf("%s is not 40 chars long", got.Name) } }) } From 129b6a870b5847880712158bd30d745283151b76 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 11 Dec 2019 09:54:44 -0800 Subject: [PATCH 3/6] Update log messages --- pkg/util/lock/lock.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index 6c5eee6ac8b1..2455cc246d9e 100644 --- a/pkg/util/lock/lock.go +++ b/pkg/util/lock/lock.go @@ -38,16 +38,16 @@ var ( // WriteFile decorates ioutil.WriteFile with a file lock and retry func WriteFile(filename string, data []byte, perm os.FileMode) error { spec := UserMutexSpec(filename) - glog.Infof("acquiring lock for %s: %+v", filename, spec) + glog.Infof("WriteFile acquiring %s: %+v", filename, spec) releaser, err := mutex.Acquire(spec) if err != nil { - return errors.Wrapf(err, "error acquiring lock for %s", filename) + return errors.Wrapf(err, "failed to acquire lock for %s: %+v", filename, spec) } defer releaser.Release() if err = ioutil.WriteFile(filename, data, perm); err != nil { - return errors.Wrapf(err, "error writing file %s", filename) + return errors.Wrapf(err, "writefile failed for %s", filename) } return err } @@ -62,7 +62,6 @@ func UserMutexSpec(path string) mutex.Spec { } } name := getMutexNameForPath(fmt.Sprintf("%s-%s", path, id)) - glog.Infof("mutex name for %s: %s", path, name) s := mutex.Spec{ Name: name, Clock: clock.WallClock, From c5f65baf6134057af80be0be34e0678c1849743d Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 11 Dec 2019 11:52:36 -0800 Subject: [PATCH 4/6] Remove uid from hash name, as it is not necessary now that we lock on filenames rather than descriptive names --- pkg/util/lock/lock.go | 23 ++++------------------- pkg/util/lock/lock_test.go | 21 +++++++++++++++++---- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index 2455cc246d9e..78f965e76fb4 100644 --- a/pkg/util/lock/lock.go +++ b/pkg/util/lock/lock.go @@ -21,7 +21,6 @@ import ( "fmt" "io/ioutil" "os" - "os/user" "time" "github.com/golang/glog" @@ -37,7 +36,7 @@ var ( // WriteFile decorates ioutil.WriteFile with a file lock and retry func WriteFile(filename string, data []byte, perm os.FileMode) error { - spec := UserMutexSpec(filename) + spec := pathSpec(filename) glog.Infof("WriteFile acquiring %s: %+v", filename, spec) releaser, err := mutex.Acquire(spec) if err != nil { @@ -52,18 +51,10 @@ func WriteFile(filename string, data []byte, perm os.FileMode) error { return err } -// UserMutexSpec returns a mutex spec that will not collide with other users -func UserMutexSpec(path string) mutex.Spec { - id := forceID - if forceID == "" { - u, err := user.Current() - if err == nil { - id = u.Uid - } - } - name := getMutexNameForPath(fmt.Sprintf("%s-%s", path, id)) +// pathSpec returns a mutex spec for a path +func pathSpec(path string) mutex.Spec { s := mutex.Spec{ - Name: name, + Name: fmt.Sprintf("mk%x", sha1.Sum([]byte(path)))[0:40], Clock: clock.WallClock, // Poll the lock twice a second Delay: 500 * time.Millisecond, @@ -72,9 +63,3 @@ func UserMutexSpec(path string) mutex.Spec { } return s } - -func getMutexNameForPath(path string) string { - // juju requires that names match ^[a-zA-Z][a-zA-Z0-9-]*$", and be under 40 chars long. - name := fmt.Sprintf("mk%x", sha1.Sum([]byte(path))) - return name[0:40] -} diff --git a/pkg/util/lock/lock_test.go b/pkg/util/lock/lock_test.go index 43656fa94369..145fd6edaa6c 100644 --- a/pkg/util/lock/lock_test.go +++ b/pkg/util/lock/lock_test.go @@ -16,14 +16,17 @@ limitations under the License. package lock -import "testing" +import ( + "testing" -func TestUserMutexSpec(t *testing.T) { - forceID = "test" + "github.com/juju/mutex" +) +func TestUserMutexSpec(t *testing.T) { var tests = []struct { description string path string + expected string }{ { description: "standard", @@ -59,12 +62,22 @@ func TestUserMutexSpec(t *testing.T) { }, } + seen := map[string]string{} + for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { - got := UserMutexSpec(tc.path) + got := pathSpec(tc.path) if len(got.Name) != 40 { t.Errorf("%s is not 40 chars long", got.Name) } + if seen[got.Name] != "" { + t.Fatalf("lock name collision between %s and %s", tc.path, seen[got.Name]) + } + m, err := mutex.Acquire(got) + if err != nil { + t.Errorf("acquire for spec %+v failed: %v", got, err) + } + m.Release() }) } } From 3171493e5129ed195a1363ed13181d72e1773cee Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 11 Dec 2019 12:01:32 -0800 Subject: [PATCH 5/6] Rename to PathMutexSpec --- pkg/minikube/bootstrapper/certs.go | 2 +- pkg/minikube/kubeconfig/settings.go | 2 +- pkg/util/lock/lock.go | 6 +++--- pkg/util/lock/lock_test.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/minikube/bootstrapper/certs.go b/pkg/minikube/bootstrapper/certs.go index 611b5416cce8..d0731b24cd01 100644 --- a/pkg/minikube/bootstrapper/certs.go +++ b/pkg/minikube/bootstrapper/certs.go @@ -70,7 +70,7 @@ func SetupCerts(cmd command.Runner, k8s config.KubernetesConfig) error { // // If another process updates the shared certificate, it's invalid. // TODO: Instead of racey manipulation of a shared certificate, use per-profile certs - spec := lock.UserMutexSpec(filepath.Join(localPath, "certs")) + spec := lock.PathMutexSpec(filepath.Join(localPath, "certs")) glog.Infof("acquiring lock: %+v", spec) releaser, err := mutex.Acquire(spec) if err != nil { diff --git a/pkg/minikube/kubeconfig/settings.go b/pkg/minikube/kubeconfig/settings.go index 0cd09ec542ec..85b8f7ca9d3f 100644 --- a/pkg/minikube/kubeconfig/settings.go +++ b/pkg/minikube/kubeconfig/settings.go @@ -119,7 +119,7 @@ func PopulateFromSettings(cfg *Settings, apiCfg *api.Config) error { // activeContext is true when minikube is the CurrentContext // If no CurrentContext is set, the given name will be used. func Update(kcs *Settings) error { - spec := lock.UserMutexSpec(filepath.Join(kcs.filePath(), "settings.Update")) + spec := lock.PathMutexSpec(filepath.Join(kcs.filePath(), "settings.Update")) glog.Infof("acquiring lock: %+v", spec) releaser, err := mutex.Acquire(spec) if err != nil { diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index 78f965e76fb4..19990d378ae5 100644 --- a/pkg/util/lock/lock.go +++ b/pkg/util/lock/lock.go @@ -36,7 +36,7 @@ var ( // WriteFile decorates ioutil.WriteFile with a file lock and retry func WriteFile(filename string, data []byte, perm os.FileMode) error { - spec := pathSpec(filename) + spec := PathMutexSpec(filename) glog.Infof("WriteFile acquiring %s: %+v", filename, spec) releaser, err := mutex.Acquire(spec) if err != nil { @@ -51,8 +51,8 @@ func WriteFile(filename string, data []byte, perm os.FileMode) error { return err } -// pathSpec returns a mutex spec for a path -func pathSpec(path string) mutex.Spec { +// PathMutexSpec returns a mutex spec for a path +func PathMutexSpec(path string) mutex.Spec { s := mutex.Spec{ Name: fmt.Sprintf("mk%x", sha1.Sum([]byte(path)))[0:40], Clock: clock.WallClock, diff --git a/pkg/util/lock/lock_test.go b/pkg/util/lock/lock_test.go index 145fd6edaa6c..8ec94e5f24b1 100644 --- a/pkg/util/lock/lock_test.go +++ b/pkg/util/lock/lock_test.go @@ -66,7 +66,7 @@ func TestUserMutexSpec(t *testing.T) { for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { - got := pathSpec(tc.path) + got := PathMutexSpec(tc.path) if len(got.Name) != 40 { t.Errorf("%s is not 40 chars long", got.Name) } From d811f6e4284964f7dcf80903eeb88682165b37bf Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Wed, 11 Dec 2019 12:10:28 -0800 Subject: [PATCH 6/6] Remove unused forceID var --- pkg/util/lock/lock.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index 19990d378ae5..8d2e833ab0cf 100644 --- a/pkg/util/lock/lock.go +++ b/pkg/util/lock/lock.go @@ -29,11 +29,6 @@ import ( "github.com/pkg/errors" ) -var ( - // forceID is a user id for consistent testing - forceID = "" -) - // WriteFile decorates ioutil.WriteFile with a file lock and retry func WriteFile(filename string, data []byte, perm os.FileMode) error { spec := PathMutexSpec(filename)