backups: include clusterroles/bindings that reference serviceaccounts #470
Conversation
skriss
force-pushed
the
backup-cluster-roles-and-bindings
branch
2 times, most recently
from
b482958 to
5084517
Compare
skriss
force-pushed
the
backup-cluster-roles-and-bindings
branch
from
5084517 to
dfe5b01
Compare
pkg/backup/service_account_action.go
Outdated
| a.log.Info("Running serviceAccountAction") | ||
| defer a.log.Info("Done running serviceAccountAction") | ||
|
|
||
| crbList, err := a.client.List(metav1.ListOptions{}) |
There was a problem hiding this comment.
I don't think we can use a real client here. Doing so would mean that we list all ClusterRoleBindings in the entire cluster every time we see a ServiceAccount.
We should either look into using a Lister from a SharedInformer, or caching the list per Backup UID (with something like a 15 minute expiration).
There was a problem hiding this comment.
ah, good point. I'll look into one of those.
There was a problem hiding this comment.
We could just fetch the ClusterRoleBindings in the action's constructor and store them for the life of the action since actions are created anew for each backup - WDYT?
skriss
force-pushed
the
backup-cluster-roles-and-bindings
branch
from
dfe5b01 to
b94f10d
Compare
|
LGTM, although we should probably add some logging indicating what's going on. We could either do it per plugin (when adding additional items), or globally in the item backupper. |
skriss
force-pushed
the
backup-cluster-roles-and-bindings
branch
from
b94f10d to
5598567
Compare
|
Added logging in the action (some of the other actions already do this, plus can include more context there on why the additional items are being included). |
pkg/backup/service_account_action.go
Outdated
| for _, roleBinding := range a.roleBindings { | ||
| for _, subj := range roleBinding.Subjects { | ||
| if subj.Kind == rbac.ServiceAccountKind && subj.Namespace == namespace && subj.Name == name { | ||
| a.log.Infof("Adding clusterrole %s and clusterrolebinding %s to additionalItems since serviceaccount %s/%s is a subject", |
There was a problem hiding this comment.
I would move the logging down to the bindings and roles loops, separately. WDYT?
There was a problem hiding this comment.
Oops, missed this comment. I had it there first then moved it :) I thought it was more informative to have the CR and CRB in the same log statement since they're linked.
There was a problem hiding this comment.
My thinking was that you might run into the same clusterrrole for multiple clusterrolebindings for a single serviceaccount, but this is fine as it is.
pkg/backup/service_account_action.go
Outdated
| // serviceAccountAction implements ItemAction. | ||
| type serviceAccountAction struct { | ||
| log logrus.FieldLogger | ||
| roleBindings []rbac.ClusterRoleBinding |
There was a problem hiding this comment.
Sorry for the late comment, but could you call this clusterRoleBindings? Every time I see roleBindings, I think of the non-cluster scoped version.
Signed-off-by: Steve Kriss <[email protected]>
skriss
force-pushed
the
backup-cluster-roles-and-bindings
branch
from
5598567 to
041cfc2
Compare
|
LGTM |
Signed-off-by: Steve Kriss [email protected]
Fixes #339
Add a backup item action for serviceaccounts that looks for clusterrolebindings that have the SA as a subject, and includes the clusterrolebinding & associated clusterrole in the list of additional items to backup.