From 885bb2b4e92373a4bb9cecda39b28f33c3e52d79 Mon Sep 17 00:00:00 2001
From: Ayush Rangwala <arangwala@vmware.com>
Date: Sun, 2 May 2021 22:32:58 +0530
Subject: [PATCH] Cleanup Webhook server setup

- Called the webhook server cleanup function

- Only ignore controlplane cleanup when using existing cluster

- Added test to check the directory exists post stop
---
 pkg/envtest/envtest_test.go | 24 ++++++++++++++++++++++--
 pkg/envtest/server.go       | 10 ++++++----
 pkg/envtest/webhook.go      |  4 ++--
 pkg/envtest/webhook_test.go |  2 +-
 pkg/webhook/server_test.go  |  2 +-
 5 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/pkg/envtest/envtest_test.go b/pkg/envtest/envtest_test.go
index 18ed4c22c8..b0eb104b85 100644
--- a/pkg/envtest/envtest_test.go
+++ b/pkg/envtest/envtest_test.go
@@ -18,16 +18,19 @@ package envtest
 
 import (
 	"context"
+	"os"
 	"path/filepath"
 	"time"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -65,8 +68,8 @@ var _ = Describe("Test", func() {
 				Name: crd.GetName(),
 			}
 			var placeholder v1beta1.CustomResourceDefinition
-			err := c.Get(context.TODO(), crdObjectKey, &placeholder)
-			if err != nil && apierrors.IsNotFound(err) {
+			if err = c.Get(context.TODO(), crdObjectKey, &placeholder); err != nil &&
+				apierrors.IsNotFound(err) {
 				// CRD doesn't need to be deleted.
 				continue
 			}
@@ -845,4 +848,21 @@ var _ = Describe("Test", func() {
 			close(done)
 		}, 30)
 	})
+
+	Describe("Stop", func() {
+		It("should cleanup webhook /tmp folder with no error when using existing cluster", func(done Done) {
+			useExistingCluster := true
+			env := &Environment{UseExistingCluster: &useExistingCluster}
+			_, err := env.Start()
+			Expect(err).NotTo(HaveOccurred())
+			Expect(env.Stop()).To(Succeed())
+
+			// check if the /tmp/envtest-serving-certs-* dir doesnt exists any more
+			_, err = os.Stat(env.WebhookInstallOptions.LocalServingCertDir)
+			Expect(err).To(HaveOccurred())
+			Expect(os.IsNotExist(err)).To(BeTrue())
+
+			close(done)
+		}, 30)
+	})
 })
diff --git a/pkg/envtest/server.go b/pkg/envtest/server.go
index 614d861fde..148b0ad405 100644
--- a/pkg/envtest/server.go
+++ b/pkg/envtest/server.go
@@ -158,13 +158,15 @@ func (te *Environment) Stop() error {
 			return err
 		}
 	}
+
+	if err := te.WebhookInstallOptions.Cleanup(); err != nil {
+		return err
+	}
+
 	if te.useExistingCluster() {
 		return nil
 	}
-	err := te.WebhookInstallOptions.Cleanup()
-	if err != nil {
-		return err
-	}
+
 	return te.ControlPlane.Stop()
 }
 
diff --git a/pkg/envtest/webhook.go b/pkg/envtest/webhook.go
index 9f96e87930..e647b8dade 100644
--- a/pkg/envtest/webhook.go
+++ b/pkg/envtest/webhook.go
@@ -165,12 +165,12 @@ func (o *WebhookInstallOptions) PrepWithoutInstalling() error {
 	if err != nil {
 		return err
 	}
+
 	if err := parseWebhook(o); err != nil {
 		return err
 	}
 
-	err = o.ModifyWebhookDefinitions(hookCA)
-	if err != nil {
+	if err = o.ModifyWebhookDefinitions(hookCA); err != nil {
 		return err
 	}
 
diff --git a/pkg/envtest/webhook_test.go b/pkg/envtest/webhook_test.go
index 1064190745..40f7a98221 100644
--- a/pkg/envtest/webhook_test.go
+++ b/pkg/envtest/webhook_test.go
@@ -100,6 +100,6 @@ var _ = Describe("Test", func() {
 type rejectingValidator struct {
 }
 
-func (v *rejectingValidator) Handle(ctx context.Context, req admission.Request) admission.Response {
+func (v *rejectingValidator) Handle(_ context.Context, _ admission.Request) admission.Response {
 	return admission.Denied(fmt.Sprint("Always denied"))
 }
diff --git a/pkg/webhook/server_test.go b/pkg/webhook/server_test.go
index 1426b4dec9..ca7da4ce49 100644
--- a/pkg/webhook/server_test.go
+++ b/pkg/webhook/server_test.go
@@ -43,7 +43,7 @@ var _ = Describe("Webhook Server", func() {
 
 	BeforeEach(func() {
 		ctx, ctxCancel = context.WithCancel(context.Background())
-		// closed in indivual tests differently
+		// closed in individual tests differently
 
 		servingOpts = envtest.WebhookInstallOptions{}
 		Expect(servingOpts.PrepWithoutInstalling()).To(Succeed())