fix behavior of rewrite when registry is docker.io (#719)

Author: CamrynCarter

cmd/hauler/cli/store/add.go

@@ -619,7 +619,13 @@ func rewriteReference(ctx context.Context, s *store.Layout, oldRef name.Referenc
 	// index.docker.io. Preserve the original registry when the source is non-docker.
 	if newRegistry == "index.docker.io" && !strings.HasPrefix(rawRewrite, "docker.io") && !strings.HasPrefix(rawRewrite, "index.docker.io") {
 		newRegistry = oldRegistry
-		newRepo = strings.TrimPrefix(newRepo, "library/") //if rewrite has library/ prefix in path it is stripped off unless registry specified in rewrite
+		rewriteRepo := strings.TrimPrefix(rawRewrite, "/")
+		if i := strings.LastIndex(rewriteRepo, ":"); i != -1 {
+			rewriteRepo = rewriteRepo[:i]
+		}
+		if !strings.HasPrefix(rewriteRepo, "library/") {
+			newRepo = strings.TrimPrefix(newRepo, "library/")
+		}
 	}
 	oldTotal := oldRepo + ":" + oldTag
 	newTotal := newRepo + ":" + newTag
@@ -1452,6 +1458,7 @@ func fetchChart(ctx context.Context, s *store.Layout, j chartJob, tempRoot strin
 // rewrite. A rewrite that omits a tag inherits ref's.
 func rewriteChartReference(ctx context.Context, s *store.Layout, ref name.Reference, rewrite string) error {
 	rewrite = strings.TrimPrefix(rewrite, "/")
+	rawRewrite := rewrite
 	newRef, err := name.ParseReference(rewrite)
 	if err != nil {
 		// error... don't continue with a bad reference
@@ -1474,6 +1481,13 @@ func rewriteChartReference(ctx context.Context, s *store.Layout, ref name.Refere
 	// rename chart name in store
 	oldRepo := ref.Context().RepositoryStr()
 	newRepo := newRef.Context().RepositoryStr()
+	rewriteRepo := rawRewrite
+	if i := strings.LastIndex(rewriteRepo, ":"); i != -1 {
+		rewriteRepo = rewriteRepo[:i]
+	}
+	if !strings.HasPrefix(rewriteRepo, "library/") {
+		newRepo = strings.TrimPrefix(newRepo, "library/")
+	}
 	newTag := newRef.Identifier()
 	if tag, ok := newRef.(name.Tag); ok {
 		newTag = tag.TagStr()

cmd/hauler/cli/store/add_test.go

@@ -375,6 +375,108 @@ func TestRewriteReference(t *testing.T) {
 		// condition fires → registry reverts to host, no library/ to strip
 		assertAnnotationsInStore(t, s, "newrepo/img:v2", host+"/newrepo/img:v2")
 	})
+
+	// The library/-detection must look at rawRewrite's path (not the
+	// go-containerregistry-normalized newRepo, which always carries "library/" for
+	// single-segment repos), so that a rewrite which explicitly asks for
+	// "library/..." is honored instead of being unconditionally stripped.
+
+	t.Run("path-only rewrite with explicit library/ prefix is preserved", func(t *testing.T) {
+		s := newTestStore(t)
+		seedStoreDescriptor(t, s, map[string]string{
+			ocispec.AnnotationRefName:     "library/nginx:latest",
+			consts.ContainerdImageNameKey: "index.docker.io/library/nginx:latest",
+		})
+
+		oldRef, _ := name.NewTag("nginx:latest")
+		newRef, _ := name.NewTag("library/nginx:v2")
+		rawRewrite := "library/nginx:v2"
+
+		if err := rewriteReference(ctx, s, oldRef, newRef, rawRewrite); err != nil {
+			t.Fatalf("rewriteReference: %v", err)
+		}
+		// rewriteRepo (derived from rawRewrite) starts with "library/" → must be kept
+		assertAnnotationsInStore(t, s, "library/nginx:v2", "index.docker.io/library/nginx:v2")
+	})
+
+	t.Run("leading slash rewrite with explicit library/ prefix is preserved", func(t *testing.T) {
+		s := newTestStore(t)
+		seedStoreDescriptor(t, s, map[string]string{
+			ocispec.AnnotationRefName:     "library/nginx:latest",
+			consts.ContainerdImageNameKey: "index.docker.io/library/nginx:latest",
+		})
+
+		oldRef, _ := name.NewTag("nginx:latest")
+		newRef, _ := name.NewTag("library/nginx:v2")
+		// AddImageCmd passes the pre-trim rewrite string through as rawRewrite, so a
+		// leading "/" must still be handled correctly here.
+		rawRewrite := "/library/nginx:v2"
+
+		if err := rewriteReference(ctx, s, oldRef, newRef, rawRewrite); err != nil {
+			t.Fatalf("rewriteReference: %v", err)
+		}
+		assertAnnotationsInStore(t, s, "library/nginx:v2", "index.docker.io/library/nginx:v2")
+	})
+}
+
+func TestRewriteChartReference(t *testing.T) {
+	ctx := newTestContext(t)
+
+	// A chart rewritten to a bare single-segment name must not keep an erroneous
+	// "library/" prefix picked up from go-containerregistry's docker hub
+	// normalization, unless the rewrite explicitly asked for one.
+
+	t.Run("path-only rewrite strips library/ prefix from docker hub normalization", func(t *testing.T) {
+		s := newTestStore(t)
+		seedStoreDescriptor(t, s, map[string]string{
+			ocispec.AnnotationRefName: "library/mychart:1.0.0",
+		})
+
+		ref, _ := name.NewTag("mychart:1.0.0")
+		if err := rewriteChartReference(ctx, s, ref, "mychart:2.0.0"); err != nil {
+			t.Fatalf("rewriteChartReference: %v", err)
+		}
+		assertArtifactInStore(t, s, "mychart:2.0.0")
+	})
+
+	t.Run("explicit library/ prefix in rewrite is preserved", func(t *testing.T) {
+		s := newTestStore(t)
+		seedStoreDescriptor(t, s, map[string]string{
+			ocispec.AnnotationRefName: "library/mychart:1.0.0",
+		})
+
+		ref, _ := name.NewTag("mychart:1.0.0")
+		if err := rewriteChartReference(ctx, s, ref, "library/mychart:2.0.0"); err != nil {
+			t.Fatalf("rewriteChartReference: %v", err)
+		}
+		assertArtifactInStore(t, s, "library/mychart:2.0.0")
+	})
+
+	t.Run("leading slash rewrite with explicit library/ prefix is preserved", func(t *testing.T) {
+		s := newTestStore(t)
+		seedStoreDescriptor(t, s, map[string]string{
+			ocispec.AnnotationRefName: "library/mychart:1.0.0",
+		})
+
+		ref, _ := name.NewTag("mychart:1.0.0")
+		if err := rewriteChartReference(ctx, s, ref, "/library/mychart:2.0.0"); err != nil {
+			t.Fatalf("rewriteChartReference: %v", err)
+		}
+		assertArtifactInStore(t, s, "library/mychart:2.0.0")
+	})
+
+	t.Run("rewrite omitting tag inherits the source tag", func(t *testing.T) {
+		s := newTestStore(t)
+		seedStoreDescriptor(t, s, map[string]string{
+			ocispec.AnnotationRefName: "library/mychart:1.0.0",
+		})
+
+		ref, _ := name.NewTag("mychart:1.0.0")
+		if err := rewriteChartReference(ctx, s, ref, "myneworg/mychart"); err != nil {
+			t.Fatalf("rewriteChartReference: %v", err)
+		}
+		assertArtifactInStore(t, s, "myneworg/mychart:1.0.0")
+	})
 }
 
 // --------------------------------------------------------------------------

cmd/hauler/cli/store/info.go

@@ -562,6 +562,26 @@ func newItemWithDigest(s *store.Layout, digestStr string, desc ocispec.Descripto
 	return item
 }
 
+// resolveDisplayReference returns the fully-qualified reference string to display
+// for desc. ContainerdImageNameKey already holds the canonical "registry/repo:tag"
+// string exactly as computed when the artifact was stored (see rewriteReference in
+// cmd/hauler/cli/store/add.go), so it's used verbatim. Re-parsing it through
+// name.ParseReference and calling .Name() would re-trigger go-containerregistry's
+// Docker Hub "library/" namespace normalization for any single-segment repo under
+// index.docker.io, undoing a rewrite like "hello-world-custom" back to
+// "library/hello-world-custom". AnnotationRefName, used as a fallback, has no
+// registry component, so it still needs reference.Parse to fill one in.
+func resolveDisplayReference(desc ocispec.Descriptor) (string, error) {
+	if refName := desc.Annotations[consts.ContainerdImageNameKey]; refName != "" {
+		return refName, nil
+	}
+	ref, err := reference.Parse(desc.Annotations[ocispec.AnnotationRefName])
+	if err != nil {
+		return "", err
+	}
+	return ref.Name(), nil
+}
+
 func newItem(s *store.Layout, desc ocispec.Descriptor, m ocispec.Manifest, plat string, o *flags.InfoOpts) item {
 	var size int64 = 0
 	for _, l := range m.Layers {
@@ -570,11 +590,7 @@ func newItem(s *store.Layout, desc ocispec.Descriptor, m ocispec.Manifest, plat
 
 	ctype := resolveCtype(desc, m.Config.MediaType)
 
-	refName := desc.Annotations[consts.ContainerdImageNameKey]
-	if refName == "" {
-		refName = desc.Annotations[ocispec.AnnotationRefName]
-	}
-	ref, err := reference.Parse(refName)
+	refName, err := resolveDisplayReference(desc)
 	if err != nil {
 		return item{}
 	}
@@ -584,7 +600,7 @@ func newItem(s *store.Layout, desc ocispec.Descriptor, m ocispec.Manifest, plat
 	}
 
 	return item{
-		Reference: ref.Name(),
+		Reference: refName,
 		Type:      ctype,
 		Platform:  plat,
 		Digest:    desc.Digest.String(),
@@ -637,17 +653,13 @@ func fallbackItem(desc ocispec.Descriptor, plat string, problem store.BlobResult
 		plat = "-"
 	}
 
-	refName := desc.Annotations[consts.ContainerdImageNameKey]
-	if refName == "" {
-		refName = desc.Annotations[ocispec.AnnotationRefName]
-	}
-	ref, err := reference.Parse(refName)
+	refName, err := resolveDisplayReference(desc)
 	if err != nil {
 		return item{}
 	}
 
 	return item{
-		Reference:    ref.Name(),
+		Reference:    refName,
 		Type:         resolveCtype(desc, ""),
 		Platform:     plat,
 		Layers:       0,

cmd/hauler/cli/store/info_test.go

@@ -215,6 +215,82 @@ func TestNewItem(t *testing.T) {
 	}
 }
 
+func TestResolveDisplayReference(t *testing.T) {
+	// ContainerdImageNameKey already holds the fully-qualified reference exactly as
+	// computed by rewriteReference (see add.go), so it must be returned verbatim.
+	// Re-parsing it through the reference package would re-trigger
+	// go-containerregistry's docker hub "library/" normalization for a
+	// single-segment repo, undoing a rewrite like "hello-world-custom" back to
+	// "library/hello-world-custom".
+	t.Run("ContainerdImageNameKey is used verbatim, without re-injecting library/", func(t *testing.T) {
+		desc := ocispec.Descriptor{
+			Annotations: map[string]string{
+				consts.ContainerdImageNameKey: "index.docker.io/hello-world-custom:v2",
+				ocispec.AnnotationRefName:     "hello-world-custom:v2",
+			},
+		}
+		got, err := resolveDisplayReference(desc)
+		if err != nil {
+			t.Fatalf("resolveDisplayReference: %v", err)
+		}
+		if want := "index.docker.io/hello-world-custom:v2"; got != want {
+			t.Errorf("got %q, want %q", got, want)
+		}
+	})
+
+	t.Run("falls back to AnnotationRefName parsed when ContainerdImageNameKey absent", func(t *testing.T) {
+		desc := ocispec.Descriptor{
+			Annotations: map[string]string{
+				ocispec.AnnotationRefName: "hello-world-custom:v2",
+			},
+		}
+		got, err := resolveDisplayReference(desc)
+		if err != nil {
+			t.Fatalf("resolveDisplayReference: %v", err)
+		}
+		if want := "hauler/hello-world-custom:v2"; got != want {
+			t.Errorf("got %q, want %q", got, want)
+		}
+	})
+
+	t.Run("returns error when fallback ref cannot be parsed", func(t *testing.T) {
+		desc := ocispec.Descriptor{Annotations: map[string]string{}}
+		if _, err := resolveDisplayReference(desc); err == nil {
+			t.Fatal("expected error, got nil")
+		}
+	})
+}
+
+func TestNewItem_ReferenceUsesContainerdImageNameVerbatim(t *testing.T) {
+	desc := ocispec.Descriptor{
+		Annotations: map[string]string{
+			consts.ContainerdImageNameKey: "index.docker.io/hello-world-custom:v2",
+			ocispec.AnnotationRefName:     "hello-world-custom:v2",
+		},
+	}
+	m := ocispec.Manifest{Config: ocispec.Descriptor{MediaType: consts.DockerConfigJSON}}
+	o := &flags.InfoOpts{TypeFilter: "all"}
+
+	got := newItem(nil, desc, m, "linux/amd64", o)
+	if want := "index.docker.io/hello-world-custom:v2"; got.Reference != want {
+		t.Errorf("got Reference %q, want %q", got.Reference, want)
+	}
+}
+
+func TestFallbackItem_ReferenceUsesContainerdImageNameVerbatim(t *testing.T) {
+	desc := ocispec.Descriptor{
+		Annotations: map[string]string{
+			consts.ContainerdImageNameKey: "index.docker.io/hello-world-custom:v2",
+			ocispec.AnnotationRefName:     "hello-world-custom:v2",
+		},
+	}
+
+	got := fallbackItem(desc, "linux/amd64", store.BlobResult{})
+	if want := "index.docker.io/hello-world-custom:v2"; got.Reference != want {
+		t.Errorf("got Reference %q, want %q", got.Reference, want)
+	}
+}
+
 func TestInfoCmd(t *testing.T) {
 	ctx := newTestContext(t)
 	s := newTestStore(t)