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

govulncheck-action: Support using a later patch than go directive patch version in go.mod #70853

Open
kaovilai opened this issue Dec 15, 2024 · 16 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@kaovilai
Copy link

Go version

n/a

Output of go env in your module/workspace:

n/a

What did you do?

want to use later golang than go.mod but compatible with go.mod

Support using a later patch release than what's in go.mod

Please ensure the action inherits actions/setup-go#481 when implemented.

What did you see happen?

1.22.0 go.mod results in 1.22.0 being used and no option to use later 1.22

What did you expect to see?

1.22.0 go.mod should use 1.22.7+ or latest available compatible with 1.22.0

@hyangah
Copy link
Contributor

hyangah commented Dec 15, 2024

Sorry that I may be misunderstanding. Isn't check-latest going to do the right thing already? Or can't you use go-version-input with check-latest instead of go-version-file?

Out of curiosity:

  1. what prevents from running go run golang.org/x/vuln/cmd/govulncheck@latest directly from your own github action workflow set up with actions/setup-go? That will give you a full control over how you want to configure Go in your CI. Looks like almost half of the configuration knobs in the current arrangement are to use actions/setup-go rather than the govulncheck invocation itself.

  2. why do you want to use a newer version than the min version specified in the go.mod when scanning for vulnerability? if a module requires 1.22 (or 1.22.0) in its go.mod, but is affected by a vulnerability fixed in 1.22.7, the mentioned setup may prevent detection of the vulnerability. If that module is a library module, ideally the module should change their go.mod to specify 1.22.7 as the minimum requirement. Isn't it?

@kaovilai
Copy link
Author

kaovilai commented Dec 15, 2024

Isn't check-latest going to do the right thing already?

@hyangah no, see actions/setup-go#481 and also recent run results.
https://github.com/kopia/kopia/actions/runs/12328329173/job/34421677211?pr=4303#step:4:26

Recent run

Run actions/[email protected]
  with:
    check-latest: true
    go-version-file: go.mod
    cache: false
    token: ***
  env:
    UNIX_SHELL_ON_WINDOWS: true
    ENABLE_UNICODE_FILENAMES: 
    ENABLE_LONG_FILENAMES: 
Setup go version spec 1.22.0

This issue is to ensure that however govulncheck-action is adopting setup-go, that it is working in govulncheck-action.

@kaovilai
Copy link
Author

kaovilai commented Dec 15, 2024

If that module is a library module, ideally the module should change their go.mod to specify 1.22.7 as the minimum requirement. Isn't it?

No. Because that would enforce minimum version on importing deps, not an ideal trait if you are trying to be a library for others to import or build upon downstream.

A good example of how to bump is here https://github.com/kubernetes/kubernetes/pull/127600/files Notice that this repo never bump away from 1.<minorGoVersion>.0 in their go.mod, instead relying on https://go.dev/doc/toolchain default behavior where the go binary's version is the one used as long as its compatible.

Propagating minimum in go.mod go directive would break vmware-tanzu/velero#8397

@kaovilai
Copy link
Author

See also grpc/grpc-go#7831

@dmitshur
Copy link
Contributor

dmitshur commented Dec 17, 2024

The original issue reports:

1.22.0 go.mod results in 1.22.0 being used and no option to use later 1.22

I have a question about that – what is the behavior for the following go.mod file:

module example
go 1.22.0
toolchain go1.22.10

