capabilities: update the definition of the protobuf so that the zero protobuf struct results in pre-23.1 serverless behavior #96736
Assignees
Labels
branch-release-23.1
Used to mark GA and release blockers, technical advisories, and bugs for 23.1
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
release-blocker
Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
T-multitenant
Issues owned by the multi-tenant virtual team
Comments
arulajmani
added
the
C-bug
arulajmani
changed the title
arulajmani
added
release-blocker
|
What do you think should happen if we make a backup of a tenant record in version X, then restore it in version Y after the migration has run already? The idea to use zero as the grant value would solve that, but maybe the DR team has better ideas. |
|
We've discussed this extensively with the team. At this stage the consensus is to ensure that the default value of the info protobuf results in pre-v23.1 capability behavior for Serverless tenants without the need for a new migration. I will change the title of this issue accordingly. It is important to do this before the capability system can be enabled in Serverless. |
knz
changed the title
|
We further discussed this with @jeffswenson . We only need to change the default behavior for the specific capabilities that were granted in serverless previously, namely that needed for AdminSplit and AdminScatter. For all the other capabilities the Go default value should remain "capability not granted". |
arulajmani
added a commit
to arulajmani/cockroach
that referenced
this issue
Mar 13, 2023
AdminSplit and AdminScatter requests are subject to capability checks. Previously, these capabilities were codified in the "enabled" form. As such, by default, secondary tenants did not have the ability to perform these operations. This is in violation of what secondary tenants could do prior to 23.1, at a time before capabilities existed. Moreover, RESTORE/IMPORT rely on performing these operations for performance. This made disallowing these operations by default a performance regression. This patch flips the phrasing of how these capabilities are stored on the proto to use the "disable" verbiage. As such, secondary tenants are able to perform splits and scatters by default. As part of this change, we also clean up a testing knob that was used by various backup, CDC, and logictests to override capability checks in the authorizer. This isn't required with the new default behaviour. We also add some missing E2E tests for the `CanAdminUnsplit` capability which were missing when it was introduced. Fixes cockroachdb#96736 Release note: None
arulajmani
added a commit
to arulajmani/cockroach
that referenced
this issue
Mar 14, 2023
AdminSplit and AdminScatter requests are subject to capability checks. Previously, these capabilities were codified in the "enabled" form. As such, by default, secondary tenants did not have the ability to perform these operations. This is in violation of what secondary tenants could do prior to 23.1, at a time before capabilities existed. Moreover, RESTORE/IMPORT rely on performing these operations for performance. This made disallowing these operations by default a performance regression. This patch flips the phrasing of how these capabilities are stored on the proto to use the "disable" verbiage. As such, secondary tenants are able to perform splits and scatters by default. However, no change is made to the public interface -- users above the `tenantcapabilitiespb` package continue to interact with these capabilities as they were before, oblivious to how these things are stored on disk. There's a few testing changes here: - As part of this change, we also clean up a testing knob that was used by various backup, CDC, and logictests to override capability checks in the authorizer. This isn't required with the new default behaviour. - We also add some missing E2E tests for the `CanAdminUnsplit` capability which were missing when it was introduced. - The construction of `TestMultiTenantAdminFunction` was making it cumborsome to work with now that we have changed the default behavior of splits and scatters. Instead of trying to wrestle with that test, I decided to convert some of the SPLIT/SCATTER tests to the datadriven E2E tests we have for capabilities. At some point, we should get rid of this entire thing, but I'm going to limit scope here for now. Fixes cockroachdb#96736 Release note: None
arulajmani
added a commit
to arulajmani/cockroach
that referenced
this issue
Mar 14, 2023
AdminSplit and AdminScatter requests are subject to capability checks. Previously, these capabilities were codified in the "enabled" form. As such, by default, secondary tenants did not have the ability to perform these operations. This is in violation of what secondary tenants could do prior to 23.1, at a time before capabilities existed. Moreover, RESTORE/IMPORT rely on performing these operations for performance. This made disallowing these operations by default a performance regression. This patch flips the phrasing of how these capabilities are stored on the proto to use the "disable" verbiage. As such, secondary tenants are able to perform splits and scatters by default. However, no change is made to the public interface -- users above the `tenantcapabilitiespb` package continue to interact with these capabilities as they were before, oblivious to how these things are stored on disk. As part of this patch, we also clean up a testing knob that was used by various backup, CDC, and logictests to override capability checks in the Authorizer. We no longer need this with the new defaults. Fixes cockroachdb#96736 Release note: None
craig bot
pushed a commit
that referenced
this issue
Mar 15, 2023
97321: copy: enhance copyfrom tests with kvtrace feature and more tests r=cucaroach a=cucaroach Epic: CRDB-18892 Informs: #91831 Release note: None 98264: colfetcher: track KV CPU time in the direct columnar scan r=yuzefovich a=yuzefovich This commit addresses a minor TODO to track the KV CPU time when direct columnar scans are used. In the regular columnar scan this time is tracked by the cFetcher, but with the KV projection pushdown the cFetcher is used on the KV server side, so we need to augment the ColBatchDirectScan to track it. Notably, this means that the decoding done on the KV server side is included. Additionally, this commit clarifies how the KV CPU time is obtained from the cFetcher (we don't need to use a helper (unlike in the case of `bytesRead` and `batchRequestsIssued` fields which are written to on `cFetcher.Close`), and we don't need the mutex protection there either). Epic: None Release note: None 98546: multitenant: allow secondary tenants to split/scatter by default r=knz a=arulajmani AdminSplit and AdminScatter requests are subject to capability checks. Previously, these capabilities were codified in the "enabled" form. As such, by default, secondary tenants did not have the ability to perform these operations. This is in violation of what secondary tenants could do prior to 23.1, at a time before capabilities existed. Moreover, RESTORE/IMPORT rely on performing these operations for performance. This made disallowing these operations by default a performance regression. This patch flips the phrasing of how these capabilities are stored on the proto to use the "disable" verbiage. As such, secondary tenants are able to perform splits and scatters by default. However, no change is made to the public interface -- users above the `tenantcapabilitiespb` package continue to interact with these capabilities as they were before, oblivious to how these things are stored on disk. There's a few testing changes here: - As part of this change, we also clean up a testing knob that was used by various backup, CDC, and logictests to override capability checks in the authorizer. This isn't required with the new default behaviour. - We also add some missing E2E tests for the `CanAdminUnsplit` capability which were missing when it was introduced. Fixes #96736 Release note: None 98615: sql_test: re-skip TestShowTraceReplica r=msirek a=msirek TestShowTraceReplica wasn't failing under stress, but failed in TeamCity once enabled. This re-skips the test until it can be reliably reproduced and debugged. Informs #98598 Release note: None Co-authored-by: Tommy Reilly <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Arul Ajmani <[email protected]> Co-authored-by: Mark Sirek <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the problem
Once #96390 lands, the tenant capabilities subsystem will be hooked up. As a result, all secondary tenant requests will be subject to capability checks. As of the time of writing, the only
AdminRequestthat has capabilities associated with it isAdminSplit. However, this will soon expand to other operations such as scatters, relocate range, relocate lease etc.Internally, operations such as RESTORE/IMPORT etc. use these Admin requests. We will need to do something to prevent these operations from failing for existing (Serverless) tenants after upgrading to 23.1.
(An option that was discussed internally, is to add a migration that grants secondary tenants
can_admin_split(and any other capability they need that is added after writing up this issue) inServerless. )We are considering instead flipping the boolean meaning of the capabilities for pre-23.1 operations so that their default value coincides with what CC Serverless needs.
Jira issue: CRDB-24300
Epic: CRDB-23344
The text was updated successfully, but these errors were encountered: