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

crane: edit with embedded data field leads to unexpected validation errors #1552

Closed
tianon opened this issue Feb 1, 2023 · 3 comments · Fixed by #1584
Closed

crane: edit with embedded data field leads to unexpected validation errors #1552

tianon opened this issue Feb 1, 2023 · 3 comments · Fixed by #1584
Labels
bug Something isn't working

Comments

@tianon
Copy link
Contributor

tianon commented Feb 1, 2023

I have an image manifest where the config descriptor includes the data field (opencontainers/image-spec#826), and I used crane edit config ... to modify the config, which worked fine. However, I then ran crane config ... to get back my config object and was given an error message like Error: fetching config: error verifying Digest; got "sha256:edc39d41392fdfa59df991da06102815a14796a2904b14468799545f902f71f7", want "sha256:a181593bfa4535abc8b6ee849bb4d7ba67ac80487285b29ff30f5f1f4040ff24".

IMO, crane edit config on something that includes the data field should probably either update the data field as well as digest (which it already updates) or strip data completely on edit (and either is probably pretty reasonable, but a note that it's stripping data would probably be helpful 😇).

The other half of this is that I would've expected crane config ... to either let me know that it's a digest mismatch because it's trying to read the data field or for it to give me a warning that the digest of the data field is wrong and then go fetch the real blob. 🙇

https://explore.ggcr.dev/?image=tianon/true:oci is the image I was testing with (in case that's useful) -- I was playing with the whitespace on the JSON objects when I ran into this.

Something like this would probably reproduce pretty well:

$ crane cp tianon/true:oci tianon/test:true-oci
2023/01/31 20:59:15 Copying from tianon/true:oci to tianon/test:true-oci
2023/01/31 20:59:16 mounted blob: sha256:a181593bfa4535abc8b6ee849bb4d7ba67ac80487285b29ff30f5f1f4040ff24
2023/01/31 20:59:16 existing blob: sha256:c53fb220cbad89d19e819673ea42ffcb938319825ef36f1b96df37af3d4665f5
2023/01/31 20:59:16 tianon/test:true-oci: digest: sha256:9539d634310afdef42e80de479edeb625cb6dc96df804ea7e78e06f41505f17a size: 1281

$ crane config tianon/test:true-oci | jq -c . | crane edit config tianon/test:true-oci
2023/01/31 20:59:37 pushed blob: sha256:3e66437aef35899e2572e13b04066ac2c705212ae7efa4d29a052449f36aeff7
2023/01/31 20:59:38 tianon/test:true-oci: digest: sha256:36cf1d62e0a0a8b8044f7d9c8d1c4bc1c77426558ad9a4e2edae6e4faf72c295 size: 1221
tianon/test:true-oci

$ crane config tianon/test:true-oci
Error: fetching config: error verifying Digest; got "sha256:a181593bfa4535abc8b6ee849bb4d7ba67ac80487285b29ff30f5f1f4040ff24", want "sha256:3e66437aef35899e2572e13b04066ac2c705212ae7efa4d29a052449f36aeff7"

$ crane version
(devel)

$ # this is a test build of crane that Jon made for me off PR #1551 😇
@tianon tianon added the bug Something isn't working label Feb 1, 2023
@tianon
Copy link
Contributor Author

tianon commented Mar 2, 2023

If data is non-empty, should this update the field's value or delete it? (I don't have a strong opinion either way)

@jonjohnsonjr
Copy link
Collaborator

I think if data is non-empty and correct, we should update it.

I am not sure what to do if it's non-empty and incorrect. Maybe leave it alone?

For me, crane edit is a particularly strange command to think about because I primarily use it to intentionally do things that are outside of what is "correct" in order to test how other systems handle incorrect inputs.

@tianon
Copy link
Contributor Author

tianon commented Mar 2, 2023

Yeah, that's fair - every time I hit this, it's because I did crane edit config, then run crane config and it complains at me, so I run crane edit manifest and delete the data field so I can then crane config to get the base64 --wrap=0 so I can crane edit manifest and plop it back in.

Leaving already-wrong data blobs alone makes sense, and if I get an extra minute I might make a PR for something like that. 🙇 (likely based on #1583)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants