Use setup-envtest from (test) code

[tomasaschan]

I like being able to run the tests of a project with simply go test ./..., without requiring any local setup before that (other than a Go toolchain of the appropriate version being available). If one uses envtest to mock a Kubernetes cluster, one is expected to run some version of setup-envtest, to ensure the appropriate binaries are available; typically either like this

source <(setup-envtest use <version> -p env)
go test ./...

or e.g.

KUBEBUILDER_ASSETS=$(setup-envtest use <version> -p path) go test./...

Other than setup-envtest being written with purely this use case in mind, there's no technical reason we couldn't allow triggering this from within the test harness, rather than outside of it.

Here's a proof-of-concept implementation that runs the relevant parts of setup-envtest triggered from within the test instead, allowing

go test ./...

even for tests that use envtest.

The main reason it's not very straightforward to write this code, is that the error handling in setup-envtest is based on panic-and-recover, rather than passing error values. But it also feels a bit odd to go into the internals of the tool, rather than the tool being built to support the use case in the first place.

If I send a PR to make this a supported feature and use case for setup-envtest, what are the chances of that being accepted? :)

[alvaroaleman]

Yeah, I agree adding code to the envtest.Environment to download the binaries if needed would be useful

[fsommar]

Agreed, this would definitely be useful!

[sbueringer]

Also sounds useful to me, we just have to check what the impact on our dependencies is. setup-envtest today is a separate go module.

[tomasaschan]

@sbueringer I started porting code over in my fork, and while it's nowhere near ready to actually review yet (loads of local changes that are in a very unpolished - and uncommitted - state at the moment...) the go.mod changes are probably near done already: there's a few version bumps, and a new dependency on github.com/spf13/afero. Does that sound like a blocker to you?

[sbueringer]

Doesn't look like a showstopper to me. indirect dependency bumps look definitely fine
afero itself only has a few dependencies. Maybe it's worth looking into if we actually need afero. I'm mostly wondering if eventually we could end up in a situation that we don't find compile compatible combinations of k8s.io/* and afero transitive dependencies.

Today we're relatively safe as a vast majority of our dependencies are inherited from k/k. So whenever k/k is able to compile, CR is as well.

[alvaroaleman]

Yeah I'd suggest using the stdlibs fs abstraction rather than afero to avoid the added deps

[tomasaschan]

So far I've focused my efforts on lift-and-shifting the reusable packages from tools/setup-envtest to pkg/envtest/setup and re-implementing the workflows with proper error handling (rather than the panic-and-recover based stuff going on in the CLI code...). It's... a bit messy 😅

My plan is to

1. re-implement the workflows as exported functions of the pkg/envtest/setup package
2. make the CLI just use those instead
3. perhaps provide some convenience wrapper on an envtest.Enviroment (or just fallback code) that makes it as easy as possible to run this from tests

and I can easily add

4. replace all usage of Afero with the stdlib fs abstractions

before I submit it for proper review.

[sbueringer]

@tomasaschan Just a heads up. We have to migrate the envtest binaries from the GCS bucket to controller-tools releases. This will require some refactoring to setup-envtest to make it possible to pull from this new location. It should be mostly orthogonal to your work, but just wanted to let you know.

I'm working on this now. Probably the location change has to merge first as we probably want to backport it (we don't know at which point the old location will go away, because the GCS bucket is owned by Google and not the community)

[tomasaschan]

@sbueringer Will the code remain in the same github repo, or move to its own? I just locally added a dependency from setup-envtest to the main controller-runtime module in order to make the cli use the new implementation; will that, in the long term, be an issue?

I guess the worst case is that my changes will end up needing PRs to multiple repos. We'll cross that bridge when we get there.

[sbueringer]

Will the code remain in the same github repo, or move to its own?

Same repo, same packages. I'll just add an alternate implementation for the client stuff

[sbueringer]

Very early prototype to give you an impression: https://github.com/kubernetes-sigs/controller-runtime/compare/main...sbueringer:controller-runtime:pr-setup-envtest-ct-rel?expand=1

(I'll try to get this done ASAP, hopefully within the next week or so there should be a PR up)

[tomasaschan]

@sbueringer Ah, I see. I think that should be fine; I've moved the remote package into controller-runtime proper (so the dependency arrow is setup-envtest -> controller-runtime, not the other way around) but I did it basically without changes, so it should be easy to rebase my changes on top of yours.

The main thing to keep in mind is that there should be a good way to configure a "default" client if the user provides no parameters.

[sbueringer]

Do we still need a separate module for setup-envtest? If it doesn' introduce any additional dependency anymore I would get rid of it entirely (so folks can do go install sigs.k8s.io/controller-runtime/tools/setup-envtest@<CR-version>)

Today people have to pin to commits which is one reason why we were thinking about moving the binary into a separate repository. But If we can get rid of the additional module that all becomes easier.

[tomasaschan]

I don't particularly mind; we might as well use the same module for everything. I (and the kubebuilder init-generated Makefile, which I suppose is not insignificant in this context...) tend to use go install .../setup-envtest@latest anyway... But a big part of my reason to do this work is to not have to install it at all, which makes it matter even less how the modules are structured.

I guess one argument for keeping them separate is that it makes it possible to add some dependencies to setup-envtest that don't have to bloat the dependency tree of controller-runtime itself. For example, the main method is currently quite bloated with both arg parsing and help text etc, and it might be nice to at some point port that to use something like Cobra; but Cobra is currently not a dependency for controller-runtime, so if we keep the modules separate we can do that with smaller regard for CR itself.

[tomasaschan]

@sbueringer I opened a draft PR with my changes so far, so you can have a look at it if you'd like. Not a whole lot more work to do, but how much time I have to spend on this is a little unpredictable - I might be able to mark it ready for review later today, or not until in a couple of weeks 😅

[sbueringer]

PR is ready for review: #2811

[sbueringer]

#2811 is merged, so this work should be unblocked now

[tomasaschan]

Sweet! Then I just have to find time to work on this, too 😅

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[sbueringer]

remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[sbueringer]

/remove-lifecycle rotten

Sorry pretty low bandwith at the moment (and this won't change soon). But still on my radar to review #2810

[vincepri]

Same, following along and took a look at the PR. One pre-requisite I'd say we need to think about is that today it's possible to use setup envtest to create a container image to be used in isolated environment (without an internet connection). We need to ensure that if binaries are already available locally, we don't make any calls outside and it's still possible to setup envtest images this way; or maybe eventually we could think about offering base images as well.

[tomasaschan]

@vincepri I'm pretty sure that's a use case that is explicitly covered by tests, so that should be fine.