-
Notifications
You must be signed in to change notification settings - Fork 1.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
CVE-2024-24790 bump go to 1.23 #8146
Conversation
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.
Please don't merge this PR until reaching an agreement.
53a9b8f
to
9f8c0a7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8146 +/- ##
==========================================
- Coverage 59.07% 59.05% -0.03%
==========================================
Files 364 364
Lines 30298 30307 +9
==========================================
- Hits 17899 17898 -1
- Misses 10956 10965 +9
- Partials 1443 1444 +1 ☔ View full report in Codecov by Sentry. |
@sdx-jkataja |
9f8c0a7
to
ae4229e
Compare
Sorry, had forgotten to do with the latest changes. Done. |
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.
From prior experience, setting go to 1.22 without a patch version causes problems with the build system. Looks like this CVE is fixed in golang 1.22.4, so setting to 1.22 doesn't necessarily guarantee that. I think the right fix would be to either set this to 1.22.4, or maybe 1.22.5 since .5 also includes some CVE fixes. Also, note that we may want to update other files that specify golang versions to guarantee that we're 1.22.4 or later.
Yes. Should I revert it to how it was setting go to 1.22.6? Do other reviewers @blackpiglet and @mateusoliveira43 agree? |
@sseago can you give an example of problem in build systems? from what I checked, CI job ran from this branch already uses latest 1.22 possible (1.22.6), for example. Reference https://github.com/vmware-tanzu/velero/actions/runs/10556235652/job/29241563973?pr=8146#step:3:24 |
@mateusoliveira43 I don't remember exactly -- @shubham-pampattiwar hit it too, but I think the result was an error complaining that if the |
@mateusoliveira43 we originally had "1.22" -- @shubham-pampattiwar changed it to "1.22.0" because he was having problems building. I reproduced the issue myself locally as well -- basically for at least some actions, if you don't have a patch version for 1.22 and later in your go.mod, then you must specify a go toolchain patch version, so you might as well specify the golang minimum version explicitly. |
@mateusoliveira43 @sseago yeah I had hit this issue on 1.22 IIRC. But I just tried running the |
I had a similar problem in the past, fixed by updating the Go version I had locally If everyone can run velero (from default branch) without the need of specifying patch version in |
ae4229e
to
be353f1
Compare
Signed-off-by: Janne Kataja <janne.kataja@sdx.com>
Signed-off-by: Janne Kataja <janne.kataja@sdx.com>
be353f1
to
0a381f4
Compare
Would you find acceptable to move to go 1.23 ? |
@sdx-jkataja I don't think we should move to 1.23 here. The CVE is fixed in 1.22.5. CVE fixes should be minimal in nature. At some point Velero will move to 1.23, but that should be driven more by development concerns, assuming that any necessary security fixes exist on 1.22.x. |
I regret to withdraw the pull request, but didn't how I could contribute with the conflicting asks. |
Thank you for contributing to Velero!
Please add a summary of your change
Fixes vulnerability CVE-2024-24790 by updating Go to 1.22 Edit: in place of 1.22.0
Does your change fix a particular issue?
Fixes #(issue)
Please indicate you've done the following:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
.