-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
vendor: github.com/containerd/containerd v1.7.7 #4315
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
66c9cef
vendor: github.com/containerd/continuity v0.4.2
thaJeztah 6a38cee
vendor: github.com/stretchr/testify v1.8.4
thaJeztah b78ab95
vendor: golang.org/x/sys v0.10.0
thaJeztah 0fc29e2
vendor: golang.org/x/crypto v0.11.0, golang.org/x/text v0.11.0
thaJeztah 23a2482
vendor: golang.org/x/net v0.13.0
thaJeztah f7e0ee1
vendor: github.com/containerd/containerd v1.7.7
thaJeztah File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The problem in here looks pretty clearly a bug in the new version of
platforms.Parse()where it does not do the job of parsing the string to a struct https://github.com/containerd/containerd/blame/main/platforms/platforms.go#L240-L253 . We shouldn't try to patch this by covering up these side-effects in the caller to make the test pass. There are 10+ calls ofplatforms.Parse()in buildkit. The only reason this showed up in purl test is likely that the integration tests are not running on windows atm. (might be missing some coverage as well).I think the containerd patches should be reverted and if a string can be parsed to a struct with
OSVersion/OSFeaturesthen the first step is to define howOSVersionis represented in string form.cc @dmcgowan
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.
Which PR specifically? Seems this should be easy to unit test but for some reason none of these have unit tests. Microsoft folks last changed this code, we can request better test coverage around it. I don't know enough about what the behavior should even be for these fields.
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.
Changes to
parser.Parsein containerd/containerd#8778 . They read the configuration of the current system instead of parsing the input, so the return value will always be different depending on the caller's operating system or windows versions.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.
Yes, I agree that the latest changes to
Parse()are far from ideal. It's digging in deeper into an already taken direction, whereParse()doesn't just parse but also "fills in the missing pieces" by applying defaults that are based on the host;amd64(arch only) gets "parsed" intolinux/amd64,darwin/amd64,windows/amd64(etc) depending on the hostlinux(os only) gets "parsed" intolinux/amd64,linux/arm64(etc), depending on the hostwindowsgets "parsed" intowindows/<host-arch>/<host OS version>The first two are already somewhat problematic (depending on "what" the result is used for), but the
os-versionis (imo) even more problematic because there is no string-representation of that field.I guess in an ideal world,
Parsewould do just that; parse the string representation into a struct (possibly with some amount of normalizing), but leave it to the consumer to fill in defaults (if any). Some options could be to;Parseaccept functional argument(s) to apply defaults (and/or normalizing)Parseaccept a struct with defaultsAnd ideally also have a string representation of the additional fields (
os-version,os-features?), so that information can be preserved when serializing to a string.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.
Yes, the first two could maybe be better with a more explicit API, but these are not blocking for us. These are the "incomplete" values that make it more obvious that the behavior might be undefined and if we wanted to we could just not allow these (we never document/promote them anyway and users should never use them imho). But for the new windows one, these are the full representations and there is no way to provide input where the parser would just parse the input in a stable form.
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.
CC @kiashok @profnandaa for visibility on this from the MSFT side.
To summarise,
Parse()should just parse and return a struct with fields accurately populated based on the string we've just parsed. We absolutely needOSVersionon Windows to satisfy the matcher and fetch an image that can actually run on the host we're running on, but that value should be explicitly set (if needed) before sending it to the matcher. We should not add defaults that can vary based on the environment we're running on, when parsing.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.
Correct; parsing should just parse what's in the string. The current "fill in host OS or host Architecture if missing" is something we should look at deprecating (but not currently the highest priority as it's not a commonly used or advertised feature).
Yes, matching should be a separate concern from Parsing; in an ideal world, the platform's string representation would also allow for
os_versionto be included, in which case matching should probably skip heuristics (and just do what's requested) but that's not the case yet.But I guess a specific
MatchComparercould be used to either fill in the missingos_versionbased on the host/environment, or to set theos_versionbefore matching.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.
It definitely needs to be up to the
MatchComparerthat decides how the host's details are intermingled, because the samePlatformobject can be a match or a non-match depending on the type of container to be run: Process-Isolated, HyperV-isolated, and Host-Process containers all have different rules forOSVersionvalues are selectable (They're approximately super-sets in that order, as it happens; emptyOSVersionis semantically meaningful: "there's no OS present in this image" which only Host-Process mode can execute) andMatchCompareris the thing we want to vary in containerd to achieve this; I don't recall if the actual mechanism for this variation has been implemented yet though, I recall that it turns out to be a frontend (containerd/CRI, buildkit) concern and there was some issues found in a containerd/cri prototype, but I haven't looked for a while.So the
platforms.Parse(and presumably the reverse?) is intermingling concerns that should be in the Windows-specificMatchComparerimplementations. (Which is already a thing: you can only currently get one via the Windows-build DefaultComparer call, as I recall; that could be nicer... And it implements the Process-Isolation logic)It's a fair way down the track, but at some point we may also want, e.g.,
--platform windows/amd64//10.0.0.19242to be able to be used with a HyperV runtime to build Windows-ABI-incompatible images from the same Dockerfile on a single host, rather than needing to maintain a build-farm of down-level hosts. Presumably that'll rely on being able to parse an OSVersion out of the string, either in buildkit or internally to containerd.Outside MatchComparers, purl's use of the same function has different purpose, highlighting the problem. Here, we need to specify the string form of a specific image rather than an execution platform, and cannot assume the host's details; so for this code to be correct, we need
OSVersionin the canonical string format, or any purl formed for for a Windows image index reference will only be able to round-trip back to a HostProcess image, breaking the current use-case of attestations/provenance on Windows. (That said, GitHub Search says the only caller ofPurlToRefis the BuildKit test suite, so incorrect parsing is probably a less-pressing issue than ambiguous stringification)I assume from those current use-cases that a partial Platform value (filled in by host-provided-defaults) isn't desirable here.
Poking around at the purl docs (I haven't looked at them before AFAIK) it seems like
platformis not currently specified for thedockertype, but I assume we're treating it as "containerd'splatforms.Parseimplementation-defined string serialisation". I seearchis specified for theocitype, but buildkit doesn't support parsing theocitype, and the otherPlatformfields are not present there.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.
Yes, I don't see any other way around it. I would just make it
windows/amd64/10.0.0.19242,windows/arm64/10.0.0.19242,windows/arm64/v9/10.0.0.19242as we know it is only used for windows. I'd also suggest that if platform is justwindows/amd64then theOSVersionis not taken from host (even for matcher or build target platform) but there is a defined value that is normalized to empty string. Same asv8forarm64andv7forarm.Yes, we don't define any other serialization in buildkit.