-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add rbac specificity for dbpurge
#21088
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
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
… jakehwll/dbpurge-rbac-specificity
dannykopping
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hell yeah 👍
It needs some tests please, though.
| { | ||
| Identifier: rbac.RoleIdentifier{Name: "dbpurge"}, | ||
| DisplayName: "DB Purge Daemon", | ||
| Site: rbac.Permissions(map[string][]policy.Action{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
… jakehwll/dbpurge-rbac-specificity
| Identifier: rbac.RoleIdentifier{Name: "dbpurge"}, | ||
| DisplayName: "DB Purge Daemon", | ||
| Site: rbac.Permissions(map[string][]policy.Action{ | ||
| rbac.ResourceSystem.Type: {policy.ActionDelete}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate we need system delete, but I see why
… jakehwll/dbpurge-rbac-specificity
mtojek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Feel free to merge.
Related to [`internal#1139`](coder/internal#1139) This implements some prometheus metrics for records being removed from the database. Currently we're tracking the following fields being removed from the DB by this. They're viewable in the `/api/v2/debug/metrics` endpoint. * `expired_api_keys` * `aibridge_records` * `connection_logs` * `duration` ``` # HELP coderd_dbpurge_iteration_duration_seconds Duration of each dbpurge iteration in seconds. # TYPE coderd_dbpurge_iteration_duration_seconds histogram coderd_dbpurge_iteration_duration_seconds_bucket{success="true",le="1"} 1 coderd_dbpurge_iteration_duration_seconds_bucket{success="true",le="5"} 1 coderd_dbpurge_iteration_duration_seconds_bucket{success="true",le="10"} 1 coderd_dbpurge_iteration_duration_seconds_bucket{success="true",le="30"} 1 coderd_dbpurge_iteration_duration_seconds_bucket{success="true",le="60"} 1 coderd_dbpurge_iteration_duration_seconds_bucket{success="true",le="300"} 1 coderd_dbpurge_iteration_duration_seconds_bucket{success="true",le="600"} 1 coderd_dbpurge_iteration_duration_seconds_bucket{success="true",le="+Inf"} 1 coderd_dbpurge_iteration_duration_seconds_sum{success="true"} 0.014787814 coderd_dbpurge_iteration_duration_seconds_count{success="true"} 1 # HELP coderd_dbpurge_records_purged_total Total number of records purged by type. # TYPE coderd_dbpurge_records_purged_total counter coderd_dbpurge_records_purged_total{record_type="aibridge_records"} 0 coderd_dbpurge_records_purged_total{record_type="audit_logs"} 0 coderd_dbpurge_records_purged_total{record_type="connection_logs"} 0 coderd_dbpurge_records_purged_total{record_type="expired_api_keys"} 0 coderd_dbpurge_records_purged_total{record_type="workspace_agent_logs"} 0 ``` | Position | Pull-request | | -------- | ------------ | | ✅ | [feat: add prometheus observability metrics for `dbpurge`](#21074) | | | [feat: add rbac specificity for `dbpurge`](#21088) |
|
@jakehwll the test you added works but it's overfitting. By that I mean it's basically reimplementing dbpurge by calling all of its methods. It does check that all the methods called have the appropriate RBAC permissions, but this became stale the moment it was written. When we change dbpurge behaviour (i.e. add new steps) those will either have to be reflected here or there will be a testing blindspot. Instead, I'd suggest that you extract the |
Related to
internal#1139Continuation of #21074
This implements some RBAC role specificity for
dbpurge, ensuring that we follow the least-privileged model for removing data from the database. It is specified as following.dbpurgedbpurge