Skip to content

Conversation

ivanvc
Copy link
Member

@ivanvc ivanvc commented Sep 8, 2025

Move to the script executing and installing govulncheck. This way, the installed version matches the GOTOOLCHAIN defined in scripts/test_lib.sh. The current issue is that the Prow infrastructure is still using Go 1.24; there's a version mismatch if we don't specify GOTOOLCHAIN. With this, we no longer need the multi-image hack removed in kubernetes/test-infra#35363, introduced in kubernetes/test-infra#35050.

As reference, this is the failure before using GOTOOLCHAIN (from #20635): https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/etcd-io_etcd/20635/pull-etcd-govulncheck/1965163337700347904

stderr: govulncheck: loading packages:
stderr: There are errors with the provided package patterns:
stderr: 
stderr: /home/prow/go/src/github.com/etcd-io/etcd/api/authpb/auth.pb.go:4:1: package requires newer Go version go1.25 (application built with go1.24) 
...

Now, after pushing these two commits in that branch, the result is a successful run: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/etcd-io_etcd/20635/pull-etcd-govulncheck/1965196061718876160

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@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

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivanvc

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

@ivanvc ivanvc marked this pull request as ready for review September 8, 2025 23:37
@ivanvc ivanvc force-pushed the run-govulncheck-with-project-gotoolchain branch from 7adc798 to b02eb2c Compare September 8, 2025 23:47
Copy link

codecov bot commented Sep 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.04%. Comparing base (ab98b5b) to head (26a9faa).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

see 142 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #20636      +/-   ##
==========================================
- Coverage   69.02%   62.04%   -6.99%     
==========================================
  Files         420      406      -14     
  Lines       34783    33661    -1122     
==========================================
- Hits        24008    20884    -3124     
- Misses       9373    11216    +1843     
- Partials     1402     1561     +159     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab98b5b...26a9faa. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ivanvc
Copy link
Member Author

ivanvc commented Sep 9, 2025

/retest

@ivanvc
Copy link
Member Author

ivanvc commented Sep 9, 2025

@ivanvc
Copy link
Member Author

ivanvc commented Sep 9, 2025

/cc @ahrtr @jmhbnz

@k8s-ci-robot k8s-ci-robot requested review from ahrtr and jmhbnz September 9, 2025 19:06
@ahrtr
Copy link
Member

ahrtr commented Sep 9, 2025

The current issue is that the Prow infrastructure is still using Go 1.24

We already set the environment variable GOTOOLCHAIN, so this isn't a problem any more?

Move to the script executing and installing govulncheck

I recall that the golang team recommends to use the latest govulncheck. Need to double check.

@ivanvc
Copy link
Member Author

ivanvc commented Sep 9, 2025

We already set the environment variable GOTOOLCHAIN, so this isn't a problem any more?

It's still a problem without this pull request. Maybe I wasn't clear in my description. Let me try to make this clear.

The problem is that currently installing govulncheck is done straight in the Makefile:

etcd/Makefile

Lines 196 to 199 in ab98b5b

run-govulncheck:
ifeq (, $(shell command -v govulncheck))
$(shell go install golang.org/x/vuln/cmd/govulncheck@latest)
endif

At this point, GOTOOLCHAIN is set by the end-user (or the Prow job in our case, which is unset). The default value is auto. Therefore, it will use the host's Go installation.

This is the reason why we needed to introduce different Prow jobs in kubernetes/test-infra#35050, as the release branches were using Go 1.23, while Prow has Go 1.24.

So, to fix this issue, we need to ensure that when we install govulncheck, we use the GOTOOLCHAIN that we set in ./scripts/test_lib.sh. Otherwise, whenever Prow updates to Go 1.25, our release branches will have an error similar to: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/etcd-io_etcd/20635/pull-etcd-govulncheck/1965163337700347904.

In this pull request, I'm moving the installation from the Makefile to the test.sh PASS, which already sourced test_lib.sh (therefore, GOTOOLCHAIN is set).

In conclusion, it's still a problem without this pull request.

I recall that the golang team recommends to use the latest govulncheck. Need to double check.

This comes from the installation instructions on https://pkg.go.dev/golang.org/x/vuln#section-readme. I didn't think it would harm pointing to a specific govulncheck version, as it still uses the external database. But I can revert this part.

@ivanvc ivanvc force-pushed the run-govulncheck-with-project-gotoolchain branch from b02eb2c to 4470ecb Compare September 9, 2025 21:46
@ivanvc
Copy link
Member Author

ivanvc commented Sep 9, 2025

Noting that this is a blocker to update the main branch to Go 1.25.

@ivanvc ivanvc force-pushed the run-govulncheck-with-project-gotoolchain branch from 4470ecb to b1a881e Compare September 9, 2025 22:43
@ivanvc
Copy link
Member Author

ivanvc commented Sep 10, 2025

/retest

@ahrtr
Copy link
Member

ahrtr commented Sep 10, 2025

In this pull request, I'm moving the installation from the Makefile to the test.sh

This makes sense. But I don't need to manage golang.org/x/vuln/cmd/govulncheck's version in tool/mod.

tools/mod/go.mod Outdated
@@ -17,6 +17,7 @@ require (
go.etcd.io/gofail v0.2.0
go.etcd.io/protodoc v0.0.0-20180829002748-484ab544e116
go.etcd.io/raft/v3 v3.6.0
golang.org/x/vuln v1.1.4
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to add this. Is there any problem to use the latest govulncheck version?

Copy link
Member Author

@ivanvc ivanvc Sep 10, 2025

Choose a reason for hiding this comment

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

Without adding it to tools/mod, we can't use go_run_tool, as I get the error:

% (cd api && 'go' 'install' 'golang.org/x/vuln/cmd/govulncheck@latest')
cannot find module providing package golang.org/x/vuln/cmd/govulncheck: import lookup disabled by -mod=readonly
Failed to install tool 'golang.org/x/vuln/cmd/govulncheck@latest'
There was a Failure in module api, aborting...
FAIL: 'govuln' FAILED at Wed Sep 10 01:53:37 PM PDT 2025        

Thinking of using Go 1.24's tool directive won't work in the future if we want to point to "latest", because the tool directive adds an indirect dependency.

I documented that in 6a7dd21 26a9faa.

@@ -31,6 +31,7 @@ import (
_ "github.com/google/yamlfmt/cmd/yamlfmt"
_ "github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-grpc-gateway"
_ "github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2"
_ "golang.org/x/vuln/cmd/govulncheck"
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this.

@ivanvc
Copy link
Member Author

ivanvc commented Sep 10, 2025

This makes sense. But I don't need to manage golang.org/x/vuln/cmd/govulncheck's version in tool/mod.

That sounds great, I'll drop that commit.

@ivanvc ivanvc force-pushed the run-govulncheck-with-project-gotoolchain branch from b1a881e to 6a7dd21 Compare September 10, 2025 21:02
Ensure that GOTOOLCHAIN is set by the time it runs, to ensure the binary
gets compiled with the right Go version.

Signed-off-by: Ivan Valdes <[email protected]>
@ivanvc ivanvc force-pushed the run-govulncheck-with-project-gotoolchain branch from 6a7dd21 to 26a9faa Compare September 10, 2025 21:07
@ivanvc
Copy link
Member Author

ivanvc commented Sep 10, 2025

/retest

Comment on lines +401 to +405
# Install govulncheck without using the tools/mod module, or run_go_tool,
# as the installation instructions point to latest. Doing it with run_go_tool,
# or even with Go 1.24's tool directive, will add the dependency (direct or
# indirect) to the tools/mod Go module.
run go install golang.org/x/vuln/cmd/govulncheck@latest
Copy link
Member

Choose a reason for hiding this comment

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

The change looks good, but the comment is a little confusing. go install will never update the dependencies (go.mod file).

For simplicity, let's just add a comment for run_go_tool something like This function is only used to run commands which are managed by tools/mod?

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

Successfully merging this pull request may close these issues.

3 participants