Merge pull request #6059 from tstromberg/clock

lock names: Remove uid suffix & hash entire path

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 {

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 {

pkg/util/lock/lock.go

@@ -17,12 +17,10 @@ limitations under the License.
 package lock
 
 import (
+	"crypto/sha1"
 	"fmt"
 	"io/ioutil"
 	"os"
-	"os/user"
-	"regexp"
-	"strings"
 	"time"
 
 	"github.com/golang/glog"
@@ -31,42 +29,27 @@ import (
 	"github.com/pkg/errors"
 )
 
-var (
-	// nonString is characters to strip from lock names
-	nonString = regexp.MustCompile(`[\W_]+`)
-	// 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 := UserMutexSpec(filename)
-	glog.Infof("acquiring lock for %s: %+v", filename, spec)
+	spec := PathMutexSpec(filename)
+	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
 }
 
-// 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
-		}
-	}
-
+// PathMutexSpec returns a mutex spec for a path
+func PathMutexSpec(path string) mutex.Spec {
 	s := mutex.Spec{
-		Name:  getMutexNameForPath(fmt.Sprintf("%s-%s", path, id)),
+		Name:  fmt.Sprintf("mk%x", sha1.Sum([]byte(path)))[0:40],
 		Clock: clock.WallClock,
 		// Poll the lock twice a second
 		Delay: 500 * time.Millisecond,
@@ -75,16 +58,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.
-	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
-}

pkg/util/lock/lock_test.go

@@ -16,11 +16,13 @@ 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
@@ -29,41 +31,53 @@ func TestUserMutexSpec(t *testing.T) {
 		{
 			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`,
 		},
 	}
 
+	seen := map[string]string{}
+
 	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)
+			got := PathMutexSpec(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()
 		})
 	}
 }