sql: fix Usage privilege on Tables/DBs after upgrading from 20.1

[RichardJCai]

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]

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]

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

[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.

[RichardJCai]

Thanks for reviewing!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)