Repository: shipwright-io/build
Pull request #885 (2021-10-04 13:56:25)

Remove -sa suffix from generated service account
# Changes

There was a bug in Calico before 3.20.0 where it unconditionally passed `pod.spec.serviceAccountName` as a label value to its WorkloadEndpoint. But label values cannot exceed 63 characters while a longer ServiceAccount name is allowed. Their fix is [Use a spec field on WEPs for ServiceAccount name #1422](https://github.com/projectcalico/libcalico-go/pull/1422).

But, especially in managed Kubernetes environments, the Calico version is not necessarily that new. For example, IKS 1.21 is at 3.19 just slightly below this.

In Shipwright, we already limit the BuildRun length to 63 characters because we use it as label value on the TaskRun (and pod). But, the name of the generated ServiceAccount used to be `${buildRunName}-sa`.

I changed the logic to make the name of the ServiceAccount be the BuildRun name. Added code to still delete the one with the `-sa` suffix to not cause dangling SAs.

Used that chance to move the removal logic from buildrun.go into serviceaccount.go.

# Submitter Checklist

- [x] Includes tests if functionality changed/was added
- [ ] Includes docs if changes are user-facing
- [x] [Set a kind label on this PR](https://prow.k8s.io/command-help#kind)  
- [x] Release notes block has been filled in, or marked NONE

# Release Notes

```release-note
The -sa suffix from generated service accounts has been removed so that BuildRuns with names longer than 60 characters also work with older Calico versions
```

openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **saschaschwarze0** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=shipwright-io%2Fbuild).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/shipwright-io/build/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["saschaschwarze0"]} -->

Patch:
diff --git a/pkg/reconciler/buildrun/buildrun.go b/pkg/reconciler/buildrun/buildrun.go
index dee37907..8ae3ce6d 100644
--- a/pkg/reconciler/buildrun/buildrun.go
+++ b/pkg/reconciler/buildrun/buildrun.go
@@ -310,14 +310,9 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 
 			taskRunStatus := trCondition.Status
 
