Fix defers and goroutines in tests (#1595)

Many of our tests use goroutines and defer statements - sometimes inconsistently or incorrectly. This PR fixes such uses, either resolving incorrect uses of asynchronous code in tests or moving to using t.Cleanup() instead of defer.

* Fix defers and goroutines in tests

Signed-off-by: Will Beason <willbeason@google.com>

* Fix logger

Signed-off-by: Will Beason <willbeason@google.com>

* Clean up environment variables

Signed-off-by: Will Beason <willbeason@google.com>

* Standardize stopping manager

Signed-off-by: Will Beason <willbeason@google.com>

* Revert changes to pkg/controller/config

There's something weird going on in this package, but I'm not going to
track it down in this PR.

Signed-off-by: Will Beason <willbeason@google.com>

* Resolve PR comments

Signed-off-by: Will Beason <willbeason@google.com>

* Fix test failure message

Signed-off-by: Will Beason <willbeason@google.com>

* Work on PR comments

Signed-off-by: Will Beason <willbeason@google.com>

* Add warning to avoid DeleteObject

Signed-off-by: Will Beason <willbeason@google.com>

* Fix merge conflicts

Signed-off-by: Will Beason <willbeason@google.com>

apis/status/v1beta1/constraintpodstatus_types_test.go

@@ -1,13 +1,13 @@
 package v1beta1
 
 import (
-	"os"
 	"strings"
 	"testing"
 
 	. "github.com/onsi/gomega"
 	"github.com/open-policy-agent/gatekeeper/pkg/fakes"
 	"github.com/open-policy-agent/gatekeeper/pkg/operations"
+	"github.com/open-policy-agent/gatekeeper/test/testutils"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -21,17 +21,7 @@ func TestNewConstraintStatusForPod(t *testing.T) {
 	podNS := "a-gk-namespace"
 	cstrName := "a-constraint"
 	cstrKind := "AConstraintKind"
-	err := os.Setenv("POD_NAMESPACE", podNS)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	defer func() {
-		err = os.Unsetenv("POD_NAMESPACE")
-		if err != nil {
-			t.Fatal(err)
-		}
-	}()
+	testutils.Setenv(t, "POD_NAMESPACE", podNS)
 
 	scheme := runtime.NewScheme()
 	g.Expect(AddToScheme(scheme)).NotTo(HaveOccurred())

apis/status/v1beta1/constrainttemplatepodstatus_types_test.go

@@ -1,12 +1,12 @@
 package v1beta1
 
 import (
-	"os"
 	"testing"
 
 	. "github.com/onsi/gomega"
 	"github.com/open-policy-agent/gatekeeper/pkg/fakes"
 	"github.com/open-policy-agent/gatekeeper/pkg/operations"
+	"github.com/open-policy-agent/gatekeeper/test/testutils"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -18,17 +18,7 @@ func TestNewConstraintTemplateStatusForPod(t *testing.T) {
 	podNS := "a-gk-namespace"
 	templateName := "a-template"
 
-	err := os.Setenv("POD_NAMESPACE", podNS)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	defer func() {
-		err = os.Unsetenv("POD_NAMESPACE")
-		if err != nil {
-			t.Fatal(err)
-		}
-	}()
+	testutils.Setenv(t, "POD_NAMESPACE", podNS)
 
 	scheme := runtime.NewScheme()
 	g.Expect(AddToScheme(scheme)).NotTo(HaveOccurred())

apis/status/v1beta1/mutatorpodstatus_types_test.go

@@ -1,13 +1,13 @@
 package v1beta1
 
 import (
-	"os"
 	"testing"
 
 	. "github.com/onsi/gomega"
 	"github.com/open-policy-agent/gatekeeper/pkg/fakes"
 	"github.com/open-policy-agent/gatekeeper/pkg/mutation/mutators/testhelpers"
 	"github.com/open-policy-agent/gatekeeper/pkg/operations"
+	"github.com/open-policy-agent/gatekeeper/test/testutils"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -18,17 +18,7 @@ func TestNewMutatorStatusForPod(t *testing.T) {
 	podName := "some-gk-pod-m"
 	podNS := "a-gk-namespace-m"
 	mutator := testhelpers.NewDummyMutator("a-mutator", "spec.value", nil)
-	err := os.Setenv("POD_NAMESPACE", podNS)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	t.Cleanup(func() {
-		err = os.Unsetenv("POD_NAMESPACE")
-		if err != nil {
-			t.Error(err)
-		}
-	})
+	testutils.Setenv(t, "POD_NAMESPACE", podNS)
 
 	scheme := runtime.NewScheme()
 	g.Expect(AddToScheme(scheme)).NotTo(HaveOccurred())

pkg/controller/config/config_controller_test.go

@@ -676,12 +676,9 @@ type testExpectations interface {
 // has been fully removed before applying our new object.
 func ensureDeleted(ctx context.Context, c client.Client, toDelete client.Object) func() error {
 	gvk := toDelete.GetObjectKind().GroupVersionKind()
-	name := toDelete.GetName()
-	namespace := toDelete.GetNamespace()
+	key := client.ObjectKeyFromObject(toDelete)
 
 	return func() error {
-		key := client.ObjectKey{Namespace: namespace, Name: name}
-
 		u := &unstructured.Unstructured{}
 		u.SetGroupVersionKind(gvk)
 
@@ -707,26 +704,26 @@ func ensureDeleted(ctx context.Context, c client.Client, toDelete client.Object)
 
 // ensureCreated attempts to create toCreate in Client c as toCreate existed when ensureCreated was called.
 func ensureCreated(ctx context.Context, c client.Client, toCreate client.Object) func() error {
+	gvk := toCreate.GetObjectKind().GroupVersionKind()
+	key := client.ObjectKeyFromObject(toCreate)
+
 	// As ensureCreated returns a closure, it is possible that the value toCreate will be modified after ensureCreated
 	// is called but before the closure is called. Creating a copy here ensures the object to be created is consistent
 	// with the way it existed when ensureCreated was called.
 	toCreateCopy := toCreate.DeepCopyObject()
 
 	return func() error {
-		instance, ok := toCreateCopy.DeepCopyObject().(client.Object)
+		instance, ok := toCreateCopy.(client.Object)
 		if !ok {
 			return fmt.Errorf("instance was %T which is not a client.Object", instance)
 		}
 
-		key := client.ObjectKey{Namespace: instance.GetNamespace(), Name: instance.GetName()}
-		gvk := instance.GetObjectKind().GroupVersionKind()
-
 		err := c.Create(ctx, instance)
 		if apierrors.IsAlreadyExists(err) {
 			return fmt.Errorf("a copy of %v %v already exists - run ensureDeleted to ensure a fresh copy exists for testing",
 				gvk, key)
 		} else if err != nil {
-			return fmt.Errorf("creating %v %v", gvk, key)
+			return fmt.Errorf("creating %v %v: %v", gvk, key, err)
 		}
 
 		return nil

pkg/controller/constrainttemplate/constrainttemplate_controller.go

@@ -49,7 +49,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
@@ -60,7 +60,7 @@ const (
 	ctrlName      = "constrainttemplate-controller"
 )
 
-var log = logf.Log.WithName("controller").WithValues("kind", "ConstraintTemplate", logging.Process, "constraint_template_controller")
+var logger = log.Log.WithName("controller").WithValues("kind", "ConstraintTemplate", logging.Process, "constraint_template_controller")
 
 var gvkConstraintTemplate = schema.GroupVersionKind{
 	Group:   v1beta1.SchemeGroupVersion.Group,
@@ -179,6 +179,7 @@ func newReconciler(mgr manager.Manager, opa *opa.Client, wm *watch.Manager, cs *
 		tracker:       tracker,
 		getPod:        getPod,
 	}
+
 	if getPod == nil {
 		reconciler.getPod = reconciler.defaultGetPod
 	}
@@ -248,7 +249,7 @@ type ReconcileConstraintTemplate struct {
 // Reconcile reads that state of the cluster for a ConstraintTemplate object and makes changes based on the state read
 // and what is in the ConstraintTemplate.Spec.
 func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
-	log := log.WithValues("template_name", request.Name)
+	logger := logger.WithValues("template_name", request.Name)
 	// Short-circuit if shutting down.
 	if r.cs != nil {
 		running := r.cs.Enter()
@@ -279,7 +280,7 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 	if containsString(finalizerName, ct.GetFinalizers()) {
 		RemoveFinalizer(ct)
 		if err := r.Update(ctx, ct); err != nil && !errors.IsNotFound(err) {
-			log.Error(err, "update error")
+			logger.Error(err, "update error")
 			return reconcile.Result{Requeue: true}, nil
 		}
 	}
@@ -291,13 +292,13 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 		ctUnversioned, err := r.opa.GetTemplate(ctx, ctRef)
 		result := reconcile.Result{}
 		if err != nil {
-			log.Info("missing constraint template in OPA cache, no deletion necessary")
+			logger.Info("missing constraint template in OPA cache, no deletion necessary")
 			logAction(ctRef, deletedAction)
 			r.metrics.registry.remove(request.NamespacedName)
 		} else {
 			result, err = r.handleDelete(ctx, ctUnversioned)
 			if err != nil {
-				log.Error(err, "deletion error")
+				logger.Error(err, "deletion error")
 				logError(request.NamespacedName.Name)
 				r.metrics.registry.add(request.NamespacedName, metrics.ErrorStatus)
 				return reconcile.Result{}, err
@@ -312,7 +313,7 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 
 	status, err := r.getOrCreatePodStatus(ctx, ct.Name)
 	if err != nil {
-		log.Info("could not get/create pod status object", "error", err)
+		logger.Info("could not get/create pod status object", "error", err)
 		return reconcile.Result{}, err
 	}
 	status.Status.TemplateUID = ct.GetUID()
@@ -321,15 +322,15 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 
 	unversionedCT := &templates.ConstraintTemplate{}
 	if err := r.scheme.Convert(ct, unversionedCT, nil); err != nil {
-		log.Error(err, "conversion error")
+		logger.Error(err, "conversion error")
 		r.metrics.registry.add(request.NamespacedName, metrics.ErrorStatus)
 		logError(request.NamespacedName.Name)
 		return reconcile.Result{}, err
 	}
 
 	unversionedProposedCRD, err := r.opa.CreateCRD(ctx, unversionedCT)
 	if err != nil {
-		log.Error(err, "CRD creation error")
+		logger.Error(err, "CRD creation error")
 		r.tracker.TryCancelTemplate(unversionedCT) // Don't track templates that failed compilation
 		r.metrics.registry.add(request.NamespacedName, metrics.ErrorStatus)
 		var createErr *v1beta1.CreateCRDError
@@ -344,7 +345,7 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 		}
 
 		if updateErr := r.Update(ctx, status); updateErr != nil {
-			log.Error(updateErr, "update error")
+			logger.Error(updateErr, "update error")
 			return reconcile.Result{Requeue: true}, nil
 		}
 		logError(request.NamespacedName.Name)
@@ -353,7 +354,7 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 
 	proposedCRD := &apiextensionsv1.CustomResourceDefinition{}
 	if err := r.scheme.Convert(unversionedProposedCRD, proposedCRD, nil); err != nil {
-		log.Error(err, "CRD conversion error")
+		logger.Error(err, "CRD conversion error")
 		r.tracker.TryCancelTemplate(unversionedCT) // Don't track templates that failed compilation
 		r.metrics.registry.add(request.NamespacedName, metrics.ErrorStatus)
 		logError(request.NamespacedName.Name)
@@ -376,15 +377,15 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec
 		currentCRD = nil
 
 	default:
-		log.Error(err, "client.Get CRD error")
+		logger.Error(err, "client.Get CRD error")
 		logError(request.NamespacedName.Name)
 		r.metrics.registry.add(request.NamespacedName, metrics.ErrorStatus)
 		return reconcile.Result{}, err
 	}
 
 	result, err := r.handleUpdate(ctx, ct, unversionedCT, proposedCRD, currentCRD, status)
 	if err != nil {
-		log.Error(err, "update error")
+		logger.Error(err, "update error")
 		logError(request.NamespacedName.Name)
 		r.metrics.registry.add(request.NamespacedName, metrics.ErrorStatus)
 	} else if !result.Requeue {
@@ -415,31 +416,31 @@ func (r *ReconcileConstraintTemplate) handleUpdate(
 	status *statusv1beta1.ConstraintTemplatePodStatus,
 ) (reconcile.Result, error) {
 	name := proposedCRD.GetName()
-	log := log.WithValues("name", ct.GetName(), "crdName", name)
+	logger := logger.WithValues("name", ct.GetName(), "crdName", name)
 
-	log.Info("loading code into OPA")
+	logger.Info("loading code into OPA")
 	beginCompile := time.Now()
 
 	// It's important that opa.AddTemplate() is called first. That way we can
 	// rely on a template's existence in OPA to know whether a watch needs
 	// to be removed
 	if _, err := r.opa.AddTemplate(ctx, unversionedCT); err != nil {
 		if err := r.metrics.reportIngestDuration(ctx, metrics.ErrorStatus, time.Since(beginCompile)); err != nil {
-			log.Error(err, "failed to report constraint template ingestion duration")
+			logger.Error(err, "failed to report constraint template ingestion duration")
 		}
 		err := r.reportErrorOnCTStatus(ctx, "ingest_error", "Could not ingest Rego", status, err)
 		r.tracker.TryCancelTemplate(unversionedCT) // Don't track templates that failed compilation
 		return reconcile.Result{}, err
 	}
 
 	if err := r.metrics.reportIngestDuration(ctx, metrics.ActiveStatus, time.Since(beginCompile)); err != nil {
-		log.Error(err, "failed to report constraint template ingestion duration")
+		logger.Error(err, "failed to report constraint template ingestion duration")
 	}
 
 	// Mark for readiness tracking
 	t := r.tracker.For(gvkConstraintTemplate)
 	t.Observe(unversionedCT)
-	log.Info("[readiness] observed ConstraintTemplate", "name", unversionedCT.GetName())
+	logger.Info("[readiness] observed ConstraintTemplate", "name", unversionedCT.GetName())
 
 	var newCRD *apiextensionsv1.CustomResourceDefinition
 	if currentCRD == nil {
@@ -454,26 +455,26 @@ func (r *ReconcileConstraintTemplate) handleUpdate(
 	}
 
 	if currentCRD == nil {
-		log.Info("creating crd")
+		logger.Info("creating crd")
 		if err := r.Create(ctx, newCRD); err != nil {
 			err := r.reportErrorOnCTStatus(ctx, "create_error", "Could not create CRD", status, err)
 			return reconcile.Result{}, err
 		}
 	} else if !reflect.DeepEqual(newCRD, currentCRD) {
-		log.Info("updating crd")
+		logger.Info("updating crd")
 		if err := r.Update(ctx, newCRD); err != nil {
 			err := r.reportErrorOnCTStatus(ctx, "update_error", "Could not update CRD", status, err)
 			return reconcile.Result{}, err
 		}
 	}
 	// This must go after CRD creation/update as otherwise AddWatch will always fail
-	log.Info("making sure constraint is in watcher registry")
+	logger.Info("making sure constraint is in watcher registry")
 	if err := r.addWatch(makeGvk(ct.Spec.CRD.Spec.Names.Kind)); err != nil {
-		log.Error(err, "error adding template to watch registry")
+		logger.Error(err, "error adding template to watch registry")
 		return reconcile.Result{}, err
 	}
 	if err := r.Update(ctx, status); err != nil {
-		log.Error(err, "update error")
+		logger.Error(err, "update error")
 		return reconcile.Result{Requeue: true}, nil
 	}
 	return reconcile.Result{}, nil
@@ -482,8 +483,8 @@ func (r *ReconcileConstraintTemplate) handleUpdate(
 func (r *ReconcileConstraintTemplate) handleDelete(
 	ctx context.Context,
 	ct *templates.ConstraintTemplate) (reconcile.Result, error) {
-	log := log.WithValues("name", ct.GetName())
-	log.Info("removing from watcher registry")
+	logger := logger.WithValues("name", ct.GetName())
+	logger.Info("removing from watcher registry")
 	gvk := makeGvk(ct.Spec.CRD.Spec.Names.Kind)
 	if err := r.removeWatch(gvk); err != nil {
 		return reconcile.Result{}, err
@@ -591,15 +592,15 @@ type namedObj interface {
 }
 
 func logAction(template namedObj, a action) {
-	log.Info(
+	logger.Info(
 		fmt.Sprintf("template was %s", string(a)),
 		logging.EventType, fmt.Sprintf("template_%s", string(a)),
 		logging.TemplateName, template.GetName(),
 	)
 }
 
 func logError(name string) {
-	log.Info(
+	logger.Info(
 		"unable to ingest template",
 		logging.EventType, "template_ingest_error",
 		logging.TemplateName, name,