sql: fix Usage privilege on Tables/DBs after upgrading from 20.1 by RichardJCai · Pull Request #65010 · cockroachdb/cockroach

RichardJCai · 2021-05-11T19:46:26Z

In 20.2, I made a mistake when introducing USAGE privilege by
adding it before ZONECONFIG privilege in the bitfield.
Due to this, when updating from 20.1 to 20.2, ZONECONFIG privilege will be
interpretted as USAGE.

Fortunately, ZONECONFIG was only a valid privilege on tables and databases
while USAGE is invalid on both those objects. Due to this, whenever we encounter
USAGE on a privilege descriptor versioned 20.1 (InitialVersion) we can fix
this by removing the bit for USAGE and adding the bit for ZONECONFIG on the
privilege descriptor.

Release note: None

cockroach-teamcity · 2021-05-11T19:46:32Z

This change is

ajwerner

Can you go back and create a 20.1 backup that you restore in the style of https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/backupccl/restore_old_versions_test.go#L31 to make sure that post-restore everything works as you might expect?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/sql/catalog/descpb/privilege.go, line 226 at r1 (raw file):

	// InitialVersion is for descriptors that were created in versions 20.1 and
	// earlier.
	if p.Version == InitialVersion {

What happens if you create a descriptor in 20.1 and grant ZONCONFIG in 20.1 and then set an owner in 20.2?

RichardJCai

Will do

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

pkg/sql/catalog/descpb/privilege.go, line 226 at r1 (raw file):

Previously, ajwerner wrote…

What happens if you create a descriptor in 20.1 and grant ZONCONFIG in 20.1 and then set an owner in 20.2?

Is it wrong to assume that this would be called before you try to set an owner in 20.2? I thought you'd have to retrieve the descriptor which then would be fixed before an owner is set.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/sql/catalog/descpb/privilege.go, line 226 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Is it wrong to assume that this would be called before you try to set an owner in 20.2? I thought you'd have to retrieve the descriptor which then would be fixed before an owner is set.

I mean 20.2 before this patch release. I'm asking whether you can have Version == OwnerVersion and still have the wrong bit set.

RichardJCai

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

pkg/sql/catalog/descpb/privilege.go, line 226 at r1 (raw file):

Previously, ajwerner wrote…

I mean 20.2 before this patch release. I'm asking whether you can have Version == OwnerVersion and still have the wrong bit set.

Ah I see what you mean. If they're on a 20.2 patch release before this version, if the descriptor is corrupted, it would have to have been created in 20.1 (we don't actually update the Version field) and have Version == InitialVersion.

We never actually update the Version field so I believe this is fine.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/sql/catalog/descpb/privilege.go, line 226 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Ah I see what you mean. If they're on a 20.2 patch release before this version, if the descriptor is corrupted, it would have to have been created in 20.1 (we don't actually update the Version field) and have Version == InitialVersion.

We never actually update the Version field so I believe this is fine.

If we don't upgrade it, what does it mean?

RichardJCai

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

pkg/sql/catalog/descpb/privilege.go, line 226 at r1 (raw file):

Previously, ajwerner wrote…

If we don't upgrade it, what does it mean?

When I first added ownership, instead of deciding to migrate all table descriptors to have owners, I added a version field to determine if having an empty owner would be valid.

https://github.com/RichardJCai/cockroach/blob/0faf0a1a168105551c0d6d126eff25068e865e01/pkg/sql/authorization.go#L117

The version field is only used currently to validate if having an empty owner is valid
https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/catalog/descpb/privilege.go#L299

ajwerner

mod a backup test

Reviewed 1 of 4 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/sql/catalog/descpb/privilege.go, line 226 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

When I first added ownership, instead of deciding to migrate all table descriptors to have owners, I added a version field to determine if having an empty owner would be valid.

https://github.com/RichardJCai/cockroach/blob/0faf0a1a168105551c0d6d126eff25068e865e01/pkg/sql/authorization.go#L117

The version field is only used currently to validate if having an empty owner is valid
https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/catalog/descpb/privilege.go#L299

Cool, I’m satisfied. Nit:early return if the version is new enough. Also add more commentary indicating that the version doesn’t get upgraded today and we’ll have fixed this bug by the time we ever do want to do the upgrade.

RichardJCai · 2021-05-12T23:04:43Z

Added a backup test and some logic some restore works. cc @cockroachdb/bulk-io and @pbardea for review PTAL at the second commit mostly.

