-
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
Fix for rebase omitting os/arch config properties #752
Fix for rebase omitting os/arch config properties #752
Conversation
* Quick fix for missing `os` and `architecture` config properties, required by config spec [1] * Also include unspec'd `os.version` property which is required for Windows images to run on Windows containers [2] This implementation takes these three properties from newBase, which most accurately defines the newly rebased image's os/architecture. If the properties were taken from original, it would prevent reuse of de-facto ABI-compatible top layers with differing old and new base images (i.e. data-only top layers rebased from amd64 to arm, Windows os.version upgrades). This does not address additional, optional config properties that are currently being omitted during rebase such as `author` and `created`. Fixes [google#751] [1]: https://github.com/opencontainers/image-spec/blob/master/config.md#properties [2]: https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/version-compatibility#matching-container-host-version-with-container-image-versions Signed-off-by: Micah Young <ymicah@vmware.com>
0a59b66
to
c8eb274
Compare
Codecov Report
@@ Coverage Diff @@
## master #752 +/- ##
==========================================
- Coverage 79.06% 79.02% -0.05%
==========================================
Files 102 102
Lines 4725 4734 +9
==========================================
+ Hits 3736 3741 +5
- Misses 548 550 +2
- Partials 441 443 +2
Continue to review full report at Codecov.
|
Does this describe your current use case or is it just hypothetical? I see now that my original comments might not have made much sense. We want, largely, the config from the original image to remain intact, and this change takes the os/arch from the new base such that you can re-use top layers across different architectures. I'm wondering if this isn't universally true? For example, what if I want to use gcr.io/distroless/static as a base image for multiple architectures of a go binary? There's nothing platform-specific about the gcr.io/distroless/static base layers, and the platform-specific bits would be in the top layers (just the binary). This seems like the opposite scenario from what you've described. I think we're looking at two separate issues:
Given that we currently don't set any platform info at all, I'm fine merging this and letting the new base image "win" for platform stuff, since you'd demonstrated a need for it, but we probably want some way to handle these conflicts e.g. to support the scenario I described. @imjasonh does what I'm saying here make sense? Do you have any problem with just merging this? I don't think there's any backward-compat issues. |
Hey @jonjohnsonjr I'd be happy to use original for the source of these fields if it makes more sense to start, then later we can implement handling for resolving configfile property conflicts. It will let me remove the workaround in my own codebase and in its place, I can handle cherry-picking the conflicting values from new base after mutate.Rebase and letting everything else come from original. I'm without power at home after a big storm in the eastern US so I wouldn't be able to adjust the PR until later today. |
I thought about this a little bit more and I think the scenario I described is rather unlikely. Most base images contain platform-specific stuff, and distroless is a strange exception, so I'm content to merge this PR. I think it's a reasonable default, and we can provide other ways to achieve what I described.
Let's merge this to unblock you downstream, but some follow-up that might be worthwhile:
Sorry to hear that :( hope you're doing alright. Thanks for the PR! |
Fixes [#751] for missing
os
,architecture
andos.version
properties aftermutate.Rebase
os
,architecture
properties are required by config spec [1]os.version
is unspec'd but required for Windows images to run on Windows containers [2]This implementation takes these three properties from newBase, which most accurately defines the newly rebased image's os/architecture. If the properties were taken from original, it would prevent reuse of de-facto ABI-compatible top layers with differing old and new base images (i.e. data-only top layers rebased from amd64 to arm, Windows os.version upgrades).
This does not address additional, optional config properties that are currently being omitted during rebase such as
author
andcreated
, which I feel could be addressed in a separate issue.