Skip to content

common/libimage/manifests.list.Add(): add from lists without images #382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

common/libimage/manifests.list.Add(): add from lists without images #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/libimage/manifest_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ func (m *ManifestList) Add(ctx context.Context, name string, options *ManifestLi
}
}

ref, err := m.parseNameToExtantReference(ctx, systemContext, name, false, "image to add to manifest list")
ref, err := m.parseNameToExtantReference(ctx, systemContext, name, options.All, "image to add to manifest list")
if err != nil {
return "", err
}
Expand Down
26 changes: 26 additions & 0 deletions common/libimage/manifest_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,32 @@ func TestCreateAndRemoveManifestList(t *testing.T) {
require.Equal(t, []string{"localhost/manifestlist:latest"}, rmReports[0].Untagged)
}

// TestAddListToList ensures that we can add one list to another, adding all of
// the entries in the source list to the destination list.
func TestAddManifestListToManifestList(t *testing.T) {
const imageListAmd64MemberName = "registry.k8s.io/pause@sha256:59eec8837a4d942cc19a52b8c09ea75121acc38114a2c68b98983ce9356b8610"
const imageListArm64MemberName = "registry.k8s.io/pause@sha256:f365626a556e58189fc21d099fc64603db0f440bff07f77c740989515c544a39"

runtime := testNewRuntime(t)
ctx := context.Background()

firstList, err := runtime.CreateManifestList("firstlist")
require.NoError(t, err)
require.NotNil(t, firstList)
_, err = firstList.Add(ctx, imageListAmd64MemberName, nil)
require.NoError(t, err)
_, err = firstList.Add(ctx, imageListArm64MemberName, nil)
require.NoError(t, err)
require.Equalf(t, 2, len(firstList.list.Instances()), "expected to have added two items to list")

secondList, err := runtime.CreateManifestList("mythirdlist")
require.NoError(t, err)
require.NotNil(t, secondList)
_, err = secondList.Add(ctx, firstList.ID(), &ManifestListAddOptions{All: true})
require.NoError(t, err)
require.Equalf(t, len(firstList.list.Instances()), len(secondList.list.Instances()), "expected to have all of the items in the list")
}

// TestAddSomeArtifacts ensures that we don't fail to add artifact manifests to
// a manifest list, even (or especially) when their config blobs aren't valid
// OCI or Docker config blobs.
Expand Down
33 changes: 21 additions & 12 deletions common/libimage/manifests/manifests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,7 @@ func (l *list) Add(ctx context.Context, sys *types.SystemContext, ref types.Imag
Features, OSFeatures, Annotations []string
Size int64
ConfigInfo types.BlobInfo
MediaType string
ArtifactType string
URLs []string
}
Expand Down Expand Up @@ -550,6 +551,7 @@ func (l *list) Add(ctx context.Context, sys *types.SystemContext, ref types.Imag
Features: append([]string{}, lists.Docker().Manifests[i].Platform.Features...),
OSFeatures: append([]string{}, platform.OSFeatures...),
Size: instance.Size,
MediaType: instance.MediaType,
ArtifactType: instance.ArtifactType,
Annotations: mapToSlice(instance.Annotations),
URLs: instance.URLs,
Expand Down Expand Up @@ -583,10 +585,12 @@ func (l *list) Add(ctx context.Context, sys *types.SystemContext, ref types.Imag
Features: append([]string{}, lists.Docker().Manifests[i].Platform.Features...),
OSFeatures: append([]string{}, platform.OSFeatures...),
Size: instance.Size,
MediaType: instance.MediaType,
ArtifactType: instance.ArtifactType,
Annotations: mapToSlice(instance.Annotations),
URLs: instance.URLs,
}
manifestDigest = instanceDigest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but: below, is the !added case reachable? The digest chosen by list.ChooseInstance should always be found, shouldn’t it?

I’m primarily worried about the !added path below not adding MediaType, making this harder to follow. Shouldn’t !added be an “this is supposed to be never reached” error path?

Copy link
Contributor

@mtrmac mtrmac Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes behavior - manifestDigest used to be the from first instance, now it is the last one. Is that desirable?

… well, I’m not sure it is useful either way.

But, Podman’s ImageEngine.ManifestAdd does use the returned digest, to update annotations. Does that make sense to do?? At a guess, it might make more sense to explicitly return "", and to forbid the annotation options in Podman.

instanceInfos = append(instanceInfos, instanceInfo)
added = true
}
Expand All @@ -606,22 +610,28 @@ func (l *list) Add(ctx context.Context, sys *types.SystemContext, ref types.Imag
instanceInfo.ArtifactType = m.ArtifactType
}
}
instanceInfo.MediaType = primaryManifestType
instanceInfo.Size = int64(len(primaryManifestBytes))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Consider setting these in the instanceInfo{ literal above.)

