Skip to content

Commit

Permalink
Only obtain the zstd:chunked TOC digest once
Browse files Browse the repository at this point in the history
Make it structually clear that the code is all using the same value,
making it less likely for the verifier and other uses to get out of sync.

Also avoids some redundant parsing and error paths.
The conversion path looks longer, but that's just moving the parsing
from the called function (which is redundant for other callers).

Should not change behavior.

Signed-off-by: Miloslav Trmač <[email protected]>
  • Loading branch information
mtrmac committed Apr 13, 2024
1 parent 3beea1e commit 1f47b38
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 18 deletions.
4 changes: 2 additions & 2 deletions pkg/chunked/compression_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64,
// readZstdChunkedManifest reads the zstd:chunked manifest from the seekable stream blobStream. The blob total size must
// be specified.
// This function uses the io.github.containers.zstd-chunked. annotations when specified.
func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, annotations map[string]string) ([]byte, []byte, int64, error) {
func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, tocDigest digest.Digest, annotations map[string]string) ([]byte, []byte, int64, error) {
footerSize := int64(internal.FooterSizeSupported)
if blobSize <= footerSize {
return nil, nil, 0, errors.New("blob too small")
Expand All @@ -145,7 +145,7 @@ func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, ann

if offsetMetadata := annotations[internal.ManifestInfoKey]; offsetMetadata != "" {
var err error
footerData, err = internal.ReadFooterDataFromAnnotations(annotations)
footerData, err = internal.ReadFooterDataFromAnnotations(tocDigest, annotations)
if err != nil {
return nil, nil, 0, err
}
Expand Down
7 changes: 2 additions & 5 deletions pkg/chunked/internal/compression.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,13 +231,10 @@ func footerDataToBlob(footer ZstdChunkedFooterData) []byte {
}

// ReadFooterDataFromAnnotations reads the zstd:chunked footer data from the given annotations.
func ReadFooterDataFromAnnotations(annotations map[string]string) (ZstdChunkedFooterData, error) {
func ReadFooterDataFromAnnotations(tocDigest digest.Digest, annotations map[string]string) (ZstdChunkedFooterData, error) {
var footerData ZstdChunkedFooterData

footerData.ChecksumAnnotation = annotations[ManifestChecksumKey]
if footerData.ChecksumAnnotation == "" {
return footerData, fmt.Errorf("manifest checksum annotation %q not found", ManifestChecksumKey)
}
footerData.ChecksumAnnotation = tocDigest.String()

offsetMetadata := annotations[ManifestInfoKey]

Expand Down
27 changes: 17 additions & 10 deletions pkg/chunked/storage_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/containers/storage/pkg/archive"
"github.com/containers/storage/pkg/chunked/compressor"
"github.com/containers/storage/pkg/chunked/internal"
"github.com/containers/storage/pkg/chunked/toc"
"github.com/containers/storage/pkg/fsverity"
"github.com/containers/storage/pkg/idtools"
"github.com/containers/storage/pkg/system"
Expand Down Expand Up @@ -264,15 +265,19 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges
return nil, errors.New("enable_partial_images not configured")
}

_, hasZstdChunkedTOC := annotations[internal.ManifestChecksumKey]
zstdChunkedTOCDigestString, hasZstdChunkedTOC := annotations[internal.ManifestChecksumKey]
estargzTOCDigestString, hasEstargzTOC := annotations[estargz.TOCJSONDigestAnnotation]

if hasZstdChunkedTOC && hasEstargzTOC {
return nil, errors.New("both zstd:chunked and eStargz TOC found")
}

if hasZstdChunkedTOC {
return makeZstdChunkedDiffer(ctx, store, blobSize, annotations, iss, &storeOpts)
zstdChunkedTOCDigest, err := digest.Parse(zstdChunkedTOCDigestString)
if err != nil {
return nil, fmt.Errorf("parsing zstd:chunked TOC digest %q: %w", zstdChunkedTOCDigestString, err)
}
return makeZstdChunkedDiffer(ctx, store, blobSize, zstdChunkedTOCDigest, annotations, iss, &storeOpts)
}
if hasEstargzTOC {
estargzTOCDigest, err := digest.Parse(estargzTOCDigestString)
Expand Down Expand Up @@ -307,8 +312,8 @@ func makeConvertFromRawDiffer(ctx context.Context, store storage.Store, blobDige
}, nil
}

func makeZstdChunkedDiffer(ctx context.Context, store storage.Store, blobSize int64, annotations map[string]string, iss ImageSourceSeekable, storeOpts *types.StoreOptions) (*chunkedDiffer, error) {
manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(iss, blobSize, annotations)
func makeZstdChunkedDiffer(ctx context.Context, store storage.Store, blobSize int64, tocDigest digest.Digest, annotations map[string]string, iss ImageSourceSeekable, storeOpts *types.StoreOptions) (*chunkedDiffer, error) {
manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(iss, blobSize, tocDigest, annotations)
if err != nil {
return nil, fmt.Errorf("read zstd:chunked manifest: %w", err)
}
Expand All @@ -317,11 +322,6 @@ func makeZstdChunkedDiffer(ctx context.Context, store storage.Store, blobSize in
return nil, err
}

tocDigest, err := digest.Parse(annotations[internal.ManifestChecksumKey])
if err != nil {
return nil, fmt.Errorf("parse TOC digest %q: %w", annotations[internal.ManifestChecksumKey], err)
}

return &chunkedDiffer{
fsVerityDigests: make(map[string]string),
blobSize: blobSize,
Expand Down Expand Up @@ -1691,7 +1691,14 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
blobFile.Close()
blobFile = nil

manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(fileSource, c.blobSize, annotations)
tocDigest, err := toc.GetTOCDigest(annotations)
if err != nil {
return graphdriver.DriverWithDifferOutput{}, fmt.Errorf("internal error: parsing just-created zstd:chunked TOC digest: %w", err)
}
if tocDigest == nil {
return graphdriver.DriverWithDifferOutput{}, fmt.Errorf("internal error: just-created zstd:chunked missing TOC digest")
}
manifest, tarSplit, tocOffset, err := readZstdChunkedManifest(fileSource, c.blobSize, *tocDigest, annotations)
if err != nil {
return graphdriver.DriverWithDifferOutput{}, fmt.Errorf("read zstd:chunked manifest: %w", err)
}
Expand Down
7 changes: 6 additions & 1 deletion pkg/chunked/zstdchunked_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ import (
"testing"

"github.com/containers/storage/pkg/chunked/internal"
"github.com/containers/storage/pkg/chunked/toc"
"github.com/klauspost/compress/zstd"
"github.com/opencontainers/go-digest"
"github.com/stretchr/testify/require"
)

type seekable struct {
Expand Down Expand Up @@ -148,7 +150,10 @@ func TestGenerateAndParseManifest(t *testing.T) {
t: t,
}

manifest, _, _, err := readZstdChunkedManifest(s, 8192, annotations)
tocDigest, err := toc.GetTOCDigest(annotations)
require.NoError(t, err)
require.NotNil(t, tocDigest)
manifest, _, _, err := readZstdChunkedManifest(s, 8192, *tocDigest, annotations)
if err != nil {
t.Error(err)
}
Expand Down

0 comments on commit 1f47b38

Please sign in to comment.