From d67afe18d6bbddf9bcb2348508dbe02d09ac39df Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Wed, 27 Sep 2023 15:14:18 -0700 Subject: [PATCH] sql: introduce tenant RO setting to allow full access to zone configs TLDR: this commit introduces another cluster setting that controls whether the virtual clusters have unrestricted ability to modify zone configs. Previously, we already had `sql.zone_configs.allow_for_secondary_tenant.enabled` setting (which controls whether VCs can modify zone configs at all), but even if that was enabled, VCs could not modify constraints and lease preferences other than locality (and only regions and zones). However, this restriction can be too cumbersome in UA, so this commit introduces another cluster setting `sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled` that controls whether VCs have full access to zone config modification (note that region and zone validation is still preserved, and retrieval of that information is made lazy). The new setting is disabled by default, but it's overridden in the config profiles `virtual+noapp`, `virtual+noapp+repl`, `virtual+app+sharedservice`, and `virtual+app+sharedservice+repl`. Release note: None --- .../zone_configs_secondary_tenants_restricted | 20 +++++ ...one_configs_secondary_tenants_unrestricted | 21 +++++ .../tests/3node-tenant/generated_test.go | 14 ++++ pkg/configprofiles/profiles.go | 1 + pkg/configprofiles/testdata/virtual-app | 2 + pkg/configprofiles/testdata/virtual-app-repl | 2 + pkg/configprofiles/testdata/virtual-noapp | 2 + .../testdata/virtual-noapp-repl | 2 + pkg/sql/exec_util.go | 8 +- pkg/sql/opt_exec_factory.go | 2 +- pkg/sql/scatter.go | 2 +- pkg/sql/set_zone_config.go | 83 +++++++++++++------ pkg/sql/set_zone_config_test.go | 39 +++++---- 13 files changed, 154 insertions(+), 44 deletions(-) create mode 100644 pkg/ccl/logictestccl/testdata/logic_test/zone_configs_secondary_tenants_restricted create mode 100644 pkg/ccl/logictestccl/testdata/logic_test/zone_configs_secondary_tenants_unrestricted diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone_configs_secondary_tenants_restricted b/pkg/ccl/logictestccl/testdata/logic_test/zone_configs_secondary_tenants_restricted new file mode 100644 index 000000000000..d3f87ae957aa --- /dev/null +++ b/pkg/ccl/logictestccl/testdata/logic_test/zone_configs_secondary_tenants_restricted @@ -0,0 +1,20 @@ +# LogicTest: 3node-tenant + +# 'sql.zone_configs.allow_for_secondary_tenant.enabled' is enabled, but +# 'sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled' is not, +# so we should be able to modify zone configs except when modifying constraints +# other than regions and zones. + +statement ok +CREATE TABLE t (k INT PRIMARY KEY); + +statement ok +ALTER TABLE t CONFIGURE ZONE USING num_replicas = 5; + +# This statement was correctly allowed, but it failed during validation because +# 3node-tenant config doesn't define locality information. +statement error region "us-east1" not found +ALTER TABLE t CONFIGURE ZONE USING constraints = '[+region=us-east1]'; + +statement error operation is disabled within a virtual cluster +ALTER TABLE t CONFIGURE ZONE USING constraints = '[+ssd]'; diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone_configs_secondary_tenants_unrestricted b/pkg/ccl/logictestccl/testdata/logic_test/zone_configs_secondary_tenants_unrestricted new file mode 100644 index 000000000000..1cf3e0108513 --- /dev/null +++ b/pkg/ccl/logictestccl/testdata/logic_test/zone_configs_secondary_tenants_unrestricted @@ -0,0 +1,21 @@ +# LogicTest: 3node-tenant +# tenant-cluster-setting-override-opt: sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled=true + +# Both 'sql.zone_configs.allow_for_secondary_tenant.enabled' and +# 'sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled' are +# enabled, so we should be able to modify zone configs in unlimited fashion +# (regions and zones are still subject to validation). + +statement ok +CREATE TABLE t (k INT PRIMARY KEY); + +statement ok +ALTER TABLE t CONFIGURE ZONE USING num_replicas = 5; + +# This statement was correctly allowed, but it failed during validation because +# 3node-tenant config doesn't define locality information. +statement error region "us-east1" not found +ALTER TABLE t CONFIGURE ZONE USING constraints = '[+region=us-east1]'; + +statement ok +ALTER TABLE t CONFIGURE ZONE USING constraints = '[+ssd]'; diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index 8c7ae216c92c..027c6485e9d4 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -2599,6 +2599,20 @@ func TestTenantLogicCCL_zone_config_secondary_tenants_disallowed( runCCLLogicTest(t, "zone_config_secondary_tenants_disallowed") } +func TestTenantLogicCCL_zone_configs_secondary_tenants_restricted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runCCLLogicTest(t, "zone_configs_secondary_tenants_restricted") +} + +func TestTenantLogicCCL_zone_configs_secondary_tenants_unrestricted( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runCCLLogicTest(t, "zone_configs_secondary_tenants_unrestricted") +} + func TestTenantExecBuild_distsql_tenant( t *testing.T, ) { diff --git a/pkg/configprofiles/profiles.go b/pkg/configprofiles/profiles.go index 45bf773d60ff..4c17d60a04f1 100644 --- a/pkg/configprofiles/profiles.go +++ b/pkg/configprofiles/profiles.go @@ -86,6 +86,7 @@ var virtClusterInitTasks = []autoconfigpb.Task{ "SET CLUSTER SETTING trace.redact_at_virtual_cluster_boundary.enabled = false", // Enable zone config changes in secondary tenants (this ought to be configurable per-tenant, but is not possible yet in v23.1). "SET CLUSTER SETTING sql.virtual_cluster.feature_access.zone_configs.enabled = true", + "SET CLUSTER SETTING sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled = true", // Enable multi-region abstractions in secondary tenants. "SET CLUSTER SETTING sql.virtual_cluster.feature_access.multiregion.enabled = true", // Disable range coalescing (as long as the problems related diff --git a/pkg/configprofiles/testdata/virtual-app b/pkg/configprofiles/testdata/virtual-app index a501446e2a87..15b8cac274f3 100644 --- a/pkg/configprofiles/testdata/virtual-app +++ b/pkg/configprofiles/testdata/virtual-app @@ -14,6 +14,7 @@ SELECT variable, value FROM [SHOW ALL CLUSTER SETTINGS] WHERE variable IN ( 'trace.redact_at_virtual_cluster_boundary.enabled', 'sql.virtual_cluster.feature_access.zone_configs.enabled', +'sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled', 'sql.virtual_cluster.feature_access.multiregion.enabled', 'spanconfig.range_coalescing.system.enabled', 'spanconfig.range_coalescing.application.enabled', @@ -34,6 +35,7 @@ sql.create_virtual_cluster.default_template template sql.drop_virtual_cluster.enabled false sql.virtual_cluster.feature_access.multiregion.enabled true sql.virtual_cluster.feature_access.zone_configs.enabled true +sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled true trace.redact_at_virtual_cluster_boundary.enabled false system-sql diff --git a/pkg/configprofiles/testdata/virtual-app-repl b/pkg/configprofiles/testdata/virtual-app-repl index 197111832b43..9fd541fb4a07 100644 --- a/pkg/configprofiles/testdata/virtual-app-repl +++ b/pkg/configprofiles/testdata/virtual-app-repl @@ -14,6 +14,7 @@ SELECT variable, value FROM [SHOW ALL CLUSTER SETTINGS] WHERE variable IN ( 'trace.redact_at_virtual_cluster_boundary.enabled', 'sql.virtual_cluster.feature_access.zone_configs.enabled', +'sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled', 'sql.virtual_cluster.feature_access.multiregion.enabled', 'sql.virtual_cluster.feature_access.multiregion.enabled', 'spanconfig.range_coalescing.system.enabled', @@ -35,6 +36,7 @@ sql.create_virtual_cluster.default_template template sql.drop_virtual_cluster.enabled false sql.virtual_cluster.feature_access.multiregion.enabled true sql.virtual_cluster.feature_access.zone_configs.enabled true +sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled true trace.redact_at_virtual_cluster_boundary.enabled false system-sql diff --git a/pkg/configprofiles/testdata/virtual-noapp b/pkg/configprofiles/testdata/virtual-noapp index 60622042784f..86ffeb74f22e 100644 --- a/pkg/configprofiles/testdata/virtual-noapp +++ b/pkg/configprofiles/testdata/virtual-noapp @@ -9,6 +9,7 @@ SELECT variable, value FROM [SHOW ALL CLUSTER SETTINGS] WHERE variable IN ( 'trace.redact_at_virtual_cluster_boundary.enabled', 'sql.virtual_cluster.feature_access.zone_configs.enabled', +'sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled', 'sql.virtual_cluster.feature_access.multiregion.enabled', 'sql.virtual_cluster.feature_access.multiregion.enabled', 'spanconfig.range_coalescing.system.enabled', @@ -30,6 +31,7 @@ sql.create_virtual_cluster.default_template template sql.drop_virtual_cluster.enabled false sql.virtual_cluster.feature_access.multiregion.enabled true sql.virtual_cluster.feature_access.zone_configs.enabled true +sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled true trace.redact_at_virtual_cluster_boundary.enabled false system-sql diff --git a/pkg/configprofiles/testdata/virtual-noapp-repl b/pkg/configprofiles/testdata/virtual-noapp-repl index 323a2f54d4ad..eb82890857dc 100644 --- a/pkg/configprofiles/testdata/virtual-noapp-repl +++ b/pkg/configprofiles/testdata/virtual-noapp-repl @@ -9,6 +9,7 @@ SELECT variable, value FROM [SHOW ALL CLUSTER SETTINGS] WHERE variable IN ( 'trace.redact_at_virtual_cluster_boundary.enabled', 'sql.virtual_cluster.feature_access.zone_configs.enabled', +'sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled', 'sql.virtual_cluster.feature_access.multiregion.enabled', 'sql.virtual_cluster.feature_access.multiregion.enabled', 'spanconfig.range_coalescing.system.enabled', @@ -30,6 +31,7 @@ sql.create_virtual_cluster.default_template template sql.drop_virtual_cluster.enabled false sql.virtual_cluster.feature_access.multiregion.enabled true sql.virtual_cluster.feature_access.zone_configs.enabled true +sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled true trace.redact_at_virtual_cluster_boundary.enabled false system-sql diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 5d232d6c18da..06170bce1cf7 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -3784,13 +3784,13 @@ func (cfg *ExecutorConfig) GetRowMetrics(internal bool) *rowinfra.Metrics { return cfg.RowMetrics } -// RequireSystemTenantOrClusterSetting returns a setting disabled error if +// requireSystemTenantOrClusterSetting returns a setting disabled error if // executed from inside a secondary tenant that does not have the specified // cluster setting. -func (cfg *ExecutorConfig) RequireSystemTenantOrClusterSetting( - setting *settings.BoolSetting, +func requireSystemTenantOrClusterSetting( + codec keys.SQLCodec, settings *cluster.Settings, setting *settings.BoolSetting, ) error { - if cfg.Codec.ForSystemTenant() || setting.Get(&cfg.Settings.SV) { + if codec.ForSystemTenant() || setting.Get(&settings.SV) { return nil } return errors.WithDetailf(errors.WithHint( diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go index 6aa9d0d8ac01..d794c7622210 100644 --- a/pkg/sql/opt_exec_factory.go +++ b/pkg/sql/opt_exec_factory.go @@ -1974,7 +1974,7 @@ func (ef *execFactory) ConstructAlterTableSplit( return nil, err } - if err := execCfg.RequireSystemTenantOrClusterSetting(SecondaryTenantSplitAtEnabled); err != nil { + if err := requireSystemTenantOrClusterSetting(execCfg.Codec, execCfg.Settings, SecondaryTenantSplitAtEnabled); err != nil { return nil, err } diff --git a/pkg/sql/scatter.go b/pkg/sql/scatter.go index b32565232cf2..3063de46e8e7 100644 --- a/pkg/sql/scatter.go +++ b/pkg/sql/scatter.go @@ -36,7 +36,7 @@ type scatterNode struct { // Privileges: INSERT on table. func (p *planner) Scatter(ctx context.Context, n *tree.Scatter) (planNode, error) { - if err := p.ExecCfg().RequireSystemTenantOrClusterSetting(SecondaryTenantScatterEnabled); err != nil { + if err := requireSystemTenantOrClusterSetting(p.ExecCfg().Codec, p.ExecCfg().Settings, SecondaryTenantScatterEnabled); err != nil { return nil, err } diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 3c33c89802a4..e59f43fa9286 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -22,6 +22,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/telemetry" + "github.com/cockroachdb/cockroach/pkg/settings" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" @@ -260,7 +262,7 @@ func (p *planner) SetZoneConfig(ctx context.Context, n *tree.SetZoneConfig) (pla return nil, err } - if err := execCfg.RequireSystemTenantOrClusterSetting(SecondaryTenantZoneConfigsEnabled); err != nil { + if err := requireSystemTenantOrClusterSetting(execCfg.Codec, execCfg.Settings, SecondaryTenantZoneConfigsEnabled); err != nil { return nil, err } @@ -1015,7 +1017,9 @@ func validateZoneAttrsAndLocalities( } return validateZoneAttrsAndLocalitiesForSystemTenant(ctx, ss.ListNodesInternal, zone) } - return validateZoneLocalitiesForSecondaryTenants(ctx, regionProvider.GetRegions, zone) + return validateZoneLocalitiesForSecondaryTenants( + ctx, regionProvider.GetRegions, zone, execCfg.Codec, execCfg.Settings, + ) } // validateZoneAttrsAndLocalitiesForSystemTenant performs all the constraint/ @@ -1070,36 +1074,65 @@ func validateZoneAttrsAndLocalitiesForSystemTenant( return nil } +// secondaryTenantsAllZoneConfigsEnabled is an extension of +// SecondaryTenantZoneConfigsEnabled that allows virtual clusters to modify all +// type of constraints in zone configs (i.e. not only zones and regions). +var secondaryTenantsAllZoneConfigsEnabled = settings.RegisterBoolSetting( + settings.TenantReadOnly, + "sql.virtual_cluster.feature_access.zone_configs_unrestricted.enabled", + "enable unrestricted usage of ALTER CONFIGURE ZONE in virtual clusters", + false, +) + // validateZoneLocalitiesForSecondaryTenants performs all the constraint/lease -// preferences validation for secondary tenants. Secondary tenants are only -// allowed to reference locality attributes as they only have access to region -// information via the serverpb.TenantStatusServer. Even then, they're only -// allowed to reference the "region" and "zone" tiers. +// preferences validation for secondary tenants. Unless +// secondaryTenantsAllZoneConfigsEnabled is set to 'true', secondary tenants are +// only allowed to reference locality attributes as they only have access to +// region information via the serverpb.TenantStatusServer. In that case they're +// only allowed to reference the "region" and "zone" tiers. // // Unlike the system tenant, we also validate prohibited constraints. This is // because secondary tenant must operate in the narrow view exposed via the // serverpb.TenantStatusServer and are not allowed to configure arbitrary // constraints (required or otherwise). func validateZoneLocalitiesForSecondaryTenants( - ctx context.Context, getRegions regionsGetter, zone *zonepb.ZoneConfig, + ctx context.Context, + getRegions regionsGetter, + zone *zonepb.ZoneConfig, + codec keys.SQLCodec, + settings *cluster.Settings, ) error { toValidate := accumulateUniqueConstraints(zone) - resp, err := getRegions(ctx) - if err != nil { - return err - } - regions := make(map[string]struct{}) - zones := make(map[string]struct{}) - for regionName, regionMeta := range resp.Regions { - regions[regionName] = struct{}{} - for _, zone := range regionMeta.Zones { - zones[zone] = struct{}{} + + // rs and zs will be lazily populated with regions and zones, respectively. + // These should not be accessed directly - use getRegionsAndZones helper + // instead. + var rs, zs map[string]struct{} + getRegionsAndZones := func() (regions, zones map[string]struct{}, _ error) { + if rs != nil { + return rs, zs, nil } + resp, err := getRegions(ctx) + if err != nil { + return nil, nil, err + } + rs, zs = make(map[string]struct{}), make(map[string]struct{}) + for regionName, regionMeta := range resp.Regions { + rs[regionName] = struct{}{} + for _, zone := range regionMeta.Zones { + zs[zone] = struct{}{} + } + } + return rs, zs, nil } for _, constraint := range toValidate { switch constraint.Key { case "zone": + _, zones, err := getRegionsAndZones() + if err != nil { + return err + } _, found := zones[constraint.Value] if !found { return pgerror.Newf( @@ -1109,6 +1142,10 @@ func validateZoneLocalitiesForSecondaryTenants( ) } case "region": + regions, _, err := getRegionsAndZones() + if err != nil { + return err + } _, found := regions[constraint.Value] if !found { return pgerror.Newf( @@ -1118,13 +1155,11 @@ func validateZoneLocalitiesForSecondaryTenants( ) } default: - return errors.WithHint(pgerror.Newf( - pgcode.CheckViolation, - "invalid constraint attribute: %q", - constraint.Key, - ), - `only "zone" and "region" are allowed`, - ) + if err := requireSystemTenantOrClusterSetting( + codec, settings, secondaryTenantsAllZoneConfigsEnabled, + ); err != nil { + return err + } } } return nil diff --git a/pkg/sql/set_zone_config_test.go b/pkg/sql/set_zone_config_test.go index 1d67c96f589e..67e63d0d0bda 100644 --- a/pkg/sql/set_zone_config_test.go +++ b/pkg/sql/set_zone_config_test.go @@ -12,13 +12,17 @@ package sql import ( "context" + "strings" "testing" "github.com/cockroachdb/cockroach/pkg/config/zonepb" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/status/statuspb" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/gogo/protobuf/proto" @@ -70,6 +74,10 @@ func TestValidateZoneAttrsAndLocalitiesForSecondaryTenants(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + ctx := context.Background() + codec := keys.MakeSQLCodec(serverutils.TestTenantID()) + settings := cluster.MakeTestingClusterSettings() + getRegions := func(ctx context.Context) (*serverpb.RegionsResponse, error) { return &serverpb.RegionsResponse{ Regions: map[string]*serverpb.RegionsResponse_Region{ @@ -125,33 +133,36 @@ func TestValidateZoneAttrsAndLocalitiesForSecondaryTenants(t *testing.T) { }, { cfg: `constraints: ["+rack=us-east1"]`, - errRe: `invalid constraint attribute: "rack"`, + errRe: `operation is disabled within a virtual cluster`, }, { cfg: `constraints: ["-rack=us-east1"]`, - errRe: `invalid constraint attribute: "rack"`, + errRe: `operation is disabled within a virtual cluster`, }, { cfg: `constraints: ["+ssd"]`, - errRe: `invalid constraint attribute: ""`, + errRe: `operation is disabled within a virtual cluster`, }, { cfg: `constraints: ["-ssd"]`, - errRe: `invalid constraint attribute: ""`, + errRe: `operation is disabled within a virtual cluster`, }, } - for _, tc := range testCases { - var zone zonepb.ZoneConfig - err := yaml.UnmarshalStrict([]byte(tc.cfg), &zone) - require.NoError(t, err) - - err = validateZoneLocalitiesForSecondaryTenants(context.Background(), getRegions, &zone) - if tc.errRe == "" { + for _, anyConstraintAllowed := range []bool{false, true} { + secondaryTenantsAllZoneConfigsEnabled.Override(ctx, &settings.SV, anyConstraintAllowed) + for _, tc := range testCases { + var zone zonepb.ZoneConfig + err := yaml.UnmarshalStrict([]byte(tc.cfg), &zone) require.NoError(t, err) - } else { - require.Error(t, err) - require.True(t, testutils.IsError(err, tc.errRe), "expected %s; got %s", tc.errRe, err.Error()) + + err = validateZoneLocalitiesForSecondaryTenants(ctx, getRegions, &zone, codec, settings) + if tc.errRe == "" || (anyConstraintAllowed && strings.HasPrefix(tc.errRe, "operation is disabled within a virtual cluster")) { + require.NoError(t, err) + } else { + require.Error(t, err) + require.True(t, testutils.IsError(err, tc.errRe), "expected %s; got %s", tc.errRe, err.Error()) + } } } }