-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
62017: migration: add protected_ts_meta privilege migration r=ajwerner a=postamar In 20.2 and prior versions, the protected_ts_meta system table had incorrect superuser privileges. This was fixed in a recent commit in the 21.2 release branch. However, there was no associated migration job so the descriptors in storage remainted untouched. This remained unnoticed until now because most of the time, system table descriptors are retrieved from an in-memory cache. However, tools like doctor will read the descriptors from storage and will report validation failures. This commit adds the missing migration job which fixes the protected_ts_meta descriptor in the descriptors table by setting the proper privileges. Fixes #61950. Release note: None Co-authored-by: Marius Posta <[email protected]>
- Loading branch information
Showing
8 changed files
with
175 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| // Copyright 2021 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package migrations | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/cockroachdb/cockroach/pkg/clusterversion" | ||
| "github.com/cockroachdb/cockroach/pkg/kv" | ||
| "github.com/cockroachdb/cockroach/pkg/migration" | ||
| "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" | ||
| "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" | ||
| "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" | ||
| "github.com/cockroachdb/cockroach/pkg/util/log" | ||
| ) | ||
|
|
||
| func protectedTsMetaPrivilegesMigration( | ||
| ctx context.Context, _ clusterversion.ClusterVersion, d migration.SQLDeps, | ||
| ) error { | ||
| id := systemschema.ProtectedTimestampsMetaTable.GetID() | ||
| return descs.Txn(ctx, d.Settings, d.LeaseManager, d.InternalExecutor, d.DB, | ||
| func(ctx context.Context, txn *kv.Txn, descriptors *descs.Collection) error { | ||
| log.Infof(ctx, "%s", "updating privileges in system.protected_ts_meta descriptor") | ||
| mut, err := descriptors.GetMutableTableByID(ctx, txn, id, tree.ObjectLookupFlagsWithRequired()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if mut.GetVersion() > 1 { | ||
| // Descriptor has already been upgraded, skip. | ||
| return nil | ||
| } | ||
| // Privileges have already been fixed at this point by the descriptor | ||
| // unwrapping logic in catalogkv which runs post-deserialization changes, | ||
| // but we still need to bump the version number. | ||
| mut.Version = 2 | ||
| return descriptors.WriteDesc(ctx, false /* kvTrace */, mut, txn) | ||
| }, | ||
| ) | ||
| } |
107 changes: 107 additions & 0 deletions
107
pkg/migration/migrations/protected_ts_meta_migration_external_test.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| // Copyright 2021 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package migrations_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strconv" | ||
| "testing" | ||
|
|
||
| "github.com/cockroachdb/cockroach/pkg/base" | ||
| "github.com/cockroachdb/cockroach/pkg/clusterversion" | ||
| "github.com/cockroachdb/cockroach/pkg/keys" | ||
| "github.com/cockroachdb/cockroach/pkg/kv" | ||
| "github.com/cockroachdb/cockroach/pkg/security" | ||
| "github.com/cockroachdb/cockroach/pkg/server" | ||
| "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" | ||
| "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv" | ||
| "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" | ||
| "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" | ||
| "github.com/cockroachdb/cockroach/pkg/util/leaktest" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestProtectedTimestampMetaMigration(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
| ctx := context.Background() | ||
| tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{ | ||
| ServerArgs: base.TestServerArgs{ | ||
| Knobs: base.TestingKnobs{ | ||
| Server: &server.TestingKnobs{ | ||
| DisableAutomaticVersionUpgrade: 1, | ||
| BinaryVersionOverride: clusterversion.ByKey(clusterversion.ProtectedTsMetaPrivilegesMigration - 1), | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| defer tc.Stopper().Stop(ctx) | ||
|
|
||
| // Replicate bad descriptor privilege bug. | ||
| err := tc.Servers[0].DB().Txn(ctx, func(ctx context.Context, txn *kv.Txn) error { | ||
| id := systemschema.ProtectedTimestampsMetaTable.GetID() | ||
| mut, err := catalogkv.MustGetMutableTableDescByID(ctx, txn, keys.SystemSQLCodec, id) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| mut.Version = 1 | ||
| mut.Privileges = descpb.NewCustomSuperuserPrivilegeDescriptor( | ||
| descpb.SystemAllowedPrivileges[keys.ReplicationStatsTableID], security.NodeUserName()) | ||
| b := txn.NewBatch() | ||
| b.Put(catalogkeys.MakeDescMetadataKey(keys.SystemSQLCodec, id), mut.DescriptorProto()) | ||
| return txn.Run(ctx, b) | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| tdb := sqlutils.MakeSQLRunner(tc.ServerConn(0)) | ||
|
|
||
| checkPrivileges := func(expectedPrivileges int, expectedVersion int) { | ||
| expectedStr := fmt.Sprintf( | ||
| `{"ownerProto": "node", "users": [`+ | ||
| `{"privileges": %d, "userProto": "admin"}, `+ | ||
| `{"privileges": %d, "userProto": "root"}`+ | ||
| `], "version": 1}`, | ||
| expectedPrivileges, | ||
| expectedPrivileges, | ||
| ) | ||
| var actualStr string | ||
| tdb.QueryRow(t, ` | ||
| SELECT | ||
| crdb_internal.pb_to_json( | ||
| 'cockroach.sql.sqlbase.Descriptor', | ||
| descriptor, | ||
| false | ||
| )->'table'->>'privileges' | ||
| FROM system.descriptor WHERE id = 31 | ||
| `).Scan(&actualStr) | ||
| require.EqualValues(t, expectedStr, actualStr) | ||
| var actualVersionStr string | ||
| tdb.QueryRow(t, ` | ||
| SELECT | ||
| crdb_internal.pb_to_json( | ||
| 'cockroach.sql.sqlbase.Descriptor', | ||
| descriptor, | ||
| false | ||
| )->'table'->>'version' | ||
| FROM system.descriptor WHERE id = 31 | ||
| `).Scan(&actualVersionStr) | ||
| require.EqualValues(t, strconv.Itoa(expectedVersion), actualVersionStr) | ||
| } | ||
|
|
||
| checkPrivileges(496, 1) | ||
|
|
||
| tdb.Exec(t, `SET CLUSTER SETTING version = $1`, | ||
| clusterversion.ByKey(clusterversion.ProtectedTsMetaPrivilegesMigration).String()) | ||
|
|
||
| checkPrivileges(48, 2) | ||
| } |