In 20.2, I made a mistake when introducing USAGE privilege by adding it before ZONECONFIG privilege in the bitfield. Due to this, when updating from 20.1 to 20.2, ZONECONFIG privilege will be interpretted as USAGE. Fortunately, ZONECONFIG was only a valid privilege on tables and databases while USAGE is invalid on both those objects. Due to this, whenever we encounter USAGE on a privilege descriptor versioned 20.1 (InitialVersion) we can fix this by removing the bit for USAGE and adding the bit for ZONECONFIG on the privilege descriptor. Release note: None

pbardea

Restore change LGTM

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nihalpednekar, @pbardea, and @RichardJCai)

pkg/ccl/backupccl/restore_old_versions_test.go, line 208 at r3 (raw file):

		err := os.Symlink(exportDir, filepath.Join(tmpDir, "foo"))
		require.NoError(t, err)
		sqlDB.Exec(t, `RESTORE FROM $1`, LocalFoo)

A comment mentioning that this only affects cluster restores since database restores reset privileges on the descriptor wouldn't hurt

pkg/ccl/backupccl/testdata/restore_old_versions/create_with_privileges.sql, line 2 at r3 (raw file):

-- The below SQL is used to create the data that is then exported with BACKUP
-- finish this comment

Looks like this comment needs to be finished, but it'd be good to mention the version that you ran this against to generate the backup

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nihalpednekar, @pbardea, and @RichardJCai)

pkg/ccl/backupccl/restore_old_versions_test.go, line 208 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

A comment mentioning that this only affects cluster restores since database restores reset privileges on the descriptor wouldn't hurt

Added it to the function header comment

pkg/ccl/backupccl/testdata/restore_old_versions/create_with_privileges.sql, line 2 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Looks like this comment needs to be finished, but it'd be good to mention the version that you ran this against to generate the backup

Oops, fixed.

… prior. This is needed in addition to the previous commit to fix ZONECONFIG privilege when restoring from a version from 20.1 or prior. Release note: None

RichardJCai · 2021-05-13T17:37:54Z

Thanks for reviewing!

bors r+

craig · 2021-05-13T19:04:45Z

Build succeeded:

GitHub CI (Cockroach)

