From e63043ee9fd284ad7fcc1a36b82d35c38c28a0f3 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Thu, 12 Nov 2026 20:30:51 -0300 Subject: [PATCH] fix(oci): fail a publish at the first tag the registry rejects PushArtifact only warned when a tag's manifest PUT failed and went on with the remaining tags, so `features publish` and `templates publish` reported the subset that made it and exited 0, and pushed the collection metadata as well. The reference CLI's putManifestWithTags stops at the first rejected tag, logs "Failed to PUT manifest for tag ", and publish logs the "(!) ERR: Failed to publish" lines and exits 1 before the collection metadata and before printing a result. Go now does the same. Tags pushed before the failure stay pushed, as in TS. TestPublishParity gains a tag_failure case that runs both CLIs against a registry that rejects one tag and compares stdout, the exit code, the error lines and the PUTs each one made. Co-Authored-By: Claude Opus 5.5 --- .../collection_publish_tag_failure_test.go | 170 ++++++++++++++++++ internal/cli/publish_parity_test.go | 59 ++++++ internal/oci/push.go | 12 +- internal/oci/push_partial_test.go | 41 +++-- internal/oci/push_registry_test.go | 10 +- 5 files changed, 264 insertions(+), 28 deletions(-) create mode 100644 internal/cli/collection_publish_tag_failure_test.go diff --git a/internal/cli/collection_publish_tag_failure_test.go b/internal/cli/collection_publish_tag_failure_test.go new file mode 100644 index 0000000..ad8b3c2 --- /dev/null +++ b/internal/cli/collection_publish_tag_failure_test.go @@ -0,0 +1,170 @@ +package cli + +import ( + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "path/filepath" + "slices" + "strings" + "sync" + "testing" + + coreerrors "github.com/devcontainers/cli/internal/errors" + "github.com/devcontainers/cli/internal/log" + "github.com/devcontainers/cli/internal/oci" +) + +type pushRegistry struct { + host string + failTag string + + mu sync.Mutex + blobs map[string][]byte + manifests map[string][]byte + attempts map[string][]string +} + +func newPushRegistry(t *testing.T, failTag string) *pushRegistry { + t.Helper() + r := &pushRegistry{failTag: failTag, blobs: map[string][]byte{}, manifests: map[string][]byte{}, attempts: map[string][]string{}} + srv := httptest.NewServer(http.HandlerFunc(r.serve)) + t.Cleanup(srv.Close) + // A localhost registry is plain HTTP for both CLIs. + _, port, err := net.SplitHostPort(srv.Listener.Addr().String()) + if err != nil { + t.Fatal(err) + } + r.host = "localhost:" + port + return r +} + +func (r *pushRegistry) tagAttempts(repo string) []string { + r.mu.Lock() + defer r.mu.Unlock() + return slices.Clone(r.attempts[repo]) +} + +func (r *pushRegistry) serve(w http.ResponseWriter, req *http.Request) { + r.mu.Lock() + defer r.mu.Unlock() + path := strings.TrimPrefix(req.URL.Path, "/v2/") + notFound := func() { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"errors":[{"code":"NAME_UNKNOWN","message":"not found"}]}`)) + } + switch { + case path == "" || path == "/": + w.WriteHeader(http.StatusOK) + case strings.HasSuffix(path, "/tags/list"): + notFound() + case strings.Contains(path, "/blobs/uploads/") && req.Method == http.MethodPost: + w.Header().Set("Location", req.URL.Path+"session") + w.WriteHeader(http.StatusAccepted) + case strings.Contains(path, "/blobs/uploads/") && req.Method == http.MethodPut: + body, _ := io.ReadAll(req.Body) + digest := req.URL.Query().Get("digest") + r.blobs[digest] = body + w.Header().Set("Docker-Content-Digest", digest) + w.WriteHeader(http.StatusCreated) + case strings.Contains(path, "/blobs/"): + body, ok := r.blobs[path[strings.LastIndex(path, "/")+1:]] + if !ok { + notFound() + return + } + w.Header().Set("Content-Length", fmt.Sprint(len(body))) + if req.Method != http.MethodHead { + _, _ = w.Write(body) + } + case strings.Contains(path, "/manifests/"): + i := strings.Index(path, "/manifests/") + repo, ref := path[:i], path[i+len("/manifests/"):] + if req.Method != http.MethodPut { + body, ok := r.manifests[repo+":"+ref] + if !ok { + notFound() + return + } + w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") + w.Header().Set("Content-Length", fmt.Sprint(len(body))) + if req.Method != http.MethodHead { + _, _ = w.Write(body) + } + return + } + r.attempts[repo] = append(r.attempts[repo], ref) + if ref == r.failTag { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"errors":[{"code":"TAG_INVALID","message":"tag rejected by test"}]}`)) + return + } + body, _ := io.ReadAll(req.Body) + r.manifests[repo+":"+ref] = body + sum := sha256.Sum256(body) + w.Header().Set("Docker-Content-Digest", "sha256:"+hex.EncodeToString(sum[:])) + w.WriteHeader(http.StatusCreated) + default: + notFound() + } +} + +func writeTagFailureCollection(t *testing.T, collectionType string) string { + t.Helper() + src := filepath.Join(t.TempDir(), "src") + if collectionType == "feature" { + writeSeamFeature(t, src, "good", "1.0.0") + return src + } + writeFile(t, filepath.Join(src, "good", "devcontainer-template.json"), `{"id":"good","version":"1.0.0","name":"Good"}`, 0o644) + writeFile(t, filepath.Join(src, "good", ".devcontainer.json"), `{"image":"alpine"}`, 0o644) + return src +} + +func TestPublishCollection_FailedTagPushExits(t *testing.T) { + for _, collectionType := range []string{"feature", "template"} { + t.Run(collectionType, func(t *testing.T) { + reg := newPushRegistry(t, "1.0") + out := &captureOutput{} + logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: out.Stderr()}) + client := oci.NewClientWithAuthPolicy(logger, map[string]string{"DOCKER_CONFIG": t.TempDir()}, oci.DefaultAuthPolicy()) + + err := publishCollectionWith(t.Context(), out, client, writeTagFailureCollection(t, collectionType), reg.host, "me/coll", collectionType, "info") + if exit, ok := errors.AsType[*coreerrors.ExitCodeError](err); !ok || exit.Code != 1 { + t.Fatalf("err = %v, want exit code 1 after the rejected tag", err) + } + if out.out.Len() != 0 { + t.Errorf("stdout = %q, want no result after a failed tag push", out.out.String()) + } + + resource := reg.host + "/me/coll/good" + stderr := out.err.String() + for _, want := range []string{ + "Failed to PUT manifest for tag 1.0\n", + fmt.Sprintf("(!) ERR: Failed to publish %s: '%s'", collectionType, resource), + fmt.Sprintf("(!) ERR: Failed to publish '%s'", resource), + } { + if !strings.Contains(stderr, want) { + t.Errorf("stderr is missing %q:\n%s", want, stderr) + } + } + if strings.Contains(stderr, "Published "+collectionType) { + t.Errorf("stderr reports a successful publish:\n%s", stderr) + } + + if got, want := reg.tagAttempts("me/coll/good"), []string{"1", "1.0"}; !slices.Equal(got, want) { + t.Errorf("tag PUTs = %v, want %v: no tag is tried after the first failure", got, want) + } + if got := reg.tagAttempts("me/coll"); len(got) != 0 { + t.Errorf("collection metadata PUTs = %v, want none after a failed publish", got) + } + }) + } +} diff --git a/internal/cli/publish_parity_test.go b/internal/cli/publish_parity_test.go index 35c3e8a..44c92d2 100644 --- a/internal/cli/publish_parity_test.go +++ b/internal/cli/publish_parity_test.go @@ -1,11 +1,15 @@ package cli import ( + "bytes" + "errors" "os" "os/exec" "path/filepath" "reflect" + "slices" "sort" + "strings" "testing" "time" @@ -25,6 +29,8 @@ func TestPublishParity(t *testing.T) { if _, err := os.Stat(goCLI); err != nil { t.Skip("Go CLI not built") } + t.Run("tag failure", func(t *testing.T) { publishTagFailureParity(t, repoRoot, goCLI) }) + if !isDockerAvailable() { t.Skip("docker required") } @@ -115,3 +121,56 @@ func TestPublishParity(t *testing.T) { }) } } + +func publishTagFailureParity(t *testing.T, repoRoot, goCLI string) { + for _, collectionType := range []string{"feature", "template"} { + t.Run(collectionType, func(t *testing.T) { + reg := newPushRegistry(t, "1.0") + src := writeTagFailureCollection(t, collectionType) + type side struct { + stdout string + errLines []string + exit int + attempts []string + metadata []string + } + publish := func(cli []string, namespace string) side { + args := append(slices.Clone(cli[1:]), collectionType+"s", "publish", src, "-r", reg.host, "-n", namespace) + cmd := exec.CommandContext(t.Context(), cli[0], args...) + cmd.Dir = repoRoot + cmd.Env = append(os.Environ(), "DOCKER_CONFIG="+t.TempDir()) + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + err := cmd.Run() + var exitErr *exec.ExitError + if err != nil && !errors.As(err, &exitErr) { + t.Fatalf("run %v: %v", cli, err) + } + var errLines []string + for line := range strings.Lines(strings.ReplaceAll(stderr.String(), namespace, "")) { + for _, marker := range []string{"(!) ERR:", "Failed to PUT manifest for tag"} { + if i := strings.Index(line, marker); i >= 0 { + errLines = append(errLines, strings.TrimSpace(line[i:])) + } + } + } + return side{ + stdout: stdout.String(), + errLines: errLines, + exit: cmd.ProcessState.ExitCode(), + attempts: reg.tagAttempts(namespace + "/good"), + metadata: reg.tagAttempts(namespace), + } + } + + ts := publish([]string{"node", filepath.Join(repoRoot, "reference", "devcontainer.js")}, "tsns/coll") + goSide := publish([]string{goCLI}, "gons/coll") + if !reflect.DeepEqual(ts, goSide) { + t.Errorf("publish with a rejected tag differs:\n--- TS\n%+v\n--- Go\n%+v", ts, goSide) + } + if ts.exit != 1 || !slices.Equal(ts.attempts, []string{"1", "1.0"}) { + t.Errorf("TS side = %+v: the fake registry did not exercise the failed tag", ts) + } + }) + } +} diff --git a/internal/oci/push.go b/internal/oci/push.go index f6f293b..694be8b 100644 --- a/internal/oci/push.go +++ b/internal/oci/push.go @@ -79,23 +79,17 @@ func (c *Client) PushArtifact(ctx context.Context, ref *Ref, tgzPath string, tag return nil, err } - var publishedTags []string for _, tag := range tags { if err := repo.PushReference(ctx, manifestDesc, bytes.NewReader(manifestBytes), tag); err != nil { - c.log.Write(fmt.Sprintf("Failed to push tag %s: %v", tag, err), log.LevelWarning) - continue + c.log.Write(fmt.Sprintf("Failed to PUT manifest for tag %s\n%v", tag, err), log.LevelError) + return nil, fmt.Errorf("push tag %s of %s: %w", tag, ref.Resource, err) } - publishedTags = append(publishedTags, tag) c.log.Write(fmt.Sprintf("Published tag: %s", tag), log.LevelInfo) } - if len(publishedTags) == 0 { - return nil, fmt.Errorf("failed to publish any tags") - } - return &PushResult{ Digest: manifestDesc.Digest.String(), - PublishedTags: publishedTags, + PublishedTags: tags, }, nil } diff --git a/internal/oci/push_partial_test.go b/internal/oci/push_partial_test.go index f30983d..d485913 100644 --- a/internal/oci/push_partial_test.go +++ b/internal/oci/push_partial_test.go @@ -1,8 +1,12 @@ package oci import ( + "bytes" + "maps" "os" "path/filepath" + "slices" + "strings" "testing" "github.com/devcontainers/cli/internal/log" @@ -17,38 +21,45 @@ func writeTarball(t *testing.T, content string) string { return p } -func TestPushArtifact_BearerAuthAndPartialTagFailure(t *testing.T) { +func TestPushArtifact_BearerAuthAndStopsAtFirstFailedTag(t *testing.T) { const user, pass = "u", "p" reg := newFakeOCIRegistry() reg.requireBearer = true reg.wantUser, reg.wantPass = user, pass - reg.failTags = map[string]bool{"latest": true} + reg.failTags = map[string]bool{"1.0": true} registry := reg.start(t) ref, err := ParseRef(registry + "/ns/hello:1.0.0") if err != nil { t.Fatal(err) } - client := NewClient(log.Null, dockerConfigEnv(t, registry, user, pass)) + var logs bytes.Buffer + client := NewClient(log.New(log.Options{Writer: &logs, Level: log.LevelInfo}), dockerConfigEnv(t, registry, user, pass)) - res, err := client.PushArtifact(t.Context(), ref, writeTarball(t, "layer"), []string{"1.0.0", "latest"}, "feature", nil) - if err != nil { - t.Fatalf("push returned error, want partial success: %v", err) + res, err := client.PushArtifact(t.Context(), ref, writeTarball(t, "layer"), []string{"1", "1.0", "1.0.0", "latest"}, "feature", nil) + if err == nil { + t.Fatalf("push returned %+v, want an error for the rejected tag 1.0 (reference putManifestWithTags returns undefined)", res) } - - if got := res.PublishedTags; len(got) != 1 || got[0] != "1.0.0" { - t.Fatalf("PublishedTags = %v, want [1.0.0] (partial, no rollback)", got) + if res != nil { + t.Errorf("result = %+v, want nil alongside the error", res) + } + if !strings.Contains(err.Error(), "1.0") || !strings.Contains(err.Error(), ref.Resource) { + t.Errorf("error %q does not name the tag and resource", err) } + if !strings.Contains(logs.String(), "Failed to PUT manifest for tag 1.0\n") { + t.Errorf("log does not report the rejected tag:\n%s", logs.String()) + } + reg.mu.Lock() - _, has100 := reg.manifests["1.0.0"] - _, hasLatest := reg.manifests["latest"] + attempted := slices.Clone(reg.putManifestAttempt) + tagged := slices.Sorted(maps.Keys(reg.manifests)) tokenReqs := reg.tokenRequests reg.mu.Unlock() - if !has100 { - t.Error("1.0.0 tag was not published") + if want := []string{"1", "1.0"}; !slices.Equal(attempted, want) { + t.Errorf("tag PUTs attempted = %v, want %v: tags after the first failure must not be tried", attempted, want) } - if hasLatest { - t.Error("latest tag was published despite the injected failure") + if want := []string{"1"}; !slices.Equal(tagged, want) { + t.Errorf("tags in registry = %v, want %v (tags before the failure stay pushed)", tagged, want) } if tokenReqs == 0 { t.Error("token endpoint never called; the Bearer auth loop did not run") diff --git a/internal/oci/push_registry_test.go b/internal/oci/push_registry_test.go index a6239c3..5257887 100644 --- a/internal/oci/push_registry_test.go +++ b/internal/oci/push_registry_test.go @@ -27,10 +27,11 @@ type fakeOCIRegistry struct { failTags map[string]bool failAllManifests bool - tokenRequests int - putManifestOK []string - blobUploads int - blobHeadHits int + tokenRequests int + putManifestAttempt []string + putManifestOK []string + blobUploads int + blobHeadHits int } func newFakeOCIRegistry() *fakeOCIRegistry { @@ -159,6 +160,7 @@ func (r *fakeOCIRegistry) handleManifest(w http.ResponseWriter, req *http.Reques switch req.Method { case http.MethodPut: + r.putManifestAttempt = append(r.putManifestAttempt, ref) if r.failAllManifests || r.failTags[ref] { // 400 is a non-retryable client error, so oras returns immediately // (a 5xx would trigger retry backoff and slow the test), while still