add update-unwanted-dependencies.sh to track unwanted dependencies

[pacoxu]

What type of PR is this?

/kind feature
/area code-organization
/sig architecture

What this PR does / why we need it:

Verify: Run ./hack/lint-dependencies.sh

• check forbidden_repos, exit if has, the user should update it in unwanted-dependencies.json
• check unwanted-dependencies, exit if not has, and the user should update it in unwanted-dependencies.json

Test results:

➜  kubernetes git:(track-unwanted-dependencies-1) hack/lint-dependencies.sh
Check all unwanted dependencies and update its status.
2021/09/18 13:32:05 The following dependencies **are not allowed:**
github.com/hashicorp/golang-lru
2021/09/18 13:32:05 !!! Please update status in ./hack/unwanted-dependencies.json
exit status 1

➜  kubernetes git:(track-unwanted-dependencies-1) ✗ hack/lint-dependencies.sh
Check all unwanted dependencies and update its status.
2021/09/18 13:33:36 The following dependencies **are removed:**
k8s.io/klog
2021/09/18 13:33:36 !!! Please update status in ./hack/unwanted-dependencies.json
exit status 1

➜  kubernetes git:(track-unwanted-dependencies-1) ✗ hack/lint-dependencies.sh
Check all unwanted dependencies and update its status.
2021/09/18 13:33:19 The following dependencies are removed:
k8s.io/klog
2021/09/18 13:33:19 The following dependencies are not allowed:
k8s.io/klog/v2
2021/09/18 13:33:19 !!! Please update status in ./hack/unwanted-dependencies.json
exit status 1

➜  kubernetes git:(track-unwanted-dependencies-1) hack/lint-dependencies.sh
Check all unwanted dependencies and update its status.
All pinned versions of checked dependencies match their preferred version.

Which issue(s) this PR fixes:

Fixes #102145

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

/cc @liggitt @gautierdelorme @dims

[k8s-ci-robot]

@pacoxu: GitHub didn't allow me to request PR reviews from the following users: gautierdelorme.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind feature
/area code-organization
/sig architecture

What this PR does / why we need it:

Update: Run ./hack/update-unwanted-dependencies.sh

• Step 1: check forbidden_repos/not-allowed-dependencies, exit if has
• Step 2: check unwanted_dependencies_file if some dependencies are removed, if removed, add it to .not_allowed_dependencies

Verify: Run ./hack/lint-dependencies.sh

• Check forbidden_repos/not-allowed-dependencies, exit if has

Which issue(s) this PR fixes:

Fixes #102145

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

/cc @liggitt @gautierdelorme @dims

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.

[k8s-ci-robot]

@pacoxu: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[pacoxu]

A quick discussion with Gautier on slack, some explanation on the PR for track

My initial idea:

For forbidden repo, that’s a choice to adding them in the forbidden_repos
'''
/#List of dependencies we need to avoid dragging back into kubernetes/kubernetes
forbidden_repos=(
"k8s.io/klog" # we have switched to klog v2, so avoid klog v1
)
'''
One choice: Keep the list here.
Another choice: Or add the hack/.not_allowed_dependencies file and read the file here.

@gautierdelorme

to me hack/.not_allowed_dependencies seems more manageable but I don't really mind
telling go add the module to hack/.not_allowed_dependencies seems simpler than go edit hack/lint-dependencies.sh and add it to forbidden_repos

[pacoxu]

@gautierdelorme Updated. Thanks for your comments.

[BenTheElder]

can we perhaps document why these are forbidden / unwanted?

maybe we can have # comment lines, or at least leave some notes in this PR (for future PRs changing these presumably I can track the git blame to the PR and see the discussion there)

[pacoxu]

@gautierdelorme I updated the PR. Thanks for you comments.

[pacoxu]

can we perhaps document why these are forbidden / unwanted?

maybe we can have # comment lines, or at least leave some notes in this PR (for future PRs changing these presumably I can track the git blame to the PR and see the discussion there)

Good idea. We may choose a comment style or data structure and continue.

# we have switched to klog v2, so avoid klog v1
"k8s.io/klog" 

or

  "k8s.io/klog"  # we have switched to klog v2, so avoid klog v1

or

{
  "k8s.io/klog": "we have switched to klog v2, so avoid klog v1"
}

Currently, only github.com/go-kit/kit is unwanted and a forbidden list like

• github.com/go-openapi/analysis
• github.com/go-openapi/spec
• github.com/go-openapi/strfmt
• github.com/go-openapi/validate
• github.com/influxdata/influxdb1-client
• go.mongodb.org/mongo-driver
• k8s.io/klog

[liggitt]

agree on having a comment/reason

I wonder if structured input and writing the check in a real language like go would be preferable (for example, see cmd/importverifier/importverifier.go and the associated .import-restriction input files)

[liggitt]

inputs could be the file containing unwanted/forbidden inputs, and a file containing the output of go mod graph

the tool could then evaluate the go mod graph output against the unwanted/forbidden inputs

that structure would allow easy unit testing of hypothetical unwanted/forbidden config vs hypothetical go mod graph content

[BenTheElder]

we might also want to consider making it possible to run some of these dep tools we're writing as one program that can invoke go mod graph once, IIRC it's pretty slow to do this on a repo this size.

[liggitt]

we're building up quite a list of unwanted deps in #102145 ... would be good to get an initial version of this in.

[pacoxu]

Update the unwanted list

• github.com/go-bindata/go-bindata (Replace go-bindata with //go:embed Replace go-bindata with //go:embed #99829) unwanted
• github.com/hashicorp/golang-lru not allowed

[liggitt]

Feedback from the go team and behavior change in go1.17 indicate go mod graph isn't the right tool to determine direct/indirect dependencies (#104461). However, since go list does not return results for all build tags, there's not a clear alternative at the moment.

[dims]

Ugh, so how can we make progress here? :(

[liggitt]

hoisting from slack:

we can use go mod graph to get the full set of dependencies
which is what the tool is using now
but to resolve the "TODOs" distinguishing direct/indirect, it won't help us
so it would still be useful for keeping an unwanted dependency we've eradicated from creeping back in
but it won't be useful for keeping one that is currently an indirect dep from creeping in as a direct dep (edited) 

[dims]

/assign

[dims]

i am happy with this.

/approve

will wait for @pacoxu to peek at the last comments here and on slack before we ship this.

[pacoxu]

Updated. The change I made

1. Add github.com/json-iterator/go as unwanted.
2. As we cannot distinguishing direct/indirect, use simple reference in the struct. And add TODO to distinguish them in the future.
3. rebase using latest master branch.

[dims]

/assign @thockin @liggitt

(for /cmd approval)

[liggitt]

this is the current output if a module adds a bad dependency on klog:

!!! [0917 10:35:56] The following have transitive dependencies on k8s.io/klog, which is not allowed:
k8s.io/sample-apiserver@v0.0.0 k8s.io/klog@v1.0.0

with this PR, the output changes to:

Status in ./hack/unwanted-dependencies.json:
 {[] [github.com/go-kit/kit github.com/hashicorp/golang-lru github.com/hashicorp/hcl github.com/json-iterator/go github.com/spf13/viper]}
Status detected:
 {[] [github.com/go-kit/kit github.com/hashicorp/golang-lru github.com/hashicorp/hcl github.com/json-iterator/go github.com/spf13/viper k8s.io/klog]}
2021/09/17 10:34:21 !!! Please update ./hack/unwanted-dependencies.json

A couple observations:

1. it's hard to tell which module has the bad dependency
2. when there's a delta in the unwanted status that reintroduces an unwanted dependency, we don't actually want them to update hack/unwanted-dependencies.json to allow it (which is what the message encourages them to do)

I think this needs to be clearer when there's a mismatch in the status. For anything currently in the file that should be removed from status, that's great and we can just tell them to drop the thing from status. For anything not in the file that is newly referenced, we should print out the modules depending on the unwanted dependency so they can fix those.

[pacoxu]

I just try to refactor the output. And I will update the func diff which is not that graceful tomorrow.

➜  kubernetes git:(track-unwanted-dependencies-1) hack/lint-dependencies.sh
Check all unwanted dependencies and update its status.
All pinned versions of checked dependencies match their preferred version.

For a removed dependency k8s.io/klog which is not removed from the status. (Should remove it)

➜  kubernetes git:(track-unwanted-dependencies-1) ✗ hack/lint-dependencies.sh
Check all unwanted dependencies and update its status.
2021/09/18 13:33:36 The following dependencies **are removed:**
k8s.io/klog
2021/09/18 13:33:36 !!! Please update status in ./hack/unwanted-dependencies.json
exit status 1

A dependency k8s.io/klog/v2 which is not allowed, is still in use.

➜  kubernetes git:(track-unwanted-dependencies-1) hack/lint-dependencies.sh
Check all unwanted dependencies and update its status.
2021/09/18 13:32:05 The following dependencies **are not allowed:**
github.com/hashicorp/golang-lru
2021/09/18 13:32:05 !!! Please update status in ./hack/unwanted-dependencies.json
exit status 1

• k8s.io/klog should be removed.
• k8s.io/klog/v2 is still in use

➜  kubernetes git:(track-unwanted-dependencies-1) ✗ hack/lint-dependencies.sh
Check all unwanted dependencies and update its status.
2021/09/18 13:33:19 The following dependencies are removed:
k8s.io/klog
2021/09/18 13:33:19 The following dependencies are not allowed:
k8s.io/klog/v2
2021/09/18 13:33:19 !!! Please update status in ./hack/unwanted-dependencies.json
exit status 1

[liggitt]

This looks closer, but I think we still need to be more specific about which modules are referencing the unwanted dependencies, and what action should be taken. Something like the following gives different guidance depending on which situation the user encounters:

diff --git a/cmd/dependencyverifier/dependencyverifier.go b/cmd/dependencyverifier/dependencyverifier.go
index 26f56952996..ecf367945a3 100644
--- a/cmd/dependencyverifier/dependencyverifier.go
+++ b/cmd/dependencyverifier/dependencyverifier.go
@@ -187,21 +187,22 @@ func main() {
 	// Compare unwanted list from unwanted-dependencies.json with current status from `go mod graph`
 	removedReferences, unwantedReferences := difference(configFromFile.Status.References, config.Status.References)
 	if len(removedReferences) > 0 {
-		log.Println("The following dependencies are removed:")
+		log.Println("Good news! The following unwanted dependencies are no longer referenced:")
 		for reference := range removedReferences {
-			fmt.Println(reference)
+			log.Printf("   %s", reference)
 		}
+		log.Printf("!!! Remove the unwanted dependencies from status in %s to ensure they don't get reintroduced", dependenciesJSONPath)
 		needUpdate = true
 	}
 	if len(unwantedReferences) > 0 {
-		log.Println("The following dependencies are not allowed:")
+		log.Printf("The following unwanted dependencies marked in %s are referenced:", dependenciesJSONPath)
 		for reference := range unwantedReferences {
-			fmt.Println(reference)
+			log.Printf("   %s (referenced by %s)", reference, strings.Join(modeGraph[reference], ", "))
 		}
+		log.Printf("!!! Avoid updating referencing modules to versions that reintroduce use of unwanted dependencies\n")
 		needUpdate = true
 	}
 	if needUpdate {
-		log.Println("!!! Please update status in ./hack/unwanted-dependencies.json")
 		os.Exit(1)
 	}
 }

[pacoxu]

Updated, new output is like below.

2021/09/20 22:15:11 Good news! The following unwanted dependencies are no longer referenced:
2021/09/20 22:15:11    k8s.io/klog
2021/09/20 22:15:11 !!! Remove the unwanted dependencies from status in /Users/pacoxu/git/gopath/src/k8s.io/kubernetes/hack/unwanted-dependencies.json to ensure they don't get reintroduced

2021/09/20 22:15:11 The following unwanted dependencies marked in /Users/pacoxu/git/gopath/src/k8s.io/kubernetes/hack/unwanted-dependencies.json are referenced:
2021/09/20 22:15:11    github.com/go-kit/kit (referenced by github.com/grpc-ecosystem/go-grpc-middleware)
2021/09/20 22:15:11 !!! Avoid updating referencing modules to versions that reintroduce use of unwanted dependencies
exit status 1

[liggitt]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, pacoxu

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:

• cmd/OWNERS [liggitt]
• hack/OWNERS [dims,liggitt]

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