backupccl: fix EXECUTE privileges #114203

mgartner · 2023-11-10T00:48:26Z

backupccl: fix EXECUTE privileges

This commit fixes some bugs with restoring UDFs from a backup:

Restoring UDFs from a backup created on a v23.1 cluster will have
EXECUTE privilege added for the public role, matching the new
default privileges for functions in v23.2. This ensures that the
backed-up functions can be executed without privilege errors.
Restoring a full cluster from a v23.2 backup with UDFs will retain
the original privileges of the UDF. For example, if EXECUTE
privileges on the public role have been revoked, they will remain
revoked after the restore.
Restoring UDFs from a partial v23.2 backup, i.e., not a full cluster
backup, will now have default privileges, including EXECUTE
privilege for the public role.

There is no release note because no release includes this issue.

Release note: None

upgrade: remove grantExecuteToPublicOnAllFunctions upgrade

The grantExecuteToPublicOnAllFunctions upgrade is no longer necessary
because post-serialization changes now grant EXECUTE on functions to
the public role. The upgrade has been removed.

Release note: None

cockroach-teamcity · 2023-11-10T00:48:35Z

This change is

dt · 2023-11-10T02:51:07Z

backupccl test added here LGTM in terms of mechanics, however I'll defer to more sql/foundations people on the actual behaviors asserted as desired in the test and all the pkg/sql and catalog changes

blathers-crl · 2023-11-10T15:28:54Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

mgartner · 2023-11-10T20:58:05Z

pkg/sql/catalog/catpb/privilege.go

+	// Version23_2 corresponds to descriptors created in 23.2 and onwards. These
+	// descriptors that are function descriptions should have EXECUTE privileges
+	// for the public role.
+	Version23_2


Does this require a new cluster version? I'm seeing TestSystemDatabaseSchemaBootstrapVersionBumped fail with:

Check whether you need to bump systemschema.SystemDatabaseSchemaBootstrapVersion and then update prevSystemHash to "7d089e4c69d8d6b164b3d1a04e0f3ad5f487f9138c9585bd00b37f397691a090". The current value of SystemDatabaseSchemaBootstrapVersion is 1000023.1-32.

mgartner · 2023-11-10T21:10:36Z

pkg/sql/catalog/bootstrap/bootstrap_test.go

@@ -164,7 +164,7 @@ func TestSystemDatabaseSchemaBootstrapVersionBumped(t *testing.T) {

 	// If you need to update this value (i.e. failed this test), check whether
 	// you need to bump systemschema.SystemDatabaseSchemaBootstrapVersion too.
-	const prevSystemHash = "cd85ebe773d840ecaf39fd4ccbba6345f54d7414a95b5bf5be2fa4102c26c650"
+	const prevSystemHash = "7d089e4c69d8d6b164b3d1a04e0f3ad5f487f9138c9585bd00b37f397691a090"


Is it ok to bump this without updating ystemschema.SystemDatabaseSchemaBootstrapVersion in this case? The system schema should have no UDFs, AFAIK, so I thought just bumping this would be fine.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)

pkg/sql/catalog/catpb/privilege.go line 47 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Does this require a new cluster version? I'm seeing TestSystemDatabaseSchemaBootstrapVersionBumped fail with:

Check whether you need to bump systemschema.SystemDatabaseSchemaBootstrapVersion
        and then update prevSystemHash to "7d089e4c69d8d6b164b3d1a04e0f3ad5f487f9138c9585bd00b37f397691a090".
        The current value of SystemDatabaseSchemaBootstrapVersion is 1000023.1-32.

i wouldn't expect this to need a new cluster version. the reason this is happening is bc now the hard-coded system database descriptors that are used if you bootstrap from 23.2 no longer match the descriptors that are created if you upgrade from 23.1 to 23.2.

the full comment is

// TestSystemDatabaseSchemaBootstrapVersionBumped serves as a reminder to bump
// systemschema.SystemDatabaseSchemaBootstrapVersion whenever a new upgrade
// creates or modifies the schema of system tables. We unfortunately cannot  
// programmatically determine if an upgrade should bump the version so by  
// adding a test failure when the initial values change, the programmer and  
// code reviewers are reminded to manually check whether the version should  
// be bumped.

i think what we want here is just to change prevSystemHash

i also think we might want to use

var SystemDatabaseSchemaBootstrapVersion \= clusterversion.ByKey(clusterversion.V23\_2)

in pkg/sql/catalog/systemschema/system.go. but i'm asking about that here

pkg/sql/catalog/funcdesc/func_desc_builder.go line 121 at r3 (raw file):

	// Grant EXECUTE privilege on the function for the public role if the
	// descriptor was created before v23.2. This mimics the
	// grantExecuteToPublicOnAllFunctions descriptor upgrade.

an interesting consequence of adding this logic here is that the grantExecuteToPublicOnAllFunctions upgrade is no longer necessary. the reason is that there already is an upgrade called FirstUpgradeFromRelease that will run all the post-deserialization changes on all descriptors.

i guess there's no harm in keeping grantExecuteToPublicOnAllFunctions though.

pkg/sql/catalog/funcdesc/func_desc_builder.go line 129 at r3 (raw file):

			false, /* withGrantOption */
		)
		desc.GetPrivileges().SetVersion(catpb.Version23_2)

i believe you need a line like

fdb.changes.Add(catalog.GrantedExecuteToPublicRole)

(and create the GrantedExecuteToPublicRole enum)

mgartner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @yuzefovich)

pkg/sql/catalog/funcdesc/func_desc_builder.go line 121 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

an interesting consequence of adding this logic here is that the grantExecuteToPublicOnAllFunctions upgrade is no longer necessary. the reason is that there already is an upgrade called FirstUpgradeFromRelease that will run all the post-deserialization changes on all descriptors.

i guess there's no harm in keeping grantExecuteToPublicOnAllFunctions though.

I've removed it — the best code is no code.

pkg/sql/catalog/funcdesc/func_desc_builder.go line 129 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i believe you need a line like
fdb.changes.Add(catalog.GrantedExecuteToPublicRole)
(and create the GrantedExecuteToPublicRole enum)

I didn't need it before when this was in RunRestoreChanges (or maybe I did?), but I've added it now that I've moved this logic to RunPostDeserializationChanges.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)

pkg/sql/catalog/bootstrap/bootstrap_test.go line 167 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Is it ok to bump this without updating ystemschema.SystemDatabaseSchemaBootstrapVersion in this case? The system schema should have no UDFs, AFAIK, so I thought just bumping this would be fine.

yeah i think just bumping the hash should be fine

rafiss

lgtm! thanks for taking this on

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @mgartner)

pkg/ccl/backupccl/testdata/backup-restore/user-defined-functions line 412 at r2 (raw file):

SHOW GRANTS ON FUNCTION add
----
db1 public 100109 add(int8, int8) admin ALL true

unrelated for now, but i am surprised by our decision to include the function ID in the results, given that SHOW commands are intended for usage by humans, not computers.

This commit fixes some bugs with restoring UDFs from a backup: 1. Restoring UDFs from a backup created on a v23.1 cluster will have `EXECUTE` privilege added for the public role, matching the new default privileges for functions in v23.2. This ensures that the backed-up functions can be executed without privilege errors. 2. Restoring a full cluster from a v23.2 backup with UDFs will retain the original privileges of the UDF. For example, if EXECUTE privileges on the public role have been revoked, they will remain revoked after the restore. 3. Restoring UDFs from a partial v23.2 backup, i.e., not a full cluster backup, will now have default privileges, including `EXECUTE` privilege for the public role. Fixes cockroachdb#112866 There is no release note because no release includes this issue. Release note: None

The `grantExecuteToPublicOnAllFunctions` upgrade is no longer necessary because post-serialization changes now grant `EXECUTE` on functions to the public role. The upgrade has been removed. Release note: None

mgartner · 2023-11-16T15:40:06Z

bors r+

mgartner · 2023-11-16T15:40:50Z

pkg/ccl/backupccl/testdata/backup-restore/user-defined-functions line 412 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

unrelated for now, but i am surprised by our decision to include the function ID in the results, given that SHOW commands are intended for usage by humans, not computers.

@chengxiong-ruan any reason we included this? I'll remove it if not.

chengxiong-ruan · 2023-11-16T15:59:56Z

pkg/ccl/backupccl/testdata/backup-restore/user-defined-functions line 412 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

@chengxiong-ruan any reason we included this? I'll remove it if not.

I don't have a strong reason, just thought it might be good for debugging. But, since we also have function signature, function id is really not needed for sure.

craig · 2023-11-16T16:36:46Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-11-16T16:36:53Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 074b3e9 to blathers/backport-release-23.2-114203: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

mgartner · 2023-11-16T21:10:40Z

I still need to backport this.

mgartner requested a review from a team November 10, 2023 00:48

