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()) + } } } }