Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sql: adding/removing locality config on first region add/last region drop doesn't account for privileges #61003

Closed
arulajmani opened this issue Feb 23, 2021 · 0 comments · Fixed by #62186
Closed

sql: adding/removing locality config on first region add/last region drop doesn't account for privileges #61003

arulajmani opened this issue Feb 23, 2021 · 0 comments · Fixed by #62186
Assignees
@arulajmani
Labels
A-multiregion Related to multi-region C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker

Comments

@arulajmani
Copy link
Collaborator

Describe the problem

For illustration, consider:

cockroach/pkg/sql/alter_database.go

Line 573 in 9e41034

if err := forEachTableDesc(ctx, p, desc, hideVirtual,

The API used here, ForEachTableDesc filters out descriptors that the user doesn't have visibility to (per privileges/ownership). This opens us up to a scenario where a user may add a region to a database but not modify all of the tables that exist inside the database with the default locality config. Later, when such a table is accessed, it will fail validation as we explicitly ensure all tables inside a MR database have a locality config set on them.

Expected behavior
In the illustration above, adding a region should fail if the user doesn't have permissions to modify all of the tables inside the database. Separately, the choice of API (ForEachTableDesc) is completely wrong here.

Additional context
This issue also manifests itself when we repartition Regional By Table tables when adding/dropping subsequent regions (albeit slightly differently).
@arulajmani arulajmani added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-multiregion Related to multi-region labels Feb 23, 2021
arulajmani added a commit to arulajmani/cockroach that referenced this issue Mar 18, 2021
@arulajmani
sql: ensure user has correct privileges when adding/removing regions
b9a1cb9 
Previously we did not account for privileges on database objects when
adding the default locality ocnfig on first region add or removing the
locality config on last region drop properly. In particular, we weren't
adding/removing the locality config on any descriptor that wasn't
visible to the user. This is bad because our validation logic expects
only and all objects in multi-region databases to have a valid locality
config. This means future accesses to such descriptors would fail
validation.

The root of this problem was the API choice here, `ForEachTableDesc`,
which filters out invisible descriptors. This patch instead switches
to using `forEachTableInMultiRegionDatabase`. While here, instead of
issuing separate requests for every table, I refactored this thing to
issue a single batch request instead.

Now that we view all the descriptors inside the database, unfiltered,
we perform privilege checks on them before proceeding with the add/drop
operation. In particular, the semantics are:
- admin users are allowed to add/drop regions as they wish.
- non admin-users require either the CREATE or ZONECONFIG privilege on
all the objects inside the database.

Closes cockroachdb#61003

Release note (sql change): `ALTER DATABASE .. SET PRIMARY REGION` now
requires both CREATE and ZONECONFIG privilege on all  objects inside
the database when adding the first region to the database. Same for
dropping the last region using `ALTER DATABASE ... DROP REGION`.
@arulajmani arulajmani mentioned this issue Mar 18, 2021
Merged
arulajmani added a commit to arulajmani/cockroach that referenced this issue Mar 19, 2021
@arulajmani
sql: ensure user has correct privileges when adding/removing regions
0ea0538 
Previously we did not account for privileges on database objects when
adding the default locality ocnfig on first region add or removing the
locality config on last region drop properly. In particular, we weren't
adding/removing the locality config on any descriptor that wasn't
visible to the user. This is bad because our validation logic expects
only and all objects in multi-region databases to have a valid locality
config. This means future accesses to such descriptors would fail
validation.

The root of this problem was the API choice here, `ForEachTableDesc`,
which filters out invisible descriptors. This patch instead switches
to using `forEachTableInMultiRegionDatabase`. While here, instead of
issuing separate requests for every table, I refactored this thing to
issue a single batch request instead.

Now that we view all the descriptors inside the database, unfiltered,
we perform privilege checks on them before proceeding with the add/drop
operation. In particular, the semantics are:
- admin users are allowed to add/drop regions as they wish.
- non admin-users require the CREATE privilege or must have ownership
on all the objects inside the database.

Closes cockroachdb#61003

Release note (sql change): `ALTER DATABASE .. SET PRIMARY REGION` now
requires both CREATE and ZONECONFIG privilege on all  objects inside
the database when adding the first region to the database. Same for
dropping the last region using `ALTER DATABASE ... DROP REGION`.
craig bot pushed a commit that referenced this issue Mar 22, 2021
@the-ericwang35 @aayushshah15
@rickystewart @arulajmani
Merge #61439 #61682 #61967 #62009 #62186
7899744 
61439: sql: reference sequences used in views by their IDs r=ajwerner a=the-ericwang35

Followup to #59864, fixes #61205.
Previously, a change was introduced to store sequences
used in DEFAULT expressions to be referenced by their
IDs instead of their names, to allow sequence renaming.
In this patch, we want to do something similar by
rewriting sequence references in views to be
referenced by their IDs. This allows sequences
used in views to be renamed.

Another thing this PR does is change the output of sequences
to look like `'s'::REGCLASS` rather than `'s':::STRING::REGCLASS`,
since we're no longer type checking them (since it's unnecessary).

Release note (sql change): reference sequences used in views
by their IDs to allow these sequences to be renamed.

61682: kvserver: teach replicateQueue to replace dead/decommisioning non-voters r=aayushshah15 a=aayushshah15

The PR is very similar to #59403.

Previously, non-voters on dead or decommissioning nodes would not get
removed or replaced. This commit fixes this behaviour.

Resolves #61626

Release justification: fixes limitation where dead/decommissioning
non-voters would not be removed or replaced
Release note: None


61967: kvserver: stop spuriously refusing non-voters in replicateQueue.shouldQueue() r=aayushshah15 a=aayushshah15

Before this commit, the `replicateQueue` would refuse to queue up a
replica into the queue (in its `shouldQueue` method) for the rebalancing
case unless it could verify that a voting replica needed to be
rebalanced. This was an unfortunate oversight since it meant that unless
there was a voting replica to be rebalanced, non-voters would not get
rebalanced by the queue.

This commit fixes this bug.

Noticed while debugging a flakey test for #61682

Release justification: bug fix
Release note: None

62009: build: perform `bazel clean --expunge` on `make clean` r=rickystewart a=rickystewart

Release note: None

62186: sql: ensure user has correct privileges when adding/removing regions r=richardjcai a=arulajmani

Previously we did not account for privileges on database objects when
adding the default locality config on first region add or removing the
locality config on last region drop properly. In particular, we weren't
adding/removing the locality config on any descriptor that wasn't
visible to the user. This is bad because our validation logic expects
only and all objects in multi-region databases to have a valid locality
config. This means future accesses to such descriptors would fail
validation.

The root of this problem was the API choice here, `ForEachTableDesc`,
which filters out invisible descriptors. This patch instead switches
to using `forEachTableInMultiRegionDatabase`. While here, instead of
issuing separate requests for every table, I refactored this thing to
issue a single batch request instead.

Now that we view all the descriptors inside the database, unfiltered,
we perform privilege checks on them before proceeding with the add/drop
operation. In particular, the semantics are:
- admin users are allowed to add/drop regions as they wish.
- non admin-users require the CREATE privilege or must have ownership
on all the objects inside the database.

Closes #61003

Release note (sql change): `ALTER DATABASE .. SET PRIMARY REGION` now
requires both CREATE and ZONECONFIG privilege on all  objects inside
the database when adding the first region to the database. Same for
dropping the last region using `ALTER DATABASE ... DROP REGION`.

Co-authored-by: Eric Wang <[email protected]>
Co-authored-by: Aayush Shah <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: arulajmani <[email protected]>
@craig craig bot closed this as completed in ae36b18 Mar 22, 2021
arulajmani added a commit to arulajmani/cockroach that referenced this issue Mar 23, 2021
@arulajmani
sql: ensure user has correct privileges when adding/removing regions
1704b7d 
Previously we did not account for privileges on database objects when
adding the default locality ocnfig on first region add or removing the
locality config on last region drop properly. In particular, we weren't
adding/removing the locality config on any descriptor that wasn't
visible to the user. This is bad because our validation logic expects
only and all objects in multi-region databases to have a valid locality
config. This means future accesses to such descriptors would fail
validation.

The root of this problem was the API choice here, `ForEachTableDesc`,
which filters out invisible descriptors. This patch instead switches
to using `forEachTableInMultiRegionDatabase`. While here, instead of
issuing separate requests for every table, I refactored this thing to
issue a single batch request instead.

Now that we view all the descriptors inside the database, unfiltered,
we perform privilege checks on them before proceeding with the add/drop
operation. In particular, the semantics are:
- admin users are allowed to add/drop regions as they wish.
- non admin-users require the CREATE privilege or must have ownership
on all the objects inside the database.

Closes cockroachdb#61003

Release note (sql change): `ALTER DATABASE .. SET PRIMARY REGION` now
requires both CREATE and ZONECONFIG privilege on all  objects inside
the database when adding the first region to the database. Same for
dropping the last region using `ALTER DATABASE ... DROP REGION`.
@arulajmani arulajmani mentioned this issue Mar 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arulajmani arulajmani

Labels
A-multiregion Related to multi-region C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: ensure user has correct privileges when adding/removing regions arulajmani/cockroach
2 participants
@arulajmani @ajstorm