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

Consistently use a string type for expectedLayerDiffIDFlag #2603

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Consistently use a string type for expectedLayerDiffIDFlag #2603

merged 1 commit into from
Oct 24, 2024

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 15, 2024

Might be a fix for #2602 .

@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 15, 2024

@edsantiago This is a guess at the problem and fix; not reproduced, untested. Filing early just in case it happened to unblock you.

@edsantiago
Copy link
Member

Sigh, another one I can't reproduce, but with this vendored in I'm seeing a lot of:

# Error: copying system image from manifest list: reading blob sha256:25981b83f156d64ac55f45ea4c549fa54f4a27d20b4a9639a9efe965aed84617: not supported

(could be completely unrelated; could be that your PR lets things go through a little farther and then something else is breaking. I need to switch gears)

@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 16, 2024

That’s … not a helpful message from c/image or c/storage. Not too likely to be directly related, but also I don’t have any idea where it is coming from.

@mtrmac mtrmac mentioned this pull request Oct 16, 2024
Closed
@mtrmac mtrmac linked an issue Oct 16, 2024 that may be closed by this pull request
expectedLayerDiffIDFlag expectation mismatch #2602
Closed
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 16, 2024

I have confirmed that this fixes #2602, at least per the custom reproducer in #2602 (comment) ; this is ready for review.

@mtrmac mtrmac marked this pull request as ready for review October 16, 2024 22:42
@mtrmac mtrmac added the kind/bug A defect in an existing functionality (or a PR fixing it) label Oct 16, 2024
cgwalters
cgwalters approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

One thing I don't understand here is what's the other code that is setting this to a Digest type? It doesn't seem to be in c/image or c/storage, but maybe I'm missing it.

storage/storage_dest.go
@@ -930,7 +930,7 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D

flags := make(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was map[string]interface{} chosen here? It came in 15ac716f1690cb04f3694e7b295e4830217e8c62 but I don't understand why it couldn't be a structure.

Copy link
Collaborator Author

@mtrmac mtrmac Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See how it is used in that commit; this goes all the way back to 7426cceb6fc121882d601fdfb309e075ecd6f10d .

As for why this does not record specific fields … *shrug* it’s an escape hatch so that out-of-c/storage users can store their private data without properly maintaining a c/storage type extension or without making that data public to all c/storage users; and that’s basically what’s happening here.

@edsantiago
Copy link
Member

Still seeing not supported and blob size mismatch but I guess one bug at a time

@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 17, 2024

One thing I don't understand here is what's the other code that is setting this to a Digest type?

I don’t understand the question. The patched line is the only writer, and untrustedUncompressedDigest is a digest.Digest.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 17, 2024

Still seeing not supported

I don’t immediately know; it’s not happening when just pulling that image from Quay.

and blob size mismatch

Looking at https://github.com/containers/podman/blob/740f1d1fc710d4560d2463b11333cc9ad68e22bd/vendor/github.com/containers/storage/pkg/chunked/storage_linux.go#L1156 , that Podman version does not include containers/storage#2130 yet, so that is expected. (I’m currently working with containers/podman#24287 .)

but I guess one bug at a time

Yes please.

@edsantiago
Copy link
Member

It's possible that "not supported" is only showing up on boltdb tests. (All the error logs are different and it's really hard to catalogue them all). So I spent an hour and a half trying to reproduce on a boltdb setup, with local repo very close to the mirror cache, but no luck.

@edsantiago
Copy link
Member

Hurray for stepping afk! The problem is with VFS. containers/podman#24308

mtrmac added a commit to mtrmac/libpod that referenced this pull request Oct 18, 2024
@mtrmac
DO NOT MERGE: Testing containers/image#2603
5343f05 
go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@untrusted-digest-flag

Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac added a commit to mtrmac/libpod that referenced this pull request Oct 22, 2024
@mtrmac
DO NOT MERGE: Testing containers/image#2603
e846a20 
go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@untrusted-digest-flag

Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac added a commit to mtrmac/libpod that referenced this pull request Oct 22, 2024
@mtrmac
DO NOT MERGE: Testing containers/image#2603
d2fbea4 
go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@untrusted-digest-flag

Signed-off-by: Miloslav Trmač <[email protected]>
vrothberg
vrothberg reviewed Oct 24, 2024
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

giuseppe
giuseppe approved these changes Oct 24, 2024
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

/lgtm

@giuseppe giuseppe merged commit 94563a7 into containers:main Oct 24, 2024
10 checks passed
@mtrmac mtrmac deleted the untrusted-digest-flag branch October 25, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vrothberg vrothberg vrothberg left review comments

@cgwalters cgwalters cgwalters approved these changes

@giuseppe giuseppe giuseppe approved these changes

Assignees
No one assigned
Labels
kind/bug A defect in an existing functionality (or a PR fixing it)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

expectedLayerDiffIDFlag expectation mismatch
5 participants
@mtrmac @edsantiago @giuseppe @cgwalters @vrothberg