test: add retry to async podMutator.BackfillPermissions integration test (#303)

Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Co-authored-by: Skye Gill <[email protected]>
Co-authored-by: Kavindu Dodanduwa <[email protected]>

Author: 3 people

main.go

@@ -219,7 +219,11 @@ func main() {
 
 	setupLog.Info("restoring flagd-kubernetes-sync cluster role binding subjects from current cluster state")
 	// backfill can be handled asynchronously, so we do not need to block via the channel
-	go podMutator.BackfillPermissions(ctx, make(chan struct{}, 1))
+	go func() {
+		if err := podMutator.BackfillPermissions(ctx); err != nil {
+			setupLog.Error(err, "problem restoring flagd-kubernetes-sync cluster role binding ")
+		}
+	}()
 
 	if err := <-errChan; err != nil {
 		setupLog.Error(err, "problem running manager")

webhooks/pod_webhook.go

@@ -49,19 +49,14 @@ type PodMutator struct {
 }
 
 // BackfillPermissions recovers the state of the flagd-kubernetes-sync role binding in the event of upgrade
-func (m *PodMutator) BackfillPermissions(ctx context.Context, backfillComplete chan struct{}) {
-	defer func() {
-		backfillComplete <- struct{}{}
-	}()
-
+func (m *PodMutator) BackfillPermissions(ctx context.Context) error {
 	for i := 0; i < 5; i++ {
 		// fetch all pods with the "openfeature.dev/enabled" annotation set to "true"
 		podList := &corev1.PodList{}
 		err := m.Client.List(ctx, podList, client.MatchingFields{OpenFeatureEnabledAnnotationPath: "true"})
 		if err != nil {
 			if !goErr.Is(err, &cache.ErrCacheNotStarted{}) {
-				m.Log.Error(err, "unable to list annotated pods", "webhook", OpenFeatureEnabledAnnotationPath)
-				return
+				return err
 			}
 			time.Sleep(1 * time.Second)
 			continue
@@ -79,14 +74,9 @@ func (m *PodMutator) BackfillPermissions(ctx context.Context, backfillComplete c
 				)
 			}
 		}
-		return
-	}
-	err := goErr.New("unable to backfill permissions for the flagd-kubernetes-sync role binding: timeout")
-	m.Log.Error(
-		err,
-		"webhook",
-		OpenFeatureEnabledAnnotationPath,
-	)
+		return nil
+	}
+	return goErr.New("unable to backfill permissions for the flagd-kubernetes-sync role binding: timeout")
 }
 
 // Handle injects the flagd sidecar (if the prerequisites are all met)

webhooks/pod_webhook_test.go

@@ -1,7 +1,10 @@
 package webhooks
 
 import (
+	"errors"
 	"fmt"
+	"reflect"
+	"time"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -164,24 +167,78 @@ func podMutationWebhookCleanup() {
 var _ = Describe("pod mutation webhook", func() {
 
 	It("should backfill role binding subjects when annotated pods already exist in the cluster", func() {
-		pod1 := getPod(existingPod1Name)
-		pod2 := getPod(existingPod2Name)
-		// Pod 1 and 2 must not have been mutated by the webhook (we want the rolebinding to be updated via BackfillPermissions)
-		Expect(len(pod1.Spec.Containers)).To(Equal(1))
-		Expect(len(pod2.Spec.Containers)).To(Equal(1))
-		rb := getRoleBinding(clusterRoleBindingName)
-		Expect(rb.Subjects).To(ContainElement(v1.Subject{
-			Kind:      "ServiceAccount",
-			APIGroup:  "",
-			Name:      existingPod1ServiceAccountName,
-			Namespace: mutatePodNamespace,
-		}))
-		Expect(rb.Subjects).To(ContainElement(v1.Subject{
+		// this integration test confirms the proper execution of the  podMutator.BackfillPermissions method
+		// this method is responsible for backfilling the subjects of the open-feature-operator-flagd-kubernetes-sync
+		// cluster role binding, for previously existing pods on startup
+		// a retry is required on this test as the backfilling occurs asynchronously
+		var finalError error
+		for i := 0; i < 3; i++ {
+			pod1 := getPod(existingPod1Name)
+			pod2 := getPod(existingPod2Name)
+			// Pod 1 and 2 must not have been mutated by the webhook (we want the rolebinding to be updated via BackfillPermissions)
+
+			if len(pod1.Spec.Containers) != 1 {
+				finalError = errors.New("pod1 has had a container injected, it should not be mutated by the webhook")
+				time.Sleep(1 * time.Second)
+				continue
+			}
+			if len(pod2.Spec.Containers) != 1 {
+				finalError = errors.New("pod2 has had a container injected, it should not be mutated by the webhook")
+				time.Sleep(1 * time.Second)
+				continue
+			}
+
+			rb := getRoleBinding(clusterRoleBindingName)
+
+			unexpectedServiceAccount := ""
+			for _, subject := range rb.Subjects {
+				if !reflect.DeepEqual(subject, v1.Subject{
+					Kind:      "ServiceAccount",
+					APIGroup:  "",
+					Name:      existingPod1ServiceAccountName,
+					Namespace: mutatePodNamespace,
+				}) &&
+					!reflect.DeepEqual(subject, v1.Subject{
+						Kind:      "ServiceAccount",
+						APIGroup:  "",
+						Name:      existingPod2ServiceAccountName,
+						Namespace: mutatePodNamespace,
+					}) {
+					unexpectedServiceAccount = subject.Name
+				}
+			}
+			if unexpectedServiceAccount != "" {
+				finalError = fmt.Errorf("unexpected subject found in role binding, name: %s", unexpectedServiceAccount)
+				time.Sleep(1 * time.Second)
+				continue
+			}
+			finalError = nil
+			break
+		}
+		Expect(finalError).ShouldNot(HaveOccurred())
+	})
+
+	It("should update cluster role binding's subjects", func() {
+		pod := testPod(defaultPodName, defaultPodServiceAccountName, map[string]string{
+			"openfeature.dev":                          "enabled",
+			"openfeature.dev/featureflagconfiguration": fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagConfigurationName),
+		})
+		err := k8sClient.Create(testCtx, pod)
+		Expect(err).ShouldNot(HaveOccurred())
+
+		crb := &v1.ClusterRoleBinding{}
+		err = k8sClient.Get(testCtx, client.ObjectKey{Name: clusterRoleBindingName}, crb)
+		Expect(err).ShouldNot(HaveOccurred())
+
+		Expect(len(crb.Subjects)).Should(Equal(3))
+		Expect(crb.Subjects).To(ContainElement(v1.Subject{
 			Kind:      "ServiceAccount",
 			APIGroup:  "",
-			Name:      existingPod2ServiceAccountName,
+			Name:      defaultPodServiceAccountName,
 			Namespace: mutatePodNamespace,
 		}))
+
+		podMutationWebhookCleanup()
 	})
 
 	It("should create flagd sidecar", func() {
@@ -264,29 +321,6 @@ var _ = Describe("pod mutation webhook", func() {
 		Expect(err).Should(HaveOccurred())
 	})
 
-	It("should update cluster role binding's subjects", func() {
-		pod := testPod(defaultPodName, defaultPodServiceAccountName, map[string]string{
-			"openfeature.dev":                          "enabled",
-			"openfeature.dev/featureflagconfiguration": fmt.Sprintf("%s/%s", mutatePodNamespace, featureFlagConfigurationName),
-		})
-		err := k8sClient.Create(testCtx, pod)
-		Expect(err).ShouldNot(HaveOccurred())
-
-		crb := &v1.ClusterRoleBinding{}
-		err = k8sClient.Get(testCtx, client.ObjectKey{Name: clusterRoleBindingName}, crb)
-		Expect(err).ShouldNot(HaveOccurred())
-
-		Expect(len(crb.Subjects)).Should(Equal(3))
-		Expect(crb.Subjects).To(ContainElement(v1.Subject{
-			Kind:      "ServiceAccount",
-			APIGroup:  "",
-			Name:      defaultPodServiceAccountName,
-			Namespace: mutatePodNamespace,
-		}))
-
-		podMutationWebhookCleanup()
-	})
-
 	It("should create config map if sync provider isn't kubernetes", func() {
 		ffConfig := &corev1alpha1.FeatureFlagConfiguration{}
 		err := k8sClient.Get(

webhooks/run_test.go

@@ -15,7 +15,7 @@ import (
 	// +kubebuilder:scaffold:imports
 )
 
-func run(ctx context.Context, cfg *rest.Config, scheme *runtime.Scheme, opts *envtest.WebhookInstallOptions, backfillComplete chan struct{}) error {
+func run(ctx context.Context, cfg *rest.Config, scheme *runtime.Scheme, opts *envtest.WebhookInstallOptions) error {
 	ctrl.SetLogger(zap.New(zap.UseDevMode(true)))
 
 	mgr, err := ctrl.NewManager(cfg, ctrl.Options{
@@ -61,13 +61,19 @@ func run(ctx context.Context, cfg *rest.Config, scheme *runtime.Scheme, opts *en
 		},
 	})
 
-	go func(ctx context.Context, backfillComplete chan struct{}) {
-		podMutator.BackfillPermissions(ctx, backfillComplete)
-	}(ctx, backfillComplete)
+	// podMutator.BackfillPermissions is dependent upon mgr.Start executing correctly
+	// due to its time.Sleep within the retry loop, mgr.Start will always fail before podMutator.BackfillPermissions
+	// times out, resulting in the most relevant error being passed into the errChan
+	// mgr.Start will also only output an error value of nil once the context is closed (this occurs when the test suite is terminating)
+	errChan := make(chan error, 1)
+	go func() {
+		if err := podMutator.BackfillPermissions(ctx); err != nil {
+			errChan <- err
+		}
+	}()
+	go func() {
+		errChan <- mgr.Start(ctx)
+	}()
 
-	if err := mgr.Start(ctx); err != nil {
-		return err
-	}
-
-	return nil
+	return <-errChan
 }

webhooks/suite_test.go

@@ -158,12 +158,12 @@ var _ = BeforeSuite(func() {
 	setupPreviouslyExistingPods()
 
 	By("running webhook server")
-	backfillComplete := make(chan struct{}, 1)
 	go func() {
-		if err := run(testCtx, cfg, scheme, &testEnv.WebhookInstallOptions, backfillComplete); err != nil {
+		if err := run(testCtx, cfg, scheme, &testEnv.WebhookInstallOptions); err != nil {
 			logf.Log.Error(err, "run webhook server")
 		}
 	}()
+
 	d := &net.Dialer{Timeout: time.Second}
 	Eventually(func() error {
 		serverURL := fmt.Sprintf("%s:%d", testEnv.WebhookInstallOptions.LocalServingHost, testEnv.WebhookInstallOptions.LocalServingPort)
@@ -178,7 +178,7 @@ var _ = BeforeSuite(func() {
 		}
 		return nil
 	}).Should(Succeed())
-	<-backfillComplete
+
 	By("setting up resources")
 	setupMutatePodResources()
 	setupValidateFeatureFlagConfigurationResources()