Merge pull request #2552 from mtrmac/assorted

Assorted clean ups, optimizations, and a small bug fix

copy/sign.go

@@ -106,7 +106,7 @@ func (c *copier) createSignatures(ctx context.Context, manifest []byte, identity
  if len(c.signers) == 1 {
  return nil, fmt.Errorf("creating signature: %w", err)
  } else {
- return nil, fmt.Errorf("creating signature %d: %w", signerIndex, err)
+ return nil, fmt.Errorf("creating signature %d: %w", signerIndex+1, err)
  }
  }
  res = append(res, newSig)

copy/single.go

@@ -323,10 +323,7 @@ func checkImageDestinationForCurrentRuntime(ctx context.Context, sys *types.Syst
  if err != nil {
  return fmt.Errorf("parsing image configuration: %w", err)
  }
- wantedPlatforms, err := platform.WantedPlatforms(sys)
- if err != nil {
- return fmt.Errorf("getting current platform information %#v: %w", sys, err)
- }
+ wantedPlatforms := platform.WantedPlatforms(sys)
 
  options := newOrderedSet()
  match := false

docker/docker_image_src.go

@@ -116,10 +116,10 @@ func newImageSource(ctx context.Context, sys *types.SystemContext, ref dockerRef
  // Don’t just build a string, try to preserve the typed error.
  primary := &attempts[len(attempts)-1]
  extras := []string{}
- for i := 0; i < len(attempts)-1; i++ {
+ for _, attempt := range attempts[:len(attempts)-1] {
  // This is difficult to fit into a single-line string, when the error can contain arbitrary strings including any metacharacters we decide to use.
  // The paired [] at least have some chance of being unambiguous.
- extras = append(extras, fmt.Sprintf("[%s: %v]", attempts[i].ref.String(), attempts[i].err))
+ extras = append(extras, fmt.Sprintf("[%s: %v]", attempt.ref.String(), attempt.err))
  }
  return nil, fmt.Errorf("(Mirrors also failed: %s): %s: %w", strings.Join(extras, "\n"), primary.ref.String(), primary.err)
  }
@@ -464,26 +464,20 @@ func (s *dockerImageSource) GetSignaturesWithFormat(ctx context.Context, instanc
  var res []signature.Signature
  switch {
  case s.c.supportsSignatures:
- sigs, err := s.getSignaturesFromAPIExtension(ctx, instanceDigest)
- if err != nil {
+ if err := s.appendSignaturesFromAPIExtension(ctx, &res, instanceDigest); err != nil {
  return nil, err
  }
- res = append(res, sigs...)
  case s.c.signatureBase != nil:
- sigs, err := s.getSignaturesFromLookaside(ctx, instanceDigest)
- if err != nil {
+ if err := s.appendSignaturesFromLookaside(ctx, &res, instanceDigest); err != nil {
  return nil, err
  }
- res = append(res, sigs...)
  default:
  return nil, errors.New("Internal error: X-Registry-Supports-Signatures extension not supported, and lookaside should not be empty configuration")
  }
 
- sigstoreSigs, err := s.getSignaturesFromSigstoreAttachments(ctx, instanceDigest)
- if err != nil {
+ if err := s.appendSignaturesFromSigstoreAttachments(ctx, &res, instanceDigest); err != nil {
  return nil, err
  }
- res = append(res, sigstoreSigs...)
  return res, nil
 }
 
@@ -505,35 +499,35 @@ func (s *dockerImageSource) manifestDigest(ctx context.Context, instanceDigest *
  return manifest.Digest(s.cachedManifest)
 }
 
-// getSignaturesFromLookaside implements GetSignaturesWithFormat() from the lookaside location configured in s.c.signatureBase,
-// which is not nil.
-func (s *dockerImageSource) getSignaturesFromLookaside(ctx context.Context, instanceDigest *digest.Digest) ([]signature.Signature, error) {
+// appendSignaturesFromLookaside implements GetSignaturesWithFormat() from the lookaside location configured in s.c.signatureBase,
+// which is not nil, storing the signatures to *dest.
+// On error, the contents of *dest are undefined.
+func (s *dockerImageSource) appendSignaturesFromLookaside(ctx context.Context, dest *[]signature.Signature, instanceDigest *digest.Digest) error {
  manifestDigest, err := s.manifestDigest(ctx, instanceDigest)
  if err != nil {
- return nil, err
+ return err
  }
 
  // NOTE: Keep this in sync with docs/signature-protocols.md!
- signatures := []signature.Signature{}
  for i := 0; ; i++ {
  if i >= maxLookasideSignatures {
- return nil, fmt.Errorf("server provided %d signatures, assuming that's unreasonable and a server error", maxLookasideSignatures)
+ return fmt.Errorf("server provided %d signatures, assuming that's unreasonable and a server error", maxLookasideSignatures)
  }
 
  sigURL, err := lookasideStorageURL(s.c.signatureBase, manifestDigest, i)
  if err != nil {
- return nil, err
+ return err
  }
  signature, missing, err := s.getOneSignature(ctx, sigURL)
  if err != nil {
- return nil, err
+ return err
  }
  if missing {
  break
  }
- signatures = append(signatures, signature)
+ *dest = append(*dest, signature)
  }
- return signatures, nil
+ return nil
 }
 
 // getOneSignature downloads one signature from sigURL, and returns (signature, false, nil)
@@ -596,48 +590,51 @@ func (s *dockerImageSource) getOneSignature(ctx context.Context, sigURL *url.URL
  }
 }
 
-// getSignaturesFromAPIExtension implements GetSignaturesWithFormat() using the X-Registry-Supports-Signatures API extension.
-func (s *dockerImageSource) getSignaturesFromAPIExtension(ctx context.Context, instanceDigest *digest.Digest) ([]signature.Signature, error) {
+// appendSignaturesFromAPIExtension implements GetSignaturesWithFormat() using the X-Registry-Supports-Signatures API extension,
+// storing the signatures to *dest.
+// On error, the contents of *dest are undefined.
+func (s *dockerImageSource) appendSignaturesFromAPIExtension(ctx context.Context, dest *[]signature.Signature, instanceDigest *digest.Digest) error {
  manifestDigest, err := s.manifestDigest(ctx, instanceDigest)
  if err != nil {
- return nil, err
+ return err
  }
 
  parsedBody, err := s.c.getExtensionsSignatures(ctx, s.physicalRef, manifestDigest)
  if err != nil {
- return nil, err
+ return err
  }
 
- var sigs []signature.Signature
  for _, sig := range parsedBody.Signatures {
  if sig.Version == extensionSignatureSchemaVersion && sig.Type == extensionSignatureTypeAtomic {
- sigs = append(sigs, signature.SimpleSigningFromBlob(sig.Content))
+ *dest = append(*dest, signature.SimpleSigningFromBlob(sig.Content))
  }
  }
- return sigs, nil
+ return nil
 }
 
-func (s *dockerImageSource) getSignaturesFromSigstoreAttachments(ctx context.Context, instanceDigest *digest.Digest) ([]signature.Signature, error) {
+// appendSignaturesFromSigstoreAttachments implements GetSignaturesWithFormat() using the sigstore tag convention,
+// storing the signatures to *dest.
+// On error, the contents of *dest are undefined.
+func (s *dockerImageSource) appendSignaturesFromSigstoreAttachments(ctx context.Context, dest *[]signature.Signature, instanceDigest *digest.Digest) error {
  if !s.c.useSigstoreAttachments {
  logrus.Debugf("Not looking for sigstore attachments: disabled by configuration")
- return nil, nil
+ return nil
  }
 
  manifestDigest, err := s.manifestDigest(ctx, instanceDigest)
  if err != nil {
- return nil, err
+ return err
  }
 
  ociManifest, err := s.c.getSigstoreAttachmentManifest(ctx, s.physicalRef, manifestDigest)
  if err != nil {
- return nil, err
+ return err
  }
  if ociManifest == nil {
- return nil, nil
+ return nil
  }
 
  logrus.Debugf("Found a sigstore attachment manifest with %d layers", len(ociManifest.Layers))
- res := []signature.Signature{}
  for layerIndex, layer := range ociManifest.Layers {
  // Note that this copies all kinds of attachments: attestations, and whatever else is there,
  // not just signatures. We leave the signature consumers to decide based on the MIME type.
@@ -648,11 +645,11 @@ func (s *dockerImageSource) getSignaturesFromSigstoreAttachments(ctx context.Con
  payload, err := s.c.getOCIDescriptorContents(ctx, s.physicalRef, layer, iolimits.MaxSignatureBodySize,
  none.NoCache)
  if err != nil {
- return nil, err
+ return err
  }
- res = append(res, signature.SigstoreFromComponents(layer.MediaType, payload, layer.Annotations))
+ *dest = append(*dest, signature.SigstoreFromComponents(layer.MediaType, payload, layer.Annotations))
  }
- return res, nil
+ return nil
 }
 
 // deleteImage deletes the named image from the registry, if supported.

internal/manifest/docker_schema2_list.go

@@ -152,10 +152,7 @@ func (list *Schema2ListPublic) ChooseInstanceByCompression(ctx *types.SystemCont
 // ChooseInstance parses blob as a schema2 manifest list, and returns the digest
 // of the image which is appropriate for the current environment.
 func (list *Schema2ListPublic) ChooseInstance(ctx *types.SystemContext) (digest.Digest, error) {
- wantedPlatforms, err := platform.WantedPlatforms(ctx)
- if err != nil {
- return "", fmt.Errorf("getting platform information %#v: %w", ctx, err)
- }
+ wantedPlatforms := platform.WantedPlatforms(ctx)
  for _, wantedPlatform := range wantedPlatforms {
  for _, d := range list.Manifests {
  imagePlatform := ociPlatformFromSchema2PlatformSpec(d.Platform)

internal/manifest/oci_index.go

@@ -236,10 +236,7 @@ func (index *OCI1IndexPublic) chooseInstance(ctx *types.SystemContext, preferGzi
  if preferGzip == types.OptionalBoolTrue {
  didPreferGzip = true
  }
- wantedPlatforms, err := platform.WantedPlatforms(ctx)
- if err != nil {
- return "", fmt.Errorf("getting platform information %#v: %w", ctx, err)
- }
+ wantedPlatforms := platform.WantedPlatforms(ctx)
  var bestMatch *instanceCandidate
  bestMatch = nil
  for manifestIndex, d := range index.Manifests {

internal/pkg/platform/platform_matcher.go

@@ -153,7 +153,7 @@ var compatibility = map[string][]string{
 // WantedPlatforms returns all compatible platforms with the platform specifics possibly overridden by user,
 // the most compatible platform is first.
 // If some option (arch, os, variant) is not present, a value from current platform is detected.
-func WantedPlatforms(ctx *types.SystemContext) ([]imgspecv1.Platform, error) {
+func WantedPlatforms(ctx *types.SystemContext) []imgspecv1.Platform {
  // Note that this does not use Platform.OSFeatures and Platform.OSVersion at all.
  // The fields are not specified by the OCI specification, as of version 1.1, usefully enough
  // to be interoperable, anyway.
@@ -211,7 +211,7 @@ func WantedPlatforms(ctx *types.SystemContext) ([]imgspecv1.Platform, error) {
  Variant: v,
  })
  }
- return res, nil
+ return res
 }
 
 // MatchesPlatform returns true if a platform descriptor from a multi-arch image matches

internal/pkg/platform/platform_matcher_test.go

@@ -54,8 +54,7 @@ func TestWantedPlatforms(t *testing.T) {
  },
  } {
  testName := fmt.Sprintf("%q/%q/%q", c.ctx.ArchitectureChoice, c.ctx.OSChoice, c.ctx.VariantChoice)
- platforms, err := WantedPlatforms(&c.ctx)
- assert.Nil(t, err, testName)
+ platforms := WantedPlatforms(&c.ctx)
  assert.Equal(t, c.expected, platforms, testName)
  }
 }

pkg/blobinfocache/internal/prioritize/prioritize.go

@@ -200,8 +200,6 @@ func (css *candidateSortState) compare(xi, xj CandidateWithTime) int {
 
 // destructivelyPrioritizeReplacementCandidatesWithMax is destructivelyPrioritizeReplacementCandidates with parameters for the
 // number of entries to limit for known and unknown location separately, only to make testing simpler.
-// TODO: following function is not destructive any more in the nature instead prioritized result is actually copies of the original
-// candidate set, so In future we might wanna re-name this public API and remove the destructive prefix.
 func destructivelyPrioritizeReplacementCandidatesWithMax(cs []CandidateWithTime, primaryDigest, uncompressedDigest digest.Digest, totalLimit int, noLocationLimit int) []blobinfocache.BICReplacementCandidate2 {
  // split unknown candidates and known candidates
  // and limit them separately.

storage/storage_src.go

@@ -11,6 +11,7 @@ import (
  "fmt"
  "io"
  "os"
+ "slices"
  "sync"
 
  "github.com/containers/image/v5/docker/reference"
@@ -300,7 +301,7 @@ func (s *storageImageSource) LayerInfosForCopy(ctx context.Context, instanceDige
  uncompressedLayerType = manifest.DockerV2SchemaLayerMediaTypeUncompressed
  }
 
- physicalBlobInfos := []types.BlobInfo{}
+ physicalBlobInfos := []types.BlobInfo{} // Built reversed
  layerID := s.image.TopLayer
  for layerID != "" {
  layer, err := s.imageRef.transport.store.Layer(layerID)
@@ -340,9 +341,10 @@ func (s *storageImageSource) LayerInfosForCopy(ctx context.Context, instanceDige
  Size: size,
  MediaType: uncompressedLayerType,
  }
- physicalBlobInfos = append([]types.BlobInfo{blobInfo}, physicalBlobInfos...)
+ physicalBlobInfos = append(physicalBlobInfos, blobInfo)
  layerID = layer.Parent
  }
+ slices.Reverse(physicalBlobInfos)
 
  res, err := buildLayerInfosForCopy(man.LayerInfos(), physicalBlobInfos)
  if err != nil {