mgartner assigned dt and rafiss Nov 10, 2023

mgartner requested review from a team as code owners November 10, 2023 00:48

mgartner requested review from dt and removed request for a team November 10, 2023 00:48

dt approved these changes Nov 10, 2023

View reviewed changes

mgartner force-pushed the udf-exec-privileges-1 branch from 3ce0b7c to feb22e3 Compare November 10, 2023 15:28

mgartner requested a review from rafiss November 10, 2023 19:18

mgartner commented Nov 10, 2023

View reviewed changes

mgartner force-pushed the udf-exec-privileges-1 branch from feb22e3 to 7400246 Compare November 10, 2023 21:09

mgartner requested a review from a team as a code owner November 10, 2023 21:09

mgartner requested review from yuzefovich and removed request for a team November 10, 2023 21:09

mgartner commented Nov 10, 2023

View reviewed changes

celiala added the blocks-23.2.0-beta.1 label Nov 13, 2023

mgartner force-pushed the udf-exec-privileges-1 branch from 7400246 to cc67995 Compare November 13, 2023 14:30

rafiss requested changes Nov 13, 2023

View reviewed changes

mgartner force-pushed the udf-exec-privileges-1 branch from cc67995 to 05ff156 Compare November 13, 2023 17:02

mgartner requested a review from rafiss November 13, 2023 17:27

mgartner commented Nov 13, 2023

View reviewed changes

mgartner force-pushed the udf-exec-privileges-1 branch 3 times, most recently from d8f1c2f to 86a08ba Compare November 13, 2023 19:12

rafiss reviewed Nov 13, 2023

View reviewed changes

yuzefovich removed their request for review November 13, 2023 19:55

mgartner force-pushed the udf-exec-privileges-1 branch 2 times, most recently from ee90df1 to da9bc9f Compare November 15, 2023 16:10

rafiss requested a review from dt November 15, 2023 17:36

rafiss approved these changes Nov 15, 2023

View reviewed changes

mgartner added 2 commits November 16, 2023 10:39

upgrade: remove grantExecuteToPublicOnAllFunctions upgrade

1d3b393

The `grantExecuteToPublicOnAllFunctions` upgrade is no longer necessary because post-serialization changes now grant `EXECUTE` on functions to the public role. The upgrade has been removed. Release note: None

mgartner force-pushed the udf-exec-privileges-1 branch from da9bc9f to 1d3b393 Compare November 16, 2023 15:39

mgartner added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Nov 16, 2023

craig bot merged commit 1c90978 into cockroachdb:master Nov 16, 2023
6 of 8 checks passed

mgartner deleted the udf-exec-privileges-1 branch November 16, 2023 18:54

mgartner mentioned this pull request Nov 16, 2023

release-23.2: backupccl: fix EXECUTE privileges #114630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backupccl: fix EXECUTE privileges #114203

backupccl: fix EXECUTE privileges #114203

mgartner commented Nov 10, 2023 •

edited

Loading

cockroach-teamcity commented Nov 10, 2023

dt commented Nov 10, 2023 •

edited

Loading

blathers-crl bot commented Nov 10, 2023

mgartner Nov 10, 2023

mgartner Nov 10, 2023

rafiss left a comment

mgartner left a comment

rafiss left a comment

rafiss left a comment

mgartner commented Nov 16, 2023

mgartner commented Nov 16, 2023

chengxiong-ruan commented Nov 16, 2023

craig bot commented Nov 16, 2023

blathers-crl bot commented Nov 16, 2023

mgartner commented Nov 16, 2023

backupccl: fix EXECUTE privileges #114203

backupccl: fix EXECUTE privileges #114203

Conversation

mgartner commented Nov 10, 2023 • edited Loading

backupccl: fix EXECUTE privileges

upgrade: remove grantExecuteToPublicOnAllFunctions upgrade

cockroach-teamcity commented Nov 10, 2023

dt commented Nov 10, 2023 • edited Loading

blathers-crl bot commented Nov 10, 2023

mgartner Nov 10, 2023

Choose a reason for hiding this comment

mgartner Nov 10, 2023

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

mgartner commented Nov 16, 2023

mgartner commented Nov 16, 2023

chengxiong-ruan commented Nov 16, 2023

craig bot commented Nov 16, 2023

blathers-crl bot commented Nov 16, 2023

mgartner commented Nov 16, 2023

mgartner commented Nov 10, 2023 •

edited

Loading

dt commented Nov 10, 2023 •

edited

Loading