Skip to content
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

Jump to bottom

Refactor to make it clearer that we return the validated TOC value #1887

Merged
merged 3 commits into from
Apr 16, 2024
Merged

Refactor to make it clearer that we return the validated TOC value #1887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3beea1e
Only obtain the estargz TOC digest once
mtrmac Apr 13, 2024
1f47b38
Only obtain the zstd:chunked TOC digest once
mtrmac Apr 13, 2024
053ac61
Remove ChecksumAnntation from ZstdChunkedFooterData
mtrmac Apr 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 4 additions & 9 deletions pkg/chunked/compression_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"io"
"strconv"

"github.com/containerd/stargz-snapshotter/estargz"
"github.com/containers/storage/pkg/chunked/internal"
"github.com/klauspost/compress/zstd"
"github.com/klauspost/pgzip"
Expand All @@ -33,7 +32,7 @@ func typeToTarType(t string) (byte, error) {
return r, nil
}

func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, annotations map[string]string) ([]byte, int64, error) {
func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, tocDigest digest.Digest) ([]byte, int64, error) {
// information on the format here https://github.com/containerd/stargz-snapshotter/blob/main/docs/stargz-estargz.md
footerSize := int64(51)
if blobSize <= footerSize {
Expand Down Expand Up @@ -126,11 +125,7 @@ func readEstargzChunkedManifest(blobStream ImageSourceSeekable, blobSize int64,
return nil, 0, err
}

d, err := digest.Parse(annotations[estargz.TOCJSONDigestAnnotation])
if err != nil {
return nil, 0, err
}
if manifestDigester.Digest() != d {
if manifestDigester.Digest() != tocDigest {
return nil, 0, errors.New("invalid manifest checksum")
}

Expand All @@ -140,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 Down Expand Up @@ -238,7 +233,7 @@ func readZstdChunkedManifest(blobStream ImageSourceSeekable, blobSize int64, ann
return nil, nil, 0, err
}

decodedBlob, err := decodeAndValidateBlob(manifest, footerData.LengthUncompressed, footerData.ChecksumAnnotation)
decodedBlob, err := decodeAndValidateBlob(manifest, footerData.LengthUncompressed, tocDigest.String())
if err != nil {
return nil, nil, 0, err
}
Expand Down
7 changes: 0 additions & 7 deletions pkg/chunked/internal/compression.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ func WriteZstdChunkedManifest(dest io.Writer, outMetadata map[string]string, off
Offset: manifestOffset,
LengthCompressed: uint64(len(compressedManifest)),
LengthUncompressed: uint64(len(manifest)),
ChecksumAnnotation: "", // unused
OffsetTarSplit: uint64(tarSplitOffset),
LengthCompressedTarSplit: uint64(len(tarSplitData.Data)),
LengthUncompressedTarSplit: uint64(tarSplitData.UncompressedSize),
Expand All @@ -207,7 +206,6 @@ type ZstdChunkedFooterData struct {
Offset uint64
LengthCompressed uint64
LengthUncompressed uint64
ChecksumAnnotation string // Only used when reading a layer, not when creating it

OffsetTarSplit uint64
LengthCompressedTarSplit uint64
Expand All @@ -234,11 +232,6 @@ func footerDataToBlob(footer ZstdChunkedFooterData) []byte {
func ReadFooterDataFromAnnotations(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)
}

offsetMetadata := annotations[ManifestInfoKey]

if _, err := fmt.Sscanf(offsetMetadata, "%d:%d:%d:%d", &footerData.Offset, &footerData.LengthCompressed, &footerData.LengthUncompressed, &footerData.ManifestType); err != nil {
Expand Down
1 change: 0 additions & 1 deletion pkg/chunked/internal/compression_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ func TestGenerateAndReadFooter(t *testing.T) {
Offset: 2,
LengthCompressed: 3,
LengthUncompressed: 4,
ChecksumAnnotation: "", // unused
OffsetTarSplit: 5,
LengthCompressedTarSplit: 6,
LengthUncompressedTarSplit: 7,
Expand Down
44 changes: 25 additions & 19 deletions pkg/chunked/storage_linux.go
View file
Open in desktop
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,18 +265,26 @@ func GetDiffer(ctx context.Context, store storage.Store, blobDigest digest.Diges
return nil, errors.New("enable_partial_images not configured")
}

_, hasZstdChunkedTOC := annotations[internal.ManifestChecksumKey]
_, hasEstargzTOC := annotations[estargz.TOCJSONDigestAnnotation]
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 {
return makeEstargzChunkedDiffer(ctx, store, blobSize, annotations, iss, &storeOpts)
estargzTOCDigest, err := digest.Parse(estargzTOCDigestString)
if err != nil {
return nil, fmt.Errorf("parsing estargz TOC digest %q: %w", estargzTOCDigestString, err)
}
return makeEstargzChunkedDiffer(ctx, store, blobSize, estargzTOCDigest, iss, &storeOpts)
}

return makeConvertFromRawDiffer(ctx, store, blobDigest, blobSize, annotations, iss, &storeOpts)
Expand Down Expand Up @@ -303,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 @@ -313,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 All @@ -333,8 +337,8 @@ func makeZstdChunkedDiffer(ctx context.Context, store storage.Store, blobSize in
}, nil
}

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

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

return &chunkedDiffer{
fsVerityDigests: make(map[string]string),
blobSize: blobSize,
Expand Down Expand Up @@ -1692,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
View file
Open in desktop
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