Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace k8s client-go mocks with fakes #2375

Merged
merged 3 commits into from
Jun 24, 2021
Merged

Conversation

bbarnes52-zz
Copy link
Contributor

@bbarnes52-zz bbarnes52-zz commented Jun 23, 2021

This revision removes the generated mocks for the k8s client-go package from the spire repo. Instead, the various fake packages from the client-go repository (the canonical method for stubbing out k8s client calls in unit tests) are used.

This has a few benefits:

  1. The tests themselves are cleaner. From https://testing.googleblog.com/2013/05/testing-on-toilet-dont-overuse-mocks.html, "Some signs that you're overusing mocks are if you're mocking out more than one or two classes, or if one of your mocks specifies how more than one or two methods should behave."

  2. The k8s client-go repository does a poor job adhering to Go module management best practices, backwards incompatible changes across minor versions are commonplace. See client-go needs backwards compatibility test kubernetes/client-go#234. By generating mocks for entire packages in the client-go repo, any backwards incompatible changes to the interface of these packages cause the spire repo to be incompatible with the newer version of the k8s.io/client_go module (even if spire does not actually use the methods in question). This makes it challenging to depend on the spire repo, as if any other modules in the dependency chain use the k8s client and request a minimum version of k8s.io/client-go higher than what spire requests (currently 0.18.2) it is not uncommon for builds to fail due to backwards incompatible changes that affect the generated mocks. For example, here is the error message when we attempted to upgrade the version of k8s.io/client-go in our repo:

go: finding module for package k8s.io/client-go/kubernetes/typed/settings/v1alpha1
go: finding module for package k8s.io/client-go/kubernetes/typed/auditregistration/v1alpha1
go: downloading k8s.io/mount-utils v0.20.1
go: downloading k8s.io/component-helpers v0.20.1
go: downloading k8s.io/controller-manager v0.20.1
code.uber.internal imports
	github.com/spiffe/spire/cmd/spire-server imports
	github.com/spiffe/spire/cmd/spire-server/cli imports
	github.com/spiffe/spire/cmd/spire-server/cli/run imports
	github.com/spiffe/spire/pkg/server imports
	github.com/spiffe/spire/pkg/server/catalog imports
	github.com/spiffe/spire/pkg/server/plugin/nodeattestor/k8s/psat imports
	github.com/spiffe/spire/pkg/common/plugin/k8s/apiserver tested by
	github.com/spiffe/spire/pkg/common/plugin/k8s/apiserver.test imports
	github.com/spiffe/spire/test/mock/common/plugin/k8s/clientset imports
	k8s.io/client-go/kubernetes/typed/auditregistration/v1alpha1: module k8s.io/client-go@latest found (v1.5.2, replaced by k8s.io/[email protected]), but does not contain package k8s.io/client-go/kubernetes/typed/auditregistration/v1alpha1
code.uber.internal imports
	github.com/spiffe/spire/cmd/spire-server imports
	github.com/spiffe/spire/cmd/spire-server/cli imports
	github.com/spiffe/spire/cmd/spire-server/cli/run imports
	github.com/spiffe/spire/pkg/server imports
	github.com/spiffe/spire/pkg/server/catalog imports
	github.com/spiffe/spire/pkg/server/plugin/nodeattestor/k8s/psat imports
	github.com/spiffe/spire/pkg/common/plugin/k8s/apiserver tested by
	github.com/spiffe/spire/pkg/common/plugin/k8s/apiserver.test imports
	github.com/spiffe/spire/test/mock/common/plugin/k8s/clientset imports
k8s.io/client-go/kubernetes/typed/settings/v1alpha1: module k8s.io/client-go@latest found (v1.5.2, replaced by k8s.io/[email protected]), but does not contain package k8s.io/client-go/kubernetes/typed/settings/v1alpha1

While the root cause here is k8s poor Go module management, we can effectively mitigate this problem by limiting our exposure to the client-go API surface to just the methods we actually use.

Signed-off-by: Brian Barnes [email protected]

@azdagron
Copy link
Member

Huzzah! Any change that removes mock use in favor of fakes has my support and adoration :) We're a little heads down with preparations for 1.0, but I'll review this as soon as I get a minute.

rturner3
rturner3 previously approved these changes Jun 23, 2021
Copy link
Collaborator

@rturner3 rturner3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -117,7 +114,7 @@ oIrPuyjOmscrC627wX3LGUHwPKtNArBT8lKFfda1B1BqAk0q1/ui/A==
)

const (
testToken = "TEST-TOKEN"
_testToken = "TEST-TOKEN"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: SPIRE doesn't use this convention for unexported package-level vars

Copy link
Contributor

@zmt zmt Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should reference a specific style guide in the CONTRIBUTING.md. This convention comes directly from here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A style guide has been something we've talked about and wanted to introduce before, it has just never been prioritized. I've opened an issue to track this (#2377). Thanks for bringing it up.

azdagron
azdagron previously approved these changes Jun 23, 2021
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\o/, thanks @bbarnes52

Signed-off-by: Brian Barnes <[email protected]>
@zmt
Copy link
Contributor

zmt commented Jun 23, 2021

Great analysis and thanks for the fix. I'm looking forward to getting this on an upgrade soon. Minimizing and untangling transitive dependency conflicts w/ k8s.io components is a big win.

@rturner3 rturner3 merged commit 9067778 into spiffe:main Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants