Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic creation of service accounts based on rolebinding label selec… #310

Closed
wants to merge 2 commits into from

Conversation

tototoman
Copy link

@tototoman tototoman commented Jun 21, 2022

This PR fixes #

Checklist

  • I have signed the CLA
  • I have updated/added any relevant documentation

Description

What's the goal of this PR?

Based on #137
We have some similar need. We would like to create dynamically a service account in each namespace we request a role binding based on label selectors.
I see the difficulty here, it's not that obvious. This is a suggestion, and I'm ready to take into account your comments.

What changes did you make?

If a service account in an rbac definition does not have a specified namespace, we get the ones from the rolebindings if there are any.

What alternative solution should we consider, if any?

The current way is to have an exhaustive list of services accounts/namespaces.

@tototoman tototoman requested a review from sudermanjr as a code owner June 21, 2022 16:20
@CLAassistant
Copy link

CLAassistant commented Jun 21, 2022

CLA assistant check
All committers have signed the CLA.

@sudermanjr sudermanjr self-assigned this Jun 22, 2022
@sudermanjr
Copy link
Member

Thanks for the PR! I'll have to take some time to consider this and test out this implementation.

@tototoman
Copy link
Author

Hello,

Is there some news?
Can you please rerun the ci/circleci: insights

It shows the message: Vault token expired or not available!

Thanks

@sudermanjr
Copy link
Member

Sorry about the long tail on this. I've been re-familiarizing myself with this codebase lately, and I took a look at the implementation here.

I do really like the idea, and I think this is probably how we should have done this originally. However, I am really concerned about the lack of backward-compatibility here with the old way that rbac-manager created service accounts.

Do you have any thoughts on how we could manage that? It's possible we document the changes and do a major release, but I also want to consider how we could do this without breaking existing installations.

@tototoman
Copy link
Author

Sorry about the long tail on this. I've been re-familiarizing myself with this codebase lately, and I took a look at the implementation here.

I do really like the idea, and I think this is probably how we should have done this originally. However, I am really concerned about the lack of backward-compatibility here with the old way that rbac-manager created service accounts.

Do you have any thoughts on how we could manage that? It's possible we document the changes and do a major release, but I also want to consider how we could do this without breaking existing installations.

Thanks for your feedback, I appreciate.
Concerning the backward compatibility problem, the only difference is when someone creates a service account without specifying a namespace.

If I'm not wrong, in this case, the current context is used. This means we create the service account in the same namespace the rbac-manager is running on, while we give the permissions on the role binding to a service account with the same name, but in each of the rbac namespaces.

The second part does not change, we just make it explicit.
The only difference then is that we create with this change a service account in each namespace pointed by the selector if it does not exist, while previously the user had to create it by himself (Another CR, helm chart...).

There are indeed some situations where this could be a problem even if it's possible to find workarounds.

We could add an optional boolean to the Subject struct, by default to false, that allow to activate this feature. For example "useRoleBindingsSelectors". It will be used only if the subject is a service account and ignored on cluster role bindings.

Let me know if this solution fits you so I can update my PR accordingly,

@github-actions github-actions bot added the stale Marked as stale by stalebot label Sep 28, 2022
@github-actions github-actions bot closed this Oct 6, 2022
@bb-Ricardo
Copy link

Hi,

Any reason why this PR was just closed? Would really need this feature as it would reduce repetitive role binding definitions by a lot!

Thank you

@tototoman
Copy link
Author

Thanks for tour support. I have the same question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Marked as stale by stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants