-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
Skipping CI for Draft Pull Request. |
41cb738
to
7ee92ff
Compare
7ee92ff
to
ddc2a76
Compare
ddc2a76
to
943a70a
Compare
Let's get this one into the next 1.7 release |
943a70a
to
5be6019
Compare
5be6019
to
4a2ca38
Compare
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;
|
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? Line 3 in 114ef75
|
@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. |
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. |
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. |
@dmcgowan can you rebase this one? |
Signed-off-by: Akhil Mohan <[email protected]> (cherry picked from commit b16e357) Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Derek McGowan <[email protected]>
Allows the api version to be imported and upgraded separately from the main module. Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Derek McGowan <[email protected]> (cherry picked from commit e69efd5) Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Derek McGowan <[email protected]>
9df456d
to
3be919f
Compare
|
+1 from me! |
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.
LGTM
the "vendoring itself" is a bit awkward, but I guess that's transitional
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 | ||
) |
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.
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/[email protected]
github.com/stretchr/[email protected] gopkg.in/[email protected]
go mod graph | grep ' github.com/sirupsen/logrus'
github.com/containerd/containerd/api github.com/sirupsen/[email protected]
github.com/containerd/[email protected] github.com/sirupsen/[email protected]
Looking at the release/1.7 branch itself (so containerd/containerd v1.7);
Line 24 in 0a9ac76
github.com/containerd/ttrpc v1.2.4 |
Line 143 in 0a9ac76
gopkg.in/yaml.v3 v3.0.1 // indirect |
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/[email protected]
Fixed in: golang.org/x/[email protected]
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/[email protected]
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).
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.
OH LOL; just got merged.
It was fine for a follow-up either way; I can open a PR for consideration 😄
@dmcgowan I just noticed that
|
@thaJeztah We need a tagged release and dependencies to bump 1.7 dependencies. Which |
OH! Nevermind ; ignore me; looks like I had a stray file in my code that imported v2 🙈 |
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.