fix(mutate): preserve config blob and layers for non-Docker OCI artifacts by blackwell-systems · Pull Request #2286 · google/go-containerregistry

blackwell-systems · 2026-05-01T19:43:03Z

Summary

compute() in image.go unconditionally parses the config as a Docker image config and derives layers from RootFS.DiffIDs. For non-Docker OCI artifacts (Helm charts, WASM modules, arbitrary blobs), the config has no RootFS.DiffIDs, causing:

Layers() returns empty. remote.Write uses Layers() to determine which blobs to upload. On a cross-repository push, layer blobs are never uploaded, and the registry rejects the manifest with MANIFEST_BLOB_UNKNOWN.
The config blob digest is corrupted. RawConfigFile() re-marshals the config through the Docker ConfigFile struct, producing a different digest than the original. The manifest references a blob that was never uploaded.

Repro: crane copy registry-1.docker.io/bitnamicharts/nginx:13.2.14 <dest> (Helm chart with application/vnd.cncf.helm.config.v1+json config).

Fix

Detect non-image configs by checking the config media type against the two known image config types (DockerConfigJSON, OCIConfigJSON). When the config is neither:

computeArtifact(): new method that preserves the original config blob (no re-marshal, no digest corruption) and builds the layer list from manifest descriptors and added layers.
Layers(): falls back to base image layers + added layers instead of deriving from empty DiffIDs.
RawConfigFile(): returns the original raw bytes from the base image instead of re-marshaling.

Standard Docker/OCI image workflows are unchanged (all existing tests pass).

Testing

Added TestNonImageArtifactPreservesConfig which creates an artifact with application/vnd.cncf.helm.config.v1+json config type, mutates annotations, and verifies:

Layers() does not error
Config media type is preserved in the manifest
Config blob digest matches the raw config content (no corruption)
Annotations are present

Full ./pkg/... test suite passes across all packages.

Fixes #2251

…acts compute() unconditionally parses the config as a Docker image config and derives layers from RootFS.DiffIDs. For non-Docker OCI artifacts (Helm charts, WASM modules), the config has no DiffIDs, causing Layers() to return empty (remote.Write skips blob uploads) and RawConfigFile() to corrupt the config digest via re-marshaling. Detect non-image configs by checking the config media type. When it is not DockerConfigJSON or OCIConfigJSON, delegate to computeArtifact which preserves the original config blob and derives layers from the base image and manifest descriptors. Fixes google#2251 Signed-off-by: Dayna Blackwell <dayna@blackwell-systems.com>

codecov-commenter · 2026-05-04T18:07:23Z

Codecov Report

❌ Patch coverage is 82.22222% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.89%. Comparing base (e5983f2) to head (bf03656).

Files with missing lines	Patch %	Lines
pkg/v1/mutate/image.go	82.22%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2286      +/-   ##
==========================================
+ Coverage   56.83%   56.89%   +0.05%     
==========================================
  Files         165      165              
  Lines       11319    11334      +15     
==========================================
+ Hits         6433     6448      +15     
  Misses       4121     4121              
  Partials      765      765

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Subserial · 2026-05-04T18:08:18Z

Confirmed with the image in the original bug that this prevents corrupting the image on upload and download. I can approve this change after comments and linter errors are addressed.

…r review

Subserial

Sorry for noting this just now, but this code is very poor quality and duplicates several code paths that already exist. I've left comments, but from now on please review your generated changes to reduce this introduction of code debt.

Subserial · 2026-05-05T22:09:19Z

+	// For non-image OCI artifacts, RootFS.DiffIDs is empty so partial.DiffIDs
+	// returns nothing. Fall back to the base image's layers plus any added layers.
+	if i.manifest != nil && !isImageConfig(i.manifest.Config.MediaType) {
+		layers, err := i.base.Layers()
+		if err != nil {
+			return nil, err
+		}
+		for _, add := range i.adds {
+			if add.Layer != nil {
+				layers = append(layers, add.Layer)
+			}
+		}
+		return layers, nil
+	}
+


Same comment here. This logic already exists and is duplicated. Please reduce the footprint of this change.

To fulfill this, you can remove this block and change the above if statement to:

