From da61a87ce03875e12387e9bb6177f778a3fc6946 Mon Sep 17 00:00:00 2001
From: eryalito <adriancamesellemartin@gmail.com>
Date: Mon, 25 Sep 2023 17:05:35 +0200
Subject: [PATCH] Allow extra manually added pull secrets to managed SA (#418)

* feat: add annotation on SA to keep track of pull secrets

Add an annotation to keep track of managed pullsecrets on the created service account

* feat: use actual pull secret comparision instead of annotations

* feat: tidy current e2e tests and add cleanups

* fix: add banner

* feat: add e2e service account tests

* fix: yq syntax fix

* fix: yq eval environment variable

* fix: e2e scripts base_dir reference

* Revert "fix: e2e scripts base_dir reference"

This reverts commit e0704033694851d12af22b8d5ec9e5b59083c44d.

* fix: copy missing tests folder into e2e container

* feat: bump rok8s to @13 and ci-images to v13
---
 .circleci/config.yml                          |  4 +-
 e2e/pre.sh                                    |  8 +-
 .../cluterrolebindings/cleanup.sh             |  2 +
 e2e/rbacdefinition/cluterrolebindings/main.sh | 21 +++++
 .../cluterrolebindings/setup.sh               | 16 ++++
 .../cluterrolebindings/tests.sh               | 10 +++
 e2e/rbacdefinition/run.sh                     | 18 +++++
 e2e/rbacdefinition/serviceaccounts/cleanup.sh |  2 +
 e2e/rbacdefinition/serviceaccounts/main.sh    | 21 +++++
 e2e/rbacdefinition/serviceaccounts/setup.sh   | 18 +++++
 e2e/rbacdefinition/serviceaccounts/tests.sh   | 68 ++++++++++++++++
 e2e/test.sh                                   | 39 ++--------
 pkg/reconciler/matcher.go                     | 35 +++++++--
 pkg/reconciler/matcher_test.go                | 77 +++++++++++++++++++
 pkg/reconciler/parser.go                      |  7 ++
 15 files changed, 300 insertions(+), 46 deletions(-)
 create mode 100644 e2e/rbacdefinition/cluterrolebindings/cleanup.sh
 create mode 100644 e2e/rbacdefinition/cluterrolebindings/main.sh
 create mode 100644 e2e/rbacdefinition/cluterrolebindings/setup.sh
 create mode 100644 e2e/rbacdefinition/cluterrolebindings/tests.sh
 create mode 100644 e2e/rbacdefinition/run.sh
 create mode 100644 e2e/rbacdefinition/serviceaccounts/cleanup.sh
 create mode 100644 e2e/rbacdefinition/serviceaccounts/main.sh
 create mode 100644 e2e/rbacdefinition/serviceaccounts/setup.sh
 create mode 100644 e2e/rbacdefinition/serviceaccounts/tests.sh

diff --git a/.circleci/config.yml b/.circleci/config.yml
index 57e302fb..7102f0ff 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -1,7 +1,7 @@
 version: 2.1
 
 orbs:
-  rok8s: fairwinds/rok8s-scripts@11
+  rok8s: fairwinds/rok8s-scripts@13
   oss-docs: fairwinds/oss-docs@0
 
 references:
@@ -18,7 +18,7 @@ references:
   e2e_configuration: &e2e_configuration
     pre_script: e2e/pre.sh
     script: e2e/test.sh
-    command_runner_image: quay.io/reactiveops/ci-images:v12-buster
+    command_runner_image: quay.io/reactiveops/ci-images:v13-buster
     enable_docker_layer_caching: true
     attach-workspace: true
     requires:
diff --git a/e2e/pre.sh b/e2e/pre.sh
index e15831c8..d53a5d1f 100644
--- a/e2e/pre.sh
+++ b/e2e/pre.sh
@@ -2,7 +2,7 @@
 
 set -e
 
-wget -O /usr/local/bin/yq "https://github.com/mikefarah/yq/releases/download/2.4.0/yq_linux_amd64"
+wget -O /usr/local/bin/yq "https://github.com/mikefarah/yq/releases/download/v4.35.1/yq_linux_amd64"
 chmod +x /usr/local/bin/yq
 
 if [ -z "$CI_SHA1" ]; then
@@ -26,8 +26,10 @@ echo "** END LOADING IMAGE **"
 echo "********************************************************************"
 printf "\n\n"
 
-yq w -i deploy/3_deployment.yaml 'spec.template.spec.containers[0].image' "quay.io/reactiveops/rbac-manager:${CI_SHA1}-amd64"
-yq w -i deploy/3_deployment.yaml 'spec.template.spec.containers[0].imagePullPolicy' "IfNotPresent"
+export newImage=quay.io/reactiveops/rbac-manager:${CI_SHA1}-amd64
+yq -i '.spec.template.spec.containers[0].image = env(newImage)' deploy/3_deployment.yaml
+yq -i '.spec.template.spec.containers[0].imagePullPolicy = "IfNotPresent"' deploy/3_deployment.yaml
 cat deploy/3_deployment.yaml
 
 docker cp deploy e2e-command-runner:/
+docker cp e2e/rbacdefinition e2e-command-runner:/
diff --git a/e2e/rbacdefinition/cluterrolebindings/cleanup.sh b/e2e/rbacdefinition/cluterrolebindings/cleanup.sh
new file mode 100644
index 00000000..dc175b27
--- /dev/null
+++ b/e2e/rbacdefinition/cluterrolebindings/cleanup.sh
@@ -0,0 +1,2 @@
+kubectl delete clusterrole test-rbac-manager --ignore-not-found
+kubectl delete RBACDefinition rbac-manager-definition --ignore-not-found
\ No newline at end of file
diff --git a/e2e/rbacdefinition/cluterrolebindings/main.sh b/e2e/rbacdefinition/cluterrolebindings/main.sh
new file mode 100644
index 00000000..18726ef7
--- /dev/null
+++ b/e2e/rbacdefinition/cluterrolebindings/main.sh
@@ -0,0 +1,21 @@
+BASE_DIR=$(dirname $BASH_SOURCE)
+
+printf "\n\n"
+echo "********************************************************************"
+echo "** Test clusterrolebindings **"
+echo "********************************************************************"
+printf "\n\n"
+
+# Execute the setup, then execute the tests just if the setup contains no errors.
+# Finally always execute the cleanup and return the whole error of the steps
+error=$((0))
+bash "$BASE_DIR/setup.sh"
+error=$(( error | $? ))
+
+if [ $error -eq 0 ]; then
+bash "$BASE_DIR/tests.sh"
+error=$(( error | $? ))
+fi
+
+bash "$BASE_DIR/cleanup.sh"
+exit $(( error | $? ))
diff --git a/e2e/rbacdefinition/cluterrolebindings/setup.sh b/e2e/rbacdefinition/cluterrolebindings/setup.sh
new file mode 100644
index 00000000..170df18a
--- /dev/null
+++ b/e2e/rbacdefinition/cluterrolebindings/setup.sh
@@ -0,0 +1,16 @@
+kubectl create clusterrole test-rbac-manager --verb="create" --resource=deployment
+
+cat <<EOF | kubectl create -f -
+apiVersion: rbacmanager.reactiveops.io/v1beta1
+kind: RBACDefinition
+metadata:
+  name: rbac-manager-definition
+rbacBindings:
+  - name: admins
+    subjects:
+      - kind: ServiceAccount
+        name: test-rbac-manager
+        namespace: rbac-manager
+    clusterRoleBindings:
+      - clusterRole: test-rbac-manager
+EOF
\ No newline at end of file
diff --git a/e2e/rbacdefinition/cluterrolebindings/tests.sh b/e2e/rbacdefinition/cluterrolebindings/tests.sh
new file mode 100644
index 00000000..81e17597
--- /dev/null
+++ b/e2e/rbacdefinition/cluterrolebindings/tests.sh
@@ -0,0 +1,10 @@
+# wait up to 2 minutes for rbac-manager to create the binding
+counter=0
+until kubectl get clusterrolebinding/rbac-manager-definition-admins-test-rbac-manager; do
+  let "counter=counter+1"
+  sleep 10
+  if [ $counter -gt 11 ]; then
+    break
+  fi
+done
+kubectl auth can-i create deployments --as=system:serviceaccount:rbac-manager:test-rbac-manager
\ No newline at end of file
diff --git a/e2e/rbacdefinition/run.sh b/e2e/rbacdefinition/run.sh
new file mode 100644
index 00000000..0e098f26
--- /dev/null
+++ b/e2e/rbacdefinition/run.sh
@@ -0,0 +1,18 @@
+BASE_DIR=$(dirname $BASH_SOURCE)
+
+printf "\n\n"
+echo "********************************************************************"
+echo "** Test rbacDefinition **"
+echo "********************************************************************"
+printf "\n\n"
+
+
+bash "$BASE_DIR/cluterrolebindings/main.sh"
+if [ $? -ne 0 ]; then
+  exit 1
+fi
+
+bash "$BASE_DIR/serviceaccounts/main.sh"
+if [ $? -ne 0 ]; then
+  exit 1
+fi
\ No newline at end of file
diff --git a/e2e/rbacdefinition/serviceaccounts/cleanup.sh b/e2e/rbacdefinition/serviceaccounts/cleanup.sh
new file mode 100644
index 00000000..671b18d2
--- /dev/null
+++ b/e2e/rbacdefinition/serviceaccounts/cleanup.sh
@@ -0,0 +1,2 @@
+kubectl delete clusterrole test-rbac-manager --ignore-not-found
+kubectl delete RBACDefinition rbac-manager-definition-1 --ignore-not-found
\ No newline at end of file
diff --git a/e2e/rbacdefinition/serviceaccounts/main.sh b/e2e/rbacdefinition/serviceaccounts/main.sh
new file mode 100644
index 00000000..e3368357
--- /dev/null
+++ b/e2e/rbacdefinition/serviceaccounts/main.sh
@@ -0,0 +1,21 @@
+BASE_DIR=$(dirname $BASH_SOURCE)
+
+printf "\n\n"
+echo "********************************************************************"
+echo "** Test serviceaccounts **"
+echo "********************************************************************"
+printf "\n\n"
+
+# Execute the setup, then execute the tests just if the setup contains no errors.
+# Finally always execute the cleanup and return the whole error of the steps
+error=$((0))
+bash "$BASE_DIR/setup.sh"
+error=$(( error | $? ))
+
+if [ $error -eq 0 ]; then
+bash "$BASE_DIR/tests.sh"
+error=$(( error | $? ))
+fi
+
+bash "$BASE_DIR/cleanup.sh"
+exit $(( error | $? ))
diff --git a/e2e/rbacdefinition/serviceaccounts/setup.sh b/e2e/rbacdefinition/serviceaccounts/setup.sh
new file mode 100644
index 00000000..8e3c5ead
--- /dev/null
+++ b/e2e/rbacdefinition/serviceaccounts/setup.sh
@@ -0,0 +1,18 @@
+kubectl create clusterrole test-rbac-manager --verb="create" --resource=deployment
+
+cat <<EOF | kubectl create -f -
+apiVersion: rbacmanager.reactiveops.io/v1beta1
+kind: RBACDefinition
+metadata:
+  name: rbac-manager-definition-1
+rbacBindings:
+  - name: admins
+    subjects:
+      - kind: ServiceAccount
+        name: test-rbac-manager
+        namespace: rbac-manager
+        imagePullSecrets:
+          - robot-secret
+    clusterRoleBindings:
+      - clusterRole: test-rbac-manager
+EOF
\ No newline at end of file
diff --git a/e2e/rbacdefinition/serviceaccounts/tests.sh b/e2e/rbacdefinition/serviceaccounts/tests.sh
new file mode 100644
index 00000000..537e7249
--- /dev/null
+++ b/e2e/rbacdefinition/serviceaccounts/tests.sh
@@ -0,0 +1,68 @@
+# wait up to 2 minutes for rbac-manager to create the binding
+counter=0
+error=$((0))
+until kubectl get -n rbac-manager serviceaccount/test-rbac-manager; do
+  let "counter=counter+1"
+  sleep 10
+  if [ $counter -gt 11 ]; then
+    break
+  fi
+done
+
+kubectl get -n rbac-manager serviceaccount/test-rbac-manager
+error=$(( error | $? ))
+if [ "$error" -eq 1 ]; then
+    >&2 echo "error: The Service account must exists"
+fi
+kubectl delete -n rbac-manager serviceaccount/test-rbac-manager
+kubectl get -n rbac-manager serviceaccount/test-rbac-manager
+error=$(( error | $? ))
+if [ "$error" -eq 1 ]; then
+    >&2 echo "error: The Service account must be recreated"
+fi
+
+# ImagePullSecret is created
+contents=$(kubectl get -n rbac-manager serviceaccount/test-rbac-manager -oyaml | yq 'select(.imagePullSecrets[] | .name == "robot-secret")')
+if [ -z "$contents" ]; then
+  error=$(( error | 1 ))
+fi
+if [ "$error" -eq 1 ]; then
+    >&2 echo "error: ImagePullSecret \"robot-secret\" must exists"
+fi
+
+# ImagePullSecret is re-created if deleted
+cat <<EOF | kubectl patch -n rbac-manager serviceaccount/test-rbac-manager --type=merge -p "$(cat -)"
+{
+  "imagePullSecrets": []
+}
+EOF
+contents=$(kubectl get -n rbac-manager serviceaccount/test-rbac-manager -oyaml | yq 'select(.imagePullSecrets[] | .name == "robot-secret")')
+if [ -z "$contents" ]; then
+  error=$(( error | 1 ))
+fi
+if [ "$error" -eq 1 ]; then
+    >&2 echo "error: ImagePullSecret \"robot-secret\" must be re-created"
+fi
+
+# If ImagePullSecret is added it should not be removed
+
+cat <<EOF | kubectl patch -n rbac-manager serviceaccount/test-rbac-manager --type=json -p "$(cat -)"
+[
+  {
+    "op": "add",
+    "path": "/imagePullSecrets/-",
+    "value": {
+      "name": "new-secret-name"
+    }
+  }
+]
+EOF
+contents=$(kubectl get -n rbac-manager serviceaccount/test-rbac-manager -oyaml | yq 'select(.imagePullSecrets[] | .name == "new-secret-name")')
+if [ -z "$contents" ]; then
+  error=$(( error | 1 ))
+fi
+if [ "$error" -eq 1 ]; then
+    >&2 echo "error: ImagePullSecret \"new-secret-name\" must be kept"
+fi
+
+exit $error
\ No newline at end of file
diff --git a/e2e/test.sh b/e2e/test.sh
index 52a637a2..dfdabfe7 100755
--- a/e2e/test.sh
+++ b/e2e/test.sh
@@ -1,6 +1,6 @@
 #!/bin/bash
 
-
+BASE_DIR=$(dirname $BASH_SOURCE)
 
 printf "\n\n"
 echo "**************************"
@@ -20,36 +20,7 @@ printf "\n\n"
 kubectl apply -f deploy/
 kubectl -n rbac-manager wait deployment/rbac-manager --timeout=120s --for condition=available
 
-
-printf "\n\n"
-echo "********************************************************************"
-echo "** Test rbacDefinition **"
-echo "********************************************************************"
-printf "\n\n"
-kubectl create clusterrole test-rbac-manager --verb="create" --resource=deployment
-
-cat <<EOF | kubectl create -f -
-apiVersion: rbacmanager.reactiveops.io/v1beta1
-kind: RBACDefinition
-metadata:
-  name: rbac-manager-definition
-rbacBindings:
-  - name: admins
-    subjects:
-      - kind: ServiceAccount
-        name: test-rbac-manager
-        namespace: rbac-manager
-    clusterRoleBindings:
-      - clusterRole: test-rbac-manager
-EOF
-
-# wait up to 2 minutes for rbac-manager to create the binding
-counter=0
-until kubectl get clusterrolebinding/rbac-manager-definition-admins-test-rbac-manager; do
-  let "counter=counter+1"
-  sleep 10
-  if [ $counter -gt 11 ]; then
-    break
-  fi
-done
-kubectl auth can-i create deployments --as=system:serviceaccount:rbac-manager:test-rbac-manager
+bash "$BASE_DIR/rbacdefinition/run.sh"
+if [ $? -ne 0 ]; then
+  exit 1
+fi
diff --git a/pkg/reconciler/matcher.go b/pkg/reconciler/matcher.go
index a4163134..6e3aaad1 100644
--- a/pkg/reconciler/matcher.go
+++ b/pkg/reconciler/matcher.go
@@ -15,8 +15,6 @@
 package reconciler
 
 import (
-	"reflect"
-
 	v1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -53,13 +51,36 @@ func rbMatches(existingRB *rbacv1.RoleBinding, requestedRB *rbacv1.RoleBinding)
 }
 
 func saMatches(existingSA *v1.ServiceAccount, requestedSA *v1.ServiceAccount) bool {
-	if metaMatches(&existingSA.ObjectMeta, &requestedSA.ObjectMeta) {
-		if len(requestedSA.ImagePullSecrets) < 1 && existingSA.ImagePullSecrets == nil {
-			return true
+	if !metaMatches(&existingSA.ObjectMeta, &requestedSA.ObjectMeta) {
+		return false
+	}
+	requestedSAManagedPullSecretsAnnotation, exists := requestedSA.Annotations[ManagedPullSecretsAnnotationKey]
+	if !exists {
+		return false
+	}
+
+	existingSAManagedPullSecretsAnnotation, exists := existingSA.Annotations[ManagedPullSecretsAnnotationKey]
+	if !exists {
+		return false
+	}
+
+	if requestedSAManagedPullSecretsAnnotation != existingSAManagedPullSecretsAnnotation {
+		return false
+	}
+
+	for _, requestedSAImagePullSecrets := range requestedSA.ImagePullSecrets {
+		matches := false
+		for _, existingSAPullSecret := range existingSA.ImagePullSecrets {
+			if requestedSAImagePullSecrets.Name == existingSAPullSecret.Name {
+				matches = true
+			}
+		}
+		if !matches {
+			return false
 		}
-		return reflect.DeepEqual(&existingSA.ImagePullSecrets, &requestedSA.ImagePullSecrets)
 	}
-	return false
+
+	return true
 }
 
 func metaMatches(existingMeta *metav1.ObjectMeta, requestedMeta *metav1.ObjectMeta) bool {
diff --git a/pkg/reconciler/matcher_test.go b/pkg/reconciler/matcher_test.go
index 3a55b106..4052793a 100644
--- a/pkg/reconciler/matcher_test.go
+++ b/pkg/reconciler/matcher_test.go
@@ -17,6 +17,7 @@ package reconciler
 import (
 	"testing"
 
+	v1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -182,3 +183,79 @@ func TestRbMatches(t *testing.T) {
 		t.Fatal("RB 3 should match RB 3")
 	}
 }
+
+func TestSAMatches(t *testing.T) {
+	sa1 := v1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "sample-name",
+			Namespace:       "sample",
+			OwnerReferences: generateOwnerReferences("foo"),
+			Annotations:     map[string]string{ManagedPullSecretsAnnotationKey: "fairwinds"},
+		},
+		ImagePullSecrets: []v1.LocalObjectReference{{Name: "fairwinds"}},
+	}
+	sa2 := v1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "sample-name",
+			Namespace:       "sample",
+			OwnerReferences: generateOwnerReferences("foo"),
+			Annotations:     map[string]string{ManagedPullSecretsAnnotationKey: "fairwinds"},
+		},
+		ImagePullSecrets: []v1.LocalObjectReference{{Name: "fairwinds"}},
+	}
+	sa3 := v1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "sample-name",
+			Namespace:       "sample",
+			OwnerReferences: generateOwnerReferences("foo"),
+			Annotations:     map[string]string{ManagedPullSecretsAnnotationKey: "fairwinds,another"},
+		},
+		ImagePullSecrets: []v1.LocalObjectReference{{Name: "fairwinds"}, {Name: "another"}},
+	}
+	sa4 := v1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "sample-name",
+			Namespace:       "sample",
+			OwnerReferences: generateOwnerReferences("foo"),
+			Annotations:     map[string]string{ManagedPullSecretsAnnotationKey: "fairwinds"},
+		},
+		ImagePullSecrets: []v1.LocalObjectReference{{Name: "fairwinds"}, {Name: "extra"}},
+	}
+	sa5 := v1.ServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "sample-name",
+			Namespace:       "sample",
+			OwnerReferences: generateOwnerReferences("foo"),
+			Annotations:     map[string]string{ManagedPullSecretsAnnotationKey: "fairwinds,another"},
+		},
+		ImagePullSecrets: []v1.LocalObjectReference{{Name: "fairwinds"}},
+	}
+
+	if !saMatches(&sa1, &sa2) {
+		t.Fatal("SA 1 should match SA 2")
+	}
+
+	if saMatches(&sa1, &sa3) {
+		t.Fatal("SA 1 should not match SA 3")
+	}
+
+	if !saMatches(&sa4, &sa1) {
+		t.Fatal("SA 4 should match SA 1")
+	}
+
+	if saMatches(&sa1, &sa5) {
+		t.Fatal("SA 1 should not match SA 3")
+	}
+
+	if saMatches(&sa4, &sa3) {
+		t.Fatal("SA 4 should not match SA 3")
+	}
+
+	if saMatches(&sa5, &sa3) {
+		t.Fatal("SA 5 should not match SA 3")
+	}
+
+	if saMatches(&sa5, &sa4) {
+		t.Fatal("SA 5 should not match SA 4")
+	}
+}
diff --git a/pkg/reconciler/parser.go b/pkg/reconciler/parser.go
index 06244ffc..7f0f42b9 100644
--- a/pkg/reconciler/parser.go
+++ b/pkg/reconciler/parser.go
@@ -18,6 +18,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/sirupsen/logrus"
 	v1 "k8s.io/api/core/v1"
