Skip to content
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

[release/1.7] Add API go module #10189

Merged
merged 5 commits into from
Jun 19, 2024

Conversation

dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented May 8, 2024

Add api go module to 1.7. The plan is we do not need to update 1.7 to the 1.8 api go module if we separate it into its like we have done with main. The changes needed are minimal.

This helps the upgrade path from 1.7 to 2.0. After updating to 1.7, it should be easier to update to 2.0 since the api module can be just be upgraded to 1.8 and the latest containerd 1.7 will use the api package from the module.

For those not upgrading to 2.0 right away, the change should be invisible beside an extra dependency showing up in go module. We shouldn't need to do this for 1.6 since this only effects importers who are upgrading. In cases where an import is trying to stay on 1.6 but a dependency uses the latest version of 1.7, go mod would already forces the upgrade anyway.

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dmcgowan
Copy link
Member Author

Let's get this one into the next 1.7 release

@thaJeztah
Copy link
Member

Trying this PR in moby/moby (through a replace rule), but looks like it's not happy. Perhaps I need to force a version for the API module, or something else (or the API package must have aliases in it; or removed, not sure;

hack/vendor.sh
+ /Users/thajeztah/go/src/github.com/docker/docker/hack/with-go-mod.sh go mod tidy -modfile vendor.mod -compat 1.18
with-go-mod.sh: WARN: go.mod exists in the repository root!
with-go-mod.sh: WARN: Using your go.mod instead of our generated version -- this may misbehave!
go: downloading github.com/dmcgowan/containerd v1.7.16-0.20240528144541-4a2ca38f6508
go: downloading github.com/containerd/containerd/api v0.0.0
go: github.com/docker/docker/libcontainerd/remote imports
	github.com/containerd/containerd/api/events: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/libcontainerd/remote imports
	github.com/containerd/containerd/api/types: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/builder/builder-next imports
	github.com/containerd/containerd imports
	github.com/containerd/containerd/api/services/containers/v1: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/builder/builder-next imports
	github.com/containerd/containerd imports
	github.com/containerd/containerd/api/services/content/v1: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/builder/builder-next imports
	github.com/containerd/containerd imports
	github.com/containerd/containerd/api/services/diff/v1: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/builder/builder-next imports
	github.com/containerd/containerd imports
	github.com/containerd/containerd/api/services/events/v1: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/builder/builder-next imports
	github.com/containerd/containerd imports
	github.com/containerd/containerd/api/services/images/v1: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/builder/builder-next imports
	github.com/containerd/containerd imports
	github.com/containerd/containerd/api/services/introspection/v1: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/builder/builder-next imports
	github.com/containerd/containerd imports
	github.com/containerd/containerd/api/services/leases/v1: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0
go: github.com/docker/docker/builder/builder-next imports
	github.com/containerd/containerd imports
	github.com/containerd/containerd/api/services/namespaces/v1: reading github.com/containerd/containerd/api/go.mod at revision api/v0.0.0: unknown revision api/v0.0.0

@thaJeztah
Copy link
Member

thaJeztah commented May 28, 2024

OH! Maybe it works; but it's just version resolution; I manually added the API module, and that ... seems to go better.

Only now it enforces go1.22, because that's what the API module defines?

go 1.22.0

go 1.22.0

toolchain go1.22.2

@dmcgowan
Copy link
Member Author

@thaJeztah there is going to be no way to actually use this new API submodule until this PR gets merged. I could possibly push the dev branch into this repo so we can use the hash, however, it seems easier to just get this merged knowing there will be follows ups with an updated tag.

For the 1.22, that is currently on the api 1.8 which we could update, but it is the version we are going to import in 1.7 right now.

@dmcgowan
Copy link
Member Author

Added one more commit to so 1.7 is compatible with the 1.7 and 1.8 api. This PR is testing with 1.7 and #10278 shows it working with the latest 1.8 rc.

This should be ready to merge now.

@thaJeztah
Copy link
Member

I updated my PR in moby/moby to use both the containerd module, and the API module from this branch, and it looks happy; moby/moby#47142

