Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql: block DROP TENANT based on a session var #99607

Merged
merged 1 commit into from
Mar 30, 2023
Merged

sql: block DROP TENANT based on a session var #99607

merged 1 commit into from
Mar 30, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Mar 26, 2023 •
edited
Loading

Fixes #97972.
Epic: CRDB-23559

In clusters where we will promote tenant management operations, we would like to ensure there is one extra step needed for administrators to drop a tenant (and thus irremedially lose data). Given that sql_safe_updates is not set automatically when users open their SQL session using their own client, we need another mechanism.

This change introduces the new (hidden) session var, disable_drop_tenant. When set, tenant deletion fails with the following error message:

demo@127.0.0.1:26257/movr> drop tenant foo;
ERROR: rejected (via sql_safe_updates or disable_drop_tenant): DROP TENANT causes irreversible data loss
SQLSTATE: 01000

(The session var sql_safe_updates is also included as a blocker in the mechanism so that folk using cockroach sql get double protection).

The default value of this session var is false in single-tenant clusters, for compatibility with CC Serverless. It will be set to true via a config profile (#98466) when suitable.

Release note: None

@knz knz requested a review from stevendanna March 26, 2023 12:24
@knz knz requested a review from a team as a code owner March 26, 2023 12:24
@knz knz requested a review from yuzefovich March 26, 2023 12:24
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 26, 2023
@knz knz force-pushed the 20230326-drop-tenant branch 4 times, most recently from 885a6e9 to 6364cee Compare March 26, 2023 22:00
yuzefovich
yuzefovich reviewed Mar 27, 2023
View reviewed changes
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @stevendanna)

pkg/sql/vars.go line 1547 at r1 (raw file):

			//   default value (false) to mean that tenant deletion is enabled.
			//   This is needed for backward-compatibility with Cockroach Cloud.
			return fmt.Sprint(!enableDropTenant.Get(sv))

nit: maybe use formatBoolAsPostgresSetting?

pkg/sql/sessiondatapb/local_only_session_data.proto line 368 at r1 (raw file):

  // prepared again.
  int64 prepared_statements_cache_size = 97;

nit: dangling spaces. Also this proto doesn't use empty lines between members.

@knz knz added the A-multitenancy Related to multi-tenancy label Mar 27, 2023
@knz
sql: block DROP TENANT based on a session var
bdc8d1a 
In clusters where we will promote tenant management operations, we
would like to ensure there is one extra step needed for administrators
to drop a tenant (and thus irremedially lose data). Given that
`sql_safe_updates` is not set automatically when users open their
SQL session using their own client, we need another mechanism.

This change introduces the new (hidden) session var,
`disable_drop_tenant`. When set, tenant deletion fails with the
following error message:

```
demo@127.0.0.1:26257/movr> drop tenant foo;
ERROR: rejected (via sql_safe_updates or disable_drop_tenant): DROP TENANT causes irreversible data loss
SQLSTATE: 01000
```

(The session var `sql_safe_updates` is _also_ included as a blocker in
the mechanism so that folk using `cockroach sql` get double
protection).

The default value of this session var is `false` in single-tenant
clusters (set via a cluster setting `sql.drop_tenant.enabled`), for
compatibility with CC Serverless. The default will be set to `true`
via a config profile when suitable.

Release note: None
knz
knz commented Mar 28, 2023
View reviewed changes
Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)

pkg/sql/vars.go line 1547 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: maybe use formatBoolAsPostgresSetting?

Done.

pkg/sql/sessiondatapb/local_only_session_data.proto line 368 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: dangling spaces. Also this proto doesn't use empty lines between members.

Done.

stevendanna
stevendanna approved these changes Mar 28, 2023
View reviewed changes
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me.

Reviewed 4 of 6 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)

@knz
Copy link
Contributor Author

knz commented Mar 28, 2023

TFYR!

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Mar 28, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 28, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 28, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Build failed (retrying...):

@knz
Copy link
Contributor Author

knz commented Mar 29, 2023

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Already running a review

@knz
Copy link
Contributor Author

knz commented Mar 29, 2023

bors r-

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Canceled.

@knz
Copy link
Contributor Author

knz commented Mar 29, 2023

bors r=stevendanna single on

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Build failed (retrying...):

rafiss
rafiss reviewed Mar 30, 2023
View reviewed changes
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)

pkg/sql/exec_util.go line 764 at r2 (raw file):

var enableDropTenant = settings.RegisterBoolSetting(
	settings.SystemOnly,
	"sql.drop_tenant.enabled",

I'm curious - why doesn't this use the convention we use for other cluster settings that set default values for session vars? i.e., name it something like sql.defaults.disable_drop_tenant

@craig craig bot merged commit 331a672 into cockroachdb:master Mar 30, 2023
@craig
Copy link
Contributor

craig bot commented Mar 30, 2023

Build succeeded:

@blathers-crl blathers-crl bot mentioned this pull request Mar 30, 2023
Merged
@knz
Copy link
Contributor Author

knz commented Mar 30, 2023

@rafiss because we now have a linter that blocks the introduction of settings named sql.defaults.*.

@rafiss
Copy link
Collaborator

rafiss commented Mar 30, 2023

I see now that this is SystemOnly and non-public, so I'm no longer concerned. I just thought it would be harder to find for users, but that concern doesn't apply here.

@knz knz deleted the 20230326-drop-tenant branch March 30, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafiss rafiss rafiss left review comments

@yuzefovich yuzefovich yuzefovich left review comments

@stevendanna stevendanna stevendanna approved these changes

Assignees
No one assigned
Labels
A-multitenancy Related to multi-tenancy backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

multitenant: Protect from accidental use of DROP TENANT
5 participants
@knz @cockroach-teamcity @rafiss @stevendanna @yuzefovich