-
Notifications
You must be signed in to change notification settings - Fork 527
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
Handle rebasing images with empty layers properly #733
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
499323a
to
7983230
Compare
Codecov Report
@@ Coverage Diff @@
## master #733 +/- ##
==========================================
- Coverage 79.43% 79.39% -0.05%
==========================================
Files 102 102
Lines 4668 4683 +15
==========================================
+ Hits 3708 3718 +10
- Misses 531 534 +3
- Partials 429 431 +2
Continue to review full report at Codecov.
|
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
Small typo, otherwise lgtm, though @imjasonh is more familiar with this code and might have some comments.
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
Manifest v2 specs do not include layer entries for empty layers, but the history in the config will include an entry for empty layers (e.g. produced by `ENV` directive). History entries for empty layers include `empty_layer` field set to `true`. As a result, when rebasing and copying from the new base and original to the rebased image, it's not sufficient to simply iterate layers and pull from the same position/index in the history from config. Instead, history should be iterated and used to generate `Addendum`, while simultaneously iterating layers, but only including the layer (and advancing the iterator) if it's for a history item for a non-empty layer. Since the history in config is optional per the OCI spec [1], iteration via layers will still happen in the event that not all layers are consumed during the history pass. (This should also guard against a malformed history). [1]: https://github.com/opencontainers/image-spec/blob/master/config.md
4140ded
to
b34061e
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Fixed the typo + adjusted the test to include an empty layer. Apologies for force pushing after starting review, I made the CLA bot cranky with a different email address and so rebased it down to one commit while fixing. |
Sorry for missing this PR, it seems reasonable to me. Thanks for the fix! |
Thanks! |
Manifest v2 specs do not include layer entries for empty layers,
but the history in the config will include an entry for empty
layers (e.g. produced by
ENV
directive). History entries forempty layers include
empty_layer
field set totrue
.As a result, when rebasing and copying from the new base and
original to the rebased image, it's not sufficient to simply
iterate layers and pull from the same position/index in the
history from config. Instead, history should be iterated and
used to generate
Addendum
, while simultaneously iteratinglayers, but only including the layer (and advancing the iterator)
if it's for a history item for a non-empty layer.
Since the history in config is optional per the OCI spec,
iteration via layers will still happen in the event that not
all layers are consumed during the history pass. (This should
also guard against a malformed history).
Example
Imagine a simple
Dockerfile
:Which results in the config:
Now, rebase the image with
crane
to a differentbusybox
base:Comparison of the configuration between the two:
Notice that the original config was not only missing the history items from "our" image, but actually had the wrong history item from the old base image (in this case, just the
created
timestamp was swapped.)JFrog (Artifactory)
Besides messing with the metadata in the config, this actually causes problems with JFrog products (e.g. Artifactory, JFrog Container Registry):
The images cannot be pushed - presumably internally it tries to match the history with layers, skipping empty ones, so cannot handle when there are fewer non-empty history entries than layers.
These changes ensure that rebased images can be pushed to these registries and have the benefit of preserving history properly for the general case.