Use depguard linter to forbid dependencies #20721

ivanvc · 2025-09-24T23:25:00Z

Introduce the depguard linter to forbid required dependencies in the submodules. This is a clean way to achieve what we had before in the go.mod by using a replacement and pointing the forbidden package to a virtual non-existent directory "FORBIDDEN_DEPENDENCY". Now, if a package requires a forbidden dependency, the linter will fail, rather than having Go fail to compile/fetch dependencies.

Branched out from #19423.

Link to: #18409.

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

k8s-ci-robot · 2025-09-24T23:25:06Z

[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:

~~OWNERS~~ [ivanvc]

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

codecov · 2025-09-24T23:55:45Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.16%. Comparing base (92ac034) to head (5692026).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

see 37 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #20721      +/-   ##
==========================================
- Coverage   70.09%   69.16%   -0.94%     
==========================================
  Files         404      420      +16     
  Lines       34250    34817     +567     
==========================================
+ Hits        24008    24080      +72     
- Misses       8850     9343     +493     
- Partials     1392     1394       +2

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 92ac034...5692026. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ivanvc · 2025-09-25T04:31:48Z

/retest

serathius · 2025-09-25T07:32:51Z

tools/.golangci.yaml

+            - '**/api/**/*.go'
+            - '!**/server/etcdserver/api/**/*.go'
+          deny:
+            - pkg: go.etcd.io/etcd/api/v3$


$ at the end is unique. What's it purpose?

The problem is the next import:

etcd/api/etcdserverpb/raft_internal_stringer_test.go

Line 22 in 4601818

pb "go.etcd.io/etcd/api/v3/etcdserverpb"

If we remove the $, it fails:

% (cd api && 'golangci-lint' 'run' '--config' '/home/ivan/Code/Personal/etcd/etcd/tools/.golangci.yaml' '--show-stats=false' './...') FAIL: (code:1): % (cd api && 'golangci-lint' 'run' '--config' '/home/ivan/Code/Personal/etcd/etcd/tools/.golangci.yaml' '--show-stats=false' './...') ../api/etcdserverpb/raft_internal_stringer_test.go:22:2: import 'go.etcd.io/etcd/api/v3/etcdserverpb' is not allowed from list 'api' (depguard) pb "go.etcd.io/etcd/api/v3/etcdserverpb" ^

Have you tested the rules? I tried but in my 3 tries I always hit import cycle.

This is actually another way to test it. Remove the dollar sign, and run make verify-lint.

Oof, I removed by accident the trailing dollar sign 😅, and this is the error: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/etcd-io_etcd/20721/pull-etcd-verify/1971396972069785600

serathius · 2025-09-25T07:36:16Z

tools/.golangci.yaml

+            - "**/etcdctl/**/*.go"
+            - "**/etcdutl/**/*.go"
+            - "**/server/**/*.go"
+            - "!**/tests/antithesis/server/**/*.go"


There are no go files in tests/antithesis/server it's just Dockerfile for building instrumented server image.

Sounds good, I'll remove this line.

serathius · 2025-09-25T07:44:27Z

Have you tested the rules? I tried but in my 3 tries I always hit import cycle.

ahrtr · 2025-09-25T08:45:20Z

tools/.golangci.yaml

+            - pkg: go.etcd.io/etcd/pkg/v3
+            - pkg: go.etcd.io/etcd/v3
+            - pkg: go.etcd.io/tests/v3
+            - pkg: go.etcd.io/v3


Suggested change

- pkg: go.etcd.io/v3

I'm unsure of the original motivation behind these lines. I just copied and pasted (and fixed). Please refer to: https://github.com/etcd-io/etcd/pull/20721/files#diff-ba0784a950902285760f839a4ad720a381b2981e8a67147f5f43f7508e6343ed

// Bad imports are sometimes causing attempts to pull that code. // This makes the error more explicit. replace ( go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY go.etcd.io/etcd/api/v3 => ./FORBIDDEN_DEPENDENCY go.etcd.io/etcd/pkg/v3 => ./FORBIDDEN_DEPENDENCY go.etcd.io/etcd/tests/v3 => ./FORBIDDEN_DEPENDENCY go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY )

For me, it doesn't make sense to have go.etcd.io/etcd. If I understand correctly, we couldn't import that package as it would try to import etcd v1. At the bottom, we have go.etcd.io/etcd/v3.

Therefore, I removed go.etcd.io/etcd, and kept go.etcd.io/etcd/v3.

Could you please explain why you're suggesting deleting go.etcd.io/etcd/v3 as well?

Could you please explain why you're suggesting deleting go.etcd.io/etcd/v3 as well?

I am suggesting to delete go.etcd.io/v3 as we don't have this module.

Ah, right. My bad 🤦

ahrtr · 2025-09-25T08:59:03Z

tools/.golangci.yaml

+            - '**/api/**/*.go'
+            - '!**/server/etcdserver/api/**/*.go'


Can we separate configuration for different modules?

I suggest that we define separate rules for each module, following exactly the same way as we do now in each go.mod file.

Yes, we can separate the configuration per module. I initially had it that way, but then it looked very repetitive, and I thought of consolidating etcdutl, etcdtl, and server as they have the same rule.

However, what you're looking at is a depguard limitation. The path needs to be absolute (refer to OpenPeeDeeP/depguard#54).

So, breaking down these two lines:

'**/api/**/*.go': Any go file that is in a path with /api/. Note that this would match any files inside server/etcdserver/api

'!**/server/etcdserver/api/**/*.go': Deny files that have a path like /server/etcdserver/api. Emphasis on the exclamation mark.

You can see the same configuration in other projects. i.e., this is from woodpecker: https://github.com/woodpecker-ci/woodpecker/blob/388557d94aa9fe06841a207eba4ffa4ba0ae5a05/.golangci.yaml#L109-L112

However, what you're looking at is a depguard limitation. The path needs to be absolute (refer to OpenPeeDeeP/depguard#54).

do not get time to deep it for now. May take a close look sometime later.

But my immediate feeling is that it's more complicated & hard to maintain if we mix different modules.

but then it looked very repetitive

repetitive should be a minor concern. It's much much clearer and easy to maintain.

Alright, I'll change it, but be aware that we'll still need the negation ! paths due to depguard's limitation with paths.

Done, I did split etcdctl, etcdutl, and server, which were sharing the configuration.

Each time a new package is added, we need to verify the depguard settings

How does it differ from the current setup? We need to verify FORBIDDEN_DEPENDENCY clauses, it's as easy to miss. For example we haven't really added FORBIDDEN_DEPENDENCY clauses for cache package https://github.com/etcd-io/etcd/blob/main/cache/go.mod

We need to verify FORBIDDEN_DEPENDENCY clauses, it's as easy to miss.

The replace xxx => FORBIDDEN_DEPENDENCY is added in module level (in go.mod files); it should be stable. But depguard add rules in package & files level, which is less stable (as we may add new packages)

Either way (the current approach) or a linter, would require manual intervention when we create new packages. As far as I know, there's nothing that would magically work without us adding the forbidden imports.

Either is also prone to errors. I still can't understand why we're forbidding go.etcd.io/etcd. Because if I understand correctly, that package is v1.

If what we don't like about depguard is how they define the list of files. When I did my research I found that:

The revive linter has imports-blocklist. Unfortunately there's no way to specify it per module or per file path (like depguard).

There's another linter, gomodguard. It's available through golangci-lint. But it has the same issue as revive.

It works at a module level per its documentation

The linter looks for blocked modules in go.mod and searches for imported packages where the imported packages module is blocked. Indirect modules are not considered.

As it doesn't have a way to configure paths we would need to use it stand-alone, we can add a .gomodguard.yaml in each module.

Pro: This is very similar to how we currently do it

Cons:

Adds a new linter to execute, as it needs to be called outside of golangci-lint (maybe not a con, but a change we need)

This linter wouldn't be possible to call from the top-level module once we integrate the Go workspace.

I'll open a PR with the gomodguard approach.

There's another linter, gomodguard. It's available through golangci-lint.

It seems that we can't define rules for multiple modules in .golangci.yaml. It should be fine. Adding a separate .gomodguard.yaml for each module is fine.

2. This linter wouldn't be possible to call from the top-level module once we integrate the Go workspace.

Not sure I understood this. There is no golang source code in the top-level module, nor any replace xxx => FORBIDDEN_DEPENDENCY directives in https://github.com/etcd-io/etcd/blob/main/go.mod

Anyway, the PR #20748 looks good to me. To double confirm, can it clear the blocker to use go workspace as you mentioned in #20721 (comment)?

This linter wouldn't be possible to call from the top-level module once we integrate the Go workspace.

Not sure I understood this. There is no golang source code in the top-level module, nor any replace xxx => FORBIDDEN_DEPENDENCY directives in https://github.com/etcd-io/etcd/blob/main/go.mod

This is a minor inconvenience whenever we have the Go workspace. With the workspace, you can run (once):

$ golangci-lint ./...

From the top-level of the repository.

With this linter configured per module, we'll still need to do

$ cd module && gomodguard ./...

Anyway, the PR #20748 looks good to me. To double confirm, can it clear the blocker to use go workspace as you mentioned in #20721 (comment)?

Yes, that would clear the go.mod replaces blocker.

ahrtr · 2025-09-25T08:59:42Z

tools/.golangci.yaml

+            - "**/pkg/**/*.go"
+            - "!**/client/pkg/**/*.go"
+            - "!**/tools/rw-heatmaps/pkg/**/*.go"


suggest not to mixes multiple modules.
ditto as #20721 (comment).

Same here. This is not mixing multiple modules, it's targeting only the root pkg, excluding other pkg directories in the repo.

ivanvc · 2025-09-25T15:25:41Z

Have you tested the rules? I tried but in my 3 tries I always hit import cycle.

@serathius, yes. It's challenging to avoid falling into the cycle. But refer to: #19423 (comment)

Introduce the depguard linter to forbid required dependencies in the submodules. This is a clean way to achieve what we had before in the go.mod by using a replacement and pointing the forbidden package to a virtual non-existent directory "FORBIDDEN_DEPENDENCY". Now, if a package requires a forbidden dependency, the linter will fail, rather than having Go fail to compile/fetch dependencies. Signed-off-by: Ivan Valdes <[email protected]>

ivanvc · 2025-09-26T05:53:37Z

/retest

k8s-ci-robot added area/clientv3 approved area/etcdctl area/etcdutl area/tooling size/M labels Sep 24, 2025

ivanvc force-pushed the use-depguard-to-lint-forbidden-dependencies branch from 3367a8b to 2db0501 Compare September 24, 2025 23:35

serathius reviewed Sep 25, 2025

View reviewed changes

ahrtr reviewed Sep 25, 2025

View reviewed changes

ivanvc force-pushed the use-depguard-to-lint-forbidden-dependencies branch from 2db0501 to a133bbc Compare September 25, 2025 23:12

k8s-ci-robot added size/L and removed size/M labels Sep 25, 2025

ivanvc force-pushed the use-depguard-to-lint-forbidden-dependencies branch 3 times, most recently from 250683b to 1ab5edf Compare September 26, 2025 02:20

ivanvc requested review from serathius and ahrtr September 26, 2025 04:14

ivanvc force-pushed the use-depguard-to-lint-forbidden-dependencies branch from 1ab5edf to 5692026 Compare September 26, 2025 04:24

ivanvc mentioned this pull request Sep 30, 2025

Use gomodguard to forbid dependencies #20748

Merged

Use depguard linter to forbid dependencies #20721

Are you sure you want to change the base?

Use depguard linter to forbid dependencies #20721

Conversation

ivanvc commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 24, 2025

Uh oh!

codecov bot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ivanvc commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serathius commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanvc commented Sep 25, 2025

Uh oh!

ivanvc commented Sep 26, 2025

Uh oh!

Uh oh!

ivanvc commented Sep 24, 2025 •

edited

Loading

codecov bot commented Sep 24, 2025 •

edited

Loading