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.

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

sql: update SHOW GRANTS to include grant options #73394

Closed
rafiss opened this issue Dec 2, 2021 · 0 comments · Fixed by #75560
Closed

sql: update SHOW GRANTS to include grant options #73394

rafiss opened this issue Dec 2, 2021 · 0 comments · Fixed by #75560
Assignees
Labels
A-sql-privileges SQL privilege handling and permission checks. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@rafiss
Copy link
Collaborator

rafiss commented Dec 2, 2021

Once #67410 is completed, we will need to provide a way for users to see which GRANT options they have.

As the RFC describes (#72512), we will do this by adding a new column to SHOW GRANTS:

For example:

root@:26257/defaultdb> create table t (a int);
CREATE TABLE

root@:26257/defaultdb> create user u1;
CREATE ROLE

root@:26257/defaultdb> create user u2;
CREATE ROLE

root@:26257/defaultdb> create user u3;
CREATE ROLE

root@:26257/defaultdb> grant select on t to u1 with grant option;
GRANT

root@:26257/defaultdb> grant insert on t to u1;
GRANT

root@:26257/defaultdb> grant all on t to u2 with grant option;
GRANT

root@:26257/defaultdb> grant all on t to u3;
GRANT

root@:26257/defaultdb> grant select on t to u3 with grant option;
GRANT

root@:26257/defaultdb> grant insert on t to u3 with grant option;
GRANT

root@:26257/defaultdb> show grants on table t;
  database_name | schema_name | table_name | grantee | privilege_type | is_grantable
----------------+-------------+------------+---------+----------------+------------------
  defaultdb     | public      | t          | admin   | ALL            | True
  defaultdb     | public      | t          | root    | ALL            | True
  defaultdb     | public      | t          | u1      | INSERT         | False
  defaultdb     | public      | t          | u1      | SELECT         | True
  defaultdb     | public      | t          | u2      | ALL            | True
  defaultdb     | public      | t          | u3      | ALL            | False
  defaultdb     | public      | t          | u3      | INSERT         | True
  defaultdb     | public      | t          | u3      | SELECT         | True

Code to update:

  • The delegate queries in [show_grants.go(https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/delegate/show_grants.go).
  • Fill in the is_grantable column for information_schema.table_privileges and `information_schema.schema_privileges in information_schema.go. The column should be based on the grant option, and the code to populate it should be behind a version gate.
  • Add is_grantable column (or similar name) to information_schema.type_privileges and crdb_internal.cluster_database_privileges (also version gated).
  • NOTE: For the is_grantable column we'll need to update the logic around the ALL privilege a little. If the user does not have the ALL grant option, but they do have grant options on other privileges, we'll start showing a separate row for each grant option.

Epic CRDB-2587

@rafiss rafiss added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-privileges SQL privilege handling and permission checks. labels Dec 2, 2021
@blathers-crl blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Dec 2, 2021
craig bot pushed a commit that referenced this issue Jan 26, 2022
75226: sql: update SHOW GRANTS ON TABLE to include grant options r=rafiss,ajwerner a=ecwall

ref #73394

Release note: SHOW GRANTS ON TABLE includes is_grantable column

Co-authored-by: Evan Wall <[email protected]>
craig bot pushed a commit that referenced this issue Jan 27, 2022
72468: logictest: improve progress output r=andreimatei a=andreimatei

Logictests would print some progress messages to stdout as they run.
Each file prints when it ends and, while it's running, it prints
messages when statements finish successfully (but only up to once per
2s). This patch adds a second-level timestamp to those messages, in
order to provide more clues in the output of timed out logictest
packages. Otherwise, I'm always left guessing if the package had been
making constant progress until the timeout, or if it got stuck at some
point.

Release note: None

75095: sql: migrate has_schema_privilege from evalPrivilegeCheck to ctx.Plan… r=rafiss a=ecwall

…ner.HasPrivilege

refs #66173

Migrate has_schema_privilege from evalPrivilegeCheck to ctx.Planner.HasPrivilege.

Release note: None

75226: sql: update SHOW GRANTS ON TABLE to include grant options r=rafiss,ajwerner a=ecwall

ref #73394

Release note: SHOW GRANTS ON TABLE includes is_grantable column

Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
craig bot pushed a commit that referenced this issue Feb 2, 2022
75722: sql: update SHOW GRANTS ON SCHEMA to include grant options r=RichardJCai a=ecwall

ref #73394

Release note: SHOW GRANTS ON SCHEMA includes is_grantable column

Co-authored-by: Evan Wall <[email protected]>
craig bot pushed a commit that referenced this issue Feb 3, 2022
75854: sql: update SHOW GRANTS ON DATABASE to include grant options r=RichardJCai a=ecwall

refs #73394

Release note (sql): SHOW GRANTS ON DATABASE includes is_grantable column

75892: distsql: cleanup log tag annotation r=andreimatei a=andreimatei

A context annotation was not doing what it wanted; it was inadvertently
a no-op.

This patch also makes FlowCtx.AmbientCtx not be embedded. Embedded
AmbientCtxs are annoying because one can't easily find where they're
used.

Release note: None

75901: distsql: improve flow cleanup r=andreimatei a=andreimatei

FlowBase.Cleanup(ctx) was closing the tracing span from ctx, if any.
This was assuming that the respective ctx is the one associated with the
flow, and that the flow always has a span. These are brittle
assumptions; in particular, I intend to avoid opening spans for local
flows when there's no particular reason to (i.e. when not collecting
stats), to save on their cost.

This patch has the Flow explicitly keep track of which Span it owns, if
any.

Release note: None

75910: migration/migrations: skip TestEnsureSpanConfigReconciliation r=irfansharif a=otan

Refs: #75849

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None

Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
craig bot pushed a commit that referenced this issue Feb 5, 2022
75957: sql: update SHOW GRANTS ON TYPE to include grant options r=RichardJCai a=ecwall

refs #73394

Release note (sql change): SHOW GRANTS ON TYPE includes is_grantable column

75961: dev: integrate `bazel-remote` with `dev` r=rail a=rickystewart

* Add a new command `dev cache` which prepares the local cache.
  `dev cache --down` tears down the local cache, and `dev cache --reset`
  reboots it. My thinking is that we can extend the `dev cache` command
  with more stuff when we have it (e.g. remote caching).
* To this end we take the PID of the cache process and write it to a
  file in the cache directory. There's also a config file that users can
  manually update if they want.
* I request that people put this in their `~/.bazelrc`. It shouldn't be
  a problem if this applies to every Bazel workspace on the user's
  machine, it's just a cache.
* `dev doctor` now sets up the cache. It will now also fail unless your
  `~/.bazelrc` contains the string `--remote_cache=`. You can disable
  this with `--no-cache` or `DEV_NO_REMOTE_CACHE`.

Release note: None

76003: sql/catalog/bootstrap,*: allow system table IDs to be dynamic r=ajwerner a=ajwerner

This change is relatively small in terms of logic changes but it has large
implications for tests. The crux of the change is that we want to allow system
tables to not specify their IDs explicitly in their definition. The reason for
this is that we only reserved IDs 10-49 before handing out IDs to users. This
has the implication that new system tables in excess of 49 could overlap with
existing user descriptors. To cope with that, we'll need to use the descriptor
ID generator we normally use for user descriptors also for new system
descriptors in migrations.

Now, that could be the end of the story. The problem is that we really like
using data driven tests and we rather like printing out our keys, which
include descriptor IDs. If we were to carry on without changing the point
at which we started generating user descriptors, we'd run into real pain
every time we try to add another system table. All those datadriven tests
would have to change (and some other non-datadriven tests too).

This commit goes through that pain so hopefully for many years nobody else
has to. It does so by moving the first user descriptor we'll generate in a
newly bootstrapped cluster (or tenant) up to 100. This constant is not exported
and we can add mechanisms to override it later. This constant is chosen because
it still falls into the magical realm where the keys remain the same size.
It's also rather aesthetic. We could go bigger, but we'd pay a price for the
key for the first table you introduce. This probably doesn't matter, but I
don't want to be the one to do it.

Release note: None

Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
kvoli pushed a commit to kvoli/cockroach that referenced this issue Feb 7, 2022
refs cockroachdb#73394

Release note (sql): SHOW GRANTS ON DATABASE includes is_grantable column
@rafiss rafiss linked a pull request Feb 8, 2022 that will close this issue
@craig craig bot closed this as completed in #75560 Feb 9, 2022
craig bot pushed a commit that referenced this issue May 24, 2022
80494: server: add server-wide limit on addsstable send concurrency r=dt a=dt

While we generally prefer recipient-side limiting to better utilize all
available nodes' capacity, this adds an extra layer of limiter just in
case we find ourselves needing it. It defaults to unlimited.

Release note (sql change): The cluster setting bulkio.ingest.sender_concurrency_limit
can be used to adjust the concurrency at which any one SQL node, across all operations
that it is running such as RESTORES, IMPORTs, and schema changes, will send bulk ingest
requests to the KV storage layer.

Epic CRDB-2264

81177: ccl/sqlproxyccl: invoke rebalancing logic during RUNNING pod events r=JeffSwenson a=jaylim-crl

#### ccl/sqlproxyccl: invoke rebalancing logic during RUNNING pod events 

This commit invokes the rebalancing logic during RUNNING pod events as part of
the pod watcher. Since the rebalancing logic depends on the tenant directory,
the pod watcher will now only emit events once the directory has been updated.
This is done for better responsiveness, i.e. the moment a new SQL pod gets
added, we would like to rebalance all connections to the tenant.

Note that the Watch endpoint on the tenant directory server currently emits
events in multiple cases: changes to load, and changes to pod (added/modified/
deleted). The plan is to update the tenant directory server to only emit events
for pod updates. The next commit will rate limit the number of times the
rebalancing logic for a given tenant can be called.

At the same time, we introduce a new test static directory server which does
not automatically spin up tenants for us (i.e. SQL pods for tenants can now
be managed manually, giving more control to tests).

#### ccl/sqlproxyccl: rate limit the number of rebalances per tenant 

This commit rate limits the number of rebalances per tenant to once every
15 seconds (i.e. 1/2 of the rebalance loop interval). The main purpose of
this is to prevent a burst of pod events for the same tenant causing multiple
rebalances, which may move a lot of connections around.

Release note: None

81582: sql: add is_grantable column to SHOW GRANTS FOR role r=richardjcai a=ecwall

refs #73394

Release note (sql change): Add is_grantable column to
SHOW GRANTS FOR role to be consistent with other SHOW GRANTS
commands.

81776: bazel: bump size of `rangefeed` test r=rail a=rickystewart

This has timed out in CI.

Release note: None

Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Jay <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-privileges SQL privilege handling and permission checks. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants