moby / buildkit

concurrent, cache-efficient, and Dockerfile-agnostic builder toolkit
https://github.com/moby/moby/issues/34227
Apache License 2.0
8.09k stars 1.14k forks source link

vendor: github.com/containerd/containerd v1.7.19 and switch to github.com/containerd/platforms module #5034

Closed thaJeztah closed 3 months ago

thaJeztah commented 3 months ago

golangci: forbid uses of platforms.DefaultString()

The platforms.DefaultString() utility returns a platform-string for the local platform. Starting with containerd/platforms@38a74d2, a new format is introduced to allow including OS-version, which is relevant for Windows.

Versions of platforms.DefaultString() after containerd/platforms@38a74d2 include the OS-version by default, which can be problematic if that string is either parsed by code not yet updated to support the new format, or (in general) when sending the platform to a remote / non-matching host (e.g. a Windows client sending to a Linux machine).

Disallow using the platforms.DefaultString(), and recommend using platforms.Format(platforms.DefaultSpec()) instead (which produces the old format without OS-version). Also add a warning about the cross-platform risks of using this function.

client/client_test.go:2557:19: use of `platforms.DefaultString` forbidden because "use platforms\\.Format(platforms\\.DefaultSpec()) instead\\. Be aware that DefaultSpec is for the local platform, so must be avoided when working cross-platform" (forbidigo)
    img := imgs.Find(platforms.DefaultString())
                     ^

vendor: github.com/microsoft/hcsshim v0.11.7

full diff: https://github.com/microsoft/hcsshim/compare/v0.11.5...v0.11.7

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

Welcome to the v1.7.19 release of containerd!

The nineteenth patch release for containerd 1.7 contains various updates and splits the main module from the api module in preparation for the same change in containerd 2.0. Splitting the modules will allow 1.7 and 2.x to both exist as transitive dependencies without running into API registration errors. Projects should use this version as the minimum 1.7 version in preparing to use containerd 2.0 or to be imported alongside it.

Highlights

Container Runtime Interface (CRI)

full diff: https://github.com/containerd/containerd/compare/v1.7.18...v1.7.19

switch to github.com/containerd/platforms module

Switch to use github.com/containerd/platforms module, because containerd's platforms package has moved to a separate module. This allows updating the platforms parsing independent of the containerd module itself.

The package in containerd is deprecated, but kept as an alias to provide compatibility between codebases.

thaJeztah commented 3 months ago

Possibly this one https://github.com/moby/buildkit/blob/313a611bfd922991d03e2208464d1e04b7910d52/client/client_test.go#L2557

Would have to be changed to platforms.Format(platforms.DefaultSpec()) if we don't want the OS-version to be included on Windows; similar to https://github.com/containerd/containerd/commit/19a056163cc37077c33f4e11f8c60d52c14d9a8f

tonistiigi commented 3 months ago

I think we need to add a linter rule banning DefaultString() if it broke backwards compatibility.

tonistiigi commented 3 months ago

Possibly this one

That Find should really take a Matcher probably instead of string.

thaJeztah commented 3 months ago

I think we need to add a linter rule banning DefaultString() if it broke backwards compatibility.

Possibly 🤔 I had a look and it looks like its only used in the line I linked above.

Looking further, that img.Find() is a test-utility, so maybe it's not too problematic, assuming that the client and daemon will always be running on the same platform there.

It looks like CI is happy currently (the incompatibility change should only affect Windows, as it's now adding the os-version), but indeed that DefaultString() (IMO) should either be removed, or come with a big warning that it should not be used for cross-platform situations (e.g. Client sending it as default to a remote API)

I could change it to the old format if you prefer WDYT?;

diff --git a/client/client_test.go b/client/client_test.go
index f6c28020a..eb072132b 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -2554,7 +2554,7 @@ func testBuildExportScratch(t *testing.T, sb integration.Sandbox) {
        imgs, err := testutil.ReadImages(sb.Context(), provider, desc)
        require.NoError(t, err)
        require.Len(t, imgs.Images, 1)
-       img := imgs.Find(platforms.DefaultString())
+       img := imgs.Find(platforms.Format(platforms.DefaultSpec()))
        require.Empty(t, img.Layers)
        require.Equal(t, platforms.DefaultSpec(), img.Img.Platform)
tonistiigi commented 3 months ago

I could change it to the old format if you prefer WDYT?;

My preference would be to add a lint rule and then make the proposed change to make the linter pass. We can look at specific test again when it is enabled for WCOW, but more important to make sure we don't accidentally break something in future changes.

thaJeztah commented 3 months ago

@tonistiigi I added a forbid rule to the linter, and verified that it catches that code; let me know if that's what you meant.