sql: disallow GRANT/REVOKE on system tables #53683
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes cockroachdb#43842. Disallow GRANT/REVOKE operations on system objects to avoid potential deadlocks related to version bumps. Release justification: bug fix Release note (sql change): Disallow `GRANT/REVOKE` operations on system tables. Release note (bug fix): Fix a bug where `GRANT/REVOKE` on the `system.lease` table would result in a deadlock.
|
This change is |
ajwerner
approved these changes
Aug 31, 2020
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang)
|
It seems like we have some migrations and tests that depend on this behavior. Would it be reasonable to go back to disallowing grants on just the |
|
Do the tests seem reasonable? Should we change the tests instead? |
|
Yeah nvm the tests, we can always change those. The problem is just this |
Did we properly bake it in? If so, then I suppose yes. If not, I propose we rework this PR to allow grant and revoke by the |
ajwerner
reviewed
Mar 3, 2021
There was a problem hiding this comment.
@RichardJCai any interest in sprucing this up?
Reviewable status:
Sure I'll take this over |
|
Closing in favour of #61410 |
craig bot
pushed a commit
that referenced
this pull request
Mar 3, 2021
61214: importccl: avoid random() collisions between KV batches r=pbardea a=pbardea An import may parallelize the work to convert SQL rows into KVs. During this phase, default expressions are evaluated. Previously, IMPORT's implementations of random number generates that are evaluated in default expressions assumed that all of the given row IDs were contiguous. This is not the case since 1 row converter may be responsible for converting several non-contiguous batches of rows. This resulted in random_values colliding between different batches of KV space. This commit fixes this bug by feeding in the current position and resetting the random source backing these methods. This ensures that import treats a new contiguous batch of rows separately. Fixes #61203. Release justification: bug fix Release note (bug fix): Fix a bug where random numbers generated as default expressions during IMPORT would collide a few hundred rows apart from eachother. 61300: sql: set zone configs before backfill on ALTER TABLE LOCALITY/PRIMARY KEY r=arulajmani,ajstorm a=otan Ensure that the zone configurations are correctly set before the backfill runs, so data is backfilled to the correct location upfront for REGIONAL BY ROW tables. Note this bug also exists on existing ALTER PRIMARY KEY statements. Release justification: bug fixes and low-risk updates to new functionality Release note (bug fix): Fix bug where zone configurations on indexes were not copied before the backfill of an ALTER PRIMARY KEY. They used to be copied afterwards instead. 61349: sql: add is_temporary and is_virtual columns to crdb_internal.create_statements r=rafiss a=RichardJCai These columns are useful for filtering for generating output for SHOW CREATE ALL TABLES. Release justification: None, change to internal table. Release note: None 61356: server: fix node decommissioning itself r=knz,tbg a=erikgrinaker Previously, if a node was asked to decommission itself, the decommissioning process would sometimes hang or fail since the node would become decommissioned and lose RPC access to the rest of the cluster while it was building the response. This patch returns an empty response from the `adminServer.Decommission()` call when setting the final `DECOMMISSIONED` status, thereby avoiding further use of cluster RPC after decommissioning itself. It also defers self-decommissioning until the end if multiple nodes are being decommissioned. This change is backwards-compatible with old CLI versions, which will simply output the now-empty status result set before completing with "No more data reported on target nodes". The CLI has been updated to simply omit the empty response. Resolves #56718. A separate commit ensures errors with `codes.PermissionDenied` abort RPC retries and return immediately, to prevent operations hanging with indefinite internal retries when RPC access is lost. Release justification: bug fixes and low-risk updates to new functionality Release note (bug fix): Fixed a bug from 21.1-alpha where a node decommissioning process could sometimes hang or fail when the decommission request was submitted via the node being decommissioned. 61376: opt: prune unnecessary columns in uniqueness checks r=mgartner a=mgartner Projects now wrap semi-joins of unique checks which pass-through the columns of the unique constraint. This allows normalization rules to prune unnecessary columns from the expression. Release justification: This is a low-risk change to new functionality, implicitly partitioned unique indexes. Release note (performance improvement): The columns fetched for uniqueness checks of implicitly partitioned unique indexes are now pruned to only include columns necessary for determining uniqueness. 61410: sql: disallow GRANT/REVOKE on system tables r=RichardJCai a=RichardJCai Disallow GRANT/REVOKE operations on system objects to avoid potential deadlocks related to version bumps. Release justification: bug fix Release note (sql change): Disallow `GRANT/REVOKE` operations on system tables. Release note (bug fix): Fix a bug where `GRANT/REVOKE` on the `system.lease` table would result in a deadlock. Fixes #43842 Picking up PR from here: #53683 It seems like the `system.comments` migration that used the GRANT/REVOKE is gone now (comment here: #53683 (comment)) 61427: geomfn: prevent smaller densifyFrac for ST_{Frechet,Hausdorff}Distance r=rafiss a=otan Release justification: low-risk change to existing functionality Resolves #61367. Release note (sql change): Prevent densifyFracs < 1e-6 for ST_FrechetDistance and ST_HausdorffDistance to protect panics and out of memory errors. Co-authored-by: Paul Bardea <pbardea@gmail.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com> Co-authored-by: richardjcai <caioftherichard@gmail.com> Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #43842.
Disallow GRANT/REVOKE operations on system objects to avoid potential
deadlocks related to version bumps.
Release justification: bug fix
Release note (sql change): Disallow
GRANT/REVOKEoperations on systemtables.
Release note (bug fix): Fix a bug where
GRANT/REVOKEon thesystem.leasetable would result in a deadlock.