instanceInfos = append(instanceInfos, instanceInfo)
}

knownConfigTypes := []string{manifest.DockerV2Schema2ConfigMediaType, v1.MediaTypeImageConfig}
for _, instanceInfo := range instanceInfos {
var hasPlatformConfig bool
unparsedInstance := image.UnparsedInstance(src, instanceInfo.instanceDigest)
manifestBytes, manifestType, err := unparsedInstance.Manifest(ctx)
if err != nil {
return "", fmt.Errorf("reading manifest from %q, instance %q: %w", transports.ImageName(ref), instanceInfo.instanceDigest, err)
}
instanceManifest, err := manifest.FromBlob(manifestBytes, manifestType)
if err != nil {
return "", fmt.Errorf("parsing manifest from %q, instance %q: %w", transports.ImageName(ref), instanceInfo.instanceDigest, err)
if mb, mt, err := unparsedInstance.Manifest(ctx); err == nil {
if instanceManifest, err := manifest.FromBlob(mb, mt); err == nil {
instanceDigest, err := manifest.Digest(mb)
if err != nil {
return "", fmt.Errorf("computing digest of manifest from %q: %w", transports.ImageName(ref), err)
}
instanceInfo.instanceDigest = &instanceDigest
instanceInfo.MediaType = mt
instanceInfo.Size = int64(len(mb))
Comment on lines +628 to +630
Copy link
Contributor

@mtrmac mtrmac Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem unnecessary or undesirable — if we have a list (and a digest reference to that list, guaranteeing integrity), the values in the list are “more authoritative” than what the registry returns. And, from the other code paths, it seems that all of these values are known via other means (yes, in case of a tagged reference to a single-platform manifest, we do trust the registry, but only in that case).

instanceInfo.ConfigInfo = instanceManifest.ConfigInfo()
hasPlatformConfig = instanceInfo.ArtifactType == "" && slices.Contains(knownConfigTypes, instanceInfo.ConfigInfo.MediaType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not clear to me why silently setting hasPlatformConfig to false on an I/O error is OK.

}
}
instanceInfo.ConfigInfo = instanceManifest.ConfigInfo()
hasPlatformConfig := instanceInfo.ArtifactType == "" && slices.Contains(knownConfigTypes, instanceInfo.ConfigInfo.MediaType)
needToParsePlatformConfig := (instanceInfo.OS == "" || instanceInfo.Architecture == "")
if hasPlatformConfig && needToParsePlatformConfig {
img, err := image.FromUnparsedImage(ctx, sys, unparsedInstance)
Expand All @@ -643,16 +653,15 @@ func (l *list) Add(ctx context.Context, sys *types.SystemContext, ref types.Imag
}
}
if instanceInfo.instanceDigest == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be simpler to do all of this earlier, in the !manifest.MIMETypeIsMultiImage conditional — the code would need to separately track instanceDigest (might be nil) vs. manifestDigest (always known) but that might also be a simplification.

manifestDigest, err = manifest.Digest(manifestBytes)
manifestDigest, err = manifest.Digest(primaryManifestBytes)
if err != nil {
return "", fmt.Errorf("computing digest of manifest from %q: %w", transports.ImageName(ref), err)
}
instanceInfo.instanceDigest = &manifestDigest
instanceInfo.Size = int64(len(manifestBytes))
} else if manifestDigest == "" {
manifestDigest = *instanceInfo.instanceDigest
}
err = l.List.AddInstance(*instanceInfo.instanceDigest, instanceInfo.Size, manifestType, instanceInfo.OS, instanceInfo.Architecture, instanceInfo.OSVersion, instanceInfo.OSFeatures, instanceInfo.Variant, instanceInfo.Features, instanceInfo.Annotations)
err = l.List.AddInstance(*instanceInfo.instanceDigest, instanceInfo.Size, instanceInfo.MediaType, instanceInfo.OS, instanceInfo.Architecture, instanceInfo.OSVersion, instanceInfo.OSFeatures, instanceInfo.Variant, instanceInfo.Features, instanceInfo.Annotations)
if err != nil {
return "", fmt.Errorf("adding instance with digest %q: %w", *instanceInfo.instanceDigest, err)
}
Expand Down
Loading