if err := i.compute(); errors.Is(err, stream.ErrNotComputed) || (i.manifest != nil && !isImageConfig(i.manifest.Config.MediaType) {

Per review feedback: - Inline artifact-specific behavior as conditional blocks within compute() instead of a separate computeArtifact() function - Share manifest layer loop, annotations, mediaType, and subject assignment between both paths - Trim isImageConfig comment - Layers() artifact fallback stays inline (no new abstractions) Net -70 lines.

…facts

blackwell-systems · 2026-05-07T02:06:59Z

Thanks for the detailed feedback. I've refactored to inline the artifact logic into compute() as suggested. The duplicated paths are eliminated and net change is -70 lines. Branch is up to date with main.

Subserial

Some of the comments seem to refer to images vs artifacts, which should be updated to images vs non-image artifacts.

Also, please do not remove arbitrary whitespace and comments from untouched code.

Subserial · 2026-05-13T20:54:44Z

+	// For non-image OCI artifacts, RootFS.DiffIDs is empty so partial.DiffIDs
+	// returns nothing. Fall back to the base image's layers plus any added layers.
+	if i.manifest != nil && !isImageConfig(i.manifest.Config.MediaType) {
+		layers, err := i.base.Layers()
+		if err != nil {
+			return nil, err
+		}
+		for _, add := range i.adds {
+			if add.Layer != nil {
+				layers = append(layers, add.Layer)
+			}
+		}
+		return layers, nil
+	}
+


To fulfill this, you can remove this block and change the above if statement to:

if err := i.compute(); errors.Is(err, stream.ErrNotComputed) || (i.manifest != nil && !isImageConfig(i.manifest.Config.MediaType) {

Address review feedback: - Remove comment at line 124 - Merge the !isImageConfig block into the ErrNotComputed condition

Subserial

I was being a bit picky on the wording. LGTM, thanks for responding to the feedback on this PR.

EronWright · 2026-05-15T22:32:45Z

Thanks @Subserial and @blackwell-systems for the fix!

…acts (google#2286) * fix(mutate): preserve config blob and layers for non-Docker OCI artifacts compute() unconditionally parses the config as a Docker image config and derives layers from RootFS.DiffIDs. For non-Docker OCI artifacts (Helm charts, WASM modules), the config has no DiffIDs, causing Layers() to return empty (remote.Write skips blob uploads) and RawConfigFile() to corrupt the config digest via re-marshaling. Detect non-image configs by checking the config media type. When it is not DockerConfigJSON or OCIConfigJSON, delegate to computeArtifact which preserves the original config blob and derives layers from the base image and manifest descriptors. Fixes google#2251 Signed-off-by: Dayna Blackwell <dayna@blackwell-systems.com> * test: replace no-op _ = layers with assertion per review * fix: rename unused originalManifest parameter to _ (revive lint) * fix: remove unused originalManifest parameter from computeArtifact per review * refactor: inline artifact logic into compute(), remove computeArtifact Per review feedback: - Inline artifact-specific behavior as conditional blocks within compute() instead of a separate computeArtifact() function - Share manifest layer loop, annotations, mediaType, and subject assignment between both paths - Trim isImageConfig comment - Layers() artifact fallback stays inline (no new abstractions) Net -70 lines. * Combine Layers() artifact check into compute error branch Address review feedback: - Remove comment at line 124 - Merge the !isImageConfig block into the ErrNotComputed condition --------- Signed-off-by: Dayna Blackwell <dayna@blackwell-systems.com>

Subserial reviewed May 4, 2026

View reviewed changes

Comment thread pkg/v1/mutate/mutate_test.go Outdated

blackwell-systems added 2 commits May 4, 2026 12:07

test: replace no-op _ = layers with assertion per review

71dcfbd

fix: rename unused originalManifest parameter to _ (revive lint)

5a95b49

Subserial reviewed May 5, 2026

View reviewed changes

Comment thread pkg/v1/mutate/image.go Outdated

fix: remove unused originalManifest parameter from computeArtifact pe…

617c502

…r review

Subserial requested changes May 5, 2026

View reviewed changes

blackwell-systems added 2 commits May 6, 2026 07:12

Merge remote-tracking branch 'upstream/main' into fix-mutate-oci-arti…

4fabb1d

…facts

blackwell-systems requested a review from Subserial May 7, 2026 02:07

Merge branch 'main' into fix-mutate-oci-artifacts

207545e

blackwell-systems added a commit to blackwell-systems/blog that referenced this pull request May 13, 2026

docs: google/go-containerregistry#2286 merged (25 total)

9cc22dc

Subserial requested changes May 13, 2026

View reviewed changes

blackwell-systems and others added 2 commits May 14, 2026 09:37

Combine Layers() artifact check into compute error branch

c863a7c

Address review feedback: - Remove comment at line 124 - Merge the !isImageConfig block into the ErrNotComputed condition

Merge branch 'main' into fix-mutate-oci-artifacts

bf03656

Subserial approved these changes May 15, 2026

View reviewed changes

Subserial merged commit 35b354b into google:main May 15, 2026
17 checks passed

EronWright mentioned this pull request May 15, 2026

Remove go-containerregistry annotation workaround in oci-push promotion step akuity/kargo#6299

Open

jabrown85 mentioned this pull request Jun 8, 2026

ggcr: #2339

Open

Conversation

blackwell-systems commented May 1, 2026

Summary

Fix

Testing

Uh oh!

Uh oh!

codecov-commenter commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Subserial commented May 4, 2026

Uh oh!

Uh oh!

Subserial left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Subserial May 5, 2026

Choose a reason for hiding this comment

Uh oh!

Subserial May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blackwell-systems commented May 7, 2026

Uh oh!

Subserial left a comment

Choose a reason for hiding this comment

Uh oh!

Subserial May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Subserial left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EronWright commented May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented May 4, 2026 •

edited

Loading

Subserial left a comment •

edited

Loading