@@ -39,6 +40,8 @@ type Parser struct {
 	parsedServiceAccounts     []v1.ServiceAccount
 }
 
+const ManagedPullSecretsAnnotationKey string = "rbacmanager.reactiveops.io/managed-pull-secrets"
+
 // Parse determines the desired Kubernetes resources an RBAC Definition refers to
 func (p *Parser) Parse(rbacDef rbacmanagerv1beta1.RBACDefinition) error {
 	if rbacDef.RBACBindings == nil {
@@ -70,12 +73,16 @@ func (p *Parser) parseRBACBinding(rbacBinding rbacmanagerv1beta1.RBACBinding, na
 			for _, secret := range requestedSubject.ImagePullSecrets {
 				pullsecrets = append(pullsecrets, v1.LocalObjectReference{Name: secret})
 			}
+			annotations := make(map[string]string)
+			managedPullSecrets := strings.Join(requestedSubject.ImagePullSecrets, ",")
+			annotations[ManagedPullSecretsAnnotationKey] = managedPullSecrets
 			p.parsedServiceAccounts = append(p.parsedServiceAccounts, v1.ServiceAccount{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:            requestedSubject.Name,
 					Namespace:       requestedSubject.Namespace,
 					OwnerReferences: p.ownerRefs,
 					Labels:          kube.Labels,
+					Annotations:     annotations,
 				},
 				ImagePullSecrets:             pullsecrets,
 				AutomountServiceAccountToken: requestedSubject.AutomountServiceAccountToken,