Skip to content

sql: filter out zones from crdb_internal.zones if user has no privilege#42046

Closed
otan wants to merge 1 commit intocockroachdb:masterfrom
otan-cockroach:otan-crdb_internal
Closed

sql: filter out zones from crdb_internal.zones if user has no privilege#42046
otan wants to merge 1 commit intocockroachdb:masterfrom
otan-cockroach:otan-crdb_internal

Conversation

@otan
Copy link
Copy Markdown
Contributor

@otan otan commented Oct 30, 2019

Addresses the fix suggested by #40917 -- but doesn't actually fix the SHOW ZONE CONFIGURATION issue - only SHOW ALL ZONE CONFIGURATIONS -- i can fix the SHOW ZONE separately. I still think this fix is worthwhile

If we do not have permissions in a table for a specific zone, do not show it for the crdb_internal.zone virtual table

Release note (bug fix): filter out zones from crdb_internal.zones if user has no privileges

@otan otan requested a review from a team October 30, 2019 21:53
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@otan otan requested a review from solongordon October 30, 2019 22:02
@otan otan force-pushed the otan-crdb_internal branch 4 times, most recently from fa4d536 to 8b8f309 Compare October 31, 2019 15:36
@otan
Copy link
Copy Markdown
Contributor Author

otan commented Oct 31, 2019

(i think the test is flaky)

Release note (bug fix): filter out zones from crdb_internal.zones if
user has no privileges
@otan otan force-pushed the otan-crdb_internal branch from 8b8f309 to 6de55a1 Compare October 31, 2019 16:46
22 RANGE liveness

query TT
SELECT * FROM [SHOW ALL ZONE CONFIGURATIONS] ORDER BY 1
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realised this doesn't work, woops.

@otan
Copy link
Copy Markdown
Contributor Author

otan commented Oct 31, 2019

realised that this doesn't actually fix SHOW ALL - i'm going to close this and file a separate ticket as it's a little bit more hairy to fix than the ticket referenced.

@otan otan closed this Oct 31, 2019
craig bot pushed a commit that referenced this pull request Oct 31, 2019
41914: sqlmigrations: don't run baked-in migrations r=andreimatei a=andreimatei

Before this patch, the migration runner had a mechanism by which it
attempted to not run migrations that were included in the cluster
bootstrap schema. That mechanism was weak. A migration could be marked
as includedInBootstrap, and then the migration runner would skip it if
the current node had just performed a bootstrap. However, the respective
migration would still be run (aimlessly) by a 2nd node joining the
cluster, or even by the same node that just skipped them, after a restart.

This patch improves the mechanism such that migrations included in the
bootstrap image are never run if the cluster's bootstrap version is high
enough. It does so by allowing one to mark migrations with the version
at which they've been introduced, and the runner will skip them if the
cluster's bootstrap version is >=.  This means that migrations that want
to be marked in this way also need to introduce a cluster version.

The benefit of this change, besides faster node startup time, removal of
confusing entries from the system.event_log table and more
determinism (because now some migrations are never run instead of not
run on single node clusters but run on multi-node clusters), is that
logic tests that hardcode expected cluster events are more easily
written. Before this patch, those tests had to explicitly ignore some
events that are generated by migrations (because, again, depending on
the configuration, the migration would sometimes run and sometimes not).
It's now the second time this happens to a change that I personally want
to make, and it's very surprising and scary when it happens to you. You
can see the edit in the event_log logic test for a case that I'm not
correcting (there's probably others in the same file but I haven't
looked).

Release note: None.

41991: storage/engine: move MVCCIncrementalIterator from storageccl to engine r=petermattis a=hueypark

This is prerequisites for [storage/engine: add Pebble support to ExportToSst](#41739). Recovered some tests from [engine: Add ExportToSst method with all logic moved to C++](75f1314).

See #41945

Release note: None

42066: sql: SHOW ZONE CONFIGURATION only if user has permissions r=otan a=otan

Previously, SHOW ZONE CONFIGURATION did not respect permissions on
tables. Patch that up by checking permissions when using this command.

Resolves the command in #40917.

Has merge conflicts with #42046; I will fix them up with whatever lands first.

Release note (bug fix): fix bug where zones would display in `SHOW ZONE CONFIGURATION` even though the user has no permission to view that table

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants