Verify behavior around pre-existing service accounts

[sudermanjr]

We don't currently have a known pattern around how to handle pre-existing service accounts. We should test what happens and decide on a defined policy.

[sudermanjr]

reported by @thejosephstevens

[thejosephstevens]

Hey all! The super quick feedback I'd give on the functionality here is, as a consumer I don't actually much care what the outcome is as long as it's in the docs. Part of the reason I landed in this spot is the existing documentation talks at length about generating all of the bindings at runtime, but it doesn't actually mention (unless I missed a section) anything about management of the subjects. Since there was no reference I assume it was BYO, and that turned out to work for our original installs. It also seemed to follow from the notion that it would set up bindings with other resources (like groups) that don't have a visible kube resource backing them.

FWIW, the spec I was running when I ran into my issues was this:

apiVersion: v1
kind: ServiceAccount
metadata:
  name: node-labeler
  namespace: infra
imagePullSecrets:
 - name: image-pull-creds
---
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: ClusterRole
metadata:
  name: node-labeler
rules:
- apiGroups: [""]
  resources: ["nodes"]
  verbs: ["list", "get", "patch"]
---
apiVersion: rbacmanager.reactiveops.io/v1beta1
kind: RBACDefinition
metadata:
  name: node-labeler
rbacBindings:
  - name: node-labeler
    subjects:
      - kind: ServiceAccount
        name: node-labeler
    clusterRoleBindings:
      - namespace: infra
        clusterRole: node-labeler

With this config, rbac-manager logged errors on creating the service account due to missing namespace (prior to investigation of this, it was hooking up to an existing SA just fine, so it may have been throwing an error without me noticing).

[thejosephstevens]

Further updates, it appears that if you reference an existing subject it doesn't cause a failure as I previously thought, the binding actually does get created correctly. However it does result in the rbac-manager throwing errors at runtime. What I would suggest from a user perspective here is, attempt to create the subject, but if the subject is already found, go ahead and adopt the discovered subject. That way you can get a best of both worlds where in my deployment processes I can deploy SAs ahead of time (so deploys can reference them and not run into race conditions waiting for rbac-manager to resolve state), but still get the stability guarantees that if I delete an SA, rbac-manager will create one and get me back to a stable state.

[lucasreed]

My only hangup with adopting a service account as it's own when the SA exists beforehand is that if you remove the RBACDefinition some time down the road, the service account will be deleted too. That would most likely be an undesirable outcome.

[sudermanjr]

My vote is to fail on the pre-existence of a serviceAccount. This would simplify the issue and ensure that rbac-manager remains the source of truth for those service accounts. This would also tie nicely into #103

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[lucasreed]

I added more documentation in this section of the README in #128

I did not change any behavior currently, so rbac-manager still logs an error if the serviceaccount is pre-existing but the permissions are indeed granted. Also rbac-manager does not delete the pre-existing service account when removing the RBACDefinition.

Happy to discuss other ideas for this in a new issue if this doesn't satisfy requirement, but I'll close this issue for now.