From 7d7bc9504ac444964e56f7ce62643158389c5e22 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 17 Jun 2023 21:17:04 +0000 Subject: [PATCH] sql: add an advisory check between default_tenant and alter tenant Prior to this patch, it was just too easy to mistakenly configure `default_tenant` to a non-existent tenant or one without service; or to disable the service for a tenant currently serving default traffic. This patch makes this mistake harder, by adding a coherence check between the cluster setting and the service mode. The coherence check can be disabled via the (new) cluster setting `server.controller.default_tenant.check_service.enabled`. Release note: None --- pkg/cli/democluster/BUILD.bazel | 1 + pkg/cli/democluster/demo_cluster.go | 3 +- pkg/multitenant/BUILD.bazel | 2 + pkg/multitenant/tenant_config.go | 40 +++++++++++++++++ pkg/server/authentication.go | 5 ++- pkg/server/server_controller.go | 13 ------ pkg/server/server_controller_http.go | 7 +-- pkg/server/server_controller_sql.go | 3 +- pkg/sql/logictest/testdata/logic_test/tenant | 47 ++++++++++++++++++++ pkg/sql/set_cluster_setting.go | 16 ++++++- pkg/sql/tenant_update.go | 18 ++++++++ 11 files changed, 133 insertions(+), 22 deletions(-) create mode 100644 pkg/multitenant/tenant_config.go diff --git a/pkg/cli/democluster/BUILD.bazel b/pkg/cli/democluster/BUILD.bazel index 445c7058a842..922f29506263 100644 --- a/pkg/cli/democluster/BUILD.bazel +++ b/pkg/cli/democluster/BUILD.bazel @@ -23,6 +23,7 @@ go_library( "//pkg/cli/democluster/api", "//pkg/jobs", "//pkg/kv/kvserver/liveness/livenesspb", + "//pkg/multitenant", "//pkg/roachpb", "//pkg/rpc", "//pkg/security", diff --git a/pkg/cli/democluster/demo_cluster.go b/pkg/cli/democluster/demo_cluster.go index 5ea714831420..365458fe0900 100644 --- a/pkg/cli/democluster/demo_cluster.go +++ b/pkg/cli/democluster/demo_cluster.go @@ -28,6 +28,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/cli/cliflags" "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" + "github.com/cockroachdb/cockroach/pkg/multitenant" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/security" @@ -517,7 +518,7 @@ func (c *transientCluster) Start(ctx context.Context) (err error) { // Choose the tenant to use when no tenant is specified on a // connection or web URL. if _, err := ie.Exec(ctx, "default-tenant", nil, - `SET CLUSTER SETTING `+server.DefaultTenantSelectSettingName+` = $1`, + `SET CLUSTER SETTING `+multitenant.DefaultTenantSelectSettingName+` = $1`, demoTenantName); err != nil { return err } diff --git a/pkg/multitenant/BUILD.bazel b/pkg/multitenant/BUILD.bazel index f048c1cbfbb6..4e2821f07e9c 100644 --- a/pkg/multitenant/BUILD.bazel +++ b/pkg/multitenant/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "constants.go", "cost_controller.go", "doc.go", + "tenant_config.go", "tenant_usage.go", ], importpath = "github.com/cockroachdb/cockroach/pkg/multitenant", @@ -18,6 +19,7 @@ go_library( "//pkg/roachpb", "//pkg/settings", "//pkg/sql/isql", + "//pkg/sql/sem/catconstants", "//pkg/sql/sqlliveness", "//pkg/util/metric", "//pkg/util/stop", diff --git a/pkg/multitenant/tenant_config.go b/pkg/multitenant/tenant_config.go new file mode 100644 index 000000000000..d5cc3422d671 --- /dev/null +++ b/pkg/multitenant/tenant_config.go @@ -0,0 +1,40 @@ +// Copyright 2023 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 multitenant + +import ( + "github.com/cockroachdb/cockroach/pkg/settings" + "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" +) + +// DefaultTenantSelectSettingName is the name of the setting that +// configures the default tenant to use when a client does not specify +// a specific tenant. +var DefaultTenantSelectSettingName = "server.controller.default_tenant" + +// DefaultTenantSelect determines which tenant serves requests from +// clients that do not specify explicitly the tenant they want to use. +var DefaultTenantSelect = settings.RegisterStringSetting( + settings.SystemOnly, + DefaultTenantSelectSettingName, + "name of the tenant to use to serve requests when clients don't specify a tenant", + catconstants.SystemTenantName, +).WithPublic() + +// VerifyTenantService determines whether there should be an advisory +// interlock between changes to the tenant service and changes to the +// above cluster setting. +var VerifyTenantService = settings.RegisterBoolSetting( + settings.SystemOnly, + "server.controller.default_tenant.check_service.enabled", + "verify that a tenant's service is coherently set with the value of "+DefaultTenantSelectSettingName, + true, +) diff --git a/pkg/server/authentication.go b/pkg/server/authentication.go index b82f1246a3c4..d01dcf8a58b7 100644 --- a/pkg/server/authentication.go +++ b/pkg/server/authentication.go @@ -24,6 +24,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/multitenant" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/security/password" @@ -845,7 +846,7 @@ func findAndDecodeSessionCookie( // findSessionCookieValueForTenant finds the encoded session in the provided // aggregated session cookie value established in multi-tenant clusters that's // associated with the provided tenant name. If an empty tenant name is provided, -// we default to the defaultTenantSelect cluster setting value. +// we default to the DefaultTenantSelect cluster setting value. // // If the method cannot find a match between the tenant name and session, or // if the provided session cookie is nil, it will return an empty string. @@ -875,7 +876,7 @@ func findSessionCookieValueForTenant( return mtSessionStr, nil } if tenantName == "" { - tenantName = defaultTenantSelect.Get(&st.SV) + tenantName = multitenant.DefaultTenantSelect.Get(&st.SV) } var encodedSession string for idx, val := range sessionSlice { diff --git a/pkg/server/server_controller.go b/pkg/server/server_controller.go index 351cdd4030de..a52466b7d731 100644 --- a/pkg/server/server_controller.go +++ b/pkg/server/server_controller.go @@ -17,7 +17,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/pgwire" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirecancel" @@ -131,18 +130,6 @@ func newServerController( return c } -// DefaultTenantSelectSettingName is the name of the setting that -// configures the default tenant to use when a client does not specify -// a specific tenant. -var DefaultTenantSelectSettingName = "server.controller.default_tenant" - -var defaultTenantSelect = settings.RegisterStringSetting( - settings.SystemOnly, - DefaultTenantSelectSettingName, - "name of the tenant to use to serve requests when clients don't specify a tenant", - catconstants.SystemTenantName, -).WithPublic() - // tenantServerWrapper implements the onDemandServer interface for SQLServerWrapper. type tenantServerWrapper struct { stopper *stop.Stopper diff --git a/pkg/server/server_controller_http.go b/pkg/server/server_controller_http.go index c692bafbcebb..2e0439aee12a 100644 --- a/pkg/server/server_controller_http.go +++ b/pkg/server/server_controller_http.go @@ -16,6 +16,7 @@ import ( "net/http" "strings" + "github.com/cockroachdb/cockroach/pkg/multitenant" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -93,7 +94,7 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) { // tenant names or sessions (common during development). Otherwise the user can // get into a state where they cannot load DB Console assets at all due to invalid // cookies. - defaultTenantName := roachpb.TenantName(defaultTenantSelect.Get(&c.st.SV)) + defaultTenantName := roachpb.TenantName(multitenant.DefaultTenantSelect.Get(&c.st.SV)) s, err = c.getServer(ctx, defaultTenantName) if err != nil { log.Warningf(ctx, "unable to find server for default tenant %q: %v", defaultTenantName, err) @@ -123,7 +124,7 @@ func getTenantNameFromHTTPRequest(st *cluster.Settings, r *http.Request) roachpb } // No luck so far. Use the configured default. - return roachpb.TenantName(defaultTenantSelect.Get(&st.SV)) + return roachpb.TenantName(multitenant.DefaultTenantSelect.Get(&st.SV)) } func getSessionFromCookie(sessionStr string, name roachpb.TenantName) string { @@ -221,7 +222,7 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler { // if it's one of the valid logins. Otherwise, we just use the // first one in the list. tenantSelection := tenantNameToSetCookieSlice[0].name - defaultName := defaultTenantSelect.Get(&c.st.SV) + defaultName := multitenant.DefaultTenantSelect.Get(&c.st.SV) for _, t := range tenantNameToSetCookieSlice { if t.name == defaultName { tenantSelection = t.name diff --git a/pkg/server/server_controller_sql.go b/pkg/server/server_controller_sql.go index d7837b81744c..218e7e81a24d 100644 --- a/pkg/server/server_controller_sql.go +++ b/pkg/server/server_controller_sql.go @@ -14,6 +14,7 @@ import ( "context" "net" + "github.com/cockroachdb/cockroach/pkg/multitenant" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/sql/pgwire" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirecancel" @@ -60,7 +61,7 @@ func (c *serverController) sqlMux( case pgwire.PreServeReady: tenantName := roachpb.TenantName(status.GetTenantName()) if tenantName == "" { - tenantName = roachpb.TenantName(defaultTenantSelect.Get(&c.st.SV)) + tenantName = roachpb.TenantName(multitenant.DefaultTenantSelect.Get(&c.st.SV)) } s, err := c.getServer(ctx, tenantName) diff --git a/pkg/sql/logictest/testdata/logic_test/tenant b/pkg/sql/logictest/testdata/logic_test/tenant index e9bcbf9d2994..8e13a94a5483 100644 --- a/pkg/sql/logictest/testdata/logic_test/tenant +++ b/pkg/sql/logictest/testdata/logic_test/tenant @@ -502,3 +502,50 @@ RESET disable_drop_tenant statement ok DROP TENANT nodelete + +subtest regression_105115 + +statement ok +CREATE TENANT noservice + +statement error shared service not enabled for tenant "noservice" +SET CLUSTER SETTING server.controller.default_tenant = noservice + +statement ok +SET CLUSTER SETTING server.controller.default_tenant.check_service.enabled = false + +statement ok +SET CLUSTER SETTING server.controller.default_tenant = noservice + +statement ok +RESET CLUSTER SETTING server.controller.default_tenant.check_service.enabled + +statement ok +RESET CLUSTER SETTING server.controller.default_tenant + +statement ok +DROP TENANT noservice; +CREATE TENANT withservice; +ALTER TENANT withservice START SERVICE SHARED + +statement ok +SET CLUSTER SETTING server.controller.default_tenant = withservice + +statement error cannot stop service while tenant is selected as default +ALTER TENANT withservice STOP SERVICE + +statement ok +SET CLUSTER SETTING server.controller.default_tenant.check_service.enabled = false + +statement ok +ALTER TENANT withservice STOP SERVICE + +# clean up +statement ok +RESET CLUSTER SETTING server.controller.default_tenant.check_service.enabled + +statement ok +RESET CLUSTER SETTING server.controller.default_tenant + +statement ok +DROP TENANT withservice diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index dcefa75404b1..6f4ee69a6d94 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -20,6 +20,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/docs" + "github.com/cockroachdb/cockroach/pkg/multitenant" + "github.com/cockroachdb/cockroach/pkg/multitenant/mtinfopb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/telemetry" @@ -146,7 +148,7 @@ func checkPrivilegesForSetting(ctx context.Context, p *planner, name string, act return nil } -// SetClusterSetting sets session variables. +// SetClusterSetting sets cluster settings. // Privileges: super user. func (p *planner) SetClusterSetting( ctx context.Context, n *tree.SetClusterSetting, @@ -250,7 +252,6 @@ func (p *planner) getAndValidateTypedClusterSetting( } func (n *setClusterSettingNode) startExec(params runParams) error { - if strings.HasPrefix(n.name, "sql.defaults") { params.p.BufferClientNotice( params.ctx, @@ -299,6 +300,17 @@ func (n *setClusterSettingNode) startExec(params runParams) error { // Report tracked cluster settings via telemetry. // TODO(justin): implement a more general mechanism for tracking these. switch n.name { + case multitenant.DefaultTenantSelectSettingName: + if multitenant.VerifyTenantService.Get(&n.st.SV) && expectedEncodedValue != "" { + tr, err := GetTenantRecordByName(params.ctx, n.st, params.p.InternalSQLTxn(), roachpb.TenantName(expectedEncodedValue)) + if err != nil { + return errors.Wrapf(err, "failed to lookup tenant %q", expectedEncodedValue) + } + if tr.ServiceMode != mtinfopb.ServiceModeShared { + return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "shared service not enabled for tenant %q", expectedEncodedValue) + } + } case catpb.AutoStatsEnabledSettingName: switch expectedEncodedValue { case "true": diff --git a/pkg/sql/tenant_update.go b/pkg/sql/tenant_update.go index 5dace6c28915..03c7e1c6cb4d 100644 --- a/pkg/sql/tenant_update.go +++ b/pkg/sql/tenant_update.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/multitenant" "github.com/cockroachdb/cockroach/pkg/multitenant/mtinfopb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" @@ -111,6 +112,23 @@ func validateTenantInfo( } } + // Sanity check. Note that this interlock is not a guarantee that + // the cluster setting will never be set to an invalid tenant. There + // is a race condition between changing the cluster setting and the + // check here. Generally, other subsystems should always tolerate + // when the cluster setting is set to a tenant without service (or + // even one that doesn't exist). + if multitenant.VerifyTenantService.Get(&settings.SV) && + info.ServiceMode == mtinfopb.ServiceModeNone && + info.Name != "" && + multitenant.DefaultTenantSelect.Get(&settings.SV) == string(info.Name) { + return errors.WithHintf( + pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "cannot stop service while tenant is selected as default"), + "Update the cluster setting %q to a different value.", + multitenant.DefaultTenantSelectSettingName) + } + return nil }