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.

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

multitenant: add can_prepare_txns tenant capability #137457

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Dec 13, 2024

Informs #22329.

This commit adds a new can_prepare_txns tenant capability, so that we adon't allow secondary tenants to prepare transactions by default. Allowing aan untrusted tenant to prepare transactions would allow it to block the aprogress of system-wide backups, so it is too dangerous to allow by default.

Release note: None

Copy link

blathers-crl bot commented Dec 13, 2024

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/2pcTenant branch from 0c05a52 to f7f7e69 Compare December 13, 2024 22:24
@nvanbenschoten nvanbenschoten marked this pull request as ready for review December 13, 2024 22:24
@nvanbenschoten nvanbenschoten requested review from a team as code owners December 13, 2024 22:24
@nvanbenschoten nvanbenschoten requested review from mw5h and removed request for a team December 13, 2024 22:24
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/2pcTenant branch 3 times, most recently from 0f93871 to d24646c Compare December 18, 2024 16:54
Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mw5h, @nvanbenschoten, and @rafiss)


pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_prepare_txns line 48 at r1 (raw file):

----
ok

Do you think it's worth adding a test where we revoke the capability at this point (one the transaction has been prepared)? Just to make sure we're still able to finalize the prepared transaction by either committing it or rolling it back, and don't leave it hanging.

Informs cockroachdb#22329.

This commit adds a new `can_prepare_txns` tenant capability, so that we
don't allow secondary tenants to prepare transactions by default. Allowing
an untrusted tenant to prepare transactions would allow it to block the
progress of system-wide backups, so it is too dangerous to allow by default.

Release note: None
Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @mw5h, and @rafiss)


pkg/ccl/multitenantccl/tenantcapabilitiesccl/testdata/can_prepare_txns line 48 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Do you think it's worth adding a test where we revoke the capability at this point (one the transaction has been prepared)? Just to make sure we're still able to finalize the prepared transaction by either committing it or rolling it back, and don't leave it hanging.

Good idea! Done.

@craig craig bot merged commit 88b6c94 into cockroachdb:master Dec 18, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants