Go vulnerability check doesn't run on submodules

[ivanvc]

Bug report criteria

• [X] This bug report is not security related, security issues should be disclosed privately via etcd maintainers.
• [X] This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
• [X] You have read the etcd bug reporting guidelines.
• [X] Existing open issues along with etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

As pointed out by #18168, the current govulncheck GitHub action is only running for the top-level module, ignoring the submodules.

What did you expect to happen?

To run across all the submodules so it can find vulnerable dependencies included by them.

How can we reproduce it (as minimally and precisely as possible)?

Review any of the action runs, i.e., https://github.com/etcd-io/etcd/actions/runs/9482641629/job/26127954499. It runs just against the top-level module.

Anything else we need to know?

This issue is also present in the release 3.4 and 3.5 branches.

Etcd version (please run commands below)

$ etcd --version
# paste output here

$ etcdctl version
# paste output here

Etcd configuration (command line flags or environment variables)

paste your configuration here

Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

$ etcdctl member list -w table
# paste output here

$ etcdctl --endpoints=<member list> endpoint status -w table
# paste output here

Relevant log output

No response

[ivanvc]

Linking #18170, #18171, and #18172.

[ivanvc]

/assign @henrybear327

[henrybear327]

Thanks to @ivanvc for the quick actions!

[henrybear327]

Regarding the follow-up of https://github.com/etcd-io/etcd/discussions/18168#discussioncomment-9770426, @ivanvc do we want to build a new CI check mandating that all existing dependencies across all go.mod files should have the same version? Do we consider only direct dependencies? Or all?

Any ideas :)

[ivanvc]

@henrybear327, I think the second part (ensuring that the dependencies across the Go submodules have consistent versions) is outside the scope of this issue. If you agree, I'd like to open a new issue for it, and I consider this issue done when we have integrated govulncheck as a Makefile target.

/assign @ivanvc (Makefile improvement)

[ivanvc]

@ahrtr, @jmhbnz, as I'm moving the govuln check to the Makefile (so we can port it to a prow job), I realized there are two options:

1. Add a verify-govuln target, and call it along with verify. This way we don't need to add a new prow job, as the current pull-etcd-verify would run this.
2. Add a new target independent from verify. We'll need a new prow job, but maybe having it as a separate job makes more sense given that it's checking for vulnerabilities.

I lean towards 1. Do you have any thoughts or opinions?

[jmhbnz]

@ahrtr, @jmhbnz, as I'm moving the govuln check to the Makefile (so we can port it to a prow job), I realized there are two options:

1. Add a `verify-govuln` target, and call it along with `verify`. This way we don't need to add a new prow job, as the current `pull-etcd-verify` would run this.

2. Add a new target independent from `verify`. We'll need a new prow job, but maybe having it as a separate job makes more sense given that it's checking for vulnerabilities.

I lean towards 1. Do you have any thoughts or opinions?

My vote would go for option one. Though can we make it at the end of the make verify list so that our other verification runs first for developer ux reasons.

[ahrtr]

Either way works for me. But I prefer to get in included in a separate workflow check (option 2), because we need more / special attention to any security failures.

[ivanvc]

@jmhbnz, I hope you don't mind, but I'm implementing it as a different target :pray:. This way we have a different prow job that runs exclusively this check.

[jmhbnz]

@jmhbnz, I hope you don't mind, but I'm implementing it as a different target 🙏. This way we have a different prow job that runs exclusively this check.

No issue, it's a good point from @ahrtr.

[ivanvc]

There's no need to backport this. It already works with GitHub actions for both branches. Also, as Benjamin pointed out, we don't need multi-module checking for 3.4 as there's a single module.

I'm keeping this issue as I'm using it to track the migration of that job from GitHub actions to the prow infra, which will be only for the main branch.

[ivanvc]

Reopening, as I found out that the release-3.5 branch is not running properly. I'm about to open a pull request to address the issue.