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: grant/revoke privileges for udfs #85874

Merged
merged 4 commits into from
Aug 12, 2022

Conversation

chengxiong-ruan
Copy link
Contributor

There are 4 commits:
(1) add syntax support for grant/revoke privileges of function.
(2) implement grant/revoke privilege for functions (mostly just reading correct descriptors)
(3) fix a minor issue when hydrating a mutable function which use table implicit type (mutable table implicit type cannot be fetched)
(4) populate information_schema.role_routine_grants, and we can test privileges on that.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@chengxiong-ruan chengxiong-ruan force-pushed the udf-grant-revoke branch 3 times, most recently from 785b324 to 68b2221 Compare August 10, 2022 14:20
@chengxiong-ruan chengxiong-ruan marked this pull request as ready for review August 10, 2022 15:50
@chengxiong-ruan chengxiong-ruan requested review from a team August 10, 2022 15:50
@chengxiong-ruan chengxiong-ruan requested a review from a team as a code owner August 10, 2022 15:50
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

I'd like to ask you to try out my suggestion in 3. Otherwise, :lgtm:

Reviewed 9 of 9 files at r1, 8 of 8 files at r2, 3 of 3 files at r3, 2 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @chengxiong-ruan)


pkg/sql/information_schema.go line 1594 at r4 (raw file):

var informationSchemaRoleRoutineGrantsTable = virtualSchemaTable{
	// TODO(chengxiong): add builtin function privileges as well.
	comment: "privileges granted on functions (incomplete; only contains privileges of user-defined functions)",

Should we file an issue to list privileges for the builtins?


pkg/sql/catalog/descs/hydrate.go line 268 at r3 (raw file):

	return func(ctx context.Context, id descpb.ID) (tree.TypeName, catalog.TypeDescriptor, error) {
		var typDesc catalog.TypeDescriptor
		typDesc, err := tc.GetMutableTypeVersionByID(ctx, txn, id)

What would happen if we always got an immutable type here with AvoidLeased? I have a feeling that the choice to get a Mutable type here is vestigial. I'd be curious to learn if anything breaks. I think always resolving an immutable here would be strictly better if it doesn't cause any problems.

Copy link
Contributor Author

@chengxiong-ruan chengxiong-ruan left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)


pkg/sql/information_schema.go line 1594 at r4 (raw file):

Previously, ajwerner wrote…

Should we file an issue to list privileges for the builtins?

Yes, I'll file one.


pkg/sql/catalog/descs/hydrate.go line 268 at r3 (raw file):

Previously, ajwerner wrote…

What would happen if we always got an immutable type here with AvoidLeased? I have a feeling that the choice to get a Mutable type here is vestigial. I'd be curious to learn if anything breaks. I think always resolving an immutable here would be strictly better if it doesn't cause any problems.

yes, I believe getting an immutable with AvoidLeased will work here since we just want the fresh metadata and not really mutate anything. I'll make the change.

Copy link
Contributor Author

@chengxiong-ruan chengxiong-ruan left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)


pkg/sql/information_schema.go line 1594 at r4 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Yes, I'll file one.

oh, I don't need to file one, I'll reuse the existing issue: #84297


pkg/sql/catalog/descs/hydrate.go line 268 at r3 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

yes, I believe getting an immutable with AvoidLeased will work here since we just want the fresh metadata and not really mutate anything. I'll make the change.

changed it to always get a fresh immutable, also changed to get immutbale db and schema.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 3 files at r7.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

@chengxiong-ruan
Copy link
Contributor Author

TFTR!
bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Aug 11, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 12, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 12, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 12, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 12, 2022

Merge conflict.

This commits extends existing privilege statement syntax to
support user-defined functions.

Release note: None
This commit implements  support for GRANT...ON FUNCTION statements and
REVOKE...ON FUNCTION statements for udfs.

Release note: None
With UDF introduced, it's getting more often than before that
we need to hydrate a table implicit type. Before this commit
we always try to get a mutable type when hydrating a mutable
descriptor. However, getting a mutable table implicit type is
not allowed (which is good). But this defense breaks hydration
of mutable function which use a table implict type. In this
commit, we change it to always fetch immutable types but with
`AvoidedLease` flag. Database and Schema are also changed to
fetch a fresh immutable.

Release note: None
Previously the role_routine_grants table was unimplemented.
This commit populates the table with user-defined functions
privileges and leaves a TODO for all builtin functions.

Release note: None
@chengxiong-ruan
Copy link
Contributor Author

🙏
bors r+

@craig
Copy link
Contributor

craig bot commented Aug 12, 2022

Build succeeded:

@craig craig bot merged commit 504b437 into cockroachdb:master Aug 12, 2022
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.

3 participants