vendor: github.com/containerd/containerd v1.7.7 #4315

thaJeztah · 2023-10-10T15:45:20Z

vendor: github.com/containerd/containerd v1.7.7

full diff:

tonistiigi

Looks like the windows error is legit

tonistiigi · 2023-10-10T21:35:51Z

Same as #4301 (comment)

=== FAIL: util/purl TestPURLToRef/pkg:docker/busybox@latest?platform=windows%2Famd64 (0.00s)
    image_test.go:155: 
        	Error Trace:	D:/a/buildkit/buildkit/util/purl/image_test.go:155
        	Error:      	Not equal: 
        	            	expected: v1.Platform{Architecture:"amd64", OS:"windows", OSVersion:"", OSFeatures:[]string(nil), Variant:""}
        	            	actual  : v1.Platform{Architecture:"amd64", OS:"windows", OSVersion:"10.0.20348", OSFeatures:[]string(nil), Variant:""}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -3,3 +3,3 @@
        	            	  OS: (string) (len=7) "windows",
        	            	- OSVersion: (string) "",
        	            	+ OSVersion: (string) (len=10) "10.0.20348",
        	            	  OSFeatures: ([]string) <nil>,
        	Test:       	TestPURLToRef/pkg:docker/busybox@latest?platform=windows%2Famd64
    --- FAIL: TestPURLToRef/pkg:docker/busybox@latest?platform=windows%2Famd64 (0.00s)

=== FAIL: util/purl TestPURLToRef (0.00s)

crazy-max · 2023-10-11T14:33:48Z

This one is expected, see #4316

thaJeztah · 2023-10-11T14:51:37Z

util/purl/image_test.go

