Skip to content

🌱 Bump Go 1.24 #12128

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

Merged
merged 5 commits into from
May 15, 2025
Merged

🌱 Bump Go 1.24 #12128

merged 5 commits into from
May 15, 2025

Conversation

sivchari
Copy link
Member

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Part of #11642

/area dependency

@k8s-ci-robot k8s-ci-robot added area/dependency Issues or PRs related to dependency changes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 25, 2025
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 25, 2025
@sivchari
Copy link
Member Author

Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24)
Failed executing command with error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24)

We should merge #12088 at first since the golangci-lint version that Go1.24 is available is only golangci-lint v2.

@fabriziopandini
Copy link
Member

/hold

We usually use the same go version used by K8s version we are importing, and we import the same K8s version used by the controller runtime version we are using.
(so most probably we should stick to go 1.23 until we bump to CR 0.21)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2025
@sivchari
Copy link
Member Author

sivchari commented May 5, 2025

Thanks!
Other PRs about dependency could be merged, when new CR is published.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2025
@sbueringer
Copy link
Member

sbueringer commented May 9, 2025

I would like to do the CR bump. I'm usually using CAPI as a final verification for CR before the CR release is published. I will coordinate with you that this PR is merged right before.

Can you please rebase this PR?

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2025
@sivchari
Copy link
Member Author

sivchari commented May 9, 2025

@sbueringer
rebased it.

EDIT
I'll take it later.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 9, 2025
@sivchari
Copy link
Member Author

sivchari commented May 9, 2025

This failure is led by Go 1.24 since the go test run the vet implicitly before run test. Since 1.24, printf linter is called every time, then I disabled it on golanci.yml to deal with these issues on another PR. But it seems to deal with it too including with this PR.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 9, 2025
@sivchari
Copy link
Member Author

sivchari commented May 9, 2025

Okay, it works correctly 😄

@sivchari
Copy link
Member Author

sivchari commented May 9, 2025

=== Symbol Results ===

Vulnerability #1: GO-2025-3563
    Request smuggling due to acceptance of invalid chunked data in net/http
  More info: https://pkg.go.dev/vuln/GO-2025-3563
  Standard library
    Found in: net/http/[email protected]
    Fixed in: net/http/[email protected]
    Example traces found:
      #1: cmd/clusterctl/client/repository/repository_gitlab.go:184:28: repository.gitLabRepository.GetFile calls io.ReadAll, which eventually calls internal.chunkedReader.Read

Your code is affected by 1 vulnerability from the Go standard library.
This scan found no other vulnerabilities in packages you import or modules you
require.
Use '-show verbose' for more details.
=== Symbol Results ===

No vulnerabilities found.

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

Vulnerability #1: GO-2025-3563
    Request smuggling due to acceptance of invalid chunked data in net/http
  More info: https://pkg.go.dev/vuln/GO-2025-3563
  Standard library
    Found in: net/http/[email protected]
    Fixed in: net/http/[email protected]
    Example traces found:
      #1: infrastructure/container/docker.go:130:24: container.dockerRuntime.PullContainerImage calls io.ReadAll, which eventually calls internal.chunkedReader.Read

Your code is affected by 1 vulnerability from the Go standard library.
This scan found no other vulnerabilities in packages you import or modules you
require.
Use '-show verbose' for more details.
make: *** [verify-govulncheck] Error 1

We met the vulnerability, then I decide to upgrade from go1.24.0 to go1.24.2.

@sivchari
Copy link
Member Author

sivchari commented May 9, 2025

/retest

@k8s-ci-robot k8s-ci-robot requested a review from sbueringer May 13, 2025 11:01
@sivchari
Copy link
Member Author

Thx! rebased.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2025
@sbueringer sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 13, 2025
@sbueringer
Copy link
Member

Thx!

/lgtm

Waiting with merge until we can be sure that we'll get a CR v0.21.0 release ahead of the CAPI alpha we want to do in a few weeks

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6d2002d939e9353ec159a0f77321ec13626a72da

@chrischdi
Copy link
Member

lgtm, besides google cloud build image bump

@sivchari sivchari force-pushed the bump-go1.24 branch 2 times, most recently from bf0ae43 to d1c4e1e Compare May 13, 2025 15:38
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2025
@k8s-ci-robot k8s-ci-robot requested a review from sbueringer May 13, 2025 16:00
@chrischdi
Copy link
Member

Last open point: update the cloudbuild yaml files to use
gcr.io/k8s-staging-test-infra/gcb-docker-gcloud@sha256:fe4041d57c711070436d3c5b7702534b7158f464e9772aa9e333a5e0e24fd854 / v20250513-9264efb079 which contains go 1.24 🎉

@sbueringer sbueringer mentioned this pull request May 14, 2025
15 tasks
cloudbuild.yaml Outdated
@@ -5,7 +5,7 @@ options:
substitution_option: ALLOW_LOOSE
machineType: 'E2_HIGHCPU_8'
steps:
- name: 'gcr.io/k8s-staging-test-infra/gcb-docker-gcloud@sha256:4e830b673791d5595719bc6c4ca62dce3746b4e20d749e45004254bc6ef0a140' # v20250116-2a05ea7e3d
- name: 'gcr.io/k8s-staging-test-infra/gcb-docker-gcloud@sha256:fe4041d57c711070436d3c5b7702534b7158f464e9772aa9e333a5e0e24fd854' # v20250513-9264efb079
Copy link
Member

@sbueringer sbueringer May 14, 2025

Choose a reason for hiding this comment

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

I cannot pull this image

docker pull gcr.io/k8s-staging-test-infra/gcb-docker-gcloud@sha256:fe4041d57c711070436d3c5b7702534b7158f464e9772aa9e333a5e0e24fd854
Error response from daemon: manifest for gcr.io/k8s-staging-test-infra/gcb-docker-gcloud@sha256:fe4041d57c711070436d3c5b7702534b7158f464e9772aa9e333a5e0e24fd854 not found: manifest unknown: Requested entity was not found.

Copy link
Member

Choose a reason for hiding this comment

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

I think this sha works sha256:63840f133e0dfeea0af9ef391210da7fab9d2676172e2967fccab0cd6110c4e7

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx! I fixed it.

@sbueringer
Copy link
Member

We met the vulnerability, then I decide to upgrade from go1.24.0 to go1.24.2.

Nice catch btw.

I discussed this with Christian and Fabrizio. Given that we are going to merge the CR bump ahead of the next release anyway we would like to merge the current PR as soon as it's ready (last finding should be: #12128 (comment))

Signed-off-by: sivchari <[email protected]>
@sbueringer
Copy link
Member

/hold cancel
/lgtm
/approve

(double checked the gcb-docker-gcloud images, now all good)

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 15, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ba67208b2af6fd5eb5c6279aa2f6e8e00a4792f0

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2025
@k8s-ci-robot k8s-ci-robot merged commit 0028772 into kubernetes-sigs:main May 15, 2025
18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants