From 4b0bff6605d6a4076abb4f4fb8568242f365f59a Mon Sep 17 00:00:00 2001 From: "Jason T. Greene" Date: Fri, 11 Aug 2023 01:43:48 -0500 Subject: [PATCH] Introduce (global) default config file locking Adds LockDefault() and UnlockDefault() which lock an associate file mapping to the same default containers file that Write() selects. This functionality allows for multiple read-and-write operations to be atomically executed by serializing access from multiple updaters. As an example, if two parallel updaters are inserting a different entry into a map, one will likely be omitted from being overwritten with data from a stale read. Instead, using these functions, callers can coordinate, ensuring writes are never interposed between another caller's read-update pattern. Signed-off-by: Jason T. Greene --- Makefile | 10 +- cmd/flocksim/main.go | 30 ++++++ pkg/config/config.go | 74 +++++++++++++- pkg/config/config_test.go | 198 +++++++++++++++++++++++++++++++++++++ pkg/util/util_darwin.go | 12 +++ pkg/util/util_supported.go | 4 +- pkg/util/util_windows.go | 31 +++++- 7 files changed, 351 insertions(+), 8 deletions(-) create mode 100644 cmd/flocksim/main.go create mode 100644 pkg/util/util_darwin.go diff --git a/Makefile b/Makefile index 303017027..a3f1f872c 100644 --- a/Makefile +++ b/Makefile @@ -55,9 +55,16 @@ endif bin/netavark-testplugin: $(GO_BUILD) -o $@ ./libnetwork/netavark/testplugin/ +.PHONY: bin/flocksim +bin/flocksim: + $(GO_BUILD) -o $@ ./cmd/flocksim/ + .PHONY: netavark-testplugin netavark-testplugin: bin/netavark-testplugin +.PHONY: flocksim +flocksim: bin/flocksim + .PHONY: docs docs: $(MAKE) -C docs @@ -97,7 +104,7 @@ install: test: test-unit .PHONY: test-unit -test-unit: netavark-testplugin +test-unit: netavark-testplugin flocksim go test --tags $(BUILDTAGS) -v ./libimage go test --tags $(BUILDTAGS) -v ./libnetwork/... go test --tags $(BUILDTAGS) -v ./pkg/... @@ -111,6 +118,7 @@ clean: ## Clean artifacts $(MAKE) -C docs clean find . -name \*~ -delete find . -name \#\* -delete + rm -rf bin .PHONY: seccomp.json seccomp.json: $(sources) diff --git a/cmd/flocksim/main.go b/cmd/flocksim/main.go new file mode 100644 index 000000000..cbcfffd98 --- /dev/null +++ b/cmd/flocksim/main.go @@ -0,0 +1,30 @@ +package main + +import ( + "bufio" + "fmt" + "os" + + "github.com/containers/storage/pkg/lockfile" +) + +// flocksim is a testing tool used by the config lock tests +func main() { + if len(os.Args) < 2 { + fmt.Printf("Usage: %s \n", os.Args[0]) + os.Exit(1) + } + + lock, err := lockfile.GetLockFile(os.Args[1]) + if err != nil { + fmt.Fprintln(os.Stderr, err.Error()) + os.Exit(1) + } + + lock.Lock() + fmt.Println("acquired lock, hit enter to release") + + reader := bufio.NewReader(os.Stdin) + _, _ = reader.ReadString('\n') + lock.Unlock() +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 41f58185c..8d129a7dc 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -17,6 +17,7 @@ import ( "github.com/containers/common/pkg/capabilities" "github.com/containers/common/pkg/util" "github.com/containers/storage/pkg/ioutils" + "github.com/containers/storage/pkg/lockfile" "github.com/containers/storage/pkg/unshare" units "github.com/docker/go-units" selinux "github.com/opencontainers/selinux/go-selinux" @@ -1191,9 +1192,10 @@ func rootlessConfigPath() (string, error) { } var ( - configErr error - configMutex sync.Mutex - config *Config + configErr error + configMutex sync.Mutex + config *Config + configLockFile *lockfile.LockFile ) // Default returns the default container config. @@ -1221,6 +1223,72 @@ func defConfig() (*Config, error) { return config, configErr } +// Locks the config default file through an associated runtime file lock +// This method enables callers to coordinate updates to the default +// configuration file, and ensure atomicity across multiple operations. +// Typically a caller would call this method before reading values, +// perform permutations to the config, write the config, and finally +// call UnlockDefault(). Since this lock is global, callers should +// aim to minimize the length of time the lock is held. +// +// Multiple calls to this method will reference count the underlying +// lock. +func LockDefault() error { + lockFile, err := getLockFile() + if err != nil { + return err + } + lockFile.Lock() + return nil +} + +// Unlocks the config default file through an associated runtime file lock. +// The underlying lock is reference counted. The file will only become +// unlocked when a corresponding number of calls to this method have been +// made matching the number of preceding LockDefault() calls. +func UnlockDefault() { + lockFile, err := getLockFile() + if err != nil { + return + } + lockFile.Unlock() +} + +func getLockFilePath() (string, error) { + lockDir, err := util.GetRuntimeDir() + if err != nil { + return "", err + } + + lockDir = filepath.Join(lockDir, "containers") + if err := os.MkdirAll(lockDir, 0o755); err != nil { + return "", err + } + + return filepath.Join(lockDir, "config.lck"), nil +} + +func getLockFile() (*lockfile.LockFile, error) { + configMutex.Lock() + defer configMutex.Unlock() + + if configLockFile != nil { + return configLockFile, nil + } + + path, err := getLockFilePath() + if err != nil { + return nil, err + } + lockFile, err := lockfile.GetLockFile(path) + if err != nil { + return nil, err + } + + configLockFile = lockFile + return lockFile, nil +} + func Path() string { if path := os.Getenv("CONTAINERS_CONF"); path != "" { return path diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 8c12e80e2..002fe8fec 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -1,13 +1,17 @@ package config import ( + "bufio" "bytes" + "errors" "fmt" "io" "os" + "os/exec" "runtime" "sort" "strings" + "time" "github.com/containers/common/pkg/apparmor" "github.com/containers/common/pkg/capabilities" @@ -1013,4 +1017,198 @@ env=["foo=bar"]` gomega.Expect(config.Containers.BaseHostsFile).To(gomega.Equal("/etc/hosts2")) gomega.Expect(config.Containers.EnableLabeledUsers).To(gomega.BeTrue()) }) + + It("container conf lock operations", func() { + // This test tests two competing goroutines which both add a connection + // definition at the same time. Through the use of LockDefault/UnlockDefault + // each goroutine should be serialized and correctly observe the other's + // state + const oneURI = "https://qa/run/one.sock" + const twoURI = "https://qa/run/two.sock" + + if _, err := os.Stat("../../bin/flocksim"); err != nil { + Skip("flocksim not present") + return + } + + defer os.Unsetenv("CONTAINERS_CONF") + f, err := os.CreateTemp("", "container-common-test") + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + defer f.Close() + defer os.Remove(f.Name()) + + os.Setenv("CONTAINERS_CONF", f.Name()) + primaryConfig, err := Default() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + err = primaryConfig.Write() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + configPath := f.Name() + lockPath, err := getLockFilePath() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + err = LockDefault() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + result := make(chan error, 3) + go func() { + // Since the contract for lock is global, simulate another process + // through a manual direct lock. + cmd, inStream, outStream, errStream, err := startCommandWithStreams("../../bin/flocksim", lockPath) + if err != nil { + result <- err + } + + output := singleLineSlurp(outStream) + errResult := allLineSlurp(errStream) + + // Wait until we get a lock acquired message, if we don't an error was thrown + if len(<-output) == 0 { + _ = cmd.Wait() + result <- errors.New(<-errResult) + return + } + + defer func() { + // Tell flocksim to shutdown + _, _ = inStream.Write([]byte("done\n")) + _ = inStream.Close() + result <- waitCommandWithTimeout(cmd, 10) + }() + + // Indicate success, we have a lock + result <- nil + + racingConfig := Config{} + err = readConfigFromFile(configPath, &racingConfig) + if err != nil { + result <- err + return + } + + sd, ok := racingConfig.Engine.ServiceDestinations["one"] + if !ok { + result <- errors.New("expected service definition missing") + return + } + + if sd.URI != oneURI { + result <- errors.New("service definition did not match expected URI") + return + } + + racingConfig.Engine.ServiceDestinations = map[string]Destination{ + "two": { + URI: twoURI, + Identity: "/.ssh/id_two", + }, + } + + result <- racingConfig.Write() + }() + + // Not mandatory, just yielding to increase chance of failure if problem + time.Sleep(1 * time.Second) + + primaryConfig, err = ReadCustomConfig() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + primaryConfig.Engine.ActiveService = "one" + primaryConfig.Engine.ServiceDestinations = map[string]Destination{ + "one": { + URI: oneURI, + Identity: "/.ssh/id_one", + }, + } + err = primaryConfig.Write() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + UnlockDefault() + // Verify goroutine acquired lock successfully + gomega.Expect(<-result).ToNot(gomega.HaveOccurred()) + + // Block until goroutine finishes + err = LockDefault() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Verify goroutine's write was successful + gomega.Expect(<-result).ToNot(gomega.HaveOccurred()) + + // Verify goroutine exit was successful + gomega.Expect(<-result).ToNot(gomega.HaveOccurred()) + + primaryConfig, err = ReadCustomConfig() + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + sd, ok := primaryConfig.Engine.ServiceDestinations["two"] + gomega.Expect(ok).To(gomega.BeTrue()) + gomega.Expect(sd.URI).To(gomega.Equal(twoURI)) + }) }) + +func waitCommandWithTimeout(cmd *exec.Cmd, seconds int) error { + wChan := make(chan error, 1) + go func() { + wChan <- cmd.Wait() + }() + select { + case err := <-wChan: + return err + case <-time.After(time.Duration(seconds) * time.Second): + _ = cmd.Process.Kill() + return fmt.Errorf("Timeout running %s had to kill: %w", cmd.Args[0], <-wChan) + } +} + +func startCommandWithStreams(executable string, args ...string) (*exec.Cmd, io.WriteCloser, io.ReadCloser, io.ReadCloser, error) { + cmd := exec.Command(executable, args...) + outStream, err := cmd.StdoutPipe() + if err != nil { + return nil, nil, nil, nil, err + } + errStream, err := cmd.StderrPipe() + if err != nil { + return nil, nil, nil, nil, err + } + inStream, err := cmd.StdinPipe() + if err != nil { + return nil, nil, nil, nil, err + } + err = cmd.Start() + if err != nil { + return nil, nil, nil, nil, err + } + + return cmd, inStream, outStream, errStream, nil +} + +func singleLineSlurp(stream io.ReadCloser) chan string { + outChan := make(chan string, 1) + + go func() { + reader := bufio.NewReader(stream) + str, err := reader.ReadString('\n') + if err != nil { + str = "" + stream.Close() + } + outChan <- str + }() + + return outChan +} + +func allLineSlurp(stream io.ReadCloser) chan string { + outChan := make(chan string, 1) + + go func() { + reader := bufio.NewReader(stream) + bytes, err := io.ReadAll(reader) + if err != nil { + bytes = []byte{} + stream.Close() + } + outChan <- string(bytes) + }() + + return outChan +} diff --git a/pkg/util/util_darwin.go b/pkg/util/util_darwin.go new file mode 100644 index 000000000..488909012 --- /dev/null +++ b/pkg/util/util_darwin.go @@ -0,0 +1,12 @@ +package util + +import "os" + +// getRuntimeDir returns the runtime directory +func GetRuntimeDir() (string, error) { + tmpDir, ok := os.LookupEnv("TMPDIR") + if !ok { + tmpDir = "/tmp" + } + return tmpDir, nil +} diff --git a/pkg/util/util_supported.go b/pkg/util/util_supported.go index 0cd53af53..a0083f8a9 100644 --- a/pkg/util/util_supported.go +++ b/pkg/util/util_supported.go @@ -1,5 +1,5 @@ -//go:build linux || darwin || freebsd -// +build linux darwin freebsd +//go:build linux || freebsd +// +build linux freebsd package util diff --git a/pkg/util/util_windows.go b/pkg/util/util_windows.go index 1525bdc34..196e964b0 100644 --- a/pkg/util/util_windows.go +++ b/pkg/util/util_windows.go @@ -4,10 +4,37 @@ package util import ( - "errors" + "os" ) // getRuntimeDir returns the runtime directory func GetRuntimeDir() (string, error) { - return "", errors.New("this function is not implemented for windows") + tmpDir, ok := os.LookupEnv("TEMP") + if ok { + return tmpDir, nil + } + + tmpDir, ok = os.LookupEnv("LOCALAPPDATA") + if ok { + tmpDir += `\Temp` + } + + if !ok { + tmpDir, ok = os.LookupEnv("USERPROFILE") + if !ok { + tmpDir, ok = os.LookupEnv("HOME") + } + // Append to either match + if ok { + tmpDir += `\AppData\Local\Temp` + } + } + + if !ok { + // Hope for the best + return `C:\Temp`, nil + } + _ = os.MkdirAll(tmpDir, 0o700) + + return tmpDir, nil }