From 20855d0e327d7b289fb80b40e3e174996d158177 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 15 Dec 2020 00:09:30 +0000 Subject: [PATCH] sql: remove limitations on tenants setting cluster settings This change also removes the limits in the SET CLUSTER SETTING code path that prevented tenants writing to their settings table before those changes would have had any effect. Release note: none. --- pkg/ccl/serverccl/BUILD.bazel | 2 -- pkg/ccl/serverccl/server_sql_test.go | 7 +++-- pkg/sql/exec_util.go | 6 ---- pkg/sql/set_cluster_setting.go | 45 +++++----------------------- 4 files changed, 12 insertions(+), 48 deletions(-) diff --git a/pkg/ccl/serverccl/BUILD.bazel b/pkg/ccl/serverccl/BUILD.bazel index abbe55463fb6..569b67a7282f 100644 --- a/pkg/ccl/serverccl/BUILD.bazel +++ b/pkg/ccl/serverccl/BUILD.bazel @@ -26,7 +26,6 @@ go_test( "//pkg/server", "//pkg/server/serverpb", "//pkg/sql", - "//pkg/sql/pgwire/pgcode", "//pkg/testutils/serverutils", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", @@ -36,7 +35,6 @@ go_test( "//pkg/util/log", "//pkg/util/randutil", "//pkg/util/timeutil", - "@com_github_lib_pq//:pq", "@com_github_stretchr_testify//require", "@org_golang_x_crypto//bcrypt", ], diff --git a/pkg/ccl/serverccl/server_sql_test.go b/pkg/ccl/serverccl/server_sql_test.go index ca96a7199f8a..57ebe6603930 100644 --- a/pkg/ccl/serverccl/server_sql_test.go +++ b/pkg/ccl/serverccl/server_sql_test.go @@ -10,7 +10,6 @@ package serverccl import ( "context" - "errors" "io/ioutil" "net/http" "testing" @@ -18,13 +17,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/httputil" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/lib/pq" "github.com/stretchr/testify/require" ) @@ -75,10 +72,14 @@ func TestTenantCannotSetClusterSetting(t *testing.T) { db := serverutils.StartTenant(t, tc.Server(0), base.TestTenantArgs{TenantID: roachpb.MakeTenantID(10), AllowSettingClusterSettings: false}) defer db.Close() _, err := db.Exec(`SET CLUSTER SETTING sql.defaults.vectorize=off`) + require.NoError(t, err) + /* TODO(dt): re-introduce when system-settings are prevented in tenants. + _, err = db.Exec(`SET CLUSTER SETTING kv.snapshot_rebalance.max_rate = '2MiB';`) var pqErr *pq.Error ok := errors.As(err, &pqErr) require.True(t, ok, "expected err to be a *pq.Error but is of type %T. error is: %v", err) require.Equal(t, pq.ErrorCode(pgcode.InsufficientPrivilege.String()), pqErr.Code, "err %v has unexpected code", err) + */ } func TestTenantUnauthenticatedAccess(t *testing.T) { diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 28d7868bd9c8..6118a40f5f69 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -906,12 +906,6 @@ var _ base.ModuleTestingKnobs = &TenantTestingKnobs{} // ModuleTestingKnobs implements the base.ModuleTestingKnobs interface. func (*TenantTestingKnobs) ModuleTestingKnobs() {} -// CanSetClusterSettings is a helper method that returns whether the tenant can -// set in-memory cluster settings. -func (k *TenantTestingKnobs) CanSetClusterSettings() bool { - return k != nil && k.ClusterSettingsUpdater != nil -} - // BackupRestoreTestingKnobs contains knobs for backup and restore behavior. type BackupRestoreTestingKnobs struct { // AllowImplicitAccess allows implicit access to data sources for non-admin diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index 249ad5d68cfc..5376b7a702cb 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -77,12 +77,6 @@ func checkPrivilegesForSetting(ctx context.Context, p *planner, name string, act func (p *planner) SetClusterSetting( ctx context.Context, n *tree.SetClusterSetting, ) (planNode, error) { - if !p.execCfg.TenantTestingKnobs.CanSetClusterSettings() && !p.execCfg.Codec.ForSystemTenant() { - // Setting cluster settings is disabled for phase 2 tenants if a test does - // not explicitly allow for setting in-memory cluster settings. - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "only the system tenant can SET CLUSTER SETTING") - } - name := strings.ToLower(n.Name) st := p.EvalContext().Settings v, ok := settings.Lookup(name, settings.LookupForLocalAccess) @@ -99,12 +93,6 @@ func (p *planner) SetClusterSetting( return nil, errors.AssertionFailedf("expected writable setting, got %T", v) } - if _, ok := setting.(*settings.VersionSetting); ok && p.execCfg.TenantTestingKnobs.CanSetClusterSettings() { - // A tenant that is allowed to set in-memory cluster settings is - // attempting to set the cluster version setting, which is disallowed. - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "only the system tenant can set version settings") - } - var value tree.TypedExpr if n.Value != nil { // For DEFAULT, let the value reference be nil. That's a RESET in disguise. @@ -174,31 +162,6 @@ func (n *setClusterSettingNode) startExec(params runParams) error { if !params.p.ExtendedEvalContext().TxnImplicit { return errors.Errorf("SET CLUSTER SETTING cannot be used inside a transaction") } - - if !params.p.execCfg.Codec.ForSystemTenant() { - // Sanity check that this tenant is able to set in-memory settings. - if !params.p.execCfg.TenantTestingKnobs.CanSetClusterSettings() { - return errors.Errorf("tenants cannot set cluster settings, this permission should have been checked at plan time") - } - var encodedValue string - if n.value == nil { - encodedValue = n.setting.EncodedDefault() - } else { - value, err := n.value.Eval(params.p.EvalContext()) - if err != nil { - return err - } - if _, ok := n.setting.(*settings.VersionSetting); ok { - return errors.Errorf("tenants cannot change cluster version setting, this should've been checked at plan time") - } - encodedValue, err = toSettingString(params.ctx, n.st, n.name, n.setting, value, nil /* prev */) - if err != nil { - return err - } - } - return params.p.execCfg.TenantTestingKnobs.ClusterSettingsUpdater.Set(n.name, encodedValue, n.setting.Typ()) - } - execCfg := params.extendedEvalCtx.ExecCfg var expectedEncodedValue string if err := execCfg.DB.Txn(params.ctx, func(ctx context.Context, txn *kv.Txn) error { @@ -272,6 +235,14 @@ func (n *setClusterSettingNode) startExec(params runParams) error { ); err != nil { return err } + + if params.p.execCfg.TenantTestingKnobs != nil { + if err := params.p.execCfg.TenantTestingKnobs.ClusterSettingsUpdater.Set( + n.name, encoded, n.setting.Typ(), + ); err != nil { + return err + } + } } // Report tracked cluster settings via telemetry.