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

feat: manage mappings for pre-defined service accounts #1548

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

krancour
Copy link
Member

Fixes #1368

Adds two new ServiceAccount resources (kargo-admin and kargo-viewer) if the API server and OIDC are both enabled. (Without both of those enabled, it seems we would not need these.) How these are mapped to claims like sub, email, or groups can be specified at the time of Kargo installation.

Each of these new ServiceAccounts are bound to appropriate cluster-level permissions. For kargo-admin, those permissions are a near-complete subset of the API server's own permissions. (Access to create SubjectAccessReviews is not needed.) For kargo-viewer, those permissions are an even narrower subset of the API server's permissions.

The kargo-admin SA does not get any permissions the API server itself does not also get. This includes not having access to Secret resources cluster-wide. Since #1486, we dynamically expand the API server's permissions to include access to Project-specific Secrets as Projects are created. This PR uses the same mechanism to dynamically expand the kargo-admin ServiceAccount's permissions as well.

cc @gdsoumya

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Copy link

netlify bot commented Feb 27, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit 1df56a5
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/65df8ca4755d410008e587b7
😎 Deploy Preview https://deploy-preview-1548.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.46%. Comparing base (6b8d3ef) to head (4939335).
Report is 6 commits behind head on main.

❗ Current head 4939335 differs from pull request most recent head 1df56a5. Consider uploading reports for the commit 1df56a5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1548      +/-   ##
==========================================
+ Coverage   30.30%   30.46%   +0.15%     
==========================================
  Files         190      189       -1     
  Lines       19874    19908      +34     
==========================================
+ Hits         6023     6064      +41     
+ Misses      13593    13577      -16     
- Partials      258      267       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jessesuen
Copy link
Member

jessesuen commented Feb 27, 2024

I see that these will hardwire to the install namespace (.Release.Namespace).

There was an idea (perhaps even proposed by you) where these ServiceAccounts did not necessarily have to reside in the kargo installation namespace. The idea being kargo install namespace would remain purely for the kargo workloads, and configuration could be managed elsewhere.

WDYT about making this configurable? e.g. I'd like the ability to install these in like a kargo-global namespace.

@krancour
Copy link
Member Author

When I proposed that, my intent was to avoid people messing with the kargo namespace. Or to at least avoid normalizing that. It bothers me less here because it's the chart managing the SA and not a person.

That said, the decision to put these SAs in the kargo ns here had more to do with needing to put them in a ns that I know for sure will exist.

It's unconventional, but I guess I could make the chart manage a second namespace. I'll tinker with that tomorrow.

@jessesuen
Copy link
Member

Offline discussion:

Because the controller has the logic to dynamically expand permissions for project-specific secrets, I think it makes sense to treat these ServiceAccounts as special, always existing. So I've reversed my thoughts on this and think that kargo-admin and kargo-viewer should be special, well-known ServiceAccounts that people can bind SSO claims to as an operator-level decision, and reside in the kargo install namespace.

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
@krancour krancour added this pull request to the merge queue Feb 28, 2024
Merged via the queue into akuity:main with commit 5b955f6 Feb 28, 2024
14 checks passed
@krancour krancour deleted the krancour/predefined-sas branch February 28, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Predefined ServiceAccounts to improve SSO/RBAC UX
2 participants