Improve go mod checks with hack/verify-go-modules.sh #3373

gman0 · 2025-04-14T08:10:34Z

Summary

This PR makes it so that:

go mod files are checked against the real state of things (with go mod tidy -diff),
dependencies are in line with k8s repo -- and the script will exit with an error if they are not
- this concludes the discussion in ✨ hack/verify-go-modules.sh: compare dependency versions #3312 (comment)

/kind feature

Part of: #3375

Release Notes

NONE

kcp-ci-bot · 2025-04-14T08:10:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mjudeikis for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

gman0 · 2025-04-14T08:20:49Z

I'll push a separate PR that gets go mod in line with what this script expects.

stevekuznetsov · 2025-04-14T12:58:28Z

hack/verify-go-modules.sh

-mapfile -t DIRS < <(find "${REPO_ROOT}" -name go.mod -print0 | xargs -0 dirname)
+# This is the list of directories that host the individual kcp go modules.
+#
+#   KEEP IT UP TO DATE and SORTED!


You might consider using go list from the root to a) get a full list of modules and b) build the DAG for dependencies, so that keeping the list in order and updated is not a human problem.

Yeah I was also thinking about that too, but then we have only a couple of these nested go modules in the repo so I wasn't sure if that warrants all the code complexity that would bring. If I have time I could push this in a separate commit to see the diff and decide?

Adding a Go workspace at the root would make it pretty simple as well. Up to you

@stevekuznetsov I've pushed 4a0a660 that generates the list and sorts it. Does that look more or less reasonable?

embik · 2025-04-16T07:42:21Z

The more I look at this the more I wonder if it's a good idea? Tests are currently failing for golang.org packages, but as long as kcp compiles and passes our e2e testing, is it really necessary to keep these in sync with the fork?

I'm mostly concerned about vulnerable dependencies we need to upgrade to pull in a patch (as is sometimes the case with golang.org). Do we now need to wait for upstream to pull in these dependencies and cut a patch release that we then rebase on? We can't do these upgrades in our k/k fork because there is no test coverage there.

I can see why we initially wanted to do this in previous issues / PRs, but would better test coverage maybe be the way to go here?

mjudeikis · 2025-04-16T11:48:52Z

I think this could he helpers script to check this , but does not do this blocking way. This way we still have flexibility to "deviate" when we need to (due to security reasons).

embik · 2025-04-16T12:14:45Z

I think this could he helpers script to check this , but does not do this blocking way. This way we still have flexibility to "deviate" when we need to (due to security reasons).

@gman0 already added this in #3312, check out e.g. https://public-prow.kcp.k8c.io/view/s3/prow-public-data/pr-logs/pull/kcp-dev_kcp/3380/pull-kcp-verify/1912205336006103040#1:build-log.txt%3A219.

gman0 · 2025-04-16T12:22:38Z

That's all good points Marvin, thanks! While that's true that the stdlib dependencies should be exempt from the checks, we still could check dependencies to modules from k8s.io and make sure they are in line?

Otherwise I could revert the exiting on failure functionality in this PR and we leave it as a warning if that would be better? This PR checks that gomod files are tidy'd, and i think that's an improvement.

embik · 2025-04-16T12:27:39Z

we still could check dependencies to modules from k8s.io and make sure they are in line?

