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 e070403.

* fix: copy missing tests folder into e2e container

* feat: bump rok8s to @13 and ci-images to v13

.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:

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:/

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

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 | $? ))

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

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

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

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

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 | $? ))

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

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

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

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 {