Skip to content

e2e tests should be opt-in #4116

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

Open
applejag opened this issue Aug 29, 2024 · 7 comments
Open

e2e tests should be opt-in #4116

applejag opened this issue Aug 29, 2024 · 7 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it. triage/not-reproducible Indicates an issue can not be reproduced as described.

Comments

@applejag
Copy link

What broke? What's expected?

The e2e tests should not be running by default!

I did a review of a project that uses kubebuilder, and I naiively thought I could just clone the repository and do go test ./...

But then the e2e tests started installing CRDs and stuff into a Kubernetes cluster I just happened to have selected in my ~/.kube/config.

I've tracked it to this part of the template:

// Define a set of end-to-end (e2e) tests to validate the behavior of the controller.
var _ = Describe("controller", Ordered, func() {
// Before running the tests, set up the environment by creating the namespace,
// installing CRDs, and deploying the controller.
BeforeAll(func() {
By("creating manager namespace")
cmd := exec.Command("kubectl", "create", "ns", namespace)
_, err := utils.Run(cmd)
ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to create namespace")
By("installing CRDs")
cmd = exec.Command("make", "install")
_, err = utils.Run(cmd)
ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to install CRDs")
By("deploying the controller-manager")
cmd = exec.Command("make", "deploy", fmt.Sprintf("IMG=%s", projectImage))
_, err = utils.Run(cmd)
ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to deploy the controller-manager")
})

Had to spend some hours cleaning up, as the tests installed a bunch of other stuff too, like a Prometheus Operator and cert-manager.

What I suggest kubebuilder does is add a build tag to the top of these files, like so:

//go:build e2e
// +build e2e

And then the Makefile needs to be slightly adjusted:

 .PHONY: test-e2e  # Run the e2e tests against a Kind k8s instance that is spun up.
 test-e2e:
-        go test ./test/e2e/ -v -ginkgo.v
+        go test ./test/e2e/ -v -tags e2e -ginkgo.v

That way, tests don't default to modifying your production infrastructure.

Reproducing this issue

  1. New project
  2. go test ./...

KubeBuilder (CLI) Version

Version: main.version{KubeBuilderVersion:"v4.1.1", KubernetesVendor:"unknown", GitCommit:"unknown", BuildDate:"unknown", GoOs:"linux", GoArch:"amd64"}

PROJECT version

3

Plugin versions

- go.kubebuilder.io/v4

Other versions

go version go1.23.0 linux/amd64

from go.mod

k8s.io/api v0.31.0
k8s.io/apimachinery v0.31.0
k8s.io/client-go v0.31.0
sigs.k8s.io/controller-runtime v0.19.0

$ kubectl version --client
Client Version: v1.30.2
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3

Extra Labels

No response

@applejag applejag added the kind/bug Categorizes issue or PR as related to a bug. label Aug 29, 2024
@camilamacedo86
Copy link
Member

Hi @applejag,

Thank you for raising this issue. However, it's important to clarify a few key points:

Make Targets and Deployment

Any make target that deploys, undeploys, or uses kubectl will operate against the default Kubernetes context. This is the expected behavior not only for Kubebuilder /kubectl but also for other tools like Terraform, Helm, and similar tools. If you run commands against the wrong environment, it can cause significant issues—not just with the test-e2e target, but with any target, including make deploy, make undeploy, make install, and make uninstall.

Purpose of test-e2e

End-to-end (e2e) tests are often used in CI environments where a cluster is spun up specifically for testing, dependencies are installed, tests are executed, and everything is torn down afterward. The primary use case for test-e2e is in dev environments (for troubleshooting and testing) and CI jobs. You can see an example here: Example PR.

This scenario would no longer occur with the changes merged on master (next future release)

If you are using the latest master version, you won't face the issue because:

  1. If test-e2e is run without kind being up and running locally, it will fail, see:

# TODO(user): To use a different vendor for e2e tests, modify the setup under 'tests/e2e'.
# The default setup assumes Kind is pre-installed and builds/loads the Manager Docker image locally.
# Prometheus and CertManager are installed by default; skip with:
# - PROMETHEUS_INSTALL_SKIP=true
# - CERT_MANAGER_INSTALL_SKIP=true
.PHONY: test-e2e
test-e2e: ## Run the e2e tests. Expected an isolated environment using Kind.
@command -v kind >/dev/null 2>&1 || { \
echo "Kind is not installed. Please install Kind manually."; \
exit 1; \
}
@kind get clusters | grep -q 'kind' || { \
echo "No Kind cluster is running. Please start a Kind cluster before running the e2e tests."; \
exit 1; \
}
go test ./test/e2e/ -v -ginkgo.v

  1. The function to load the built image is now called before Prometheus and CertManager are installed, see:

var _ = BeforeSuite(func() {
By("building the manager(Operator) image")
cmd := exec.Command("make", "docker-build", fmt.Sprintf("IMG=%s", projectImage))
_, err := utils.Run(cmd)
ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to build the manager(Operator) image")
// TODO(user): If you want to change the e2e test vendor from Kind, ensure the image is
// built and available before running the tests. Also, remove the following block.
By("loading the manager(Operator) image on Kind")
err = utils.LoadImageToKindClusterWithName(projectImage)
ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to load the manager(Operator) image into Kind")
// Setup Prometheus and CertManager before the suite if not skipped
if !skipPrometheusInstall {
_, _ = fmt.Fprintf(GinkgoWriter, "Installing Prometheus Operator...\n")
Expect(utils.InstallPrometheusOperator()).To(Succeed(), "Failed to install Prometheus Operator")
}
if !skipCertManagerInstall {
_, _ = fmt.Fprintf(GinkgoWriter, "Installing CertManager...\n")
Expect(utils.InstallCertManager()).To(Succeed(), "Failed to install CertManager")
}
})

Why test-e2e Should Remain in the Scaffold

The test-e2e target provides a good starting point for developers, introducing common practices and demonstrating how to verify their changes in the ci. These tests have been part of Kubebuilder for nearly a year, underscoring their value to the community. Making test-e2e optional or removing it would reduce its educational value and bring complexities to maintain, to document and to use when it is a common practice that we would like to encourage.

Moreover, developers who do not wish to use test-e2e can simply choose not to run it or modify the Makefile to remove or disable it. Most important, remove the test-e2e will not bring the safeguard as described above.

Finally, we already have changes in place to address issues like the one you encountered without removing the test-e2e tests, and we welcome any suggestions for further improvements. Let’s continue improving the scaffolds, and if these improvements don’t resolve the concerns, we can revisit the idea of make optional the test-e2e tests based on community feedback.

So, if you don't mind, we are closing this issue with the fix implemented in this PR: Fix PR.

However, please feel free to open new issues if you encounter other concerns or re-open if you see fit.

Thank you!

@applejag
Copy link
Author

I did not suggest removing the e2e tests. They are very good and should remain.

But the point of this issue was that they run and modify my environments when I run a simple go test ./...

Running make test-e2e should still run the e2e tests, and my suggested changes does not change that fact.

In the PR you linked it would still modify the wrong clusters as the kubectl apply commands do not specify a --context. So with your changes it would go:

  1. Try load image into specific kind cluster
  2. Run kubectl apply on whatever cluster you happen to have currently selected

It feels like you missed what I wrote in the issue description.

I do not seem able to reopen the issue. Could you help me with that?

@camilamacedo86
Copy link
Member

camilamacedo86 commented Aug 30, 2024

HI @applejag

Following the comments inline

In the PR you linked it would still modify the wrong clusters as the kubectl apply commands do not specify a --context

Yes, we will not check the context. This is consistent with the behavior of other tools like Makefile targets, Helm commands, Terraform, etc., which do not check the context either. It is expected behavior that kubectl and Makefile targets in Kubebuilder will use the context defined by the user. So, it will not be changed for this specific target or any of them. Indeed check the context here does not solve anything. Tests can still be executed without the targets. All reasons for not to do that was described and discussed the slack. Please, feel free to read the whole thread: https://kubernetes.slack.com/archives/CAR30FCJZ/p1724837864980059

Running make test-e2e should still run the e2e tests, and my suggested changes does not change that fact.

No, the changes ensure that Prometheus or CertManager are only installed if the cluster is a KIND cluster. You definitely wouldn't use KIND in a production or shared environment, so in your scenario, the tests would fail before reaching that point.

Additionally, we are working on another PR to address further improvements regards this scenario, as checking the context in the target alone won't fully address the scenario that you faced: #4117

@applejag
Copy link
Author

Alright we're again getting off topic. Running make test-e2e may modify kind clusters from current context, I never suggested otherwise.

What I'm saying is that please add Go build tags to the e2e tests!

That way only when you run make test-e2e will the e2e test run. And not when you run go test ./...

See my code samples in the issue description.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Apr 11, 2025

Hi @applejag 👋

Thanks again for your input!

I understand your point about running:

go test ./...

In that case, yes — all tests would be executed without exception, which can have implications depending on what's present in the test tree.

However, based on the structure we follow, all tests under ./test/e2e/ are strictly end-to-end tests. We intentionally avoid placing unit or other types of tests there. Given that, adding a build tag like:

-tags e2e

might introduce unnecessary complexity without a clear benefit.

Also, as noted in this comment, the original motivation for using the e2e tag no longer seems relevant. The E2E tests currently fail before attempting to install CertManager — specifically during the image load into the Kind cluster.

Moreover, we already have logic in place to skip CertManager installation if any CertManager APIs are found on the cluster when the E2E tests are running.


✅ Summary

Given all that, this change seems to provide unnecessary ROI.

Would you be open to testing the latest scaffold output from Kubebuilder to confirm everything works as expected? If so, we could go ahead and close this issue.

If you still find a valid need for this, we’d be happy to revisit it — ideally by creating a new issue that clearly outlines:

  • What the need is
  • When it arises
  • Why it's required
  • Based on the latest scaffolded output

Let me know what you think! 😊

@camilamacedo86 camilamacedo86 added triage/not-reproducible Indicates an issue can not be reproduced as described. triage/needs-information Indicates an issue needs more information in order to work on it. labels Apr 11, 2025
@applejag
Copy link
Author

When looking at the latest scaffold from kubebuilder v4.5.1 I do not see any protection against if the user runs go test ./....

It seems to still just go ahead and try to create namespace and install CRDs to the currently active kubeconfig context.

The Makefile only checks if kind is installed and has a cluster running, but I can't even find anything in the Makefile that would check that you're targeting a kind cluster with kubectl.

The setup also does not have any sort of protection against if the user changes their kubeconfig context while the tests are running.

So with all of this I still strongly suggest that kubebuilder adds in build tags to make the user opt-in to running e2e tests as a failsafe to prevent them from accidentally overriding CRDs or whatnot in production clusters.

Additional protection I think is needed:

  1. when starting the e2e test suite, get a kubeconfig from kind using kind get kubeconfig and save that to a temporary file
  2. on every kubectl command provide the --kubeconfig flag pointing to this kubeconfig

This is "in addition of" and not "instead of" the build tag, as I think it's wrong to assume the user's kind cluster isn't used for something else.

@applejag
Copy link
Author

I tried implementing the following in our project and it works really well:

func SetKindKubeconfig() error {
	kindOptions := []string{"get", "kubeconfig", "--name", GetKindCluster()}
	cmd := exec.Command("kind", kindOptions...)
	kubeconfig, err := Run(cmd)
	if err != nil {
		return err
	}
	file, err := os.CreateTemp("", "kubeconfig-*")
	if err != nil {
		return err
	}
	defer func() {
		if err := file.Close(); err != nil {
			warnError(err)
		}
	}()
	if _, err := file.WriteString(kubeconfig); err != nil {
		return err
	}
	path := file.Name()
	if err := os.Setenv("KUBECONFIG", path); err != nil {
		return err
	}
	_, _ = fmt.Fprintf(GinkgoWriter, "export KUBECONFIG=%q\n", path)
	return nil
}

This actually solved a lot of my pain points that I had with kubebuilder. When combining this with changing the default kind cluster name to something project specific, then I have a much stronger guarantee that this side project won't affect any other kind or production clusters.

Are you interested in getting such a solution upstreamed? If so then I can try make a PR on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it. triage/not-reproducible Indicates an issue can not be reproduced as described.
Projects
None yet
Development

No branches or pull requests

2 participants