Skip to content

multitenant: misc fixes related to tenant capabilities#101907

Merged
craig[bot] merged 8 commits intocockroachdb:masterfrom
knz:20230420-cap-cleanup
Apr 20, 2023
Merged

multitenant: misc fixes related to tenant capabilities#101907
craig[bot] merged 8 commits intocockroachdb:masterfrom
knz:20230420-cap-cleanup

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Apr 20, 2023

See individual commits for details.

The last commit in particular probably addresses #99087.

Epic: CRDB-23559

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@knz knz force-pushed the 20230420-cap-cleanup branch from d179a70 to 19a9cf1 Compare April 20, 2023 13:48
@knz knz requested review from arulajmani and stevendanna April 20, 2023 15:55
@knz knz marked this pull request as ready for review April 20, 2023 15:57
@knz knz requested a review from a team as a code owner April 20, 2023 15:57
@knz knz requested a review from a team April 20, 2023 15:57
@knz knz requested review from a team as code owners April 20, 2023 15:57
@knz knz requested a review from a team April 20, 2023 15:57
@knz knz force-pushed the 20230420-cap-cleanup branch from b344d37 to e60e4fe Compare April 20, 2023 16:06
@knz knz added backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only backport-23.1.0 labels Apr 20, 2023
@knz knz force-pushed the 20230420-cap-cleanup branch from e60e4fe to 21b689c Compare April 20, 2023 17:52
Copy link
Copy Markdown
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 3 of 3 files at r1, 9 of 9 files at r2, 6 of 6 files at r3, 3 of 3 files at r4, 4 of 4 files at r5, 17 of 18 files at r6, 9 of 9 files at r7, 6 of 6 files at r8, 3 of 3 files at r9, 4 of 4 files at r10, 1 of 1 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz and @stevendanna)


pkg/kv/kvserver/tenantrate/factory.go line 29 at r3 (raw file):

type TestingKnobs struct {
	TimeSource timeutil.TimeSource
	Authorizer tenantcapabilities.Authorizer

nit: consider adding a comment here that calls out we don't use this knob if the RPCContext is set. Or, alternatively, we could call this OverrideAuthorizer and use it regardless of whether an RPCContext exists or not. This isn't meaningful, but the latter might be more intuitive.


pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go line 106 at r4 (raw file):

			// All allowable request types must be explicitly opted into the
			// reqMethodToCap map. If a request type is missing from the map
			// (!hasCap), we must be conservative and assume it is

Now that we've added this test, should we pull out the !hasCap from here and make a stronger assertion about it instead? Maybe a Fatal?


pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go line 334 at r12 (raw file):

		return false
	default:
		log.Warningf(ctx, "unknown authorizer mode: %d", mode)

Should this, and in other places, use a Fatal?

knz added 8 commits April 20, 2023 22:33
The NoopAuthorizer is meant to pretend that the tenant is not limited
in any way. Previous to this patch, it was still subject to rate
limiting; this patch fixes it.

Release note: None
Prior to this patch, an implementation of the `Authorizer` interface
was called "Noop". Unfortunately, "no-op" does not tell the reader
clearly what an *authorization policy* does. What does it allow? What
does it reject?

This patch clarifies that situation by renaming it to
"AllowEverythingAuthorizer".

Additionally, the RPC context for tenant servers was configured
with this authorizer implementation, which is conceptually confusing:
it would conceptually allow RPC clients to solicit KV operations from
it. Instead, this patch introduces a new "AllowNothingAuthorizer" for
that case.

(Note: this is purely conceptual since the RPC service for tenant
server does not, in fact, offer the roachpb.Internal service.)

Release note: None
Prior to this patch, `NewStore()` was tolerant to a missing authorizer
and would synthetize a "allow everything" authorizer in that case.

This is dangerous, because it means any programming error in the
caller where someone forgets to configure the authorizer, would result
in no rate limiting, without any indication something is amiss.

This commit rectifies the situation by requiring an authorizer to be
configured in all cases.

Release note: None
We want to ensure all KV method types have an entry in the map. This
commit adds a test for it, and adds the missing entry for `WriteBatch`
that was detected by the test.

Release note: None
Prior to this patch, there was a "authorizer enabled" cluster setting
with just two values on/off.

When off, it would let through all requests.

This was inadequate because for CC Serverless, we want an escape hatch
that disables capability checks, but still disallows sensitive
requests -- i.e. we want a way to restore the pre-v23.1 behavior.

This change achieves this by replacing the previous setting with a new
setting `server.secondary_tenants.authorizer.mode` with 3 values:

- `on`: use capability checks.
- `allow-all`: allow all operations. This can be used e.g. as
  a escape hatch when lifting a single-tenant application into
  multi-tenancy.
- `v222`: restore the pre-v23.1 behavior. In this mode, non-privileged
  operations are allowed for all tenants, and privileged operations are
  only allowed for the system tenant.

Release note: None
If the cluster version hasn't been upgraded to when capabilities were
introduced, it's better not to start looking at capabilities just yet;
because otherwise error messages would be confusing.

Release note: None
Prior to this patch, we were repeating the following cascade of checks
in each authorizer method:

1. check the cluster setting
2. check whether the reader is bound already
3. check whether the reader has data for the tenant already

Repeating all these checks every time is error prone. Instead, this
change groups them under `getMode`.

Release note: None
Copy link
Copy Markdown
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! 1 of 0 LGTMs obtained (waiting on @arulajmani and @stevendanna)


pkg/kv/kvserver/tenantrate/factory.go line 29 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: consider adding a comment here that calls out we don't use this knob if the RPCContext is set. Or, alternatively, we could call this OverrideAuthorizer and use it regardless of whether an RPCContext exists or not. This isn't meaningful, but the latter might be more intuitive.

Good idea. Done.


pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go line 106 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Now that we've added this test, should we pull out the !hasCap from here and make a stronger assertion about it instead? Maybe a Fatal?

nah; I can see someone modifying the map, forgetting to run the test, then running their server with the binary. This is a good defense.


pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go line 334 at r12 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should this, and in other places, use a Fatal?

log.Fatal is almost always code smell and should be generally avoided (and so should be panic).

But this is probably a good use case for ReportOrPanic. Added a commit to do that.

@knz knz force-pushed the 20230420-cap-cleanup branch from 21b689c to aa0e26f Compare April 20, 2023 20:45
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Apr 20, 2023

bors r=arulajmani

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Apr 20, 2023

Build succeeded:

@craig craig bot merged commit 17331ef into cockroachdb:master Apr 20, 2023
@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Apr 20, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from c943d29 to blathers/backport-release-23.1-101907: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


error creating merge commit from c943d29 to blathers/backport-release-23.1.0-101907: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.0 failed. See errors above.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants