server: make error handling around baseStatusServer.checkCancelPrivilege consistent #77676
Assignees
Labels
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Comments
matthewtodd
added
the
C-bug
craig bot
pushed a commit
that referenced
this issue
Mar 21, 2022
78176: server: proper error handling in sessions API r=xinhaoz a=xinhaoz Fixes #76288 Previously, part of the code in the sessions API that handles privelege checking would swallow non-privelege related server errors. The function `requireViewActivityOrViewActivityRedactedPermission` was being used to check if the user has either of the above roles. This function returns an error as a single value, which can be either a privelege or non-privelege related error. It is intended for use when either role is required to use the API, with the error returned immediately, however the sessions API is should work for users without these permissions if given the correct request parameters. This commit replaces the use of `requireViewActivityOrViewActivityRedactedPermission` with `hasRoleOption` to check for the possession of the VIEWACTIVITY or VIEWACTIVITYREDACTED roles. This allows us to use both the result of the role check and return errors encountered immediately. Release note: None 78180: server: checkCancelPrivelege should return proper gRPC error status r=xinhaoz a=xinhaoz Resolves #77676 The callers of `checkCancelPrivilege` expects returned errors to be proper gRPC error statuses. Previously, there was one plain error being returned that was not properly wrapped and logged. This commit converts that error into a serverError. Release note: None Co-authored-by: Xin Hao Zhang <[email protected]>
craig bot
pushed a commit
that referenced
this issue
Jan 27, 2023
95562: backupccl: explicitly parse SHOW BACKUP options r=benbardin a=msbutler Fixes: #82912 Release note (sql change): Previously, SHOW BACKUP options would get parsed as `kv_options`, which meant that a user could not pass multiple values to a show backup option, causing feature gaps in SHOW BACKUP relative to BACKUP and RESTORE. This patch rewrites the show backup option parser, closing the following feature gaps: 1. A user can now pass and check multiple KMS URIs in SHOW BACKUP 2. A user can pass locality aware incremental_locations, allowing a user to also pass the check_files parameter to a locality aware backup chain that also specifies the backup incremental location. Note that while this patch introduces a couple new words to the CRDB SQL syntax, the same SHOW BACKUP options should remain documented, specifically: - [public option] -> value - AS_JSON -> N/A - CHECK_FILES -> N/A - INCREMENTAL_LOCATION -> string, with potentially multiple uris - DEBUG_IDS -> N/A - KMS -> string, with potentially multiple uris - PRIVILEGES -> N/A - ENCRYPTION_PASSPHRASE -> string 95745: sql: remove redundant session iteration r=xinhaoz,yuzefovich a=ecwall Fixes #95743 Improves session/query cancelation with the following 1) Replaces session scanning by session ID with map lookup. 2) Replaces active query scanning by query ID with map lookup (session containing query to cancel is still scanned for). 3) Does not serialize entire session to get session username or id. Informs #77676 77676 was closed but some test cases incorrectly mentioned that addressing 77676 fixed them. This PR correctly fixes these test cases. Release note: None Co-authored-by: Michael Butler <[email protected]> Co-authored-by: Evan Wall <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
At the moment, some callers expect the returned errors to be
GrpcErrors, but they are not always. We should work through the caller needs and either makecheckCancelPrivilegealways returnGrpcErrors or make it never returnGrpcErrors and place the responsibility of wrapping them on the callers.Jira issue: CRDB-13708
The text was updated successfully, but these errors were encountered: