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: support owners for synthetic privileges #86181

Closed
RichardJCai opened this issue Aug 15, 2022 · 3 comments
Closed

sql: support owners for synthetic privileges #86181

RichardJCai opened this issue Aug 15, 2022 · 3 comments
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. 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

@RichardJCai
Copy link
Contributor

RichardJCai commented Aug 15, 2022 •
edited
Loading

Right now synthetic privileges do not have owners.

We should support a way to specify owners, in the meanwhile defaulting to admin is okay

This is only specific to EXTERNAL CONNECTIONS right now, the question is do we want an owner column in the system.external_connections table or to create a system.object_owners table akin to system.privileges
Jira issue: CRDB-18618
Epic CRDB-11725
@RichardJCai RichardJCai added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. 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) labels Aug 15, 2022
adityamaru added a commit to adityamaru/cockroach that referenced this issue Aug 17, 2022
@adityamaru
externalconn: grant ALL on CREATE EXTERNAL CONNECTION
3d4400a 
Previously, the user that created the external connection
was not granted any privileges as the creator/owner of the
object. This diff changes this by granting the user `ALL`
privileges.

When synthetic privileges have a concept of owners, we should
also set the owner of the External Connection object to the
creator. This can happen once cockroachdb#86181
is addressed.

There was also a small bug in the grant/revoke code where
external connection names that required to be wrapped in
`""` egs: `"foo-kms"` were being incorrectly stringified when
creating the synthetic privilege path. This resulted in:

```
CREATE EXTERNAL CONNECTION "foo-kms" AS ...
GRANT USAGE ON EXTERNAL CONNECTION "foo-kms" TO baz
```

To fail with a "foo-kms" cannot be found, even though it was
infact created before attempting the grant. Tests have been added to
test this case.

Fixes: cockroachdb#86261

Release note (bug fix): Users that create an external connection
are now granted `ALL` privileges on the object.
@adityamaru adityamaru mentioned this issue Aug 17, 2022
Merged
adityamaru added a commit to adityamaru/cockroach that referenced this issue Aug 18, 2022
@adityamaru
externalconn: grant ALL on CREATE EXTERNAL CONNECTION
8b149a0 
Previously, the user that created the external connection
was not granted any privileges as the creator/owner of the
object. This diff changes this by granting the user `ALL`
privileges.

When synthetic privileges have a concept of owners, we should
also set the owner of the External Connection object to the
creator. This can happen once cockroachdb#86181
is addressed.

There was also a small bug in the grant/revoke code where
external connection names that required to be wrapped in
`""` egs: `"foo-kms"` were being incorrectly stringified when
creating the synthetic privilege path. This resulted in:

```
CREATE EXTERNAL CONNECTION "foo-kms" AS ...
GRANT USAGE ON EXTERNAL CONNECTION "foo-kms" TO baz
```

To fail with a "foo-kms" cannot be found, even though it was
infact created before attempting the grant. Tests have been added to
test this case.

Fixes: cockroachdb#86261

Release note (bug fix): Users that create an external connection
are now granted `ALL` privileges on the object.
craig bot pushed a commit that referenced this issue Aug 18, 2022
@DrewKimball @adityamaru
@ecwall @ericharmeling
Merge #86055 #86193 #86336 #86349 #86397
6e7e8e9 
86055: sql: add index recommendation to execution_insights table r=j82w a=j82w

Adds index recommendation to execution_insights table
part of #81024

Release justification: Category 2: Bug fixes and
low-risk updates to new functionality

Release note (sql change): Adds index recommendation
to execution_insights

86193: opt: use only required columns in provided ordering for project r=DrewKimball a=DrewKimball

Project operators can only pass through their input ordering.
However, the provided ordering may have to be remapped in order to
ensure it only refers to output columns, since the `Project` can add
and remove columns. The `Project` uses its `internalFDs` field to
accomplish the remapping; these are constructed when the `Project`
is added to the memo by combining the functional dependencies of the
input and the projections.

The problem occurs when transformation rules cause the input of the
`Project` to "reveal" additional functional dependencies. For example,
one side of a union may be eliminated and the FDs of the remaining side
used in the result. This can cause the `Project` to output an ordering
that is equivalent to the required ordering according to its own FDs,
but which a parent operator cannot tell is equivalent because its FDs
were calculated before the tranformation rule fired. This can cause
panics later down the line when the provided ordering does not match
up with the required ordering.

In the following example, an exploration rule transforms the join into
two joins unioned together, one over each disjunct. After the
transformation, a normalization rule fires that removes the
`t0.rowid IS NULL` side because rowids are non-null. This reveals the
`t1.rowid = t0.rowid` FD, which later causes `t0.rowid` to be used in
a provided ordering rather than `t1.rowid`. For the reasons mentioned
above, this later causes a panic when a `Project` attempts to remap to
the required `t1.rowid` ordering.
```
CREATE TABLE t0 (c0 INT);
CREATE TABLE t1 (c0 INT);

SELECT * FROM t0 CROSS JOIN t1
WHERE (t0.rowid IS NULL) OR (t1.rowid IN (t0.rowid))
ORDER BY t1.rowid;
```

This commit prevents the panic by making `Project` operators remap the
input provided ordering to use columns from the required ordering
(which are a subset of the output columns). This prevents the disparity
between required and provided orderings that can cause panics down the
line. In the example given above, the `t1.rowid` column would be chosen
for the provided ordering because it is in the required ordering.

Fixes #85393

Release note (bug fix): fixed a vulnerability in the optimizer that could
cause a panic in rare cases when planning complex queries with `ORDER BY`.

Release justification: fixes release blocker

86336: externalconn: grant `ALL` on `CREATE EXTERNAL CONNECTION` r=adityamaru a=adityamaru

Previously, the user that created the external connection
was not granted any privileges as the creator/owner of the
object. This diff changes this by granting the user `ALL`
privileges.

When synthetic privileges have a concept of owners, we should
also set the owner of the External Connection object to the
creator. This can happen once #86181
is addressed.

There was also a small bug in the grant/revoke code where
external connection names that required to be wrapped in
`""` egs: `"foo-kms"` were being incorrectly stringified when
creating the synthetic privilege path. This resulted in:

```
CREATE EXTERNAL CONNECTION "foo-kms" AS ...
GRANT USAGE ON EXTERNAL CONNECTION "foo-kms" TO baz
```

To fail with a "foo-kms" cannot be found, even though it was
infact created before attempting the grant. Tests have been added to
test this case.

Fixes: #86261

Release note (bug fix): Users that create an external connection
are now granted `ALL` privileges on the object.

Release justification: bug fix for new functionality

86349: ttl: replace SPLIT AT with SplitTable in tests r=otan a=ecwall

refs #85800
fixes #86376

SPLIT AT was causing occasional test failures if it did not asynchronously
split the range before the TTL job started. SplitTable synchronously splits
the range before the test starts the TTL job.

Release justification: Fix test flake.

Release note: None

86397: cluster-ui: added internal prefix to transaction workload insights view r=ericharmeling a=ericharmeling

Previously, the connected component for transaction workload insights was
using the selector for the sessions data slice to load the internal app
name prefix. This isn't necessary; we can just use a constant, as is done
on other pages.

Fixes #86250.

https://www.loom.com/share/18bf4edf46fb4b1dad2fe32f9d285dcd

Release justification: bug fixes and low-risk updates to new functionality

Release note: None

Co-authored-by: j82w <[email protected]>
Co-authored-by: DrewKimball <[email protected]>
Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>
@rafiss
Copy link
Collaborator

rafiss commented Aug 23, 2022

Is "owner" in this case the same as "grantor"?

@RichardJCai
Copy link
Contributor Author

Is "owner" in this case the same as "grantor"?

No this actually refers to the object owner, but perhaps that should be determined by the object instead? Ie do we want an owners column in system.externalconnections`

@RichardJCai
Copy link
Contributor Author

Gonna close this as this is specific to external connections right now and I believe it's better to add a column for owner in the EC table

cc @adityamaru

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. 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
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rafiss @RichardJCai