common/libimage/manifests.list.Add(): add from lists without images #382
Conversation
When adding the contents of a list to another list, keep track of the MediaType that described the list elements, so that we can handle cases where the entry being referred to is unreadable. When adding a non-list manifest to a list, default to reusing the MediaType of the non-list for its entry in the list to which it's being added. If we can't read a manifest or configuration blob from an item that we're adding to a list, don't fail if we already have the information that we would try to read from the configuration blob. Signed-off-by: Nalin Dahyabhai <[email protected]>
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6420 |
|
LGTM but not my area of expertise |
There was a problem hiding this comment.
The core of the changes, preserving per-instance MediaType, makes sense to me.
I don’t see why silently ignoring errors on the read of unparsedInstance.Manifest, and silently using different values than on success, is correct. (I also don’t think it helps — eventually a … manifest push will need to read the data, won’t it?)
| } | ||
| } | ||
| instanceInfo.MediaType = primaryManifestType | ||
| instanceInfo.Size = int64(len(primaryManifestBytes)) |
There was a problem hiding this comment.
(Consider setting these in the instanceInfo{ literal above.)
| Annotations: mapToSlice(instance.Annotations), | ||
| URLs: instance.URLs, | ||
| } | ||
| manifestDigest = instanceDigest |
There was a problem hiding this comment.
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?
| instanceInfo.MediaType = mt | ||
| instanceInfo.Size = int64(len(mb)) | ||
| instanceInfo.ConfigInfo = instanceManifest.ConfigInfo() | ||
| hasPlatformConfig = instanceInfo.ArtifactType == "" && slices.Contains(knownConfigTypes, instanceInfo.ConfigInfo.MediaType) |
There was a problem hiding this comment.
It’s not clear to me why silently setting hasPlatformConfig to false on an I/O error is OK.
| Annotations: mapToSlice(instance.Annotations), | ||
| URLs: instance.URLs, | ||
| } | ||
| manifestDigest = instanceDigest |
There was a problem hiding this comment.
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.
| instanceInfo.Variant = config.Variant | ||
| } | ||
| } | ||
| if instanceInfo.instanceDigest == nil { |
There was a problem hiding this comment.
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.
| instanceInfo.instanceDigest = &instanceDigest | ||
| instanceInfo.MediaType = mt | ||
| instanceInfo.Size = int64(len(mb)) |
There was a problem hiding this comment.
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).
4 participants
When adding the contents of a list to another list, keep track of the MediaType that described the list elements, so that we can handle cases where the entry being referred to is unreadable.
When adding a non-list manifest to a list, default to reusing the MediaType of the non-list for its entry in the list to which it's being added.
If we can't read a manifest or configuration blob from an item that we're adding to a list, don't fail if we already have the information that we would try to read from the configuration blob.
Fixes containers/podman#27228.