sql: gate tenant's multi-region abstraction usage behind cluster setting #79536
Conversation
|
This change is |
| // directive. | ||
| type tenantClusterSettingOverrideMultiTenantMultiRegionAbstractionsAllowed struct{} | ||
|
|
||
| var _ tenantClusterSettingOverrideOpt = &tenantClusterSettingOverrideMultiTenantMultiRegionAbstractionsAllowed{} |
There was a problem hiding this comment.
That's a lot of words 😛
pkg/sql/descriptor.go
Outdated
| @@ -361,6 +361,23 @@ var DefaultPrimaryRegion = settings.RegisterStringSetting( | |||
| "", | |||
| ).WithPublic() | |||
|
|
|||
| const secondaryTenantsMultiRegionAbstractionsEnabledSettingName = "sql.multi_region.abstractions.enabled" | |||
There was a problem hiding this comment.
Include "tenant" in the setting name somewhere?
arulajmani
force-pushed
the
gate-mr-syntax
branch
from
7405293 to
ff333d4
Compare
There was a problem hiding this comment.
Dismissed @irfansharif from a discussion.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @irfansharif)
pkg/sql/descriptor.go, line 364 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Include "tenant" in the setting name somewhere?
Good point, done.
pkg/sql/logictest/logic.go, line 2158 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
That's a lot of words 😛
🚀 🚀
arulajmani
force-pushed
the
gate-mr-syntax
branch
from
ff333d4 to
d56af6d
Compare
This patch introduces a tenant read-only cluster setting called `sql.multi_region.allow_abstractions_for_secondary_tenants.enabled` which allows the operator to control if secondary tenants can make use of multi-region abstractions. It defaults to false. This setting is checked against when creating new MR databases or altering existing ones to make use of MR features (by setting the primary region). It has nothing to do with MR databases that may have been created previously which could happen if this setting was ever flipped to true by the operator. This setting doesn't effect the system tenant. It also takes precedence over `sql.defaults.primary_region`, which is tenant writeable. Release note (sql change): introduces new cluster setting which allows the operator to control if a secondary tenant can make use of MR abstractions. The setting is called `sql.multi_region.allow_abstractions_for_secondary_tenants.enabled`.
arulajmani
force-pushed
the
gate-mr-syntax
branch
from
d56af6d to
f87881c
Compare
There was a problem hiding this comment.
Skimmed this and it seems reasonable to me. One question: do we want to prevent restoring MR-enabled databases to clusters with this setting set to false? If we don't it seems to be a pretty easy back-door to walk through.
Reviewed 1 of 2 files at r2.
Reviewable status:
pkg/sql/descriptor.go, line 403 at r2 (raw file):
return nil, errors.WithHint(pgerror.Newf( pgcode.InvalidDatabaseDefinition, "setting %s disallows use of multi-region abstractions",
Nit: If this is going to be seen by Serverless users, is this the right error here? Maybe something like "multi-region abstractions have been disabled on this cluster".
arulajmani
requested review from
a team and
shermanCRL
and removed request for
a team
There was a problem hiding this comment.
Skimmed this and it seems reasonable to me. One question: do we want to prevent restoring MR-enabled databases to clusters with this setting set to false? If we don't it seems to be a pretty easy back-door to walk through.
That's a good catch. I tacked on a second commit here which checks the value of this cluster setting when a secondary tenant tries to restore a MR database. @adityamaru would you mind giving the second commit a quick look?
Reviewable status:
arulajmani
force-pushed
the
gate-mr-syntax
branch
from
acfd5ac to
bca251e
Compare
| @@ -1949,6 +1954,32 @@ func renameTargetDatabaseDescriptor( | |||
| return nil | |||
| } | |||
|
|
|||
| // ensureMultiRegionDatabaseRestoreIsAllowed | |||
There was a problem hiding this comment.
nit: comment is truncated
This patch makes it such that RESTORE now consults the `sql.multi_region.allow_abstractions_for_secondary_tenants.enabled` cluster setting when a multi-region database is restored by a secondary tenant. The system tenant remains unaffected. Additonally, similar to how cluster setting above overrides the default primary region on database creation, the same philosophy holds for restore as well. Release note (backup change): The `sql.multi_region.allow_abstractions_for_secondary_tenants.enabled` cluster setting is consulted when a secondary tenant tries to restore a multi-region database now.
arulajmani
force-pushed
the
gate-mr-syntax
branch
from
bca251e to
3590b0e
Compare
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
pkg/ccl/backupccl/restore_planning.go, line 1957 at r4 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
nit: comment is truncated
Done.
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
This patch introduces a tenant read-only cluster setting called
sql.multi_region.allow_abstractions_for_secondary_tenants.enabledwhich allows the operator to control if secondary tenants can make use
of multi-region abstractions. It defaults to false.
This setting is checked against when creating new MR databases or
altering existing ones to make use of MR features (by setting the
primary region). It has nothing to do with MR databases that may have
been created previously which could happen if this setting was ever
flipped to true by the operator.
This setting doesn't effect the system tenant. It also takes precedence
over
sql.defaults.primary_region, which is tenant writeable.Release note (sql change): introduces new cluster setting which allows
the operator to control if a secondary tenant can make use of MR
abstractions. The setting is called
sql.multi_region.allow_abstractions_for_secondary_tenants.enabled.