fix: fix PG coordinator context and RBAC subject #8223
Conversation
Signed-off-by: Spike Curtis <spike@coder.com>
| @@ -417,7 +417,7 @@ func (api *API) updateEntitlements(ctx context.Context) error { | |||
| if enabled { | |||
| var haCoordinator agpltailnet.Coordinator | |||
| if api.AGPL.Experiments.Enabled(codersdk.ExperimentTailnetPGCoordinator) { | |||
| haCoordinator, err = tailnet.NewPGCoord(ctx, api.Logger, api.Pubsub, api.Database) | |||
| haCoordinator, err = tailnet.NewPGCoord(api.ctx, api.Logger, api.Pubsub, api.Database) | |||
There was a problem hiding this comment.
nit: oh, I thought that this is the same context passed down
There was a problem hiding this comment.
I think so, but this is a much easier way to be sure.
| @@ -82,7 +84,21 @@ type pgCoord struct { | |||
| // NewPGCoord creates a high-availability coordinator that stores state in the PostgreSQL database and | |||
| // receives notifications of updates via the pubsub. | |||
| func NewPGCoord(ctx context.Context, logger slog.Logger, ps pubsub.Pubsub, store database.Store) (agpl.Coordinator, error) { | |||
| ctx, cancel := context.WithCancel(ctx) | |||
| ctx, cancel := context.WithCancel(dbauthz.As(ctx, rbac.Subject{ | |||
There was a problem hiding this comment.
I suspect that you learned about the need for RBAC context the hard way...
There was a problem hiding this comment.
I intellectually knew it, in as much as I helped with the design of dbauthz, but then just forgot when the time came.
There was a problem hiding this comment.
It would be safer if we can adopt any additional linter to spot these places. cc @johnstcn
There was a problem hiding this comment.
This is more easily caught by an integration test than a linter.
There was a problem hiding this comment.
very difficult to spot with a linter, because the subject could be added to the context anywhere along the stack down to the actual DB call.
There was a problem hiding this comment.
Yea, the current ruleguard linter we use doesn't have the capacity to do that kind of context analysis I don't think.
Testing should pick this stuff up pretty quick since it should always fail if you don't add a subject context.
Fixes two integration issues in the PG Coordinator noticed during system/scale testing: