diff --git a/docs/generated/eventlog.md b/docs/generated/eventlog.md index cc042a51e9d0..d100d5ae5106 100644 --- a/docs/generated/eventlog.md +++ b/docs/generated/eventlog.md @@ -314,6 +314,33 @@ An event of type `set_cluster_setting` is recorded when a cluster setting is cha | `Value` | The new value of the cluster setting. | yes | +#### Common fields + +| Field | Description | Sensitive | +|--|--|--| +| `Timestamp` | The timestamp of the event. Expressed as nanoseconds since the Unix epoch. | no | +| `EventType` | The type of the event. | no | +| `Statement` | A normalized copy of the SQL statement that triggered the event. The statement string contains a mix of sensitive and non-sensitive details (it is redactable). | partially | +| `Tag` | The statement tag. This is separate from the statement string, since the statement string can contain sensitive information. The tag is guaranteed not to. | no | +| `User` | The user account that triggered the event. The special usernames `root` and `node` are not considered sensitive. | depends | +| `DescriptorID` | The primary object descriptor affected by the operation. Set to zero for operations that don't affect descriptors. | no | +| `ApplicationName` | The application name for the session where the event was emitted. This is included in the event to ease filtering of logging output by application. Application names starting with a dollar sign (`$`) are not considered sensitive. | depends | +| `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes | + +### `set_tenant_cluster_setting` + +An event of type `set_tenant_cluster_setting` is recorded when a cluster setting override +is changed, either for another tenant or for all tenants. + + +| Field | Description | Sensitive | +|--|--|--| +| `SettingName` | The name of the affected cluster setting. | no | +| `Value` | The new value of the cluster setting. | yes | +| `TenantId` | The target Tenant ID. Empty if targeting all tenants. | no | +| `AllTenants` | Whether the override applies to all tenants. | no | + + #### Common fields | Field | Description | Sensitive | diff --git a/pkg/ccl/logictestccl/testdata/logic_test/tenant_settings b/pkg/ccl/logictestccl/testdata/logic_test/tenant_settings index a93a88c0b5e4..31872063307d 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/tenant_settings +++ b/pkg/ccl/logictestccl/testdata/logic_test/tenant_settings @@ -7,9 +7,8 @@ true user host-cluster-root -# TODO(radu): use the new ALTER TENANT statement when it is implemented. statement ok -INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (10, 'sql.notices.enabled', 'false', 'b') +ALTER TENANT 10 SET CLUSTER SETTING sql.notices.enabled = false user root @@ -22,7 +21,7 @@ false user host-cluster-root statement ok -DELETE FROM system.tenant_settings WHERE tenant_id = 10 +ALTER TENANT 10 RESET CLUSTER SETTING sql.notices.enabled user root @@ -35,7 +34,7 @@ user host-cluster-root # Set an all-tenant override. statement ok -INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (0, 'sql.notices.enabled', 'false', 'b') +ALTER TENANT ALL SET CLUSTER SETTING sql.notices.enabled = false user root @@ -48,7 +47,7 @@ user host-cluster-root # Now set a tenant-specific override which takes precedence. statement ok -INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (10, 'sql.notices.enabled', 'true', 'b') +ALTER TENANT 10 SET CLUSTER SETTING sql.notices.enabled = true user root @@ -68,7 +67,7 @@ user host-cluster-root # Remove the all-tenant override; should make no difference. statement ok -DELETE FROM system.tenant_settings WHERE tenant_id = 0 +ALTER TENANT ALL RESET CLUSTER SETTING sql.notices.enabled user root @@ -99,8 +98,7 @@ SHOW CLUSTER SETTING kv.protectedts.reconciliation.interval user host-cluster-root statement ok -INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) - VALUES (10, 'kv.protectedts.reconciliation.interval', '45s', 'd') +ALTER TENANT 10 SET CLUSTER SETTING kv.protectedts.reconciliation.interval = '45s' user root @@ -115,9 +113,13 @@ user host-cluster-root statement ok SELECT crdb_internal.create_tenant(1234) -# TODO(radu): replace with ALTER TENANT when it's available. statement ok -INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (1234, 'sql.notices.enabled', 'true', 'b') +ALTER TENANT 1234 SET CLUSTER SETTING sql.notices.enabled = true + +query I +SELECT count(*) FROM system.tenant_settings WHERE tenant_id = 1234 +---- +1 statement ok SELECT crdb_internal.destroy_tenant(1234, true) diff --git a/pkg/sql/logictest/testdata/logic_test/cluster_settings b/pkg/sql/logictest/testdata/logic_test/cluster_settings index 3014e9b003d0..6acf8ad155a1 100644 --- a/pkg/sql/logictest/testdata/logic_test/cluster_settings +++ b/pkg/sql/logictest/testdata/logic_test/cluster_settings @@ -172,7 +172,7 @@ subtest tenant-cluster-settings # Skip this test when inside a tenant, as this setting won't be visible in # there. skipif config 3node-tenant -statement error admission.kv.enabled is a system-only setting and must be set at the host cluster level using SET CLUSTER SETTING +statement error admission.kv.enabled is a system-only setting and must be set in the admin tenant using SET CLUSTER SETTING ALTER TENANT 10 SET CLUSTER SETTING admission.kv.enabled=true onlyif config 3node-tenant @@ -180,29 +180,45 @@ statement error ALTER TENANT can only be called by system operators ALTER TENANT 10 SET CLUSTER SETTING server.mem_profile.total_dump_size_limit='10M' skipif config 3node-tenant -statement error unimplemented +statement error cannot use ALTER TENANT to change cluster settings in system tenant +ALTER TENANT 1 SET CLUSTER SETTING server.mem_profile.total_dump_size_limit='10M' + +skipif config 3node-tenant +statement ok +SELECT crdb_internal.create_tenant(10) + +skipif config 3node-tenant +statement ok ALTER TENANT 10 SET CLUSTER SETTING server.mem_profile.total_dump_size_limit='10M' skipif config 3node-tenant -statement error unimplemented +statement ok ALTER TENANT ALL SET CLUSTER SETTING server.mem_profile.total_dump_size_limit='10M' skipif config 3node-tenant -statement error unimplemented +statement ok ALTER TENANT 10 RESET CLUSTER SETTING server.mem_profile.total_dump_size_limit skipif config 3node-tenant -statement error unimplemented +statement ok ALTER TENANT ALL RESET CLUSTER SETTING server.mem_profile.total_dump_size_limit +skipif config 3node-tenant statement error unimplemented SHOW CLUSTER SETTING server.mem_profile.total_dump_size_limit FOR TENANT 10 +onlyif config 3node-tenant +statement error unimplemented +SHOW CLUSTER SETTINGS FOR TENANT 10 + +skipif config 3node-tenant statement error unimplemented SHOW CLUSTER SETTINGS FOR TENANT 10 +skipif config 3node-tenant statement error unimplemented SHOW PUBLIC CLUSTER SETTINGS FOR TENANT 10 +skipif config 3node-tenant statement error unimplemented SHOW ALL CLUSTER SETTINGS FOR TENANT 10 diff --git a/pkg/sql/tenant_settings.go b/pkg/sql/tenant_settings.go index 7fc8ec92cb06..a318e92def67 100644 --- a/pkg/sql/tenant_settings.go +++ b/pkg/sql/tenant_settings.go @@ -14,13 +14,17 @@ import ( "context" "strings" + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" + "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" "github.com/cockroachdb/errors" ) @@ -40,7 +44,33 @@ type alterTenantSetClusterSettingNode struct { func (p *planner) AlterTenantSetClusterSetting( ctx context.Context, n *tree.AlterTenantSetClusterSetting, ) (planNode, error) { + // Changing cluster settings for other tenants is a more + // privileged operation than changing local cluster settings. So we + // shouldn't be allowing with just the role option + // MODIFYCLUSTERSETTINGS. + // + // TODO(knz): Using admin authz for now; we may want to introduce a + // more specific role option later. + if err := p.RequireAdminRole(ctx, "change a tenant cluster setting"); err != nil { + return nil, err + } + // Error out if we're trying to call this from a non-system tenant. + if !p.execCfg.Codec.ForSystemTenant() { + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "ALTER TENANT can only be called by system operators") + } + name := strings.ToLower(n.Name) + st := p.EvalContext().Settings + v, ok := settings.Lookup(name, settings.LookupForLocalAccess, true /* forSystemTenant - checked above already */) + if !ok { + return nil, errors.Errorf("unknown cluster setting '%s'", name) + } + // Error out if we're trying to set a system-only variable. + if v.Class() == settings.SystemOnly { + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "%s is a system-only setting and must be set in the admin tenant using SET CLUSTER SETTING", name) + } var typedTenantID tree.TypedExpr if !n.TenantAll { @@ -53,30 +83,14 @@ func (p *planner) AlterTenantSetClusterSetting( } } - st := p.EvalContext().Settings - v, ok := settings.Lookup(name, settings.LookupForLocalAccess, p.ExecCfg().Codec.ForSystemTenant()) - if !ok { - return nil, errors.Errorf("unknown cluster setting '%s'", name) - } - - if err := checkPrivilegesForSetting(ctx, p, name, "set"); err != nil { - return nil, err - } - setting, ok := v.(settings.NonMaskedSetting) if !ok { return nil, errors.AssertionFailedf("expected writable setting, got %T", v) } - // Error out if we're trying to call this from a non-system tenant or if - // we're trying to set a system-only variable. - if !p.execCfg.Codec.ForSystemTenant() { - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, - "ALTER TENANT can only be called by system operators") - } - if setting.Class() == settings.SystemOnly { - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, - "%s is a system-only setting and must be set at the host cluster level using SET CLUSTER SETTING", name) + // We don't support changing the version for another tenant just yet. + if _, isVersion := setting.(*settings.VersionSetting); isVersion { + return nil, unimplemented.NewWithIssue(77733, "cannot change the version of another tenant") } value, err := p.getAndValidateTypedClusterSetting(ctx, name, n.Value, setting) @@ -92,14 +106,109 @@ func (p *planner) AlterTenantSetClusterSetting( } func (n *alterTenantSetClusterSettingNode) startExec(params runParams) error { - return unimplemented.NewWithIssue(73857, - `unimplemented: tenant-level cluster settings not supported`) + var tenantIDi uint64 + var tenantID tree.Datum + if n.tenantID == nil { + // Processing for TENANT ALL. + // We will be writing rows with tenant_id = 0 in + // system.tenant_settings. + tenantID = tree.NewDInt(0) + } else { + // Case for TENANT . We'll check that the provided + // tenant ID is non zero and refers to a tenant that exists in + // system.tenants. + var err error + tenantIDi, tenantID, err = resolveTenantID(params, n.tenantID) + if err != nil { + return err + } + if err := assertTenantExists(params, tenantID); err != nil { + return err + } + } + + // Write the setting. + var reportedValue string + if n.value == nil { + // TODO(radu,knz): DEFAULT might be confusing, we really want to say "NO OVERRIDE" + reportedValue = "DEFAULT" + if _, err := params.p.execCfg.InternalExecutor.ExecEx( + params.ctx, "reset-tenant-setting", params.p.Txn(), + sessiondata.InternalExecutorOverride{User: security.RootUserName()}, + "DELETE FROM system.tenant_settings WHERE tenant_id = $1 AND name = $2", tenantID, n.name, + ); err != nil { + return err + } + } else { + reportedValue = tree.AsStringWithFlags(n.value, tree.FmtBareStrings) + value, err := n.value.Eval(params.p.EvalContext()) + if err != nil { + return err + } + encoded, err := toSettingString(params.ctx, n.st, n.name, n.setting, value) + if err != nil { + return err + } + if _, err := params.p.execCfg.InternalExecutor.ExecEx( + params.ctx, "update-tenant-setting", params.p.Txn(), + sessiondata.InternalExecutorOverride{User: security.RootUserName()}, + `UPSERT INTO system.tenant_settings (tenant_id, name, value, last_updated, value_type) VALUES ($1, $2, $3, now(), $4)`, + tenantID, n.name, encoded, n.setting.Typ(), + ); err != nil { + return err + } + } + + // Finally, log the event. + return params.p.logEvent( + params.ctx, + 0, /* no target */ + &eventpb.SetTenantClusterSetting{ + SettingName: n.name, + Value: reportedValue, + TenantId: tenantIDi, + AllTenants: tenantIDi == 0, + }) } func (n *alterTenantSetClusterSettingNode) Next(_ runParams) (bool, error) { return false, nil } func (n *alterTenantSetClusterSettingNode) Values() tree.Datums { return nil } func (n *alterTenantSetClusterSettingNode) Close(_ context.Context) {} +func resolveTenantID(params runParams, expr tree.TypedExpr) (uint64, tree.Datum, error) { + tenantIDd, err := expr.Eval(params.p.EvalContext()) + if err != nil { + return 0, nil, err + } + tenantID, ok := tenantIDd.(*tree.DInt) + if !ok { + return 0, nil, errors.AssertionFailedf("expected int, got %T", tenantIDd) + } + if *tenantID == 0 { + return 0, nil, pgerror.Newf(pgcode.InvalidParameterValue, "tenant ID must be non-zero") + } + if roachpb.MakeTenantID(uint64(*tenantID)) == roachpb.SystemTenantID { + return 0, nil, errors.WithHint(pgerror.Newf(pgcode.InvalidParameterValue, + "cannot use ALTER TENANT to change cluster settings in system tenant"), + "Use a regular SET CLUSTER SETTING statement.") + } + return uint64(*tenantID), tenantIDd, nil +} + +func assertTenantExists(params runParams, tenantID tree.Datum) error { + exists, err := params.p.ExecCfg().InternalExecutor.QueryRowEx( + params.ctx, "get-tenant", params.p.txn, + sessiondata.InternalExecutorOverride{User: security.RootUserName()}, + `SELECT EXISTS(SELECT id FROM system.tenants WHERE id = $1)`, tenantID) + if err != nil { + return err + } + if exists[0] != tree.DBoolTrue { + return pgerror.Newf(pgcode.InvalidParameterValue, "no tenant found with ID %v", tenantID) + } + return nil +} + // ShowTenantClusterSetting shows the value of a cluster setting for a tenant. // Privileges: super user. func (p *planner) ShowTenantClusterSetting( diff --git a/pkg/util/log/eventpb/eventlog_channels_generated.go b/pkg/util/log/eventpb/eventlog_channels_generated.go index 034059339244..74eeee9f3c79 100644 --- a/pkg/util/log/eventpb/eventlog_channels_generated.go +++ b/pkg/util/log/eventpb/eventlog_channels_generated.go @@ -40,6 +40,9 @@ func (m *Restore) LoggingChannel() logpb.Channel { return logpb.Channel_OPS } // LoggingChannel implements the EventPayload interface. func (m *SetClusterSetting) LoggingChannel() logpb.Channel { return logpb.Channel_DEV } +// LoggingChannel implements the EventPayload interface. +func (m *SetTenantClusterSetting) LoggingChannel() logpb.Channel { return logpb.Channel_DEV } + // LoggingChannel implements the EventPayload interface. func (m *AdminQuery) LoggingChannel() logpb.Channel { return logpb.Channel_SENSITIVE_ACCESS } diff --git a/pkg/util/log/eventpb/json_encode_generated.go b/pkg/util/log/eventpb/json_encode_generated.go index 5bf28df80a7b..42eb44f059b8 100644 --- a/pkg/util/log/eventpb/json_encode_generated.go +++ b/pkg/util/log/eventpb/json_encode_generated.go @@ -3219,6 +3219,55 @@ func (m *SetSchema) AppendJSONFields(printComma bool, b redact.RedactableBytes) return printComma, b } +// AppendJSONFields implements the EventPayload interface. +func (m *SetTenantClusterSetting) AppendJSONFields(printComma bool, b redact.RedactableBytes) (bool, redact.RedactableBytes) { + + printComma, b = m.CommonEventDetails.AppendJSONFields(printComma, b) + + printComma, b = m.CommonSQLEventDetails.AppendJSONFields(printComma, b) + + if m.SettingName != "" { + if printComma { + b = append(b, ',') + } + printComma = true + b = append(b, "\"SettingName\":\""...) + b = redact.RedactableBytes(jsonbytes.EncodeString([]byte(b), string(m.SettingName))) + b = append(b, '"') + } + + if m.Value != "" { + if printComma { + b = append(b, ',') + } + printComma = true + b = append(b, "\"Value\":\""...) + b = append(b, redact.StartMarker()...) + b = redact.RedactableBytes(jsonbytes.EncodeString([]byte(b), string(redact.EscapeMarkers([]byte(m.Value))))) + b = append(b, redact.EndMarker()...) + b = append(b, '"') + } + + if m.TenantId != 0 { + if printComma { + b = append(b, ',') + } + printComma = true + b = append(b, "\"TenantId\":"...) + b = strconv.AppendUint(b, uint64(m.TenantId), 10) + } + + if m.AllTenants { + if printComma { + b = append(b, ',') + } + printComma = true + b = append(b, "\"AllTenants\":true"...) + } + + return printComma, b +} + // AppendJSONFields implements the EventPayload interface. func (m *SetZoneConfig) AppendJSONFields(printComma bool, b redact.RedactableBytes) (bool, redact.RedactableBytes) { diff --git a/pkg/util/log/eventpb/misc_sql_events.proto b/pkg/util/log/eventpb/misc_sql_events.proto index f4dffdb58f41..cd571f6756f7 100644 --- a/pkg/util/log/eventpb/misc_sql_events.proto +++ b/pkg/util/log/eventpb/misc_sql_events.proto @@ -40,3 +40,17 @@ message SetClusterSetting { } +// SetTenantClusterSetting is recorded when a cluster setting override +// is changed, either for another tenant or for all tenants. +message SetTenantClusterSetting { + CommonEventDetails common = 1 [(gogoproto.nullable) = false, (gogoproto.jsontag) = "", (gogoproto.embed) = true]; + CommonSQLEventDetails sql = 2 [(gogoproto.nullable) = false, (gogoproto.jsontag) = "", (gogoproto.embed) = true]; + // The name of the affected cluster setting. + string setting_name = 3 [(gogoproto.jsontag) = ",omitempty", (gogoproto.moretags) = "redact:\"nonsensitive\""]; + // The new value of the cluster setting. + string value = 4 [(gogoproto.jsontag) = ",omitempty"]; + // The target Tenant ID. Empty if targeting all tenants. + uint64 tenant_id = 5 [(gogoproto.jsontag) = ",omitempty"]; + // Whether the override applies to all tenants. + bool all_tenants = 6 [(gogoproto.jsontag) = ",omitempty"]; +}