This migration fixes corrupted privilege descriptors in the wild resulting from the fallout of the ZONECONFIG/USAGE bug and the ALTER DATABASE CONVERT TO SCHEMA bug. Additionally, all privilege descriptors will be updated to explicitly have Owners. Privilege descriptors created before 20.2 do not have their owner field set and instead owners are implicitly inferred. Finally all privilege descriptors will be bumped up to Version21_2 after the migration so we can strictly validate privileges. Release note (bugfix): This migration will be run when users upgrade to 21.2 (Specifically 21.2.14). This migration fixes any privilege descriptors that are corrupted from the fallout of the ZONECONFIG/USAGE bug on tables and databases after upgrading from 20.1 -> 20.2 (cockroachdb#65010) and those that are corrupted after converting a database to a schema (cockroachdb#65697).

This migration fixes corrupted privilege descriptors in the wild resulting from the fallout of the ZONECONFIG/USAGE bug and the ALTER DATABASE CONVERT TO SCHEMA bug. Additionally, all privilege descriptors will be updated to explicitly have Owners. Privilege descriptors created before 20.2 do not have their owner field set and instead owners are implicitly inferred. Finally all privilege descriptors will be bumped up to Version21_2 after the migration so we can strictly validate privileges. Release note (bugfix): Summary: upon upgrading to 21.2, users will have all privileges fixed. Details: This migration will be run when users upgrade to 21.2 (Specifically 21.2.14). This migration fixes any privilege descriptors that are corrupted from the fallout of the ZONECONFIG/USAGE bug on tables and databases after upgrading from 20.1 -> 20.2 (cockroachdb#65010) and those that are corrupted after converting a database to a schema (cockroachdb#65697).

This migration fixes corrupted privilege descriptors in the wild resulting from the fallout of the ZONECONFIG/USAGE bug and the ALTER DATABASE CONVERT TO SCHEMA bug. Additionally, all privilege descriptors will be updated to explicitly have Owners. Privilege descriptors created before 20.2 do not have their owner field set and instead owners are implicitly inferred. Finally all privilege descriptors will be bumped up to Version21_2 after the migration so we can strictly validate privileges. Release note (bug fix): Summary: upon upgrading to 21.2, users will have all privileges fixed. This means any invalid privileges on all objects will be removed. Details: This migration will be run when users upgrade to 21.2 (Specifically 21.2.14). This migration fixes any privilege descriptors that are corrupted from the fallout of the ZONECONFIG/USAGE bug on tables and databases after upgrading from 20.1 -> 20.2 (cockroachdb#65010) and those that are corrupted after converting a database to a schema (cockroachdb#65697).

This migration calls RunPostDeserializationChanges on every descriptor fixing any inconsistencies with the descriptor. This migration fixes corrupted privilege descriptors in the wild resulting from the fallout of the ZONECONFIG/USAGE bug and the ALTER DATABASE CONVERT TO SCHEMA bug. Additionally, all privilege descriptors will be updated to explicitly have Owners. Privilege descriptors created before 20.2 do not have their owner field set and instead owners are implicitly inferred. Finally all privilege descriptors will be bumped up to Version21_2 after the migration so we can strictly validate privileges. Additionally the IsRepaired bit will be set to true after the fix. Release note (bug fix): Summary: upon upgrading to 21.2, users will have all privileges fixed. This means any invalid privileges on all objects will be removed. Details: This migration will be run when users upgrade to 21.2 (Specifically 21.2.14). This migration fixes any privilege descriptors that are corrupted from the fallout of the ZONECONFIG/USAGE bug on tables and databases after upgrading from 20.1 -> 20.2 (cockroachdb#65010) and those that are corrupted after converting a database to a schema (cockroachdb#65697).

66495: sql: migration for descriptors fixes r=ajwerner a=RichardJCai This migration fixes corrupted privilege descriptors in the wild resulting from the fallout of the ZONECONFIG/USAGE bug and the ALTER DATABASE CONVERT TO SCHEMA bug. Additionally, all privilege descriptors will be updated to explicitly have Owners. Privilege descriptors created before 20.2 do not have their owner field set and instead owners are implicitly inferred. Finally all privilege descriptors will be bumped up to Version21_2 after the migration so we can strictly validate privileges. Release note (bugfix): This migration will be run when users upgrade to 21.2 (Specifically 21.2.14). This migration fixes any privilege descriptors that are corrupted from the fallout of the ZONECONFIG/USAGE bug on tables and databases after upgrading from 20.1 -> 20.2 (#65010) and those that are corrupted after converting a database to a schema (#65697). Fixes #65012 Co-authored-by: richardjcai <caioftherichard@gmail.com>

RichardJCai requested review from a team and ajwerner May 11, 2021 19:49

This was referenced May 11, 2021

sql: add validating privileges after version upgrades #65011

Closed

sql: create long running migration to fix privilege descriptors for tables/db created on 20.1 or before #65012

Closed

RichardJCai force-pushed the fix_usage_privilege_20.1_to_20.2_05112021 branch 2 times, most recently from cd95643 to e6004f1 Compare May 11, 2021 20:18

ajwerner reviewed May 11, 2021

View reviewed changes

RichardJCai commented May 11, 2021

View reviewed changes

ajwerner reviewed May 11, 2021

View reviewed changes

RichardJCai commented May 11, 2021

View reviewed changes

ajwerner reviewed May 11, 2021

View reviewed changes

RichardJCai commented May 11, 2021

View reviewed changes

ajwerner reviewed May 11, 2021

View reviewed changes

RichardJCai requested review from a team, nihalpednekar and pbardea and removed request for a team May 12, 2021 23:02

RichardJCai force-pushed the fix_usage_privilege_20.1_to_20.2_05112021 branch 2 times, most recently from 48977b4 to 5125ab9 Compare May 12, 2021 23:19

pbardea approved these changes May 13, 2021

View reviewed changes

RichardJCai force-pushed the fix_usage_privilege_20.1_to_20.2_05112021 branch from 5125ab9 to 50f6587 Compare May 13, 2021 15:02

RichardJCai commented May 13, 2021

View reviewed changes

restore: fix ZONECONFIG privilege when restoring from 20.1 version or…

1e42b0d

… prior. This is needed in addition to the previous commit to fix ZONECONFIG privilege when restoring from a version from 20.1 or prior. Release note: None

RichardJCai force-pushed the fix_usage_privilege_20.1_to_20.2_05112021 branch from 50f6587 to 1e42b0d Compare May 13, 2021 17:35

craig bot merged commit 7364dde into cockroachdb:master May 13, 2021

This was referenced May 13, 2021

release-20.2: sql: fix Usage privilege on Tables/DBs after upgrading from 20.1 #65159

Merged

release-21.1: sql: fix Usage privilege on Tables/DBs after upgrading from 20.1 #65160

Merged

RichardJCai mentioned this pull request Jun 15, 2021

sql: migration for descriptors fixes #66495

Merged

Conversation

RichardJCai commented May 11, 2021

Uh oh!

cockroach-teamcity commented May 11, 2021

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

RichardJCai left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

RichardJCai left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

RichardJCai left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

RichardJCai commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbardea left a comment

Choose a reason for hiding this comment

Uh oh!

RichardJCai left a comment

Choose a reason for hiding this comment

Uh oh!

RichardJCai commented May 13, 2021

Uh oh!

craig bot commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RichardJCai commented May 12, 2021 •

edited

Loading