From a51db2d0f7d4cbbed88191765a536761fa73cf45 Mon Sep 17 00:00:00 2001 From: Matous Jobanek Date: Thu, 3 Aug 2023 19:05:13 +0200 Subject: [PATCH] verify that SA secrets refs are not deleted (#767) --- test/e2e/parallel/serviceaccount_test.go | 76 ++++++++++++++++-------- testsupport/tiers/checks.go | 8 +-- testsupport/wait/member.go | 25 +++++++- 3 files changed, 77 insertions(+), 32 deletions(-) diff --git a/test/e2e/parallel/serviceaccount_test.go b/test/e2e/parallel/serviceaccount_test.go index 746a01c0e..dfdf74afb 100644 --- a/test/e2e/parallel/serviceaccount_test.go +++ b/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)) + } } diff --git a/testsupport/tiers/checks.go b/testsupport/tiers/checks.go index c1619496c..5e12fb83b 100644 --- a/testsupport/tiers/checks.go +++ b/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) } } diff --git a/testsupport/wait/member.go b/testsupport/wait/member.go index 5999f46e2..37ee39a28 100644 --- a/testsupport/wait/member.go +++ b/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