verify that SA secrets refs are not deleted

test/e2e/parallel/serviceaccount_test.go

@@ -20,7 +20,6 @@ func TestDoNotOverrideServiceAccount(t *testing.T) {
 	// given
 	// Skipping the TestDoNotOverrideServiceAccount test instead of deleting it because we will need to create SAs as part
 	// of the environment sub-workspaces so the test & logic will be useful to keep.
-	t.Skip("skipping since the user SA was removed as part of https://github.com/codeready-toolchain/host-operator/pull/719. To be added back with https://issues.redhat.com/browse/ASC-249")
 	t.Parallel()
 	awaitilities := WaitForDeployments(t)
 	member := awaitilities.Member1()
@@ -36,35 +35,64 @@ func TestDoNotOverrideServiceAccount(t *testing.T) {
 		Resources()
 
 	// and move the user to appstudio tier
-	tiers.MoveSpaceToTier(t, awaitilities.Host(), mur.Name, "appstudio")
+	tiers.MoveSpaceToTier(t, awaitilities.Host(), mur.Name, "appstudio-env")
 	VerifyResourcesProvisionedForSpace(t, awaitilities, mur.Name)
 
-	// get the SA that is provisioned for the user in the ns
-	sa, err := member.WaitForServiceAccount(t, mur.Name, fmt.Sprintf("appstudio-%s", mur.Name))
-	require.NoError(t, err)
-	expectedSecrets := getSASecrets(t, member, mur.Name, sa.Name)
+	expectedSecrets := getSASecrets(t, member, mur.Name, "namespace-manager")
 
-	// when we add an annotation to the SA resource then it should stay there
-	sa.Annotations = map[string]string{
-		"should": "stay",
-	}
-	require.NoError(t, member.Client.Update(context.TODO(), sa))
+	for i := 0; i < 10; i++ {
+		_, err := member.UpdateServiceAccount(t, fmt.Sprintf("%s-env", mur.Name), "namespace-manager", func(sa *corev1.ServiceAccount) {
 
-	// drop the SpaceRoles annotation from the namespace to trigger the reconciliation
-	require.NoError(t, err)
-	_, err = member.UpdateNSTemplateSet(t, mur.Name, func(nsTmplSet *v1alpha1.NSTemplateSet) {
-		delete(nsTmplSet.Annotations, v1alpha1.LastAppliedSpaceRolesAnnotationKey)
-	})
-	require.NoError(t, err)
+			// when we add an annotation to the SA resource then it should stay there
+			sa.Annotations = map[string]string{
+				"should": "stay",
+			}
+			sa.Secrets = append(sa.Secrets, corev1.ObjectReference{
+				Name: fmt.Sprintf("dummy-secret-%d", i),
+			})
+			sa.ImagePullSecrets = append(sa.ImagePullSecrets, corev1.LocalObjectReference{
+				Name: fmt.Sprintf("dummy-pull-secret-%d", i),
+			})
 
-	// then
-	VerifyResourcesProvisionedForSpace(t, awaitilities, mur.Name)
-	sa, err = member.WaitForServiceAccount(t, mur.Name, fmt.Sprintf("appstudio-%s", mur.Name))
-	require.NoError(t, err)
-	assert.Equal(t, "stay", sa.Annotations["should"])
+		})
+		require.NoError(t, err)
+
+		// drop the SpaceRoles annotation from the namespace to trigger the reconciliation
+		_, err = member.UpdateNSTemplateSet(t, mur.Name, func(nsTmplSet *v1alpha1.NSTemplateSet) {
+			delete(nsTmplSet.Annotations, v1alpha1.LastAppliedSpaceRolesAnnotationKey)
+		})
+		require.NoError(t, err)
+
+		// then
+		VerifyResourcesProvisionedForSpace(t, awaitilities, mur.Name)
+		sa, err := member.WaitForServiceAccount(t, fmt.Sprintf("%s-env", mur.Name), "namespace-manager")
+		require.NoError(t, err)
+		assert.Equal(t, "stay", sa.Annotations["should"])
+	secrets:
+		for j := 0; j < i; j++ {
+			expName := fmt.Sprintf("dummy-secret-%d", j)
+			for _, secretRef := range sa.Secrets {
+				if secretRef.Name == fmt.Sprintf("dummy-secret-%d", j) {
+					continue secrets
+				}
+			}
+			assert.Fail(t, fmt.Sprintf("secret '%s' not found", expName))
+		}
+
+	pullSecrets:
+		for j := 0; j < i; j++ {
+			expName := fmt.Sprintf("dummy-pull-secret-%d", j)
+			for _, pullSecretRef := range sa.ImagePullSecrets {
+				if pullSecretRef.Name == fmt.Sprintf("dummy-pull-secret-%d", j) {
+					continue pullSecrets
+				}
+			}
+			assert.Fail(t, fmt.Sprintf("pull secret '%s' not found", expName))
+		}
 
-	// verify that the secrets created for SA is the same
-	assert.Equal(t, expectedSecrets, getSASecrets(t, member, mur.Name, sa.Name))
+		// verify that the secrets created for SA is the same
+		assert.Equal(t, expectedSecrets, getSASecrets(t, member, mur.Name, sa.Name))
+	}
 
 }
 

testsupport/tiers/checks.go

@@ -1854,19 +1854,15 @@ func toolchainSaReadRole() namespaceObjectsCheck {
 
 func namespaceManagerSA() namespaceObjectsCheck {
 	return func(t *testing.T, ns *corev1.Namespace, memberAwait *wait.MemberAwaitility, owner string) {
-		_, err := memberAwait.WaitForServiceAccount(t, ns.Name, toolchainv1alpha1.AdminServiceAccountName)
+		_, err := memberAwait.WaitForServiceAccount(t, ns.Name, toolchainv1alpha1.AdminServiceAccountName, toolchainLabelsWaitCriterion(owner)...)
 		require.NoError(t, err)
-		// fixme: decide if we want to check labels also on serviceaccounts
-		//assertExpectedToolchainLabels(t, sa, owner)
 	}
 }
 
 func pipelineServiceAccount() namespaceObjectsCheck {
 	return func(t *testing.T, ns *corev1.Namespace, memberAwait *wait.MemberAwaitility, owner string) {
-		_, err := memberAwait.WaitForServiceAccount(t, ns.Name, "appstudio-pipeline")
+		_, err := memberAwait.WaitForServiceAccount(t, ns.Name, "appstudio-pipeline", toolchainLabelsWaitCriterion(owner)...)
 		require.NoError(t, err)
-		// fixme: decide if we want to check labels also on serviceaccounts
-		//assertExpectedToolchainLabels(t, sa, owner)
 	}
 }
 

testsupport/wait/member.go

@@ -865,7 +865,7 @@ func (a *MemberAwaitility) WaitUntilRoleBindingDeleted(t *testing.T, namespace *
 	})
 }
 
