etcd-io / etcd

Distributed reliable key-value store for the most critical data of a distributed system
https://etcd.io
Apache License 2.0
46.78k stars 9.64k forks source link

Implement a check to validate that dependency versions match across submodules #18180

Closed ivanvc closed 1 week ago

ivanvc commented 2 weeks ago

What would you like to be added?

As recently discovered in #18168, there was an instance in which a dependency bump for 3.5 didn't address a CVE, as a submodule wasn't properly bumped.

The intention is to add a new check, possibly as a Makefile target. Check that all the dependencies across all the submodules require the same versions. If there's a mismatch, then fail. This should trigger a build failure in prow/GitHub actions, so we have a clear mismatch indicator.

Why is this needed?

To improve stability, and avoid human error when updating dependencies.

ivanvc commented 2 weeks ago

@ahrtr, I did a PoC for this and found out that there are inconsistencies with two dependencies. One is straightforward. However, google.golang.org/genproto/googleapis/rpc has the following versions:

  "google.golang.org/genproto/googleapis/rpc": [
    "v0.0.0-20240318140521-94a12d6c2237",
    "v0.0.0-20240513163218-0867130af1f8",
    "v0.0.0-20240520151616-dc85e6b867a5"
  ],

Technically, it's the same version (0.0.0). But, the date on which go mod fetched the dependency (and the commit sha) differs. I feel like we should have the exact same full version (including the semver labels, i.e., v0.0.0-20240520151616-dc85e6b867a5) for these unreleased dependencies. **However, while updating the dependencies, we could fetch two different commits if the dependency gets new commits while we're updating on our end***.

Thoughts?

ahrtr commented 2 weeks ago

I feel like we should have the exact same full version (including the semver labels, i.e., v0.0.0-20240520151616-dc85e6b867a5) for these unreleased dependencies.

Yes, we should have the exact full version such pseudo versions. We should try to depend tagged versions instead of such pseudo versions.

Technically it isn't a problem for such inconsistencies, and golang uses the algorithm Minimal version selection (MVS) to select the final version to build the packages. But we should also try to make them consistent because,

**However, while updating the dependencies, we could fetch two different commits if the dependency gets new commits while we're updating on our end***.

It's exactly what we need to avoid and verify in the workflow check.

ivanvc commented 2 weeks ago

Thanks, @ahrtr. Then, my PoC covers this case. I'll work on polishing/formalizing it and raise a pull request.

henrybear327 commented 2 weeks ago

Thanks to @ivanvc for driving this :)

Do we also want to make distinctions between direct and indirect imports?

ivanvc commented 2 weeks ago

Do we also want to make distinctions between direct and indirect imports?

I think so. Especially because a direct dependency from a submodule A will be indirect for submodule B, if B imports A.

ivanvc commented 1 week ago

With #18207 and #18205 merged, and as 3.4 doesn't need to scan submodules as there's only one module, we can close this issue now.