diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 19ff8217b..db556e88e 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -33,7 +33,7 @@ jobs: run: echo "name=$(echo '${{ github.ref_name }}' | tr '/' '-')" >> "$GITHUB_OUTPUT" - name: Run Go unit tests - run: go test -test.short -v -coverprofile=coverage.out -covermode=atomic ./... + run: go test -race -test.short -v -coverprofile=coverage.out -covermode=atomic ./... working-directory: src env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # to avoid GH rate limits @@ -205,7 +205,7 @@ jobs: # cache-dependency-path: src/go.sum # - name: Run sanity tests - # run: go run ./cmd/cli -C testdata/sanity compose up --debug + # run: go run -race ./cmd/cli -C testdata/sanity compose up --debug # working-directory: src go-playground-test: @@ -223,27 +223,27 @@ jobs: cache-dependency-path: src/go.sum - name: Login using GitHub token - run: go run ./cmd/cli login --debug + run: go run -race ./cmd/cli login --debug working-directory: src - name: Add dummy config id: add-dummy-config - run: echo blah | go run ./cmd/cli -C testdata/sanity config set --provider=defang -n dummy --debug + run: echo blah | go run -race ./cmd/cli -C testdata/sanity config set --provider=defang -n dummy --debug working-directory: src - name: Run sanity tests UP continue-on-error: true # until we have multi-project support in playground - run: go run ./cmd/cli -C testdata/sanity compose up --provider=defang --debug + run: go run -race ./cmd/cli -C testdata/sanity compose up --provider=defang --debug working-directory: src - name: Run sanity tests DOWN continue-on-error: true # until we have multi-project support in playground - run: go run ./cmd/cli -C testdata/sanity compose down --provider=defang --detach --debug + run: go run -race ./cmd/cli -C testdata/sanity compose down --provider=defang --detach --debug working-directory: src - name: Remove dummy config if: steps.add-dummy-config.conclusion == 'success' # only clean up if the config was added - run: go run ./cmd/cli -C testdata/sanity config rm --provider=defang -n dummy --debug + run: go run -race ./cmd/cli -C testdata/sanity config rm --provider=defang -n dummy --debug working-directory: src build-and-sign: diff --git a/src/Makefile b/src/Makefile index aa5e3cb02..12eb4bc3b 100644 --- a/src/Makefile +++ b/src/Makefile @@ -72,11 +72,11 @@ install: $(BINARY_NAME) .PHONY: test test: tidy $(GO) $(PROTOS) - set -o pipefail && go test -short ./... | $(FAIL-RED) + set -o pipefail && go test -race -short ./... | $(FAIL-RED) .PHONY: integ integ: $(GO) $(PROTOS) - set -o pipefail && go test -v -tags=integration ./... | $(FAIL-RED) + set -o pipefail && go test -race -v -tags=integration ./... | $(FAIL-RED) .PHONY: linux-amd64 linux-amd64: $(GO) test diff --git a/src/pkg/auth/auth_test.go b/src/pkg/auth/auth_test.go index 9855a208e..e5bab576f 100644 --- a/src/pkg/auth/auth_test.go +++ b/src/pkg/auth/auth_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" ) @@ -76,9 +77,9 @@ func TestPoll(t *testing.T) { }) t.Run("5xx retries until context cancelled", func(t *testing.T) { - calls := 0 + var calls atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - calls++ + calls.Add(1) http.Error(w, "internal error", http.StatusInternalServerError) })) t.Cleanup(server.Close) @@ -94,15 +95,15 @@ func TestPoll(t *testing.T) { if err == nil { t.Error("expected error after context cancellation") } - if calls < 2 { + if calls.Load() < 2 { t.Error("expected server to be called at least twice") } }) t.Run("408 retries until context cancelled", func(t *testing.T) { - calls := 0 + var calls atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - calls++ + calls.Add(1) w.WriteHeader(http.StatusRequestTimeout) })) t.Cleanup(server.Close) @@ -118,15 +119,15 @@ func TestPoll(t *testing.T) { if err == nil { t.Error("expected error after context cancellation") } - if calls < 2 { - t.Errorf("expected at least 2 calls for timeout retries, got %d", calls) + if calls.Load() < 2 { + t.Errorf("expected at least 2 calls for timeout retries, got %d", calls.Load()) } }) t.Run("4xx does not retry", func(t *testing.T) { - calls := 0 + var calls atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - calls++ + calls.Add(1) http.Error(w, "bad request", http.StatusBadRequest) })) t.Cleanup(server.Close) @@ -139,8 +140,8 @@ func TestPoll(t *testing.T) { if err == nil { t.Error("expected error for 4xx response") } - if calls != 1 { - t.Errorf("expected exactly 1 call (no retry for 4xx), got %d", calls) + if calls.Load() != 1 { + t.Errorf("expected exactly 1 call (no retry for 4xx), got %d", calls.Load()) } }) } @@ -226,9 +227,9 @@ func TestPollForAuthCode(t *testing.T) { func TestPoll_ContextDone(t *testing.T) { for _, name := range []string{"context.Canceled", "context.DeadlineExceeded"} { t.Run(name, func(t *testing.T) { - calls := 0 + var calls atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - calls++ + calls.Add(1) http.Error(w, "server error", http.StatusInternalServerError) })) t.Cleanup(server.Close) @@ -253,8 +254,8 @@ func TestPoll_ContextDone(t *testing.T) { if err == nil { t.Fatal("expected error for done context") } - if calls > 1 { - t.Errorf("Poll must not retry with done context, got %d server calls", calls) + if calls.Load() > 1 { + t.Errorf("Poll must not retry with done context, got %d server calls", calls.Load()) } }) } diff --git a/src/pkg/cli/client/byoc/aws/byoc.go b/src/pkg/cli/client/byoc/aws/byoc.go index d8a8ac5a1..babf9950b 100644 --- a/src/pkg/cli/client/byoc/aws/byoc.go +++ b/src/pkg/cli/client/byoc/aws/byoc.go @@ -835,7 +835,7 @@ func (b *ByocAws) queryOrTailLogs(ctx context.Context, cwClient cw.LogsClient, r } func (b *ByocAws) makeLogGroupARN(name string) string { - return b.driver.MakeARN("logs", "log-group:"+name) + return b.driver.MakeRegionalARN("logs", "log-group:"+name) } func (b *ByocAws) getLogGroupInputs(etag types.ETag, projectName, service, filter string, logType logs.LogType) []cw.LogGroupInput { diff --git a/src/pkg/cli/client/byoc/aws/list.go b/src/pkg/cli/client/byoc/aws/list.go index d521c813c..67462cca9 100644 --- a/src/pkg/cli/client/byoc/aws/list.go +++ b/src/pkg/cli/client/byoc/aws/list.go @@ -10,7 +10,6 @@ import ( "github.com/DefangLabs/defang/src/pkg/cli/client/byoc" "github.com/DefangLabs/defang/src/pkg/cli/client/byoc/state" "github.com/DefangLabs/defang/src/pkg/clouds/aws" - "github.com/DefangLabs/defang/src/pkg/clouds/aws/region" "github.com/DefangLabs/defang/src/pkg/term" "github.com/aws/aws-sdk-go-v2/service/s3" s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" @@ -135,7 +134,7 @@ func (b *ByocAws) listPulumiStacksAllRegions(ctx context.Context, s3client S3Cli // GetBucketLocation returns empty string for us-east-1 buckets bucketRegion := aws.Region(locationOutput.LocationConstraint) if bucketRegion == "" { - bucketRegion = region.USEast1 + bucketRegion = "us-east-1" } wg.Add(1) diff --git a/src/pkg/cli/client/byoc/baseclient.go b/src/pkg/cli/client/byoc/baseclient.go index 058f8cb2c..ca816b12d 100644 --- a/src/pkg/cli/client/byoc/baseclient.go +++ b/src/pkg/cli/client/byoc/baseclient.go @@ -73,10 +73,6 @@ func (b *ByocBaseClient) GetStackName() string { return b.PulumiStack } -func (b *ByocBaseClient) Debug(context.Context, *defangv1.DebugRequest) (*defangv1.DebugResponse, error) { - return nil, client.ErrNotImplemented("AI debugging is not yet supported for BYOC") -} - func (b *ByocBaseClient) SetCanIUseConfig(quotas *defangv1.CanIUseResponse) { b.CanIUseConfig.AllowGPU = quotas.Gpu b.CanIUseConfig.AllowScaling = quotas.AllowScaling diff --git a/src/pkg/cli/client/byoc/gcp/byoc.go b/src/pkg/cli/client/byoc/gcp/byoc.go index d952660dc..a894b279f 100644 --- a/src/pkg/cli/client/byoc/gcp/byoc.go +++ b/src/pkg/cli/client/byoc/gcp/byoc.go @@ -123,7 +123,6 @@ type ByocGcp struct { bucket string cdServiceAccount string - setupDone bool uploadServiceAccount string delegateDomainZone string @@ -170,7 +169,7 @@ func (*ByocGcp) Driver() string { } func (b *ByocGcp) SetUpCD(ctx context.Context, force bool) error { - if b.setupDone { + if b.SetupDone { return nil } // TODO: Handle project creation flow @@ -286,7 +285,7 @@ func (b *ByocGcp) SetUpCD(ctx context.Context, force bool) error { // 5. Setup Cloud Run Job term.Debugf("Using CD image: %q", b.CDImage) - b.setupDone = true + b.SetupDone = true return nil } diff --git a/src/pkg/cli/client/state.go b/src/pkg/cli/client/state.go index cc89e0d70..d87bf1cc1 100644 --- a/src/pkg/cli/client/state.go +++ b/src/pkg/cli/client/state.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "sync" "time" "github.com/google/uuid" @@ -15,6 +16,7 @@ var ( StateDir = filepath.Join(stateDir, "defang") statePath = filepath.Join(StateDir, "state.json") state State + stateOnce sync.Once ) type State struct { @@ -32,6 +34,12 @@ func initState(path string) State { return state } +func ensureState() { + stateOnce.Do(func() { + state = initState(statePath) + }) +} + func (state State) write(path string) error { if bytes, err := json.MarshalIndent(state, "", " "); err != nil { return err @@ -52,14 +60,16 @@ func (state State) termsAccepted() bool { } func GetAnonID() string { - state = initState(statePath) + ensureState() return state.AnonID } func AcceptTerms() error { + ensureState() return state.acceptTerms() } func TermsAccepted() bool { + ensureState() return state.termsAccepted() } diff --git a/src/pkg/cli/tail.go b/src/pkg/cli/tail.go index 43d26d69e..62f6afa48 100644 --- a/src/pkg/cli/tail.go +++ b/src/pkg/cli/tail.go @@ -100,10 +100,18 @@ var originalLocal = time.Local // SetUTCMode sets the local time zone to UTC. func SetUTCMode(enable bool) { + // The conditional guards avoid writing to time.Local when it already has + // the correct value; this prevents a data race with stdlib reads (e.g. + // time.Now) that we cannot synchronize because the read side is a plain + // load inside the Go runtime, eg. net/http.(*conn).setState() if enable { - time.Local = time.UTC + if time.Local != time.UTC { + time.Local = time.UTC + } } else { - time.Local = originalLocal + if time.Local != originalLocal { + time.Local = originalLocal + } } } diff --git a/src/pkg/clouds/aws/codebuild/cfn/setup.go b/src/pkg/clouds/aws/codebuild/cfn/setup.go index 1d42ff74b..6fce2b414 100644 --- a/src/pkg/clouds/aws/codebuild/cfn/setup.go +++ b/src/pkg/clouds/aws/codebuild/cfn/setup.go @@ -7,11 +7,11 @@ import ( "slices" "strconv" "strings" + "sync" "time" - common "github.com/DefangLabs/defang/src/pkg/clouds/aws" + "github.com/DefangLabs/defang/src/pkg/clouds/aws" awscodebuild "github.com/DefangLabs/defang/src/pkg/clouds/aws/codebuild" - "github.com/DefangLabs/defang/src/pkg/clouds/aws/region" "github.com/DefangLabs/defang/src/pkg/term" "github.com/aws/aws-sdk-go-v2/service/cloudformation" cfnTypes "github.com/aws/aws-sdk-go-v2/service/cloudformation/types" @@ -22,18 +22,20 @@ import ( type AwsCfn struct { awscodebuild.AwsCodeBuild stackName string + fillOnce sync.Once + fillErr error } const stackTimeout = time.Minute * 3 -func New(stack string, region region.Region) *AwsCfn { +func New(stack string, region aws.Region) *AwsCfn { if stack == "" { panic("stack must be set") } return &AwsCfn{ stackName: stack, AwsCodeBuild: awscodebuild.AwsCodeBuild{ - Aws: common.Aws{Region: region}, + Aws: aws.Aws{Region: region}, RetainBucket: true, }, } @@ -183,6 +185,13 @@ func (a *AwsCfn) upsertStackAndWait(ctx context.Context, templateBody []byte, fo type ErrStackNotFoundException = cfnTypes.StackNotFoundException func (a *AwsCfn) FillOutputs(ctx context.Context) error { + a.fillOnce.Do(func() { + a.fillErr = a.describeAndFillOutputs(ctx) + }) + return a.fillErr +} + +func (a *AwsCfn) describeAndFillOutputs(ctx context.Context) error { cfn, err := a.newClient(ctx) if err != nil { return err @@ -227,7 +236,7 @@ func (a *AwsCfn) fillWithOutputs(dso *cloudformation.DescribeStacksOutput) error } if a.AccountID == "" && a.LogGroupARN != "" { - a.AccountID = common.GetAccountID(a.LogGroupARN) + a.AccountID = aws.GetAccountID(a.LogGroupARN) } return nil } diff --git a/src/pkg/clouds/aws/codebuild/cfn/setup_test.go b/src/pkg/clouds/aws/codebuild/cfn/setup_test.go index c90022efe..fc2fbd2aa 100644 --- a/src/pkg/clouds/aws/codebuild/cfn/setup_test.go +++ b/src/pkg/clouds/aws/codebuild/cfn/setup_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/DefangLabs/defang/src/pkg" - "github.com/DefangLabs/defang/src/pkg/clouds/aws/region" ) func TestCloudFormation(t *testing.T) { @@ -15,7 +14,7 @@ func TestCloudFormation(t *testing.T) { } user := pkg.GetCurrentUser() // avoid conflict with other users in the same account - aws := New("crun-test-"+user, region.Region("us-west-2")) + aws := New("crun-test-"+user, "us-west-2") if aws == nil { t.Fatal("aws is nil") } diff --git a/src/pkg/clouds/aws/codebuild/common.go b/src/pkg/clouds/aws/codebuild/common.go index 62408cf14..8206d472a 100644 --- a/src/pkg/clouds/aws/codebuild/common.go +++ b/src/pkg/clouds/aws/codebuild/common.go @@ -1,8 +1,6 @@ package codebuild import ( - "strings" - "github.com/DefangLabs/defang/src/pkg/clouds/aws" ) @@ -20,14 +18,3 @@ type AwsCodeBuild struct { ProjectName string // CodeBuild project name RetainBucket bool // CloudFormation template input parameter } - -func (a *AwsCodeBuild) MakeARN(service, resource string) string { - return strings.Join([]string{ - "arn", - "aws", - service, - string(a.Region), - a.AccountID, - resource, - }, ":") -} diff --git a/src/pkg/clouds/aws/common.go b/src/pkg/clouds/aws/common.go index c13dadaf8..e8091406e 100644 --- a/src/pkg/clouds/aws/common.go +++ b/src/pkg/clouds/aws/common.go @@ -5,6 +5,7 @@ import ( "errors" "os/exec" "strings" + "sync" "github.com/DefangLabs/defang/src/pkg/tokenstore" "github.com/aws/aws-sdk-go-v2/aws" @@ -21,13 +22,20 @@ type Aws struct { Region Region TokenStore tokenstore.TokenStore Credentials aws.CredentialsProvider -} -// func (r Region) String() string { -// return string(r) -// } + cfgOnce sync.Once + cfg aws.Config + cfgErr error +} func (a *Aws) LoadConfig(ctx context.Context) (aws.Config, error) { + a.cfgOnce.Do(func() { + a.cfg, a.cfgErr = a.loadConfig(ctx) + }) + return a.cfg, a.cfgErr +} + +func (a *Aws) loadConfig(ctx context.Context) (aws.Config, error) { cfg, err := LoadDefaultConfig(ctx, config.WithRegion(string(a.Region))) if err != nil { return cfg, err @@ -47,6 +55,39 @@ func (a *Aws) LoadConfig(ctx context.Context) (aws.Config, error) { return cfg, err } +func (a *Aws) MakeRegionalARN(service, resourceId string) string { + if a.AccountID == "" { + panic("AWS AccountID must be set to make ARN") + } + return MakeARN( + "aws", // aws-cn, aws-us-gov + service, + string(a.Region), + a.AccountID, + resourceId, + ) +} + +func MakeARN(partition, service, region, accountId, resourceId string) string { + if partition == "" { + panic("partition must be set to make ARN") + } + if service == "" { + panic("service must be set to make ARN") + } + if resourceId == "" { + panic("resourceId must be set to make ARN") + } + return strings.Join([]string{ + "arn", + partition, + service, + region, // can be empty, eg. for IAM + accountId, // can be empty, eg. for S3 + resourceId, + }, ":") +} + func LoadDefaultConfig(ctx context.Context, optFns ...func(*config.LoadOptions) error) (aws.Config, error) { cfg, err := config.LoadDefaultConfig(ctx, optFns...) if err != nil { diff --git a/src/pkg/clouds/aws/login.go b/src/pkg/clouds/aws/login.go index d3651f5b6..a607fe9a0 100644 --- a/src/pkg/clouds/aws/login.go +++ b/src/pkg/clouds/aws/login.go @@ -35,7 +35,7 @@ import ( const ( clientIDSameDevice = "arn:aws:signin:::devtools/same-device" clientIDCrossDevice = "arn:aws:signin:::devtools/cross-device" - tokenStoreKeyPrefix = "aws-oauth-" // nolint:gosec,G101 // This is not a secret + tokenStoreKeyPrefix = "aws-oauth-" // nolint:gosec // This is not a secret ) // awsTokenCache is the on-disk representation of AWS OAuth credentials. diff --git a/src/pkg/clouds/aws/region/region.go b/src/pkg/clouds/aws/region/region.go deleted file mode 100644 index 9b3359cb0..000000000 --- a/src/pkg/clouds/aws/region/region.go +++ /dev/null @@ -1,51 +0,0 @@ -package region - -import ( - "strings" - - "github.com/DefangLabs/defang/src/pkg/clouds/aws" -) - -type Region = aws.Region - -const ( - AFSouth1 Region = "af-south-1" - APEast1 Region = "ap-east-1" - APNortheast1 Region = "ap-northeast-1" - APNortheast2 Region = "ap-northeast-2" - APNortheast3 Region = "ap-northeast-3" - APSouth1 Region = "ap-south-1" - APSouth2 Region = "ap-south-2" - APSoutheast1 Region = "ap-southeast-1" - APSoutheast2 Region = "ap-southeast-2" - APSoutheast3 Region = "ap-southeast-3" - APSoutheast4 Region = "ap-southeast-4" - CACentral Region = "ca-central-1" - CNNorth1 Region = "cn-north-1" - CNNorthwest1 Region = "cn-northwest-1" - EUCentral1 Region = "eu-central-1" - EUCentral2 Region = "eu-central-2" - EUNorth1 Region = "eu-north-1" - EUSouth1 Region = "eu-south-1" - EUSouth2 Region = "eu-south-2" - EUWest1 Region = "eu-west-1" - EUWest2 Region = "eu-west-2" - EUWest3 Region = "eu-west-3" - MECentral1 Region = "me-central-1" - MESouth1 Region = "me-south-1" - SAEast1 Region = "sa-east-1" - USGovEast1 Region = "us-gov-east-1" - USGovWest1 Region = "us-gov-west-1" - USEast1 Region = "us-east-1" - USEast2 Region = "us-east-2" - USWest1 Region = "us-west-1" - USWest2 Region = "us-west-2" -) - -func FromArn(arn string) Region { - parts := strings.Split(arn, ":") - if len(parts) < 6 || parts[0] != "arn" { - panic("invalid ARN") - } - return Region(parts[3]) -} diff --git a/src/pkg/clouds/gcp/login.go b/src/pkg/clouds/gcp/login.go index 2ceada2c1..98d4f142e 100644 --- a/src/pkg/clouds/gcp/login.go +++ b/src/pkg/clouds/gcp/login.go @@ -39,12 +39,12 @@ var ensureAPIsEnabled = func(ctx context.Context, g Gcp, apis ...string) error { } var ( - clientID = "513566466873-r6s52lv410ceuo37b2qu5122r0tu6brb.apps.googleusercontent.com" // nolint:gosec,G101 // Client ID for app is not a secret + clientID = "513566466873-r6s52lv410ceuo37b2qu5122r0tu6brb.apps.googleusercontent.com" // nolint:gosec // Client ID for app is not a secret // Client secret for app is not a secret, desktop APP client secrets is considered public information // See: https://developers.google.com/identity/protocols/oauth2/#installed // Numerous opensource projects have their google cloud client_secret committed in source code, including gcloud cli itself, and gomote: // https://github.com/golang/build/blob/master/internal/iapclient/iapclient.go#L38 - clientSecret = "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" // nolint:gosec,G101 + clientSecret = "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" // nolint:gosec scopes = []string{"email", "https://www.googleapis.com/auth/cloud-platform"} // TODO: Add all required permissions for running gcp byoc diff --git a/src/pkg/track/track.go b/src/pkg/track/track.go index ce0aad35e..088cc21fe 100644 --- a/src/pkg/track/track.go +++ b/src/pkg/track/track.go @@ -81,7 +81,7 @@ func Cmd(cmd *cobra.Command, verb string, props ...Property) { P("version", cmd.Root().Version), ) cmd.Flags().Visit(func(f *pflag.Flag) { - props = append(props, P(f.Name, f.Value)) + props = append(props, P(f.Name, f.Value.String())) }) } // This was supposed to be strings.Title but that got deprecated, so now we're stuck with strings.ToTitle which makes everything uppercase. Oh well.