sql,backupccl: allow owners to also control schedules #87287
Conversation
|
This change is |
|
First commit is from #87188. |
| datums, cols, err := params.ExecCfg().InternalExecutor.QueryRowExWithCols( | ||
| params.ctx, | ||
| "load-schedule", | ||
| params.p.Txn(), sessiondata.InternalExecutorOverride{User: username.RootUserName()}, | ||
| params.p.Txn(), sessiondata.InternalExecutorOverride{User: username.NodeUserName()}, |
There was a problem hiding this comment.
happy to support this, but why? :)
There was a problem hiding this comment.
My understanding is that NodeUser is what we should be using for intra-cluster traffic -
There was a problem hiding this comment.
Should it be running on its own behalf? Or should loadSchedule take a context and run as the user executing the query?
More concretely, I guess, can we imagine a world where we want a user to see/load some schedules but not others? (yes, I think?)
There was a problem hiding this comment.
Or should loadSchedule take a context and run as the user executing the query?
This would be tricky because only admin is allowed to select against a system table. So a non-admin owner would get an error running loadSchedule. The scan of the system table is an internal implementation detail that expects other pieces of the logic to perform the appropriate privilege checks.
can we imagine a world where we want a user to see/load some schedules but not others
Yes, I think so, but "load" is an internal implementation detail so I think the filtering needs to be in the logic that uses the loaded schedule, which is what we're doing here. We are already allowing users to only alter a limited set of schedules if they are non-admin, i.e. the schedules they own. Similarly if we want to limit what schedules a user sees we should be adding this privilege check to SHOW SCHEDULES and other user facing queries that allow for introspection.
There was a problem hiding this comment.
expects other pieces of the logic to perform the appropriate privilege checks
Yeah, exactly this - I'd worry about a developer forgetting that. It's not obvious to me that somebody calling something called loadSchedule() will be executing node-level privileges under the hood. That said, if it's obvious to you (i.e. common practice :) ) then I'm fine with it.
| isOwner := schedule.Owner() == params.p.User() | ||
| if !isAdmin && !isOwner { | ||
| return pgerror.Newf(pgcode.InsufficientPrivilege, "must be admin or owner of the "+ | ||
| "schedule %d to %s it", schedule.ScheduleID(), n.command.String()) |
There was a problem hiding this comment.
is n.command.String() helpful info here?
There was a problem hiding this comment.
🤷 it makes for a more informative error message that you're not allowed to RESUME, DROP, PAUSE it, but I guess you already know that because you're running the command.
adityamaru
force-pushed
the
do-our-own-owner-management
branch
2 times, most recently
from
44774f5 to
6bb4d6d
Compare
adityamaru
force-pushed
the
do-our-own-owner-management
branch
2 times, most recently
from
490f378 to
5b91f9d
Compare
|
The race failures are all timeouts and are being investigated here https://cockroachlabs.slack.com/archives/C023S0V4YEB/p1662488784308579. I have high confidence that they are not caused by this diff so going ahead and borsing while we investigate. bors r=benbardin |
|
Merge conflict. |
adityamaru
force-pushed
the
do-our-own-owner-management
branch
from
5b91f9d to
cea5360
Compare
This change relaxes the admin only check that was enforced when resuming, pasuing, dropping and altering a backup schedule to also allow non-admin owners of the schedules to perform these control operations. Release note (sql change): Owners of a backup schedule can now control their schedule using the supported pause, resume, drop and alter queries. Release justification: high impact change to introduce finer grained permission control to disaster recovery operations
adityamaru
force-pushed
the
do-our-own-owner-management
branch
from
cea5360 to
0e5371e
Compare
|
Unrelated failures being investigated: https://cockroachlabs.slack.com/archives/C023S0V4YEB/p1662580252355849 bors r=benbardin |
|
Build succeeded: |
|
blathers backport release-22.2 |
This change relaxes the admin only check that was enforced
when resuming, pasuing, dropping and altering a backup schedule
to also allow non-admin owners of the schedules to perform
these control operations.
Release note (sql change): Owners of a backup schedule can now
control their schedule using the supported pause, resume, drop
and alter queries.
Release justification: high impact change to introduce finer grained
permission control to disaster recovery operations