-
Notifications
You must be signed in to change notification settings - Fork 528
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
fail computing manifest if no refs are provided #887
Conversation
Codecov Report
@@ Coverage Diff @@
## master #887 +/- ##
==========================================
+ Coverage 72.52% 72.53% +0.01%
==========================================
Files 108 108
Lines 4670 4672 +2
==========================================
+ Hits 3387 3389 +2
Misses 773 773
Partials 510 510
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I found a weird quirk in kaniko where they were passing no refs into WriteToFile. Unfortunately, this produced a nil manifest, which json-encoded to null (valid json!), so I ended up with a tarball with only a manifest.json with the contents null.
Interesting! Is this intentional from their side? E.g. they're unconditionally writing stuff to a tarball, even if sometimes that's nil
? I would suspect that's a bug on their side, but failing earlier on our side is definitely reasonable.
Instead of writing out null
, should we be writing []
instead? Basically, I'm not sure where the bug is... I understand this isn't expected behavior and that we need to change something, but does returning an error here exclude producing a valid (if useless) image-less tarball? Is there any case for that being something we'd ever want to allow?
Your PR to docker makes it fail loudly when passed a null
manifest. Your PR here makes us fail loudly w hen passed a nil
map. So I'm guessing you just want this to fail more loudly and either PR would do the trick?
I'm guessing it's something in here where len(destRefs) == 0
?
pkg/v1/tarball/write.go
Outdated
@@ -222,6 +222,10 @@ func writeImagesToTar(refToImage map[name.Reference]v1.Image, m []byte, size int | |||
func calculateManifest(refToImage map[name.Reference]v1.Image) (m Manifest, err error) { | |||
imageToTags := dedupRefToImage(refToImage) | |||
|
|||
if len(imageToTags) == 0 { | |||
return nil, errors.New("at least one reference must be provided to calculate a manifest") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this is mostly because we are inconsistent in how we're wrapping errors, and we should do better here, but in the common path we're going to see this returned as:
unable to calculate manifest: at least one reference must be provided to calculate a manifest
That seems a bit redundant, so I might change this error to set of images is empty
or something along those lines.
Also interesting that this PR essentially fixes the other half: #697 |
I can't imagine this is desired. I opened GoogleContainerTools/kaniko#1534
I wouldn't say there's a bug. But I think we can improve UX. I can't imagine anyone would want to pack a useless
Yeah - basically any of the PRs to docker/kaniko/go-containerregistry would have saved me a big yakshave. I submitted a PR to each layer because I could see the same issue biting other people who use different tooling (might use go-containerregistry without kaniko, might use docker with some other build tool, etc). Once I was in the weeds and knew what was going on, I figured we should prevent other folks from needing to get in those weeds. Anyway, my vote is to just error in this case. I think this will prevent a lot more frustration than it will cause. Even |
Sorry about that :/
Indeed... this often feels like my purpose in life :)
I think erroring here is fine and less surprising than what we're currently doing. I'm happy to merge if you address my comment re: the error string 👍 |
Sorry! Missed this part. Should be done now :) Thanks so much for working through this with me :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This PR just adds some validation to ensure we're not producing invalid manifests.
null
might be a valid digest but I can't imagine any user intentionally producing an image with anull
manifest (Docker can't import that or do anything useful with it).I found a weird quirk in kaniko where they were passing no refs into
WriteToFile
. Unfortunately, this produced anil
manifest, which json-encoded tonull
(valid json!), so I ended up with a tarball with only amanifest.json
with the contentsnull
.Of course, ECR and Docker were happy to accept this tarball and just... silently do nothing with it.
I opened a PR to fix docker loads with a
null
manifest. I'll also open a PR to kaniko to reject no destinations.