@@ -91,7 +91,6 @@ func TestRefToPURL(t *testing.T) {
 func TestPURLToRef(t *testing.T) {
 	testDgst := digest.FromBytes([]byte("test")).String()
 	p := platforms.Normalize(platforms.DefaultSpec())
-	p.OSVersion = "" // OSVersion is not supported in PURL


I removed this "reset", per #4301 (comment)

But I'm somewhat wondering if that's "correct"; are package-URLs meant to be a canonical URL for a specific platform, and if so, should (could) they be os-version specific? (and include the os-version?)

Looking at containerd/containerd#8778 again, I'm starting to wonder if that change was the right thing to do (although, admitted, "Parse" was already filling in missing information). The main issue is that it does not provide for a os-version element, but now does "dream up" one, and it somewhat feels like the paints shut the option of adding an element for os-version in the string-representation.

I wonder if Parse and <insert defaults> should be separate things, but maybe that ship already sailed when it was decided to "only architecture OR platform are required, everything else being optional".

Yeah, I don't see at all how containerd/containerd#8778 could be correct. The Parse() converts from input string to struct, but with that PR it doesn't use the input string, but the properties of the machine that does the parsing, which is completely unrelated.

Same for this test. If the input for the test is empty OSVersion then one should not just magically appear from purl conversion.

I was considering doing the reverse, and update the Purl function to explicitly reset the os-version field; but that's assuming the string format would never support an os-version field. It would perhaps be slightly more correct though; WDYT?

@tonistiigi @jedevc I updated the PR, and updated PURLToRef to explicitly exclude the OSVersion (and OSFeatures) from the result.

jedevc · 2023-10-12T09:58:06Z

cc @ktock (from #4301)

ktock

Dockerfile should also be updated (can be another PR)

buildkit/Dockerfile

Line 4 in 4c93208

ARG CONTAINERD_VERSION=v1.7.2

full diff: containerd/continuity@v0.4.1...v0.4.2 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: stretchr/testify@v1.8.3...v1.8.4 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: golang/sys@v0.8.0...v0.10.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: golang/crypto@v0.2.0...v0.11.0 full diff: golang/text@v0.9.0...v0.11.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: golang/net@v0.10.0...v0.13.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: - microsoft/hcsshim@v0.10.0-rc.8...v0.11.1 - containerd/containerd@v1.7.2...v1.7.7 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2023-10-14T13:31:49Z

Rebased to get a fresh run of CI, and I noticed that the require github.com/containerd/log v0.1.0 was not grouped with other dependencies in go.mod

jedevc · 2023-10-15T18:53:55Z

Dockerfile should also be updated (can be another PR)

buildkit/Dockerfile

Line 4 in 4c93208

ARG CONTAINERD_VERSION=v1.7.2

Is there a strong reason to not do this? If there isn't, we should always keep these in sync.

thaJeztah · 2023-10-16T08:13:54Z

Is there a strong reason to not do this?

No, don't think there's a strong reason (honestly, forgot doing so)

I do think it's good to do so in either a separate commit, or a separate PR though to verify that updating just the binary / just the vendor code / vice-versa all works; updating them individually should work, and if updating one of them breaks CI, there's a serious issue in containerd.

I'll open a PR to update the binaries as well.

thaJeztah · 2023-10-16T08:18:16Z

opened Dockerfile: update containerd binary to v1.7.7 #4339

tonistiigi · 2023-11-01T00:31:45Z

util/purl/image.go

@@ -86,6 +86,21 @@ func PURLToRef(purl string) (string, *ocispecs.Platform, error) {
 			if err != nil {
 				return "", nil, err
 			}
+
+			// OS-version and OS-features are not included when serializing a


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 of platforms.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/OSFeatures then the first step is to define how OSVersion is represented in string form.

cc @dmcgowan

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.

Changes to parser.Parse in 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.

Yes, I agree that the latest changes to Parse() are far from ideal. It's digging in deeper into an already taken direction, where Parse() 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" into linux/amd64, darwin/amd64, windows/amd64 (etc) depending on the host

linux (os only) gets "parsed" into linux/amd64, linux/arm64 (etc), depending on the host

and now windows gets "parsed" into windows/<host-arch>/<host OS version>

The first two are already somewhat problematic (depending on "what" the result is used for), but the os-version is (imo) even more problematic because there is no string-representation of that field.

I guess in an ideal world, Parse would 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;

make Parse accept functional argument(s) to apply defaults (and/or normalizing)

make Parse accept a struct with defaults

And 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.

The first two are already somewhat problematic (depending on "what" the result is used for), but the os-version is (imo) even more problematic because there is no string-representation of that field.

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.

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 need OSVersion on 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.

To summarise, Parse() should just parse and return a struct with fields accurately populated based on the string we've just parsed.

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).

We absolutely need OSVersion on 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.

Yes, matching should be a separate concern from Parsing; in an ideal world, the platform's string representation would also allow for os_version to 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 MatchComparer could be used to either fill in the missing os_version based on the host/environment, or to set the os_version before matching.

It definitely needs to be up to the MatchComparer that decides how the host's details are intermingled, because the same Platform object 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 for OSVersion values are selectable (They're approximately super-sets in that order, as it happens; empty OSVersion is semantically meaningful: "there's no OS present in this image" which only Host-Process mode can execute) and MatchComparer is 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-specific MatchComparer implementations. (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.19242 to 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 OSVersion in 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 of PurlToRef is 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 platform is not currently specified for the docker type, but I assume we're treating it as "containerd's platforms.Parse implementation-defined string serialisation". I see arch is specified for the oci type, but buildkit doesn't support parsing the oci type, and the other Platform fields are not present there.

It's a fair way down the track, but at some point we may also want, e.g., --platform windows/amd64//10.0.0.19242 to

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.19242 as we know it is only used for windows. I'd also suggest that if platform is just windows/amd64 then the OSVersion is not taken from host (even for matcher or build target platform) but there is a defined value that is normalized to empty string. Same as v8 for arm64 and v7 for arm.

but I assume we're treating it as "containerd's platforms.Parse implementation-defined string serialisation

Yes, we don't define any other serialization in buildkit.

tonistiigi reviewed Oct 10, 2023

View reviewed changes

thaJeztah force-pushed the bump_containerd_1.7.7 branch from eeeeab3 to 5a1dad7 Compare October 10, 2023 20:11

thaJeztah force-pushed the bump_containerd_1.7.7 branch from 5a1dad7 to 60915b0 Compare October 11, 2023 14:29

thaJeztah commented Oct 11, 2023

View reviewed changes

thaJeztah force-pushed the bump_containerd_1.7.7 branch 2 times, most recently from 3979486 to 52afe1d Compare October 12, 2023 09:51

ktock approved these changes Oct 12, 2023

View reviewed changes

jedevc mentioned this pull request Oct 13, 2023

Bump up containerd to v1.7.6 #4301

Closed

thaJeztah added 6 commits October 14, 2023 15:28

vendor: github.com/containerd/continuity v0.4.2

66c9cef

full diff: containerd/continuity@v0.4.1...v0.4.2 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

vendor: github.com/stretchr/testify v1.8.4

6a38cee

full diff: stretchr/testify@v1.8.3...v1.8.4 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

vendor: golang.org/x/sys v0.10.0

b78ab95

full diff: golang/sys@v0.8.0...v0.10.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

vendor: golang.org/x/crypto v0.11.0, golang.org/x/text v0.11.0

0fc29e2

full diff: golang/crypto@v0.2.0...v0.11.0 full diff: golang/text@v0.9.0...v0.11.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

vendor: golang.org/x/net v0.13.0

23a2482

full diff: golang/net@v0.10.0...v0.13.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

vendor: github.com/containerd/containerd v1.7.7

f7e0ee1

full diff: - microsoft/hcsshim@v0.10.0-rc.8...v0.11.1 - containerd/containerd@v1.7.2...v1.7.7 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the bump_containerd_1.7.7 branch from 52afe1d to f7e0ee1 Compare October 14, 2023 13:28

thaJeztah mentioned this pull request Oct 14, 2023

build(deps): Bump golang.org/x/net from 0.10.0 to 0.17.0 #4323

Closed

thaJeztah mentioned this pull request Oct 16, 2023

Dockerfile: update containerd binary to v1.7.7 #4339

Merged

jedevc approved these changes Oct 16, 2023

View reviewed changes

jedevc merged commit 67d1ee9 into moby:master Oct 16, 2023

thaJeztah deleted the bump_containerd_1.7.7 branch October 16, 2023 11:09

thaJeztah mentioned this pull request Oct 26, 2023

vendor: update docker/docker and docker/cli to v25.0.0-beta.1 #4383

Merged

6 tasks

tonistiigi reviewed Nov 1, 2023

View reviewed changes

thaJeztah mentioned this pull request Jan 5, 2024

Add OSVersion while parsing ocispec.Platform containerd/containerd#9609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vendor: github.com/containerd/containerd v1.7.7 #4315

vendor: github.com/containerd/containerd v1.7.7 #4315

thaJeztah commented Oct 10, 2023

tonistiigi left a comment

tonistiigi commented Oct 10, 2023 •

edited by thaJeztah

Loading

crazy-max commented Oct 11, 2023

thaJeztah Oct 11, 2023

tonistiigi Oct 11, 2023

thaJeztah Oct 11, 2023

thaJeztah Oct 12, 2023

jedevc commented Oct 12, 2023

ktock left a comment

thaJeztah commented Oct 14, 2023

jedevc commented Oct 15, 2023

thaJeztah commented Oct 16, 2023

thaJeztah commented Oct 16, 2023

tonistiigi Nov 1, 2023

dmcgowan Nov 1, 2023

tonistiigi Nov 1, 2023

thaJeztah Nov 1, 2023

tonistiigi Nov 1, 2023

gabriel-samfira Nov 2, 2023

thaJeztah Nov 2, 2023

TBBle Nov 2, 2023

tonistiigi Nov 2, 2023 •

edited

Loading

vendor: github.com/containerd/containerd v1.7.7 #4315

vendor: github.com/containerd/containerd v1.7.7 #4315

Conversation

thaJeztah commented Oct 10, 2023

vendor: github.com/containerd/continuity v0.4.2

vendor: github.com/stretchr/testify v1.8.4

vendor: golang.org/x/sys v0.10.0

vendor: golang.org/x/crypto v0.11.0, golang.org/x/text v0.11.0

vendor: golang.org/x/net v0.13.0