diff --git a/src/pkg/cli/compose/context.go b/src/pkg/cli/compose/context.go index 2533f136d..8bf8c6ff4 100644 --- a/src/pkg/cli/compose/context.go +++ b/src/pkg/cli/compose/context.go @@ -22,6 +22,7 @@ import ( defangv1 "github.com/DefangLabs/defang/src/protos/io/defang/v1" "github.com/compose-spec/compose-go/v2/types" "github.com/docker/go-units" + "github.com/google/uuid" "github.com/moby/patternmatcher" "github.com/moby/patternmatcher/ignorefile" ) @@ -236,7 +237,9 @@ func getRemoteBuildContext(ctx context.Context, provider client.Provider, projec digest = calcDigest(buffer.Bytes()) return fmt.Sprintf("s3://cd-preview/%s%s", digest, archiveType.Extension), nil case UploadModeForce: - // Force: empty digest = always upload the tarball (to a random URL), triggering a new build + // Force: leave the digest empty so uploadArchive picks a fresh, never-used + // name, which makes the build context URL differ from the previous deploy's + // and stops the build being skipped as unchanged. default: panic("unexpected UploadMode value") } @@ -251,8 +254,19 @@ func calcDigest(data []byte) string { } func uploadArchive(ctx context.Context, provider client.Provider, projectName string, body io.Reader, archiveType ArchiveType, digest string) (string, error) { + // An empty digest is UploadModeForce asking for a URL that has never been used, + // so the build is never skipped as unchanged. The provider drivers do randomize, + // but only when the WHOLE blob name is empty — appending the extension first + // made the name ".tar.gz", which is not empty, so every forced upload landed on + // that one fixed blob. The context URL was then identical between deploys and + // the build was skipped, silently shipping a stale image. Generate the unique + // name here instead, keeping the extension so the archive type stays visible. + name := digest + if name == "" { + name = uuid.NewString() + } // Upload the archive to the fabric controller storage; TODO: use a streaming API - ureq := &defangv1.UploadURLRequest{Digest: digest + archiveType.Extension, Project: projectName} + ureq := &defangv1.UploadURLRequest{Digest: name + archiveType.Extension, Project: projectName} res, err := provider.CreateUploadURL(ctx, ureq) if err != nil { return "", err diff --git a/src/pkg/cli/compose/context_test.go b/src/pkg/cli/compose/context_test.go index 7b62f85f2..cd4c8659b 100644 --- a/src/pkg/cli/compose/context_test.go +++ b/src/pkg/cli/compose/context_test.go @@ -12,6 +12,7 @@ import ( "path" "path/filepath" "reflect" + "regexp" "strings" "testing" @@ -82,49 +83,34 @@ func TestUploadArchive(t *testing.T) { } }) - t.Run("upload with zip", func(t *testing.T) { - url, err := uploadArchive(t.Context(), client.MockProvider{UploadUrl: uploadUrl}, testproj, &bytes.Buffer{}, ArchiveTypeZip, "") - if err != nil { - t.Fatalf("uploadContent() failed: %v", err) - } - var expectedPath = path + testproj + "/" + ArchiveTypeZip.Extension - if url != server.URL+expectedPath { - t.Errorf("Expected %v, got %v", server.URL+expectedPath, url) - } - }) - - t.Run("upload with tar", func(t *testing.T) { - url, err := uploadArchive(t.Context(), client.MockProvider{UploadUrl: uploadUrl}, testproj, &bytes.Buffer{}, ArchiveTypeGzip, "") - if err != nil { - t.Fatalf("uploadContent() failed: %v", err) - } - var expectedPath = path + testproj + "/" + ArchiveTypeGzip.Extension - if url != server.URL+expectedPath { - t.Errorf("Expected %v, got %v", server.URL+expectedPath, url) - } - }) - - t.Run("force upload tar without digest", func(t *testing.T) { - url, err := uploadArchive(t.Context(), client.MockProvider{UploadUrl: uploadUrl}, testproj, &bytes.Buffer{}, ArchiveTypeGzip, "") - if err != nil { - t.Fatalf("uploadArchive() failed: %v", err) - } - var expectedPath = path + testproj + "/" + ArchiveTypeGzip.Extension - if url != server.URL+expectedPath { - t.Errorf("Expected %v, got %v", server.URL+expectedPath, url) - } - }) - - t.Run("force upload zip without digest", func(t *testing.T) { - url, err := uploadArchive(t.Context(), client.MockProvider{UploadUrl: uploadUrl}, testproj, &bytes.Buffer{}, ArchiveTypeZip, "") - if err != nil { - t.Fatalf("uploadArchive() failed: %v", err) - } - var expectedPath = path + testproj + "/" + ArchiveTypeZip.Extension - if url != server.URL+expectedPath { - t.Errorf("Expected %v, got %v", server.URL+expectedPath, url) - } - }) + // An empty digest is the "force" path: the caller wants a URL that has never + // been used, so a redeploy of identical source still rebuilds. These used to + // expect the bare extension (".tar.gz"), i.e. one shared blob for every forced + // upload — which is exactly what made forced deploys reuse a stale image. + for _, at := range []ArchiveType{ArchiveTypeGzip, ArchiveTypeZip} { + t.Run("force upload without digest"+at.Extension, func(t *testing.T) { + prefix := server.URL + path + testproj + "/" + first, err := uploadArchive(t.Context(), client.MockProvider{UploadUrl: uploadUrl}, testproj, &bytes.Buffer{}, at, "") + if err != nil { + t.Fatalf("uploadArchive() failed: %v", err) + } + second, err := uploadArchive(t.Context(), client.MockProvider{UploadUrl: uploadUrl}, testproj, &bytes.Buffer{}, at, "") + if err != nil { + t.Fatalf("uploadArchive() failed: %v", err) + } + if first == second { + t.Errorf("forced uploads reused %v; a repeated context URL makes the build a no-op", first) + } + for _, url := range []string{first, second} { + if url == prefix+at.Extension { + t.Errorf("forced upload used the shared fixed blob %v", url) + } + if !strings.HasPrefix(url, prefix) || !strings.HasSuffix(url, at.Extension) { + t.Errorf("Expected %v%v, got %v", prefix, at.Extension, url) + } + } + }) + } } func TestWalkContextFolder(t *testing.T) { @@ -183,10 +169,11 @@ func TestWalkContextFolder(t *testing.T) { func Test_getRemoteBuildContext(t *testing.T) { tests := []struct { - name string - uploadMode UploadMode - expectUrl string - expectFile string + name string + uploadMode UploadMode + expectUrl string + expectUrlRe string + expectFile string }{ { name: "Default UploadMode", @@ -195,10 +182,12 @@ func Test_getRemoteBuildContext(t *testing.T) { expectFile: "sha256-B+3Dq6U37SrlbnrfS4uIk3CDwrPJ+Q15TqUCPBEMQuA=.tar.gz", }, { - name: "Force UploadMode", - uploadMode: UploadModeForce, - expectUrl: "https://mock-bucket.s3.amazonaws.com/project1/.tar.gz", // server decides name - expectFile: ".tar.gz", + // Force must never reuse a URL: a repeated name makes the build context + // identical to the previous deploy's, so the build is skipped and a stale + // image ships. The name is a fresh UUID, so match a pattern, not a literal. + name: "Force UploadMode", + uploadMode: UploadModeForce, + expectUrlRe: `^https://mock-bucket\.s3\.amazonaws\.com/project1/[0-9a-f-]{36}\.tar\.gz$`, }, { name: "Digest UploadMode", @@ -262,7 +251,12 @@ func Test_getRemoteBuildContext(t *testing.T) { if err != nil { t.Fatalf("getRemoteBuildContext() failed: %v", err) } - if got := normalizer.Replace(url); got != tt.expectUrl { + got := normalizer.Replace(url) + if tt.expectUrlRe != "" { + if !regexp.MustCompile(tt.expectUrlRe).MatchString(got) { + t.Errorf("Expected URL matching %v, got: %v", tt.expectUrlRe, got) + } + } else if got != tt.expectUrl { t.Errorf("Expected %v, got: %v", tt.expectUrl, got) } if tt.expectFile != "" { @@ -436,3 +430,45 @@ func TestGetDockerIgnorePatterns(t *testing.T) { }) } } + +// TestForceUploadURLIsUniquePerCall pins the property that actually matters for +// UploadModeForce: two forced deploys of identical source must not reuse a URL. +// They previously both landed on ".tar.gz", so the build context was unchanged +// between deploys, the build was skipped as a no-op, and the old image kept +// running while the deploy reported success. +func TestForceUploadURLIsUniquePerCall(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + io.Copy(io.Discard, r.Body) //nolint:errcheck // discarding the upload body + r.Body.Close() + w.WriteHeader(200) + })) + t.Cleanup(server.Close) + + context := "../../../testdata/testproj" + if err := standardizeDirMode(context); err != nil { + t.Fatalf("Failed to standardize directory modes: %v", err) + } + provider := client.MockProvider{UploadUrl: server.URL} + + get := func() string { + url, err := getRemoteBuildContext(t.Context(), provider, "project1", "service1", + &types.BuildConfig{Context: context}, UploadModeForce) + if err != nil { + t.Fatalf("getRemoteBuildContext() failed: %v", err) + } + return url + } + + first, second := get(), get() + if first == second { + t.Errorf("forced uploads reused the same URL %q; the build would be skipped as unchanged", first) + } + for _, u := range []string{first, second} { + if strings.HasSuffix(u, "/.tar.gz") { + t.Errorf("forced upload fell back to the shared fixed blob: %q", u) + } + if !strings.HasSuffix(u, ".tar.gz") { + t.Errorf("forced upload lost its archive extension: %q", u) + } + } +}