I can give it one more run, but with the API module from main (to be "rc.3" - with #10279 included) if you think we should check that.

@thaJeztah
Copy link
Member

@dmcgowan can you rebase this one?

akhilerm and others added 5 commits June 13, 2024 06:22
Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
(cherry picked from commit b16e357)
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Allows the api version to be imported and upgraded separately from the
main module.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
(cherry picked from commit e69efd5)
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
@thaJeztah
Copy link
Member

@dims
Copy link
Member

dims commented Jun 17, 2024

+1 from me!

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

the "vendoring itself" is a bit awkward, but I guess that's transitional

@dmcgowan dmcgowan merged commit 1fdc9a0 into containerd:release/1.7 Jun 19, 2024
57 checks passed
Comment on lines +5 to +22
require (
github.com/containerd/ttrpc v1.2.3
github.com/containerd/typeurl/v2 v2.1.1
google.golang.org/genproto/googleapis/rpc v0.0.0-20230822172742-b8732ec3820d
google.golang.org/grpc v1.59.0
google.golang.org/protobuf v1.33.0
)

require (
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang/protobuf v1.5.3 // indirect
github.com/sirupsen/logrus v1.8.1 // indirect
github.com/stretchr/testify v1.7.0 // indirect
golang.org/x/net v0.21.0 // indirect
golang.org/x/sys v0.17.0 // indirect
golang.org/x/text v0.14.0 // indirect
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I'm looking at, and to some extent it's "moot", because this module will be used with containerd 1.7, so dependencies will already be updated based on that, but I noticed some of these are somewhat "behind" on containerd 1.7 itself, and at least one of them (gopkg.in/yaml.v3) is an untagged release, which has a vulnerability (so the api module may get flagged by over-enthusiastic scanners)

go mod graph | grep ' gopkg.in/yaml.v3'
github.com/containerd/containerd/api gopkg.in/yaml.v3@v3.0.0-20210107192922-496545a6307b
github.com/stretchr/testify@v1.7.0 gopkg.in/yaml.v3@v3.0.0-20200313102051-9f266ea9e77c

go mod graph | grep ' github.com/sirupsen/logrus'
github.com/containerd/containerd/api github.com/sirupsen/logrus@v1.8.1
github.com/containerd/ttrpc@v1.2.3 github.com/sirupsen/logrus@v1.8.1

Looking at the release/1.7 branch itself (so containerd/containerd v1.7);

github.com/containerd/ttrpc v1.2.4

gopkg.in/yaml.v3 v3.0.1 // indirect

containerd/go.mod

Lines 58 to 59 in 0a9ac76

github.com/sirupsen/logrus v1.9.3
github.com/stretchr/testify v1.8.4

Updating ttrpc to v1.2.4 would at least address one vulnerability detected by govulncheck (as it would update golang.org/x/net); containerd/ttrpc@v1.2.3...v1.2.4

govulncheck ./...
Scanning your code and 251 packages across 13 dependent modules for known vulnerabilities...

=== Symbol Results ===

Vulnerability #1: GO-2024-2687
    HTTP/2 CONTINUATION flood in net/http
  More info: https://pkg.go.dev/vuln/GO-2024-2687
  Module: golang.org/x/net
    Found in: golang.org/x/net@v0.21.0
    Fixed in: golang.org/x/net@v0.23.0
    Example traces found:
      #1: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.ConnectionError.Error
      #2: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.ErrCode.String
      #3: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.FrameHeader.String
      #4: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.FrameType.String
      #5: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.Setting.String
      #6: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.SettingID.String
      #7: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.StreamError.Error
      #8: services/version/v1/version_grpc.pb.go:13:2: version.init calls status.init, which eventually calls http2.chunkWriter.Write
      #9: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.connError.Error
      #10: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.duplicatePseudoHeaderError.Error
      #11: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.headerFieldNameError.Error
      #12: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.headerFieldValueError.Error
      #13: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.pseudoHeaderError.Error
      #14: events/task_fieldpath.pb.go:85:20: events.TaskIO.Field calls fmt.Sprint, which eventually calls http2.writeData.String

Your code is affected by 1 vulnerability from 1 module.
This scan also found 0 vulnerabilities in packages you import and 3
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.
Use '-show verbose' for more details.

We could update testify to a newer version, to get the untagged gopkg.in/yaml.v3@v3.0.0-20210107192922-496545a6307b out of the list, and to prevent any warnings about CVE-2022-28948; see f6bc986

Lastly, it's probably fine to update logrus to latest (which we should do in ttrpc as well).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH LOL; just got merged.

It was fine for a follow-up either way; I can open a PR for consideration 😄

@thaJeztah
Copy link
Member

@dmcgowan I just noticed that make vendor doesn't like this; is there some trick to make it work?

make vendor
🇩 vendor
go: finding module for package github.com/containerd/containerd/v2/pkg/tracing
go: downloading github.com/containerd/containerd/v2 v2.0.0-rc.3
go: found github.com/containerd/containerd/v2/pkg/tracing in github.com/containerd/containerd/v2 v2.0.0-rc.3
go: downloading github.com/onsi/ginkgo v1.11.0
go: finding module for package github.com/containerd/containerd/api/types/runc/options
go: github.com/containerd/containerd/pkg/cri/sbserver imports
	github.com/containerd/imgcrypt/images/encryption imports
	github.com/containerd/containerd/v2/client imports
	github.com/containerd/containerd/api/types/runc/options: module github.com/containerd/containerd/api@latest found (v1.8.0-rc.2, replaced by ./api), but does not contain package github.com/containerd/containerd/api/types/runc/options
make: *** [vendor] Error 1

@dmcgowan
Copy link
Member Author

@thaJeztah We need a tagged release and dependencies to bump 1.7 dependencies. Which make vendor do you see that in?

@thaJeztah
Copy link
Member

OH! Nevermind ; ignore me; looks like I had a stray file in my code that imported v2 🙈

@thaJeztah
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants