Skip to content

Commit

Permalink
fix: auto-pin race condition on go.mod file (#521)
Browse files Browse the repository at this point in the history 
When using `-toolexec` mode, the auto-pin process may be initiated by
multiple concurrent executions of the `toolexec` proxy. In such cases,
there is a race condition on access to `go.mod` and
`orchestrion.tool.go`, which often results in `go mod tidy` failing due
to it detecting concurrent modifications of the `go.mod` (it may also
result in corrupt content of the `orchestrion.tool.go` file).

This addresses the issue by having the auto-pin process acquire an
advisory write lock on the `go.mod` file, making sure all attempts are
properly synchronized, and only one attempt tries to modify the file at
a time.

Fixes #491
  • Loading branch information
@RomainMuller
RomainMuller authored Feb 11, 2025
1 parent 020b6fe commit 5024673
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 28 deletions.
2 changes: 1 addition & 1 deletion internal/filelock/filelock.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ import (
// Upgrading a read-lock to a write lock, or vice-versa, is not guaranteed to
// happen atomically (on Windows, it is guaranteed not to be atomic).
type Mutex struct {
path string
file *os.File
path string
locked lockState
}

Expand Down
11 changes: 9 additions & 2 deletions internal/pin/auto_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ import (
)

func TestAutoPin(t *testing.T) {
ctx := context.Background()
if d, ok := t.Deadline(); ok {
var cancel context.CancelFunc
ctx, cancel = context.WithDeadline(ctx, d)
defer cancel()
}

t.Run("simple", func(t *testing.T) {
tmp := scaffold(t, make(map[string]string))
chdir(t, tmp)
Expand All @@ -30,7 +37,7 @@ func TestAutoPin(t *testing.T) {
assert.FileExists(t, filepath.Join(tmp, config.FilenameOrchestrionToolGo))
assert.FileExists(t, filepath.Join(tmp, "go.sum"))

data, err := parseGoMod(filepath.Join(tmp, "go.mod"))
data, err := parseGoMod(ctx, filepath.Join(tmp, "go.mod"))
require.NoError(t, err)

rawTag, _ := version.TagInfo()
Expand All @@ -45,7 +52,7 @@ func TestAutoPin(t *testing.T) {

t.Setenv(envVarCheckedGoMod, "true")

AutoPinOrchestrion(context.Background(), io.Discard, io.Discard)
AutoPinOrchestrion(ctx, io.Discard, io.Discard)

assert.NoFileExists(t, filepath.Join(tmp, config.FilenameOrchestrionToolGo))
assert.NoFileExists(t, filepath.Join(tmp, "go.sum"))
Expand Down
21 changes: 11 additions & 10 deletions internal/pin/gomod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package pin

import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -47,9 +48,9 @@ type (

// parseGoMod processes the contents of the designated `go.mod` file using
// `go mod edit -json` and returns the corresponding parsed [goMod].
func parseGoMod(modfile string) (goMod, error) {
func parseGoMod(ctx context.Context, modfile string) (goMod, error) {
var stdout bytes.Buffer
if err := runGoMod("edit", modfile, &stdout, "-json"); err != nil {
if err := runGoMod(ctx, "edit", modfile, &stdout, "-json"); err != nil {
return goMod{}, fmt.Errorf("running `go mod edit -json`: %w", err)
}

Expand All @@ -74,8 +75,8 @@ func (m *goMod) requires(path string) (string, bool) {

// runGoGet executes the `go get <modSpecs...>` subcommand with the provided
// module specifications on the designated `go.mod` file.
func runGoGet(modfile string, modSpecs ...string) error {
cmd := exec.Command("go", "get", "-modfile", modfile)
func runGoGet(ctx context.Context, modfile string, modSpecs ...string) error {
cmd := exec.CommandContext(ctx, "go", "get", "-modfile", modfile)
cmd.Args = append(cmd.Args, modSpecs...)
cmd.Env = append(os.Environ(), "GOTOOLCHAIN=local")
cmd.Stdin = os.Stdin
Expand All @@ -87,8 +88,8 @@ func runGoGet(modfile string, modSpecs ...string) error {
// runGoMod executes the `go mod <command> <args...>` subcommand with the
// provided arguments on the designated `go.mod` file, sending standard output
// to the provided writer.
func runGoMod(command string, modfile string, stdout io.Writer, args ...string) error {
cmd := exec.Command("go", "mod", command, "-modfile", modfile)
func runGoMod(ctx context.Context, command string, modfile string, stdout io.Writer, args ...string) error {
cmd := exec.CommandContext(ctx, "go", "mod", command, "-modfile", modfile)
cmd.Args = append(cmd.Args, args...)
cmd.Env = append(os.Environ(), "GOTOOLCHAIN=local")
cmd.Stdin = os.Stdin
Expand All @@ -99,7 +100,7 @@ func runGoMod(command string, modfile string, stdout io.Writer, args ...string)

// runGoModEdit makes the specified changes to the `go.mod` file, then runs `go mod tidy` if needed.
// If there is a `vendor` directory, it also runs `go mod vendor` before returning.
func runGoModEdit(modfile string, edits ...goModEdit) error {
func runGoModEdit(ctx context.Context, modfile string, edits ...goModEdit) error {
if len(edits) == 0 {
// Nothing to do.
return nil
Expand All @@ -109,11 +110,11 @@ func runGoModEdit(modfile string, edits ...goModEdit) error {
for i, edit := range edits {
editFlags[i] = edit.goModEditFlag()
}
if err := runGoMod("edit", modfile, os.Stdout, editFlags...); err != nil {
if err := runGoMod(ctx, "edit", modfile, os.Stdout, editFlags...); err != nil {
return fmt.Errorf("running `go mod edit %s`: %w", editFlags, err)
}

if err := runGoMod("tidy", modfile, os.Stdout); err != nil {
if err := runGoMod(ctx, "tidy", modfile, os.Stdout); err != nil {
return fmt.Errorf("running `go mod tidy`: %w", err)
}

Expand All @@ -127,7 +128,7 @@ func runGoModEdit(modfile string, edits ...goModEdit) error {
return fmt.Errorf("checking for vendor directory %q: %w", vendorDir, err)
}

if err := runGoMod("vendor", modfile, os.Stdout); err != nil {
if err := runGoMod(ctx, "vendor", modfile, os.Stdout); err != nil {
return fmt.Errorf("running `go mod vendor`: %w", err)
}

Expand Down
29 changes: 24 additions & 5 deletions internal/pin/pin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ package pin
import (
"bytes"
"context"
"crypto/sha512"
"encoding/base64"
"errors"
"fmt"
"go/parser"
Expand All @@ -18,6 +20,7 @@ import (
"path/filepath"
"strings"

"github.com/DataDog/orchestrion/internal/filelock"
"github.com/DataDog/orchestrion/internal/goenv"
"github.com/DataDog/orchestrion/internal/injector/config"
"github.com/DataDog/orchestrion/internal/version"
Expand Down Expand Up @@ -71,6 +74,22 @@ func PinOrchestrion(ctx context.Context, opts Options) error {
return fmt.Errorf("getting GOMOD: %w", err)
}

// Acquire an advisory lock on the `go.mod` file, so that in `-toolexec` mode,
// multiple attempts to auto-pin don't try to modify the files at the same
// time. The `go mod tidy` command takes an advisory write-lock on `go.mod`,
// so we are using a separate file under [os.TempDir] to avoid deadlocking.
sha := sha512.Sum512([]byte(goMod))
flockname := filepath.Join(os.TempDir(), "orchestrion-pin_"+base64.URLEncoding.EncodeToString(sha[:])+"_go.mod.lock")
flock := filelock.MutexAt(flockname)
if err := flock.Lock(); err != nil {
return fmt.Errorf("failed to acquire lock on %q: %w", goMod, err)
}
defer func() {
if err := flock.Unlock(); err != nil {
log.Error().Err(err).Str("lock-file", goMod).Msg("Failed to release file lock")
}
}()

toolFile := filepath.Join(goMod, "..", config.FilenameOrchestrionToolGo)
dstFile, err := parseOrchestrionToolGo(toolFile)
if errors.Is(err, os.ErrNotExist) {
Expand All @@ -96,14 +115,14 @@ func PinOrchestrion(ctx context.Context, opts Options) error {

// Add the current version of orchestrion to the `go.mod` file.
var edits []goModEdit
curMod, err := parseGoMod(goMod)
curMod, err := parseGoMod(ctx, goMod)
if err != nil {
return fmt.Errorf("parsing %q: %w", goMod, err)
}

if ver, found := curMod.requires(datadogTracerV1); !found || semver.Compare(ver, "v1.72.0-rc.1") < 0 {
log.Info().Msg("Installing or upgrading " + datadogTracerV1)
if err := runGoGet(goMod, datadogTracerV1+"@>=v1.72.0-rc.1"); err != nil {
if err := runGoGet(ctx, goMod, datadogTracerV1+"@>=v1.72.0-rc.1"); err != nil {
return fmt.Errorf("go get "+datadogTracerV1+": %w", err)
}
}
Expand All @@ -119,7 +138,7 @@ func PinOrchestrion(ctx context.Context, opts Options) error {
edits = append(edits, goModRequire{Path: orchestrionImportPath, Version: version})
}

if err := runGoModEdit(goMod, edits...); err != nil {
if err := runGoModEdit(ctx, goMod, edits...); err != nil {
return fmt.Errorf("editing %q: %w", goMod, err)
}

Expand All @@ -130,13 +149,13 @@ func PinOrchestrion(ctx context.Context, opts Options) error {

if pruned {
// Run "go mod tidy" to ensure the `go.mod` file is up-to-date with detected dependencies.
if err := runGoMod("tidy", goMod, nil); err != nil {
if err := runGoMod(ctx, "tidy", goMod, nil); err != nil {
return fmt.Errorf("running `go mod tidy`: %w", err)
}
}

// Restore the previous toolchain directive if `go mod tidy` had the nerve to touch it...
if err := runGoModEdit(goMod, curMod.Toolchain); err != nil {
if err := runGoModEdit(ctx, goMod, curMod.Toolchain); err != nil {
return fmt.Errorf("restoring toolchain directive: %w", err)
}

Expand Down
27 changes: 17 additions & 10 deletions internal/pin/pin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,23 @@ import (
)

func TestPin(t *testing.T) {
ctx := context.Background()
if d, ok := t.Deadline(); ok {
var cancel context.CancelFunc
ctx, cancel = context.WithDeadline(ctx, d)
defer cancel()
}

t.Run("simple", func(t *testing.T) {
tmp := scaffold(t, make(map[string]string))
chdir(t, tmp)

require.NoError(t, PinOrchestrion(context.Background(), Options{Writer: io.Discard, ErrWriter: io.Discard}))
require.NoError(t, PinOrchestrion(ctx, Options{Writer: io.Discard, ErrWriter: io.Discard}))

assert.FileExists(t, filepath.Join(tmp, config.FilenameOrchestrionToolGo))
assert.FileExists(t, filepath.Join(tmp, "go.sum"))

data, err := parseGoMod(filepath.Join(tmp, "go.mod"))
data, err := parseGoMod(ctx, filepath.Join(tmp, "go.mod"))
require.NoError(t, err)

rawTag, _ := version.TagInfo()
Expand All @@ -46,12 +53,12 @@ func TestPin(t *testing.T) {
tmp := scaffold(t, map[string]string{"github.com/DataDog/orchestrion": "v0.9.3"})
chdir(t, tmp)

require.NoError(t, PinOrchestrion(context.Background(), Options{Writer: io.Discard, ErrWriter: io.Discard}))
require.NoError(t, PinOrchestrion(ctx, Options{Writer: io.Discard, ErrWriter: io.Discard}))

assert.FileExists(t, filepath.Join(tmp, config.FilenameOrchestrionToolGo))
assert.FileExists(t, filepath.Join(tmp, "go.sum"))

data, err := parseGoMod(filepath.Join(tmp, "go.mod"))
data, err := parseGoMod(ctx, filepath.Join(tmp, "go.mod"))
require.NoError(t, err)

rawTag, _ := version.TagInfo()
Expand All @@ -62,7 +69,7 @@ func TestPin(t *testing.T) {
tmp := scaffold(t, make(map[string]string))
chdir(t, tmp)

require.NoError(t, PinOrchestrion(context.Background(), Options{Writer: io.Discard, ErrWriter: io.Discard, NoGenerate: true}))
require.NoError(t, PinOrchestrion(ctx, Options{Writer: io.Discard, ErrWriter: io.Discard, NoGenerate: true}))

content, err := os.ReadFile(filepath.Join(tmp, config.FilenameOrchestrionToolGo))
require.NoError(t, err)
Expand All @@ -74,9 +81,9 @@ func TestPin(t *testing.T) {
tmp := scaffold(t, map[string]string{"github.com/digitalocean/sample-golang": "v0.0.0-20240904143939-1e058723dcf4"})
chdir(t, tmp)

require.NoError(t, PinOrchestrion(context.Background(), Options{Writer: io.Discard, ErrWriter: io.Discard, NoGenerate: true}))
require.NoError(t, PinOrchestrion(ctx, Options{Writer: io.Discard, ErrWriter: io.Discard, NoGenerate: true}))

data, err := parseGoMod(filepath.Join(tmp, "go.mod"))
data, err := parseGoMod(ctx, filepath.Join(tmp, "go.mod"))
require.NoError(t, err)

assert.NotContains(t, data.Require, goModRequire{"github.com/digitalocean/sample-golang", "v0.0.0-20240904143939-1e058723dcf4"})
Expand All @@ -89,9 +96,9 @@ func TestPin(t *testing.T) {
})
chdir(t, tmp)

require.NoError(t, PinOrchestrion(context.Background(), Options{Writer: io.Discard, ErrWriter: io.Discard, NoGenerate: true}))
require.NoError(t, PinOrchestrion(ctx, Options{Writer: io.Discard, ErrWriter: io.Discard, NoGenerate: true}))

data, err := parseGoMod(filepath.Join(tmp, "go.mod"))
data, err := parseGoMod(ctx, filepath.Join(tmp, "go.mod"))
require.NoError(t, err)

assert.NotContains(t, data.Require, goModRequire{"github.com/digitalocean/sample-golang", "v0.0.0-20240904143939-1e058723dcf4"})
Expand All @@ -105,7 +112,7 @@ func TestPin(t *testing.T) {
toolDotGo := filepath.Join(tmp, config.FilenameOrchestrionToolGo)
require.NoError(t, os.WriteFile(toolDotGo, nil, 0644))

require.ErrorContains(t, PinOrchestrion(context.Background(), Options{Writer: io.Discard, ErrWriter: io.Discard}), "expected 'package', found 'EOF'")
require.ErrorContains(t, PinOrchestrion(ctx, Options{Writer: io.Discard, ErrWriter: io.Discard}), "expected 'package', found 'EOF'")
})
}

Expand Down

0 comments on commit 5024673

Please sign in to comment.