-			// check if we should delete the generated service account by checking the build run spec and that the task run is csomplete
-			if resources.IsGeneratedServiceAccountUsed(buildRun) && (taskRunStatus == corev1.ConditionTrue || taskRunStatus == corev1.ConditionFalse) {
-				serviceAccount := &corev1.ServiceAccount{}
-				serviceAccount.Name = resources.GetGeneratedServiceAccountName(buildRun)
-				serviceAccount.Namespace = buildRun.Namespace
-
-				ctxlog.Info(ctx, "deleting service account", namespace, request.Namespace, name, request.Name)
-				if err := r.client.Delete(ctx, serviceAccount); err != nil && !apierrors.IsNotFound(err) {
+			// check if we should delete the generated service account by checking the build run spec and that the task run is complete
+			if taskRunStatus == corev1.ConditionTrue || taskRunStatus == corev1.ConditionFalse {
+				if err := resources.DeleteServiceAccount(ctx, r.client, buildRun); err != nil {
 					ctxlog.Error(ctx, err, "Error during deletion of generated service account.")
 					return reconcile.Result{}, err
 				}
diff --git a/pkg/reconciler/buildrun/resources/service_accounts.go b/pkg/reconciler/buildrun/resources/service_accounts.go
index 7ff1beb0..6f70e50d 100644
--- a/pkg/reconciler/buildrun/resources/service_accounts.go
+++ b/pkg/reconciler/buildrun/resources/service_accounts.go
@@ -31,6 +31,12 @@ const (
 
 // GetGeneratedServiceAccountName returns the name of the generated service account for a build run
 func GetGeneratedServiceAccountName(buildRun *buildv1alpha1.BuildRun) string {
+	return buildRun.Name
+}
+
+// GetHeritageGeneratedServiceAccountName return the name of the generated service account for
+// a build run as it was used in older releases of Shipwright Build
+func GetHeritageGeneratedServiceAccountName(buildRun *buildv1alpha1.BuildRun) string {
 	return buildRun.Name + "-sa"
 }
 
@@ -85,6 +91,33 @@ func GenerateSA(ctx context.Context, client client.Client, build *buildv1alpha1.
 	}
 }
 
+// DeleteServiceAccount deletes the service account of a completed BuildRun if the service account
+// was generated
+func DeleteServiceAccount(ctx context.Context, client client.Client, completedBuildRun *buildv1alpha1.BuildRun) error {
+	if !IsGeneratedServiceAccountUsed(completedBuildRun) {
+		return nil
+	}
+
+	serviceAccount := &corev1.ServiceAccount{}
+	serviceAccount.Name = GetGeneratedServiceAccountName(completedBuildRun)
+	serviceAccount.Namespace = completedBuildRun.Namespace
+
+	ctxlog.Info(ctx, "deleting service account", namespace, completedBuildRun.Namespace, name, completedBuildRun.Name, "serviceAccount", serviceAccount.Name)
+	if err := client.Delete(ctx, serviceAccount); err != nil {
+		if apierrors.IsNotFound(err) {
+			serviceAccount.Name = GetHeritageGeneratedServiceAccountName(completedBuildRun)
+			ctxlog.Info(ctx, "deleting service account", namespace, completedBuildRun.Namespace, name, completedBuildRun.Name, "serviceAccount", serviceAccount.Name)
+			err = client.Delete(ctx, serviceAccount)
+		}
+
+		if err != nil && !apierrors.IsNotFound(err) {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // getDefaultNamespaceSA retrieves a pipeline or default sa per namespace. This is used when users do not specify a service account
 // to use on BuildRuns
 func getDefaultNamespaceSA(ctx context.Context, client client.Client, buildRun *buildv1alpha1.BuildRun) (*corev1.ServiceAccount, error) {


Test patch:
diff --git a/pkg/reconciler/buildrun/buildrun_test.go b/pkg/reconciler/buildrun/buildrun_test.go
index 23c4d609..461ca1d3 100644
--- a/pkg/reconciler/buildrun/buildrun_test.go
+++ b/pkg/reconciler/buildrun/buildrun_test.go
@@ -227,7 +227,7 @@ var _ = Describe("Reconcile BuildRun", func() {
 					buildSample,
 					buildRunSample,
 					taskRunSample,
-					ctl.DefaultServiceAccount(buildRunSample.Name+"-sa")),
+					ctl.DefaultServiceAccount(buildRunSample.Name)),
 				)
 
 				// Call the reconciler
@@ -241,7 +241,7 @@ var _ = Describe("Reconcile BuildRun", func() {
 				_, obj, _ := client.DeleteArgsForCall(0)
 				serviceAccount, castSuccessful := obj.(*corev1.ServiceAccount)
 				Expect(castSuccessful).To(BeTrue())
-				Expect(serviceAccount.Name).To(Equal(buildRunSample.Name + "-sa"))
+				Expect(serviceAccount.Name).To(Equal(buildRunSample.Name))
 				Expect(serviceAccount.Namespace).To(Equal(buildRunSample.Namespace))
 			})
 
diff --git a/pkg/reconciler/buildrun/resources/service_accounts_test.go b/pkg/reconciler/buildrun/resources/service_accounts_test.go
index b8d858f7..988f886f 100644
--- a/pkg/reconciler/buildrun/resources/service_accounts_test.go
+++ b/pkg/reconciler/buildrun/resources/service_accounts_test.go
@@ -132,8 +132,10 @@ var _ = Describe("Operating service accounts", func() {
 	Context("Retrieving autogenerated service accounts when the spec.serviceAccount.generate key is used", func() {
 
 		It("should provide a generated sa name", func() {
-			Expect(resources.GetGeneratedServiceAccountName(buildRunSample)).To(Equal(buildRunSample.Name + "-sa"))
+			Expect(resources.GetGeneratedServiceAccountName(buildRunSample)).To(Equal(buildRunSample.Name))
+			Expect(resources.GetHeritageGeneratedServiceAccountName(buildRunSample)).To(Equal(buildRunSample.Name + "-sa"))
 		})
+
 		It("should return a generated sa with a label, ownerreference and a ref secret if it does not exists", func() {
 			buildRunSample := ctl.BuildRunWithSAGenerate(buildRunName, buildName)
 
@@ -162,7 +164,7 @@ var _ = Describe("Operating service accounts", func() {
 			buildRunSample := ctl.BuildRunWithSAGenerate(buildRunName, buildName)
 
 			// stub a GET API call for a service account
-			client.GetCalls(generateGetSAStub(buildRunName + "-sa"))
+			client.GetCalls(generateGetSAStub(buildRunName))
 
 			_, err := resources.RetrieveServiceAccount(context.TODO(), client, ctl.BuildWithOutputSecret(buildName, "default", "foosecret"), buildRunSample)
 			Expect(err).To(BeNil())
diff --git a/test/e2e/validators_test.go b/test/e2e/validators_test.go
index 8a8067fc..339e1525 100644
--- a/test/e2e/validators_test.go
+++ b/test/e2e/validators_test.go
@@ -249,7 +249,7 @@ func validateServiceAccountDeletion(buildRun *buildv1alpha1.BuildRun, namespace
 	}
 
 	saNamespacedName := types.NamespacedName{
-		Name:      buildRun.Name + "-sa",
+		Name:      buildRun.Name,
 		Namespace: namespace,
 	}
 
diff --git a/test/integration/buildruns_to_sa_test.go b/test/integration/buildruns_to_sa_test.go
index c52fc856..ef3cd7f2 100644
--- a/test/integration/buildruns_to_sa_test.go
+++ b/test/integration/buildruns_to_sa_test.go
@@ -96,7 +96,7 @@ var _ = Describe("Integration tests BuildRuns and Service-accounts", func() {
 			_, err = tb.GetBRTillStartTime(buildRunObject.Name)
 			Expect(err).To(BeNil())
 
-			sa, err := tb.GetSA(fmt.Sprintf("%s-sa", buildRunObject.Name))
+			sa, err := tb.GetSA(buildRunObject.Name)
 			Expect(err).To(BeNil())
 
 			// Verify that the sa have our Build specified secret
@@ -105,7 +105,7 @@ var _ = Describe("Integration tests BuildRuns and Service-accounts", func() {
 			_, err = tb.GetBRTillCompletion(buildRunObject.Name)
 			Expect(err).To(BeNil())
 
-			_, err = tb.GetSA(fmt.Sprintf("%s-sa", buildRunObject.Name))
+			_, err = tb.GetSA(buildRunObject.Name)
 			Expect(err).ToNot(BeNil())
 
 		})
@@ -144,7 +144,7 @@ var _ = Describe("Integration tests BuildRuns and Service-accounts", func() {
 			bro, err := tb.GetBRTillStartTime(buildRunObject.Name)
 			Expect(err).To(BeNil())
 
-			sa, err := tb.GetSA(fmt.Sprintf("%s-sa", buildRunObject.Name))
+			sa, err := tb.GetSA(buildRunObject.Name)
 			Expect(err).To(BeNil())
 
 			// Verify that the sa have our Build specified secret
@@ -177,7 +177,7 @@ var _ = Describe("Integration tests BuildRuns and Service-accounts", func() {
 			Expect(err).To(BeNil())
 
 			// confirm SA gone
-			_, err = tb.GetSA(fmt.Sprintf("%s-sa", buildRunObject.Name))
+			_, err = tb.GetSA(buildRunObject.Name)
 			Expect(err).ToNot(BeNil())
 
 		})