fix(tests): addresses flakiness in tests, fixes failures due to twist…

…ed test assertion (#790)

* fix(plugins): fix tests & ignore deleted cluster for PluginPreset

- PluginPreset test: use same name for cluster and secret
- helm test: suffix crds to distinguish from greenhouse crds
- PluginPreset: do not reconcile Plugins for deleted clusters

* feat(clusters): use testSetup to create Org with OIDC secret

* fix(clusterkubeconfigs): calculate status only once

* fix(clusters): proceed with deletion if KubeConfig invalid

* fix(organizations): reconciler needs namespace to be set

* fix(cluster): deletion should wait for IsNotFound error

* refactor(clusters): retry.RetryOnConflict does not handle Gomega panic

* fix(clusters): remove additional test timeouts

* fix(clusters): do not override cluster resource

deletion logic errored since cluster was the same as
invalidCluster and thus already deleted

* fix(clusters): use invalid host to avoid bootstrap test timeout

---------

Co-authored-by: Abhijith Ravindra <[email protected]>

pkg/controllers/cluster/bootstrap_controller_test.go

@@ -29,6 +29,7 @@ var _ = Describe("Bootstrap controller", Ordered, func() {
 	BeforeAll(func() {
 		_, _, remoteEnvTest, remoteKubeConfig = test.StartControlPlane("6888", false, false)
 		setup = test.NewTestSetup(test.Ctx, test.K8sClient, bootstrapTestCase)
+		setup.CreateOrganizationWithOIDCConfig(test.Ctx, setup.Namespace())
 	})
 
 	AfterAll(func() {
@@ -67,7 +68,7 @@ var _ = Describe("Bootstrap controller", Ordered, func() {
 				By("Creating a secret with an invalid kubeconfig for a remote cluster")
 				kubeConfigString := string(remoteKubeConfig)
 				// invalidate host
-				invalidKubeConfigString := strings.ReplaceAll(kubeConfigString, "127", "128")
+				invalidKubeConfigString := strings.ReplaceAll(kubeConfigString, "127.0.0.1", "invalid.host")
 				invalidKubeConfigSecret := setup.CreateSecret(test.Ctx, bootstrapTestCase+"-invalid",
 					test.WithSecretType(greenhouseapis.SecretTypeKubeConfig),
 					test.WithSecretData(map[string][]byte{greenhouseapis.KubeConfigKey: []byte(invalidKubeConfigString)}))

pkg/controllers/cluster/cluster_controller.go

@@ -133,6 +133,10 @@ func (r *RemoteClusterReconciler) EnsureCreated(ctx context.Context, resource li
 // EnsureDeleted - handles the deletion / cleanup of cluster resource
 func (r *RemoteClusterReconciler) EnsureDeleted(ctx context.Context, resource lifecycle.RuntimeObject) (ctrl.Result, lifecycle.ReconcileResult, error) {
 	cluster := resource.(*greenhousev1alpha1.Cluster) //nolint:errcheck
+	c := cluster.Status.StatusConditions.GetConditionByType(greenhousev1alpha1.KubeConfigValid)
+	if c != nil && c.IsFalse() {
+		return ctrl.Result{}, lifecycle.Success, nil
+	}
 	// delete all plugins that are bound to this cluster
 	deletionCount, err := deletePlugins(ctx, r.Client, cluster)
 	if err != nil {

pkg/controllers/cluster/cluster_controller_test.go

@@ -5,7 +5,6 @@ package cluster_test
 
 import (
 	"fmt"
-	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -40,6 +39,7 @@ var _ = Describe("KubeConfig controller", func() {
 		BeforeEach(func() {
 			_, _, remoteEnvTest, remoteKubeConfig = test.StartControlPlane("6885", false, false)
 			setup = test.NewTestSetup(test.Ctx, test.K8sClient, directAccessTestCase)
+			setup.CreateOrganizationWithOIDCConfig(test.Ctx, setup.Namespace())
 		})
 
 		AfterEach(func() {
@@ -81,7 +81,7 @@ var _ = Describe("KubeConfig controller", func() {
 				Eventually(func() error {
 					var namespace = new(corev1.Namespace)
 					return remoteClient.Get(test.Ctx, types.NamespacedName{Namespace: "", Name: setup.Namespace()}, namespace)
-				}, 3*time.Minute).Should(Succeed(), fmt.Sprintf("eventually the namespace %s should exist", setup.Namespace()))
+				}).Should(Succeed(), fmt.Sprintf("eventually the namespace %s should exist", setup.Namespace()))
 
 				By("Checking service account has been created in remote cluster")
 				Eventually(func() error {

pkg/controllers/cluster/kubeconfig_controller.go

@@ -97,12 +97,11 @@ func (r *KubeconfigReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 				},
 			}}
 		kubeconfig.Spec.Kubeconfig.CurrentContext = cluster.Name
-		kubeconfig.Status = calculateKubeconfigStatus(kubeconfig)
 	}
 
 	defer func() {
 		result, err := clientutil.PatchStatus(ctx, r.Client, &kubeconfig, func() error {
-			kubeconfig.Status = calculateKubeconfigStatus(kubeconfig)
+			kubeconfig.Status = calculateKubeconfigStatus(&kubeconfig)
 			return nil
 		})
 		if err != nil {
@@ -276,7 +275,7 @@ func (r *KubeconfigReconciler) organizationSecretToClusters(ctx context.Context,
 	return nil
 }
 
-func calculateKubeconfigStatus(ck v1alpha1.ClusterKubeconfig) v1alpha1.ClusterKubeconfigStatus {
+func calculateKubeconfigStatus(ck *v1alpha1.ClusterKubeconfig) v1alpha1.ClusterKubeconfigStatus {
 	// new creation
 	status := ck.Status.DeepCopy()
 	if len(status.Conditions.Conditions) == 0 {

pkg/controllers/cluster/kubeconfig_controller_test.go

@@ -4,7 +4,6 @@
 package cluster_test
 
 import (
-	"context"
 	"fmt"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -41,7 +40,7 @@ var _ = Describe("ClusterKubeconfig controller", Ordered, func() {
 	var (
 		cluster      = v1alpha1.Cluster{}
 		organization = &v1alpha1.Organization{}
-		oidcSecret   = corev1.Secret{}
+		oidcSecret   = &corev1.Secret{}
 
 		setup test.TestSetup
 	)
@@ -50,36 +49,24 @@ var _ = Describe("ClusterKubeconfig controller", Ordered, func() {
 
 		setup = *test.NewTestSetup(test.Ctx, test.K8sClient, kubeconfigTestCase)
 
-		By("Creating an organization with OIDC config")
-		organization.Name = setup.Namespace()
-		organization.Spec.MappedOrgAdminIDPGroup = mappedAdminID
-		organization.Spec.Authentication = &v1alpha1.Authentication{
-			OIDCConfig: &v1alpha1.OIDCConfig{
-				Issuer: oidcIssuer,
-				ClientIDReference: v1alpha1.SecretKeyReference{
-					Name: oidcSecretResource,
-					Key:  oidcClientIDKey,
-				},
-				ClientSecretReference: v1alpha1.SecretKeyReference{
-					Name: oidcSecretResource,
-					Key:  oidcClientSecretKey,
-				},
-			},
-		}
-		Expect(test.K8sClient.Create(context.Background(), organization)).To(Succeed())
-
 		By("Creating a secret with OIDC data")
-		oidcSecret.Name = oidcSecretResource
-		oidcSecret.Namespace = organization.Name
-		oidcSecret.Data = map[string][]byte{
-			oidcClientIDKey:     []byte(oidcClientID),
-			oidcClientSecretKey: []byte(oidcClientSecret),
-		}
-		Expect(test.K8sClient.Create(context.Background(), &oidcSecret)).To(Succeed())
+		oidcSecret = setup.CreateSecret(test.Ctx, oidcSecretResource,
+			test.WithSecretNamespace(setup.Namespace()),
+			test.WithSecretData(map[string][]byte{
+				oidcClientIDKey:     []byte(oidcClientID),
+				oidcClientSecretKey: []byte(oidcClientSecret),
+			}))
+
+		By("Creating an organization with OIDC config")
+		organization = setup.CreateOrganization(test.Ctx, setup.Namespace(),
+			test.WithMappedAdminIDPGroup(mappedAdminID),
+			test.WithOIDCConfig(oidcIssuer, oidcSecretResource, oidcClientIDKey, oidcClientSecretKey),
+		)
 
 		By("Creating a Secret with a valid KubeConfig")
 		secret := setup.CreateSecret(test.Ctx, clusterName,
 			test.WithSecretType(greenhouseapis.SecretTypeKubeConfig),
+			test.WithSecretNamespace(organization.Name),
 			test.WithSecretData(map[string][]byte{greenhouseapis.KubeConfigKey: test.KubeConfig}))
 
 		By("Checking the cluster resource has been created")
@@ -91,7 +78,7 @@ var _ = Describe("ClusterKubeconfig controller", Ordered, func() {
 
 	AfterAll(func() {
 		test.MustDeleteCluster(test.Ctx, test.K8sClient, client.ObjectKeyFromObject(&cluster))
-		Expect(test.K8sClient.Delete(test.Ctx, &oidcSecret)).To(Succeed())
+		Expect(test.K8sClient.Delete(test.Ctx, oidcSecret)).To(Succeed())
 	})
 
 	clusterKubeconfig := v1alpha1.ClusterKubeconfig{}
@@ -226,8 +213,6 @@ users:
 		Expect(test.K8sClient.Update(test.Ctx, &organization)).To(Succeed())
 
 		clusterKubeconfig := v1alpha1.ClusterKubeconfig{}
-		clusterKubeconfig.Name = cluster.Name
-		clusterKubeconfig.Namespace = setup.Namespace()
 
 		Eventually(func(g Gomega) bool {
 			g.Expect(test.K8sClient.Get(test.Ctx, types.NamespacedName{Name: cluster.Name, Namespace: setup.Namespace()}, &clusterKubeconfig)).ShouldNot(HaveOccurred(), "There should be no error getting the ClusterKubeconfig resource")

pkg/controllers/cluster/status_test.go

@@ -5,7 +5,6 @@ package cluster_test
 
 import (
 	"fmt"
-	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -150,7 +149,7 @@ var _ = Describe("Cluster status", Ordered, func() {
 			g.Expect(validCluster.Status.Nodes["test-node"].Conditions).ToNot(BeEmpty())
 			g.Expect(validCluster.Status.Nodes["test-node"].Ready).To(BeFalse())
 			return true
-		}, 2*time.Minute).Should(BeTrue()) // TODO: fix the this test as it does not need to wait for 2 minutes
+		}).Should(BeTrue())
 
 		By("updating the node ready condition")
 		node := &corev1.Node{}
@@ -205,13 +204,13 @@ var _ = Describe("Cluster status", Ordered, func() {
 	It("should reconcile the status of a cluster without a secret", func() {
 		By("checking cluster conditions")
 		Eventually(func(g Gomega) bool {
-			g.Expect(test.K8sClient.Get(test.Ctx, types.NamespacedName{Name: invalidCluster.Name, Namespace: setup.Namespace()}, &validCluster)).ShouldNot(HaveOccurred(), "There should be no error getting the cluster resource")
-			g.Expect(validCluster.Status.StatusConditions).ToNot(BeNil())
-			kubeConfigValidCondition := validCluster.Status.GetConditionByType(greenhousev1alpha1.KubeConfigValid)
+			g.Expect(test.K8sClient.Get(test.Ctx, types.NamespacedName{Name: invalidCluster.Name, Namespace: setup.Namespace()}, invalidCluster)).ShouldNot(HaveOccurred(), "There should be no error getting the cluster resource")
+			g.Expect(invalidCluster.Status.StatusConditions).ToNot(BeNil())
+			kubeConfigValidCondition := invalidCluster.Status.GetConditionByType(greenhousev1alpha1.KubeConfigValid)
 			g.Expect(kubeConfigValidCondition).ToNot(BeNil(), "The KubeConfigValid condition should be present")
 			g.Expect(kubeConfigValidCondition.Status).To(Equal(metav1.ConditionFalse))
 			g.Expect(kubeConfigValidCondition.Message).To(ContainSubstring("Secret \"" + invalidCluster.Name + "\" not found"))
-			readyCondition := validCluster.Status.GetConditionByType(greenhousev1alpha1.ReadyCondition)
+			readyCondition := invalidCluster.Status.GetConditionByType(greenhousev1alpha1.ReadyCondition)
 			g.Expect(readyCondition).ToNot(BeNil(), "The ClusterReady condition should be present")
 			g.Expect(readyCondition.Status).To(Equal(metav1.ConditionFalse))
 			g.Expect(readyCondition.Message).To(ContainSubstring("kubeconfig not valid - cannot access cluster"))

pkg/controllers/organization/suite_test.go

@@ -29,7 +29,7 @@ var _ = BeforeSuite(func() {
 	By("mocking SCIM server")
 	groupsServer = scim.ReturnDefaultGroupResponseMockServer()
 
-	test.RegisterController("organizationController", (&organizationpkg.OrganizationReconciler{}).SetupWithManager)
+	test.RegisterController("organizationController", (&organizationpkg.OrganizationReconciler{Namespace: "default"}).SetupWithManager)
 	test.RegisterWebhook("orgWebhook", admission.SetupOrganizationWebhookWithManager)
 	test.RegisterWebhook("teamWebhook", admission.SetupTeamWebhookWithManager)
 	test.RegisterWebhook("pluginDefinitionWebhook", admission.SetupPluginDefinitionWebhookWithManager)

pkg/controllers/plugin/pluginpreset_controller.go

@@ -149,6 +149,8 @@ func (r *PluginPresetReconciler) reconcilePluginPreset(ctx context.Context, pres
 		err := r.Get(ctx, client.ObjectKey{Namespace: preset.GetNamespace(), Name: generatePluginName(preset, &cluster)}, plugin)
 
 		switch {
+		case !cluster.DeletionTimestamp.IsZero():
+			continue
 		case err == nil:
 			// The Plugin exists but does not contain the labels of the PluginPreset. This Plugin is not managed by the PluginPreset and must not be touched.
 			if shouldSkipPlugin(plugin, preset, pluginDefinition) {

pkg/controllers/plugin/pluginpreset_controller_test.go

@@ -975,7 +975,7 @@ func clusterSecret(clusterName string) *corev1.Secret {
 			APIVersion: corev1.GroupName,
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "test-" + clusterName,
+			Name:      clusterName,
 			Namespace: test.TestNamespace,
 		},
 		Type: greenhouseapis.SecretTypeKubeConfig,

pkg/controllers/plugin/remote_cluster_test.go

@@ -431,7 +431,7 @@ var _ = Describe("HelmController reconciliation", Ordered, func() {
 			return err == nil
 		}).Should(BeTrue(), "release for helm chart should already exist")
 
-		teamCRDName := "teams.greenhouse.sap"
+		teamCRDName := "teams.greenhouse.fixtures"
 		teamCRDKey := types.NamespacedName{Name: teamCRDName, Namespace: ""}
 
 		By("Getting Team CRD from remote cluster")

pkg/helm/helm_test.go

@@ -144,7 +144,7 @@ var _ = Describe("helm package test", func() {
 
 			By("getting the Team CRD")
 			var teamCRD = &apiextensionsv1.CustomResourceDefinition{}
-			teamCRDName := "teams.greenhouse.sap"
+			teamCRDName := "teams.greenhouse.fixtures"
 			teamCRDKey := types.NamespacedName{Name: teamCRDName, Namespace: ""}
 			err = test.K8sClient.Get(test.Ctx, teamCRDKey, teamCRD)
 			Expect(err).ToNot(HaveOccurred(), "there must be no error getting Team CRD")

pkg/test/fixtures/myChartWithCRDs/crds/greenhouse.sap_teams.yaml

@@ -7,9 +7,9 @@ kind: CustomResourceDefinition
 metadata:
   annotations:
     controller-gen.kubebuilder.io/version: v0.15.0
-  name: teams.greenhouse.sap
+  name: teams.greenhouse.fixtures
 spec:
-  group: greenhouse.sap
+  group: greenhouse.fixtures
   names:
     kind: Team
     listKind: TeamList

pkg/test/resources.go

@@ -55,6 +55,33 @@ func NewCluster(ctx context.Context, name, namespace string, opts ...func(*green
 	return cluster
 }
 
+// WithMappedIDPGroup sets the MappedIDPGroup on an Organization
+func WithMappedAdminIDPGroup(group string) func(*greenhousev1alpha1.Organization) {
+	return func(org *greenhousev1alpha1.Organization) {
+		org.Spec.MappedOrgAdminIDPGroup = group
+	}
+}
+
+// WithOIDCConfig sets the OIDCConfig on an Organization
+func WithOIDCConfig(issuer, secretName, clientIDKey, clientSecretKey string) func(*greenhousev1alpha1.Organization) {
+	return func(org *greenhousev1alpha1.Organization) {
+		if org.Spec.Authentication == nil {
+			org.Spec.Authentication = &greenhousev1alpha1.Authentication{}
+		}
+		org.Spec.Authentication.OIDCConfig = &greenhousev1alpha1.OIDCConfig{
+			Issuer: issuer,
+			ClientIDReference: greenhousev1alpha1.SecretKeyReference{
+				Name: secretName,
+				Key:  clientIDKey,
+			},
+			ClientSecretReference: greenhousev1alpha1.SecretKeyReference{
+				Name: secretName,
+				Key:  clientSecretKey,
+			},
+		}
+	}
+}
+
 // NewOrganization returns a greenhousev1alpha1.Organization object. Opts can be used to set the desired state of the Organization.
 func NewOrganization(ctx context.Context, name string, opts ...func(*greenhousev1alpha1.Organization)) *greenhousev1alpha1.Organization {
 	org := &greenhousev1alpha1.Organization{
@@ -334,6 +361,13 @@ func WithSecretData(data map[string][]byte) func(*corev1.Secret) {
 	}
 }
 
+// WithSecretNamespace sets the namespace of the Secret
+func WithSecretNamespace(namespace string) func(*corev1.Secret) {
+	return func(s *corev1.Secret) {
+		s.Namespace = namespace
+	}
+}
+
 // NewSecret returns a Secret object. Opts can be used to set the desired state of the Secret.
 func NewSecret(ctx context.Context, name, namespace string, opts ...func(*corev1.Secret)) *corev1.Secret {
 	secret := &corev1.Secret{

pkg/test/setup.go

@@ -19,6 +19,15 @@ import (
 	"github.com/cloudoperators/greenhouse/pkg/clientutil"
 )
 
+const (
+	OIDCSecretResource  = "oidc-secret"
+	OIDCClientIDKey     = "clientID"
+	OIDCClientID        = "the-client-id"
+	OIDCClientSecretKey = "clientSecret"
+	OIDCClientSecret    = "the-client-secret"
+	OIDCIssuer          = "https://the-issuer"
+)
+
 type TestSetup struct {
 	client.Client
 	namespace string
@@ -91,6 +100,19 @@ func (t *TestSetup) CreateCluster(ctx context.Context, name string, opts ...func
 	return cluster
 }
 
+func (t *TestSetup) CreateOrganizationWithOIDCConfig(ctx context.Context, orgName string) (*greenhousev1alpha1.Organization, *corev1.Secret) {
+	GinkgoHelper()
+	secret := t.CreateSecret(ctx, OIDCSecretResource,
+		WithSecretNamespace(orgName),
+		WithSecretData(map[string][]byte{
+			OIDCClientIDKey:     []byte(OIDCClientID),
+			OIDCClientSecretKey: []byte(OIDCClientSecret),
+		}))
+
+	org := t.CreateOrganization(ctx, orgName, WithOIDCConfig(OIDCIssuer, secret.Name, OIDCClientIDKey, OIDCClientSecretKey))
+	return org, secret
+}
+
 // CreateOrganization creates a Organization within the TestSetup and returns the created Organization resource.
 func (t *TestSetup) CreateOrganization(ctx context.Context, name string, opts ...func(*greenhousev1alpha1.Organization)) *greenhousev1alpha1.Organization {
 	GinkgoHelper()

pkg/test/util.go

@@ -19,45 +19,43 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
-	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	greenhouseapis "github.com/cloudoperators/greenhouse/pkg/apis"
 	greenhousev1alpha1 "github.com/cloudoperators/greenhouse/pkg/apis/greenhouse/v1alpha1"
 	"github.com/cloudoperators/greenhouse/pkg/clientutil"
 )
 
-func UpdateClusterWithDeletionAnnotation(ctx context.Context, c client.Client, id client.ObjectKey) {
+func UpdateClusterWithDeletionAnnotation(ctx context.Context, c client.Client, id client.ObjectKey) *greenhousev1alpha1.Cluster {
+	GinkgoHelper()
 	schedule, err := clientutil.ParseDateTime(time.Now().Add(-1 * time.Minute))
 	Expect(err).ToNot(HaveOccurred(), "there should be no error parsing the time")
-	err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		cluster := &greenhousev1alpha1.Cluster{}
-		Expect(c.Get(ctx, id, cluster)).
+	cluster := &greenhousev1alpha1.Cluster{}
+	Eventually(func(g Gomega) {
+		g.Expect(c.Get(ctx, id, cluster)).
 			To(Succeed(), "there must be no error getting the cluster")
+		baseCluster := cluster.DeepCopy()
 		cluster.SetAnnotations(map[string]string{
 			greenhouseapis.MarkClusterDeletionAnnotation:     "true",
 			greenhouseapis.ScheduleClusterDeletionAnnotation: schedule.Format(time.DateTime),
 		})
-		return c.Update(ctx, cluster)
-	})
-	Expect(err).ToNot(HaveOccurred(), "there must be no error updating the object", "key", id)
+		g.Expect(c.Patch(ctx, cluster, client.MergeFrom(baseCluster))).To(Succeed(), "there must be no error updating the cluster")
+	}).Should(Succeed(), "there should be no error setting the cluster deletion annotation")
+	return cluster
 }
 
 // MustDeleteCluster is used in the test context only and removes a cluster by namespaced name.
 func MustDeleteCluster(ctx context.Context, c client.Client, id client.ObjectKey) {
 	GinkgoHelper()
-	var cluster = new(greenhousev1alpha1.Cluster)
-	UpdateClusterWithDeletionAnnotation(ctx, c, id)
-	Expect(c.Get(ctx, id, cluster)).
-		To(Succeed(), "there must be no error getting the cluster")
+
+	cluster := UpdateClusterWithDeletionAnnotation(ctx, c, id)
 	Expect(c.Delete(ctx, cluster)).
 		To(Succeed(), "there must be no error deleting object", "key", client.ObjectKeyFromObject(cluster))
 
 	Eventually(func() bool {
 		err := c.Get(ctx, client.ObjectKeyFromObject(cluster), cluster)
 		return apierrors.IsNotFound(err)
-	}).
-		Should(BeFalse(), "the object should be deleted eventually")
+	}).Should(BeTrue(), "the object should be deleted eventually")
 }
 
 // MustReturnJSONFor marshals val to JSON and returns an apiextensionsv1.JSON.