From 3fe42f7506e0d8563f94130d2db034ba7dc8a99f Mon Sep 17 00:00:00 2001 From: Nick Sieger Date: Wed, 30 Nov 2022 14:50:43 -0600 Subject: [PATCH] config: copy go's file lock and use it instead of client-go's Fixes #4814. Signed-off-by: Nick Sieger --- internal/cli/client/config.go | 9 ++- internal/filelock/filelock.go | 102 ++++++++++++++++++++++++++ internal/filelock/filelock_unix.go | 44 +++++++++++ internal/filelock/filelock_windows.go | 66 +++++++++++++++++ internal/filelock/with_lock.go | 33 +++++++++ internal/hud/server/controller.go | 25 ++++++- 6 files changed, 274 insertions(+), 5 deletions(-) create mode 100644 internal/filelock/filelock.go create mode 100644 internal/filelock/filelock_unix.go create mode 100644 internal/filelock/filelock_windows.go create mode 100644 internal/filelock/with_lock.go diff --git a/internal/cli/client/config.go b/internal/cli/client/config.go index dfd73c5d90..8ad76a58b3 100644 --- a/internal/cli/client/config.go +++ b/internal/cli/client/config.go @@ -4,7 +4,9 @@ import ( "fmt" "k8s.io/client-go/tools/clientcmd" + clientcmdapi "k8s.io/client-go/tools/clientcmd/api" + "github.com/tilt-dev/tilt/internal/filelock" "github.com/tilt-dev/tilt/pkg/model" ) @@ -13,7 +15,12 @@ type TiltClientConfig clientcmd.ClientConfig // Uses the kubernetes config-loading library to create a client config // for the given server name. func ProvideClientConfig(apiServerName model.APIServerName, configAccess clientcmd.ConfigAccess) (TiltClientConfig, error) { - config, err := configAccess.GetStartingConfig() + var config *clientcmdapi.Config + err := filelock.WithRLock(configAccess, func() error { + var e error + config, e = configAccess.GetStartingConfig() + return e + }) if err != nil { return nil, err } diff --git a/internal/filelock/filelock.go b/internal/filelock/filelock.go new file mode 100644 index 0000000000..7b94b8fa3a --- /dev/null +++ b/internal/filelock/filelock.go @@ -0,0 +1,102 @@ +// Copyright 2018, 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// TODO(Tilt team): Remove this copy of Go's internal filelock module when +// https://github.com/golang/go/issues/33974 makes a public file locking api. + +// Package filelock provides a platform-independent API for advisory file +// locking. Calls to functions in this package on platforms that do not support +// advisory locks will return errors for which IsNotSupported returns true. +package filelock + +import ( + "errors" + "io/fs" + "os" +) + +// A File provides the minimal set of methods required to lock an open file. +// File implementations must be usable as map keys. +// The usual implementation is *os.File. +type File interface { + // Name returns the name of the file. + Name() string + + // Fd returns a valid file descriptor. + // (If the File is an *os.File, it must not be closed.) + Fd() uintptr + + // Stat returns the FileInfo structure describing file. + Stat() (fs.FileInfo, error) +} + +// Lock places an advisory write lock on the file, blocking until it can be +// locked. +// +// If Lock returns nil, no other process will be able to place a read or write +// lock on the file until this process exits, closes f, or calls Unlock on it. +// +// If f's descriptor is already read- or write-locked, the behavior of Lock is +// unspecified. +// +// Closing the file may or may not release the lock promptly. Callers should +// ensure that Unlock is always called when Lock succeeds. +func Lock(f File) error { + return lock(f, writeLock) +} + +// RLock places an advisory read lock on the file, blocking until it can be locked. +// +// If RLock returns nil, no other process will be able to place a write lock on +// the file until this process exits, closes f, or calls Unlock on it. +// +// If f is already read- or write-locked, the behavior of RLock is unspecified. +// +// Closing the file may or may not release the lock promptly. Callers should +// ensure that Unlock is always called if RLock succeeds. +func RLock(f File) error { + return lock(f, readLock) +} + +// Unlock removes an advisory lock placed on f by this process. +// +// The caller must not attempt to unlock a file that is not locked. +func Unlock(f File) error { + return unlock(f) +} + +// String returns the name of the function corresponding to lt +// (Lock, RLock, or Unlock). +func (lt lockType) String() string { + switch lt { + case readLock: + return "RLock" + case writeLock: + return "Lock" + default: + return "Unlock" + } +} + +// IsNotSupported returns a boolean indicating whether the error is known to +// report that a function is not supported (possibly for a specific input). +// It is satisfied by ErrNotSupported as well as some syscall errors. +func IsNotSupported(err error) bool { + return isNotSupported(underlyingError(err)) +} + +var ErrNotSupported = errors.New("operation not supported") + +// underlyingError returns the underlying error for known os error types. +func underlyingError(err error) error { + switch err := err.(type) { + case *fs.PathError: + return err.Err + case *os.LinkError: + return err.Err + case *os.SyscallError: + return err.Err + } + return err +} diff --git a/internal/filelock/filelock_unix.go b/internal/filelock/filelock_unix.go new file mode 100644 index 0000000000..d7778d05de --- /dev/null +++ b/internal/filelock/filelock_unix.go @@ -0,0 +1,44 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build darwin || dragonfly || freebsd || illumos || linux || netbsd || openbsd + +package filelock + +import ( + "io/fs" + "syscall" +) + +type lockType int16 + +const ( + readLock lockType = syscall.LOCK_SH + writeLock lockType = syscall.LOCK_EX +) + +func lock(f File, lt lockType) (err error) { + for { + err = syscall.Flock(int(f.Fd()), int(lt)) + if err != syscall.EINTR { + break + } + } + if err != nil { + return &fs.PathError{ + Op: lt.String(), + Path: f.Name(), + Err: err, + } + } + return nil +} + +func unlock(f File) error { + return lock(f, syscall.LOCK_UN) +} + +func isNotSupported(err error) bool { + return err == syscall.ENOSYS || err == syscall.ENOTSUP || err == syscall.EOPNOTSUPP || err == ErrNotSupported +} diff --git a/internal/filelock/filelock_windows.go b/internal/filelock/filelock_windows.go new file mode 100644 index 0000000000..e2ca538304 --- /dev/null +++ b/internal/filelock/filelock_windows.go @@ -0,0 +1,66 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build windows + +package filelock + +import ( + "internal/syscall/windows" + "io/fs" + "syscall" +) + +type lockType uint32 + +const ( + readLock lockType = 0 + writeLock lockType = windows.LOCKFILE_EXCLUSIVE_LOCK +) + +const ( + reserved = 0 + allBytes = ^uint32(0) +) + +func lock(f File, lt lockType) error { + // Per https://golang.org/issue/19098, “Programs currently expect the Fd + // method to return a handle that uses ordinary synchronous I/O.” + // However, LockFileEx still requires an OVERLAPPED structure, + // which contains the file offset of the beginning of the lock range. + // We want to lock the entire file, so we leave the offset as zero. + ol := new(syscall.Overlapped) + + err := windows.LockFileEx(syscall.Handle(f.Fd()), uint32(lt), reserved, allBytes, allBytes, ol) + if err != nil { + return &fs.PathError{ + Op: lt.String(), + Path: f.Name(), + Err: err, + } + } + return nil +} + +func unlock(f File) error { + ol := new(syscall.Overlapped) + err := windows.UnlockFileEx(syscall.Handle(f.Fd()), reserved, allBytes, allBytes, ol) + if err != nil { + return &fs.PathError{ + Op: "Unlock", + Path: f.Name(), + Err: err, + } + } + return nil +} + +func isNotSupported(err error) bool { + switch err { + case windows.ERROR_NOT_SUPPORTED, windows.ERROR_CALL_NOT_IMPLEMENTED, ErrNotSupported: + return true + default: + return false + } +} diff --git a/internal/filelock/with_lock.go b/internal/filelock/with_lock.go new file mode 100644 index 0000000000..7db94b6559 --- /dev/null +++ b/internal/filelock/with_lock.go @@ -0,0 +1,33 @@ +package filelock + +import ( + "os" + + "k8s.io/client-go/tools/clientcmd" +) + +func init() { + // We're using our own file locking mechanism + clientcmd.UseModifyConfigLock = false +} + +func WithLock(configAccess clientcmd.ConfigAccess, action func() error) error { + return withLock(configAccess.GetDefaultFilename()+".lock", writeLock, action) +} + +func WithRLock(configAccess clientcmd.ConfigAccess, action func() error) error { + return withLock(configAccess.GetDefaultFilename()+".lock", readLock, action) +} + +func withLock(filename string, lt lockType, action func() error) error { + lockfile, err := os.Create(filename) + if err != nil { + return err + } + err = lock(lockfile, lt) + if err != nil { + return err + } + defer func() { _ = Unlock(lockfile) }() + return action() +} diff --git a/internal/hud/server/controller.go b/internal/hud/server/controller.go index 2c7b4aa17d..b9942a60c6 100644 --- a/internal/hud/server/controller.go +++ b/internal/hud/server/controller.go @@ -17,6 +17,7 @@ import ( "k8s.io/kubectl/pkg/proxy" "github.com/tilt-dev/tilt-apiserver/pkg/server/start" + "github.com/tilt-dev/tilt/internal/filelock" "github.com/tilt-dev/tilt/internal/store" "github.com/tilt-dev/tilt/pkg/assets" "github.com/tilt-dev/tilt/pkg/model" @@ -202,7 +203,12 @@ func (s *HeadsUpServerController) addToAPIServerConfig() error { return nil } - newConfig, err := s.configAccess.GetStartingConfig() + var newConfig *clientcmdapi.Config + err := filelock.WithRLock(s.configAccess, func() error { + var e error + newConfig, e = s.configAccess.GetStartingConfig() + return e + }) if err != nil { return err } @@ -227,7 +233,7 @@ func (s *HeadsUpServerController) addToAPIServerConfig() error { CertificateAuthorityData: clientConfig.TLSClientConfig.CAData, } - return clientcmd.ModifyConfig(s.configAccess, *newConfig, true) + return s.modifyConfig(*newConfig) } // Remove this API server's configs into the user settings directory. @@ -238,7 +244,12 @@ func (s *HeadsUpServerController) removeFromAPIServerConfig() error { return nil } - newConfig, err := s.configAccess.GetStartingConfig() + var newConfig *clientcmdapi.Config + err := filelock.WithRLock(s.configAccess, func() error { + var e error + newConfig, e = s.configAccess.GetStartingConfig() + return e + }) if err != nil { return err } @@ -252,7 +263,13 @@ func (s *HeadsUpServerController) removeFromAPIServerConfig() error { delete(newConfig.AuthInfos, name) delete(newConfig.Clusters, name) - return clientcmd.ModifyConfig(s.configAccess, *newConfig, true) + return s.modifyConfig(*newConfig) +} + +func (s *HeadsUpServerController) modifyConfig(config clientcmdapi.Config) error { + return filelock.WithLock(s.configAccess, func() error { + return clientcmd.ModifyConfig(s.configAccess, config, true) + }) } func newAPIServerProxyHandler(config *rest.Config) (http.Handler, error) {