I think this is a good point. It might make sense to do this for k8s.io/* modules only (warn about everything, but fail on k8s.io modules). At least right now I can't think of a reason where we want to diverge from any k8s.io dependencies (most of them should be coming from the fork anyway). Maybe throw sigs.k8s.io into the mix as well?

gman0 · 2025-04-16T18:24:00Z

Now the output looks like this:

Verifying dependency versions in /tmp/ws/kcp/cli/go.mod against /home/rvasek/go/pkg/mod/cache/download/github.com/kcp-dev/kubernetes/@v/v0.0.0-20250313100806-0011b8c72acd.mod
Version mismatch: golang.org/x/net is v0.36.0, but v0.26.0 expected
...
Continuing because no fatal errors found
Verifying /tmp/ws/kcp/docs/generators/cli-doc
Verifying dependency versions in /tmp/ws/kcp/docs/generators/cli-doc/go.mod against /home/rvasek/go/pkg/mod/cache/download/github.com/kcp-dev/kubernetes/@v/v0.0.0-20250313100806-0011b8c72acd.mod
Version mismatch: github.com/emicklei/go-restful/v3 is v3.12.2, but v3.11.0 expected
...
Version mismatch: k8s.io/api is v0.32.3, but v0.31.6 expected
Version mismatch: k8s.io/apimachinery is v0.32.3, but v0.31.6 expected
Version mismatch: k8s.io/client-go is v0.32.3, but v0.31.6 expected
Version mismatch: k8s.io/component-base is v0.32.3, but v0.31.6 expected
Version mismatch: sigs.k8s.io/structured-merge-diff/v4 is v4.6.0, but v4.4.1 expected
Error: dependencies from '*k8s.io/*' must be in line with /home/rvasek/go/pkg/mod/cache/download/github.com/kcp-dev/kubernetes/@v/v0.0.0-20250313100806-0011b8c72acd.mod

The script now exits with an error if go.mod and/or go.sum are dirty, or if dependencies from '*k8s.io/*' pull in different versions than the ones used in k8s.io/kubernetes. On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

gman0 · 2025-04-17T06:34:04Z

/test pull-kcp-test-e2e-multiple-runs

gman0 · 2025-04-17T08:29:05Z

pull-kcp-test-e2e-multiple-runs failed on:

        I0416 22:56:29.510259   57563 localproxy.go:193] "rewriting cluster" from="root:e2e-workspace-hl28c" to="2guoo00633d7lvld"
        fatal error: concurrent map writes
        
        goroutine 35815 [running]:
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).buildDefinitionRecursively(0xc00b1cdd80, {0x3be55c4, 0x33})
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:454 +0x336
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).buildDefinitionRecursively(0xc00b1cdd80, {0x3bd37ef, 0x2f})
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:458 +0x3e8
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).buildDefinitionRecursively(0xc00b1cdd80, {0x3b7dffe, 0x1a})
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:458 +0x3e8
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).buildDefinitionRecursively(0xc00b1cdd80, {0xc009f42360, 0x1e})
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:458 +0x3e8
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).buildDefinitionForType(0xc00b1cdd80, {0xc009f42360, 0x1e})
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:469 +0x25
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).toSchema(0xc00b1cdd80, {0xc009f42360, 0x1e})
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:485 +0x1b3
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).buildResponse(0xc00b1cdd80, {0x38321a0, 0xc002a23340}, {0x3b463b6, 0x2}, {0xc002a243c0, 0x5, 0x10?})
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:61 +0xc8
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).buildOperations(0xc00b1cdd80, {0x422aa00, 0xc00e25a190}, 0xc00dcff9e0)
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:103 +0x425
        k8s.io/kube-openapi/pkg/builder3.(*openAPI).buildOpenAPISpec(0xc00b1cdd80, {0xc00d3de860, 0x1, 0x410825?})
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:288 +0x955
        k8s.io/kube-openapi/pkg/builder3.BuildOpenAPISpecFromRoutes({0xc00d3de860, 0x1, 0x1}, 0xc000d18f30)
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/builder3/openapi.go:325 +0x56
        github.com/kcp-dev/kcp/pkg/server/openapiv3.(*ServiceCache).RegisterStaticAPIs.func1()
        	/home/prow/go/src/github.com/kcp-dev/kcp/pkg/server/openapiv3/servicecache.go:107 +0x16d
        k8s.io/kube-openapi/pkg/cached.valueFunc[...].Get(...)
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/cached/cache.go:84
        k8s.io/kube-openapi/pkg/cached.(*once[...]).Get.func1()
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/cached/cache.go:243 +0x25
        sync.(*Once).doSlow(0x0?, 0x408f60?)
        	/usr/local/go/src/sync/once.go:76 +0xb4
        sync.(*Once).Do(...)
        	/usr/local/go/src/sync/once.go:67
        k8s.io/kube-openapi/pkg/cached.(*once[...]).Get(0x100000001)
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/cached/cache.go:242 +0x3b
        k8s.io/kube-openapi/pkg/cached.(*listMerger[...]).prepareResultsLocked.func1()
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/cached/cache.go:169 +0x42
        created by k8s.io/kube-openapi/pkg/cached.(*listMerger[...]).prepareResultsLocked in goroutine 35814
        	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/cached/cache.go:168 +0x96
 
...

Is this new?

https://public-prow.kcp.k8c.io/view/s3/prow-public-data/pr-logs/pull/kcp-dev_kcp/3373/pull-kcp-test-e2e-multiple-runs/1912756450111787008

gman0 · 2025-04-17T08:29:36Z

/test pull-kcp-test-e2e-multiple-runs

kcp-ci-bot · 2025-04-17T08:53:53Z

@gman0: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kcp-test-e2e-multiple-runs	`6cc8cd2`	link	true	`/test pull-kcp-test-e2e-multiple-runs`

Full PR test history

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kcp-ci-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 14, 2025

kcp-ci-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 14, 2025

stevekuznetsov reviewed Apr 14, 2025

View reviewed changes

gman0 mentioned this pull request Apr 14, 2025

Clean gomod deps #3377

Merged

gman0 force-pushed the verify-go-modules-v2 branch 2 times, most recently from 4a0a660 to 583c0f4 Compare April 16, 2025 07:22

gman0 mentioned this pull request Apr 16, 2025

Consolidate go mod dependency versions across repositories #3375

Open

1 task

gman0 force-pushed the verify-go-modules-v2 branch from a07094e to 2fea1a2 Compare April 16, 2025 22:25

cli-doc: tidy'd and sync'd go mod

6cc8cd2

On-behalf-of: SAP [email protected] Signed-off-by: Robert Vasek <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve go mod checks with hack/verify-go-modules.sh #3373

Improve go mod checks with hack/verify-go-modules.sh #3373

gman0 commented Apr 14, 2025 •

edited

Loading

kcp-ci-bot commented Apr 14, 2025

gman0 commented Apr 14, 2025

stevekuznetsov Apr 14, 2025

gman0 Apr 14, 2025

stevekuznetsov Apr 14, 2025

gman0 Apr 15, 2025

embik commented Apr 16, 2025 •

edited

Loading

mjudeikis commented Apr 16, 2025

embik commented Apr 16, 2025

gman0 commented Apr 16, 2025

embik commented Apr 16, 2025 •

edited

Loading

gman0 commented Apr 16, 2025

gman0 commented Apr 17, 2025

gman0 commented Apr 17, 2025

gman0 commented Apr 17, 2025

kcp-ci-bot commented Apr 17, 2025

Improve go mod checks with hack/verify-go-modules.sh #3373

Are you sure you want to change the base?

Improve go mod checks with hack/verify-go-modules.sh #3373

Conversation

gman0 commented Apr 14, 2025 • edited Loading

Summary

Release Notes

kcp-ci-bot commented Apr 14, 2025

gman0 commented Apr 14, 2025

stevekuznetsov Apr 14, 2025

Choose a reason for hiding this comment

gman0 Apr 14, 2025

Choose a reason for hiding this comment

stevekuznetsov Apr 14, 2025

Choose a reason for hiding this comment

gman0 Apr 15, 2025

Choose a reason for hiding this comment

embik commented Apr 16, 2025 • edited Loading

mjudeikis commented Apr 16, 2025

embik commented Apr 16, 2025

gman0 commented Apr 16, 2025

embik commented Apr 16, 2025 • edited Loading

gman0 commented Apr 16, 2025

gman0 commented Apr 17, 2025

gman0 commented Apr 17, 2025

gman0 commented Apr 17, 2025

kcp-ci-bot commented Apr 17, 2025

gman0 commented Apr 14, 2025 •

edited

Loading

embik commented Apr 16, 2025 •

edited

Loading

embik commented Apr 16, 2025 •

edited

Loading