The toolchain directive of go.mod files is declares a suggested Go toolchain to use with a module (see https://go.dev/doc/toolchain#config and https://go.dev/ref/mod#go-mod-file-toolchain). If it still picks 1.22.0, that does seem unexpected. If it picks 1.22.10, then there is at least one option to use a later toolchain.

@kaovilai
Copy link
Author

kaovilai commented Dec 17, 2024

toolchain go1.22.10
If it still picks 1.22.0, that does seem unexpected. If it picks 1.22.10, then there is at least one option to use a later toolchain.

This is not favored at all by my requirement here: vmware-tanzu/velero#8397 which is to allow use of later toolchain without forcing it on all the downstream builders.

@kaovilai
Copy link
Author

kaovilai commented Dec 17, 2024

Same motivation as grpc/grpc-go#7624 (comment)

We want minimum go to be lowest as possible for the source code to build. The artifacts we produce however will use the newest go one can find in their build environment.

@dmitshur
Copy link
Contributor

I'm not sure I understand that point. The toolchain directive, in contrast to the go directive, applies only to the current module (the one defined by the go.mod file). It suggests the toolchain to be used when in that very module, and doesn't propagate to other modules.

@kaovilai
Copy link
Author

Ok if the toolchain directive do not force minimum version on importing modules then my point maybe moot here. If so, I would be ok with using toolchain as a workaround and hopefully that is respected by go-mod-file: go.mod.

@kaovilai
Copy link
Author

But there is still value in people who want to build the very module where toolchain directive is specified and may not yet have the downstream go that is new enough.

@kaovilai
Copy link
Author

toolchain not being minimum enforced on importing modules would eliminate 80% of my concerns.

@dmitshur
Copy link
Contributor

dmitshur commented Dec 17, 2024

@kaovilai Thanks. I agree that absent of a toolchain directive, it's viable to pick either exactly the minimum toolchain that meets the 'go' directive requirement, or the latest minor release of the same major version. Each has trade-offs, explaining why projects and CI systems may pick what they deem a better fit. (Some projects may even wish to test with multiple minor versions to cover both ends of the spectrum, similarly to how it may make sense to test with multiple major versions.) I mostly wanted to check that the toolchain directive can be helpful for this, so thanks for confirming that.

@timothy-king
Copy link
Contributor

If that module is a library module, ideally the module should change their go.mod to specify 1.22.7 as the minimum requirement. Isn't it?

I think that for libraries we should try to consider the toolchain an input determined by the main package. (Much like type flows into interface values.) Minimum may be too conservative. This is to an extent the job of the main package to see if they are vulnerable.

https://go.dev/doc/toolchain#config

I would say the more relevant point is here https://go.dev/doc/toolchain#select.

IIU-the-request-Correctly it is to test as if "GOTOOLCHAIN=auto" or "GOTOOLCHAIN=path" at CI time. I don't really consider this being a problem for libraries (previous point). But CI and user configurations might not have the same GOTOOLCHAIN settings. Some distros ship with GOTOOLCHAIN=, so it is not enough for the maintainer of the main package to test as if they are at "GOTOOLCHAIN=auto" in order to protect their users.

Thinking out loud about what the default belief for govulncheck-action should be.

  • Should it consider GOTOOLCHAIN=<name> a possibility (more conservative) and have an option to test as if they are at auto?
  • Or should it consider GOTOOLCHAIN=auto the default (much more likely IIRC [do we have telemetry on this?]) and have an option to be more conservative on request?
  • We can also just respect the GOTOOLCHAIN setting on the CI system. This would be faithful, but is going to be quite hidden from users (and confusing). Documentation might be able to help here? Not sure.

Given the nature of govulncheck to look for vulnerabilities, I think we want to be conservative with main packages. I think the simplest way to do that is to have a setting that says whether to use the minimum module version (default) or a specific GOTOOLCHAIN value.

@kaovilai
Copy link
Author

I think we want to be conservative with main packages.

Agree

I think the simplest way to do that is to have a setting that says whether to use the minimum module version (default) or a specific GOTOOLCHAIN value.

If project only specifies 1.22, actions/setup-go setup latest 1.22 such as 1.22.10
If project now specifies 1.22.0 in directive, like in golang/mod@3afcd4e from @matloob requirements it should not lose ability to use latest 1.22 like 1.22.10 in vulncheck.

@kaovilai
Copy link
Author

kaovilai commented Dec 18, 2024

The check-latest flag defaults to false. Use the default or set check-latest to false if you prefer stability and if you want to ensure a specific Go version is always used.

If check-latest is set to true, the action first checks if the cached version is the latest one. If the locally cached version is not the most up-to-date, a Go version will then be downloaded. Set check-latest to true if you want the most up-to-date Go version to always be used.

This flag is already available, it however do not work as I wanted, when go.mod go directive is 1.22.0

Perhaps another flag.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 18, 2024
@kaovilai kaovilai changed the title govulncheck-action: Support using a later patch release than what's in go.mod govulncheck-action: Support using a later patch than go directive patch version in go.mod Dec 22, 2024
kaovilai referenced this issue in sigstore/fulcio Dec 24, 2024
* update golangci-lint to v1.62.x

Signed-off-by: cpanato <[email protected]>

* update go to min go1.23.4

Signed-off-by: cpanato <[email protected]>

---------

Signed-off-by: cpanato <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants