WIP: Drop go.mod from sub-modules

[thockin]

Opinions or thoughts requested.

I am working on the Go workspaces effort this week, and hit a snag.

We have a few go.mod which are supposed to insulate their parents from deps. Specifically:

./hack/tools/go.mod
./staging/src/k8s.io/kms/internal/plugins/_mock/go.mod
./staging/src/k8s.io/code-generator/examples/go.mod

Tools I get, and I handle it. I don't add it to the workspace. Fortunately, no real code in there.

KMS README says it is named _mock, so that tooling ignores it, but it's REALLY unclear to me why we want that? At the time of this writing it doesn't actually build, so clearly it's not used? @nilekhc @enj ?

The code-gen examples are more challenging. If I include them from the workspace, then it wants to update vendor to cover their deps. If I exclude them, then running all the codegen tools needs to be done twice - once for tool ./... and once for (cd staging/src/k8s.io/code-generator/examples; tool ./...)

Simpler, maybe is just to drop those go.mod files? This pulls a few new deps into k/k (github.com/ThalesIgnite/crypto11,
github.com/miekg/pkcs11, github.com/thales-e-security/pool) but several more into k8s.io/code-generator (most of which were already managed in k/k).

Let's see what folks think (and what CI thinks).

@liggitt

/kind cleanup

NONE

[k8s-ci-robot]

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.

[thockin]

Alternative is to add those modules to the workspace. Much smaller diff (just adds those 3 packages to vendor/) but I wonder if this form is still correct? At least for mock, it caught a real bug.

[enj]

KMS README says it is named _mock, so that tooling ignores it, but it's REALLY unclear to me why we want that? At the time of this writing it doesn't actually build, so clearly it's not used? @nilekhc @enj ?

It is used in the KMS e2e, and builds just fine via its Dockerfile. pull-kubernetes-e2e-kind-kms is failing on this PR.

I wanted it kept separate so that we don't pull in github.com/ThalesIgnite/crypto11 and github.com/miekg/pkcs11 as those deps have a large Cgo based surface area and the potential to break static linking.

[enj]

At least for mock, it caught a real bug.

What bug are you referring to?

[thockin]

Bug: fbafdc9

It's POSSIBLE to special-case this module in workspaces, I am just trying to prove that special cases are REALLY REALLY worth it. If I don't special-case this, then those packages do get vendor'ed. If they get vendor'ed anyway, less tricky is better.

Or this mock is special enough to warrant a special-case? I am skeptical.

[thockin]

I am super confused on the "bug". Without this PR it references "k8s.io/kms/plugins/mock/pkcs11" which does not exist, but it DOES seem to work?

Edit: it's because the module (go.mod) calls itself "k8s.io/kms" despite being "k8s.io/kms/internal/plugins/_mock" in the source tree. That's not confusing at all.

What's the point of the "internal" directory in this case, then?

[enj]

Edit: it's because the module (go.mod) calls itself "k8s.io/kms" despite being "k8s.io/kms/internal/plugins/_mock" in the source tree. That's not confusing at all.

We can change the module name to make it less confusing.

What's the point of the "internal" directory in this case, then?

It used to have a lot more code it in it outside of the _mock directory that we didn't want to be part of the k8s.io/kms API surface area (since we expect all KMS plugins to import that repo). We only need the _ prefixed directory now.

[enj]

Or this mock is special enough to warrant a special-case? I am skeptical.

It is special enough IMO because polluting the dep tree of the project and many staging repos with PKCS11 Cgo junk is much, much worse. The current approach keeps everything in k/k for easy updating and doesn't impact the rest of the project with unnecessary deps.

[thockin]

OK. This is why I looped you in. I'll change this PR to a simple move. In the workspaces PR we will have to decide if the special carve-out is warranted or not.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin

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:

• hack/OWNERS [thockin]
• staging/src/k8s.io/code-generator/OWNERS [thockin]
• staging/src/k8s.io/kms/OWNERS [thockin]
• test/e2e/testing-manifests/auth/encrypt/OWNERS [thockin]

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

[k8s-ci-robot]

@thockin: 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-kubernetes-verify	3aafadf	link	true	/test pull-kubernetes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[enj]

Suggested change

	module k8s.io/kms/plugins/_mock
	module k8s.io/kms/_mock

[thockin]

/hold

I am still considering if this is correct or not.

[BenTheElder]

I wanted it kept separate so that we don't pull in github.com/ThalesIgnite/crypto11 and github.com/miekg/pkcs11 as those deps have a large Cgo based surface area and the potential to break static linking.

CGO isn't really relevant for managing the dep tree itself and it will only be built when actually building binaries with CGO enabled. We do try to avoid adding any new dependencies though just because the main tree is already so large.

We never attempt to ~go build vendor/..., so these packages will only actually get build when building the binaries that use them, which we already built with CGO.

[liggitt]

We do try to avoid adding any new dependencies though just because the main tree is already so large.

+100 to not adding new deps to k/k, especially ones we don't really need for shipped binaries, and especially especially not new crypto / security deps

[thockin]

@liggitt

+100 to not adding new deps to k/k

Are you OK with them getting vendor'ed butnot added to the root go.mod (in workspaces)?

See 003a2a7

[liggitt]

Are you OK with them getting vendor'ed butnot added to the root go.mod (in workspaces)?

I ... don't really understand how that would work ... vendor can only contain a single copy of each named dep, so it must be doing version resolution across everything that informs the versions selected to go into vendor, right?

[thockin]

In a workspace, the vendor directory spans all the modules

[liggitt]

but version resolution doesn't? that's... weird

[thockin]

I'm not sure I follow - vendor tracks the version that we have pinned, right?

[liggitt]

I'm not sure I follow - vendor tracks the version that we have pinned, right?

yes... but the pinning is expressed in the k/k / staging go.mod files

If vendor is now being constructed from a set of modules which are not connected in the same graph, versions of dependencies can disagree between those. In that case, which version is vendored? I want to make sure we can't end up in a scenario where k/k and staging go.mod references foo@1.0 and vendor contains code from foo@1.1.

[BenTheElder]

but version resolution doesn't? that's... weird

Initially workspaces didn't support vendor (golang/go#60056), so you wouldn't have a problem with the go module cache. golang/go#60056 might have some details ... need to find a moment to reread the proposal etc.

Per https://go.dev/blog/get-familiar-with-workspaces:

Each module within a workspace is treated as a main module when resolving dependencies.

[...]

With Go workspaces, you control all your dependencies using a go.work file in the root of your workspace directory. The go.work file has use and replace directives that override the individual go.mod files, so there is no need to edit each go.mod file individually.

[k8s-ci-robot]

PR needs rebase.

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.

[thockin]

@liggitt

1. We are already in that situation (I found 1 such and fixed it in my workspaces PR). As you know well, it's all just driven by a shel script and human reviewers.

2. It seem to be (in workspaces) that the top-level vendor is the union of all consituent go.mod files, with MVS applied. In this case, if I follow it correctly, the codegen examples depends on (for example) github.com/ThalesIgnite/crypto11 (transitively), so that will get pulled up into vendor, but not the root go.mod

3. We could move most of k/k/go.mod into k/k/go.work, but then the publishing-bot gets a bit more complicated as it has to build standaolne go.mod for each repo.

[liggitt]

2. It seem to be (in workspaces) that the top-level vendor is the union of all consituent go.mod files, with MVS applied. In this case, if I follow it correctly, the codegen examples depends on (for example) github.com/ThalesIgnite/crypto11 (transitively), so that will get pulled up into vendor, but not the root go.mod

This is the crux of the issue. Our update-vendor.sh script works hard to sync the dependency versions referenced by our root and staging go.mod files so they point at the same versions of dependencies, so that what is in vendor (and what we build with) agrees with the versions in each go.mod file.

By including modules outside the root+staging set in MVS, arbitrary ~other versions of dependencies can get vendored and used in builds, without being reflected in root+staging go.mod files. That seems ~bad to me. I would do almost anything (including deleting examples) to avoid that.

[dims]

@thockin i think you don't need this any more. right? please reopen if you do.