-func (a *MemberAwaitility) WaitForServiceAccount(t *testing.T, namespace string, name string) (*corev1.ServiceAccount, error) {
+func (a *MemberAwaitility) WaitForServiceAccount(t *testing.T, namespace string, name string, criteria ...LabelWaitCriterion) (*corev1.ServiceAccount, error) {
 	t.Logf("waiting for ServiceAccount '%s' in namespace '%s'", name, namespace)
 	serviceAccount := &corev1.ServiceAccount{}
 	err := wait.Poll(a.RetryInterval, a.Timeout, func() (done bool, err error) {
@@ -877,7 +877,7 @@ func (a *MemberAwaitility) WaitForServiceAccount(t *testing.T, namespace string,
 			return false, err
 		}
 		serviceAccount = obj
-		return true, nil
+		return matchLabelWaitCriteria(obj.ObjectMeta, criteria...), nil
 	})
 	if err != nil {
 		t.Logf("failed to wait for ServiceAccount '%s' in namespace '%s'.", name, namespace)
@@ -1273,6 +1273,27 @@ func (a *MemberAwaitility) UpdateNSTemplateSet(t *testing.T, spaceName string, m
 	return nsTmplSet, err
 }
 
+// UpdateServiceAccount tries to update the given ServiceAccount
+// If it fails with an error (for example if the object has been modified) then it retrieves the latest version and tries again
+// Returns the updated ServiceAccount
+func (a *MemberAwaitility) UpdateServiceAccount(t *testing.T, namespace, saName string, modifySA func(sa *corev1.ServiceAccount)) (*corev1.ServiceAccount, error) {
+	var sa *corev1.ServiceAccount
+	err := wait.Poll(a.RetryInterval, a.Timeout, func() (done bool, err error) {
+		freshSA := &corev1.ServiceAccount{}
+		if err := a.Client.Get(context.TODO(), types.NamespacedName{Namespace: namespace, Name: saName}, freshSA); err != nil {
+			return true, err
+		}
+		modifySA(freshSA)
+		if err := a.Client.Update(context.TODO(), freshSA); err != nil {
+			t.Logf("error updating ServiceAccount '%s': %s. Will retry again...", saName, err.Error())
+			return false, nil
+		}
+		sa = freshSA
+		return true, nil
+	})
+	return sa, err
+}
+
 // UpdateSpaceRequest tries to update the Spec of the given SpaceRequest
 // If it fails with an error (for example if the object has been modified) then it retrieves the latest version and tries again
 // Returns the updated SpaceRequest