multitenant: misc fixes related to tenant capabilities#101907
multitenant: misc fixes related to tenant capabilities#101907craig[bot] merged 8 commits intocockroachdb:masterfrom
Conversation
d179a70 to
19a9cf1
Compare
b344d37 to
e60e4fe
Compare
e60e4fe to
21b689c
Compare
arulajmani
left a comment
There was a problem hiding this comment.
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: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?
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
Release note: None
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
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
OverrideAuthorizerand 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
!hasCapfrom 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.
21b689c to
aa0e26f
Compare
|
bors r=arulajmani |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
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. |
See individual commits for details.
The last commit in particular probably addresses #99087.
Epic: CRDB-23559