feat!: limit controller secret access only to project namespaces #2761
Conversation
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com> Co-authored-by: Faeka Ansari <faeka6@gmail.com>
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2761 +/- ##
==========================================
- Coverage 49.54% 49.18% -0.37%
==========================================
Files 271 272 +1
Lines 19685 24152 +4467
==========================================
+ Hits 9753 11879 +2126
- Misses 9301 11641 +2340
- Partials 631 632 +1 ☔ View full report in Codecov by Sentry. |
|
This is on my list to review tomorrow before you start your day. |
internal/controller/management/serviceaccounts/serviceaccounts.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
krancour
force-pushed
the
limit-controller-access
branch
from
42561e7 to
7617617
Compare
| return errors.New("something went wrong") | ||
| }, | ||
| }, | ||
| assertions: func(t *testing.T, _ kargoapi.ProjectStatus, err error) { |
There was a problem hiding this comment.
trivial: we could add an assertion for projectStatus as well.
| predicate.Funcs{ | ||
| DeleteFunc: func(event.DeleteEvent) bool { | ||
| // We're only interested in soft deletes | ||
| return false | ||
| }, |
There was a problem hiding this comment.
oh, so are we are not filtering for update events 🤔
Were there any conditions that made the update predicate unnecessary or insufficient?
There was a problem hiding this comment.
these do not require changes: I am just curious
Fixes: #2448
Supersedes #2522
Don't be scurred... most of this is tests.
This is intended to mitigate sharded controllers (distributed to clusters other than the control plane's) having cluster-wide read on Secrets in the control plane's cluster. This will dynamically expand and contract the permissions of the Kargo controller(s) in a similar fashion to what's been done for the API server since #1486:
On Project create, add a RoleBinding per Kargo controller SA to grant r/o Secret access within the Project's namespace.
Nothing special to do on Project delete. Project delete takes the namespace with it, including any such RoleBindings.
On create/update of SA in Kargo's own namespace, ensure appropriate RoleBinding exists for every Project if the SA is labeled as a controller.
On delete of SA in Kargo's own namespace, ensure removal of appropriate RoleBinding for every Project.
On update of SA in Kargo's own namespace, ensure removal of appropriate RoleBinding for every Project if the SA is no-longer labeled as a controller.
Finalizers are added/removed as appropriate used to ensure necessary cleanup always occurs and that cleanup doesn't occur unnecessarily.
These changes are verified to work e2e.
Breaking change: If you have designated some namespaces as "global credential stores" using
controller.globalCredentials.namespacesat install-time, you must now manually ensure controller(s) have access to Secrets in those namespaces. The chart cannot do this because the namespaces may not exist at the time Kargo is installed. This also cannot be done dynamically with the management controller because if values are removed from the list and the management controller is restarted, it will not know what namespaces have been removed from the list and require clean up.@fykaa laid most of the groundwork for this. Thank you!
@gdsoumya requesting your 👀 on this, if you don't mind.