-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Comments
Hi @applejag, Thank you for raising this issue. However, it's important to clarify a few key points: Make Targets and DeploymentAny Purpose of
|
# 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 |
- The function to load the built image is now called before Prometheus and CertManager are installed, see:
kubebuilder/testdata/project-v4/test/e2e/e2e_suite_test.go
Lines 55 to 76 in e93492b
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!
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 Running In the PR you linked it would still modify the wrong clusters as the
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? |
HI @applejag Following the comments inline
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
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 |
Alright we're again getting off topic. Running What I'm saying is that please add Go build tags to the e2e tests! That way only when you run See my code samples in the issue description. |
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 -tags e2e might introduce unnecessary complexity without a clear benefit. Also, as noted in this comment, the original motivation for using the 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. ✅ SummaryGiven 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:
Let me know what you think! 😊 |
When looking at the latest scaffold from kubebuilder v4.5.1 I do not see any protection against if the user runs 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 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:
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. |
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 |
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:
kubebuilder/pkg/plugins/golang/v4/scaffolds/internal/templates/test/e2e/test.go
Lines 59 to 78 in e93492b
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:
And then the Makefile needs to be slightly adjusted:
That way, tests don't default to modifying your production infrastructure.
Reproducing this issue
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
The text was updated successfully, but these errors were encountered: