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.

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

chunked: ignore the tar-split data if digest is empty #1936

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Jun 3, 2024

if a digest was not specified in the TOC, ignore completely the tar-split data.

Otherwise the clients fail to pull images created before commit b5413c2.

if a digest was not specified in the TOC, ignore completely the
tar-split data.

Otherwise the clients fail to pull images created before commit
b5413c2.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@openshift-ci openshift-ci bot added the approved label Jun 3, 2024
@giuseppe
Copy link
Member Author

giuseppe commented Jun 3, 2024

@mtrmac PTAL

I wonder if it would be cleaner to also move the tar-split offsets into the TOC, instead of having half the information in the TOC and half in the annotations.

@giuseppe giuseppe marked this pull request as draft June 3, 2024 12:30
@giuseppe giuseppe marked this pull request as ready for review June 3, 2024 12:35
@giuseppe
Copy link
Member Author

giuseppe commented Jun 3, 2024

I wonder if it would be cleaner to also move the tar-split offsets into the TOC, instead of having half the information in the TOC and half in the annotations.

answering to myself: this would be a bad idea as we will need an additional HTTP request to retrieve it

Copy link
Contributor

openshift-ci bot commented Jun 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member

rhatdan commented Jun 3, 2024

/lgtm

@rhatdan
Copy link
Member

rhatdan commented Jun 3, 2024

Can we rush this into podman 5.1.1?

@openshift-merge-bot openshift-merge-bot bot merged commit 2312b28 into containers:main Jun 3, 2024
18 checks passed
@giuseppe
Copy link
Member Author

giuseppe commented Jun 3, 2024

/cherrypick release-1.54

@openshift-cherrypick-robot

@giuseppe: new pull request created: #1938

In response to this:

/cherrypick release-1.54

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Actually … with the design of #1902, the fix on #1888 wants the layer contents to be fully determined by the TOC.

So:

  • if toc.TarSplitDigest == "", the tar-split must be ignored (what this PR does)
  • if toc.TarSplitDigest != "", we must have, validate, and consume the tar-split; if tarSplitChunk.Offset <= 0, the layer is inconsistent and using it (at least by TOC) must fail.

pkg/chunked/compression_linux.go Show resolved Hide resolved
return nil, nil, nil, 0, fmt.Errorf("validating and decompressing tar-split: %w", err)
tarSplitDigest := toc.TarSplitDigest.String()
// ignore the tar-split data if the digest was not specified
if tarSplitDigest != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Absolutely non-blocking: I’d just do if tarSplitChunk.Offset > 0 && toc.TarSplitDigest != "", directly comparing digest.Digest to "" is fairly frequent, and one less nesting level is nice.)

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 12, 2024

Actually … with the design of #1902, the fix on #1888 wants the layer contents to be fully determined by the TOC.

#1967 , for the record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants