Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: copy go's file lock and use it instead of client-go's #5982

Merged
merged 2 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion internal/cli/client/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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
}
Expand Down
102 changes: 102 additions & 0 deletions internal/filelock/filelock.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// 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.

// 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
}
44 changes: 44 additions & 0 deletions internal/filelock/filelock_unix.go
Original file line number Diff line number Diff line change
@@ -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
}
124 changes: 124 additions & 0 deletions internal/filelock/filelock_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
// 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 (
"io/fs"
"syscall"
"unsafe"
)

type lockType uint32

const (
LOCKFILE_EXCLUSIVE_LOCK = 0x00000002 // from internal/syscall/windows

readLock lockType = 0
writeLock lockType = 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 := 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 := 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 ERROR_NOT_SUPPORTED, ERROR_CALL_NOT_IMPLEMENTED, ErrNotSupported:
return true
default:
return false
}
}

// portions of internal/syscall/windows below here for LockFileEx/UnlockFileEx
var _ unsafe.Pointer

// Do the interface allocations only once for common
// Errno values.
const (
errnoERROR_IO_PENDING = 997
)

var (
errERROR_IO_PENDING error = syscall.Errno(errnoERROR_IO_PENDING)
errERROR_EINVAL error = syscall.EINVAL
)

// errnoErr returns common boxed Errno values, to prevent
// allocations at runtime.
func errnoErr(e syscall.Errno) error {
switch e {
case 0:
return errERROR_EINVAL
case errnoERROR_IO_PENDING:
return errERROR_IO_PENDING
}
// TODO: add more here, after collecting data on the common
// error values see on Windows. (perhaps when running
// all.bat?)
return e
}

const (
ERROR_NOT_SUPPORTED syscall.Errno = 50
ERROR_CALL_NOT_IMPLEMENTED syscall.Errno = 120
)

var (
modkernel32 = syscall.NewLazyDLL("kernel32.dll")
procLockFileEx = modkernel32.NewProc("LockFileEx")
procUnlockFileEx = modkernel32.NewProc("UnlockFileEx")
)

func LockFileEx(file syscall.Handle, flags uint32, reserved uint32, bytesLow uint32, bytesHigh uint32, overlapped *syscall.Overlapped) (err error) {
r1, _, e1 := syscall.Syscall6(procLockFileEx.Addr(), 6, uintptr(file), uintptr(flags), uintptr(reserved), uintptr(bytesLow), uintptr(bytesHigh), uintptr(unsafe.Pointer(overlapped)))
if r1 == 0 {
err = errnoErr(e1)
}
return
}

func UnlockFileEx(file syscall.Handle, reserved uint32, bytesLow uint32, bytesHigh uint32, overlapped *syscall.Overlapped) (err error) {
r1, _, e1 := syscall.Syscall6(procUnlockFileEx.Addr(), 5, uintptr(file), uintptr(reserved), uintptr(bytesLow), uintptr(bytesHigh), uintptr(unsafe.Pointer(overlapped)), 0)
if r1 == 0 {
err = errnoErr(e1)
}
return
}
33 changes: 33 additions & 0 deletions internal/filelock/with_lock.go
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think my big concern with this is that it's global state.

it assumes you've updated all the call sites.

i'm not at all confident you've updated all the call sites (particularly the ones in our dependencies)

i'd honestly be more comfortable with a patched version of client-go that uses our locking mechanism. (we already patch apimachinery)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not ideal and could be easily overlooked if we started expanding the ways in which Tilt handles kubeconfigs. In this case, the only place in Tilt where we write config with client-go is the 2 places in this PR in the hud server controller package. I added read locks in places where we read the same config but client-go already didn't try to achieve read consistency, so those aren't strictly necessari. I could move all code dealing with reading and writing Tilt's kubeconfig to a separate package if you think that would help with the maintenance issues.

As it is, I consider this PR to be a temporary band-aid that should be fixed once Go has a public file locking API, at which point we or someone would send a PR to client-go to change the global. Realizing too that "temporary" fixes often become long-term ones, I feel that this change is worthwhile given the lower amount of maintenance we do on Tilt, the point above on how we only have two locations (in the same package) that do writes and the fact that write collisions on kubeconfigs in a dev environment are pretty unlikely.

}

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()
}
25 changes: 21 additions & 4 deletions internal/hud/server/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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.
Expand All @@ -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
}
Expand All @@ -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) {
Expand Down