Skip to content

*: fix bugs related to invalid use of PrefixEnd to compute tenant end key#110241

Merged
craig[bot] merged 6 commits intocockroachdb:masterfrom
knz:20230907-tenant-prefix
Sep 8, 2023
Merged

*: fix bugs related to invalid use of PrefixEnd to compute tenant end key#110241
craig[bot] merged 6 commits intocockroachdb:masterfrom
knz:20230907-tenant-prefix

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Sep 8, 2023

Ever since #104974 we have decided that the exclusive end key of a virtual keyspace is at "current tenant ID + 1", not "start key / PrefixEnd".

This PR completes the investigation requested in #104928 and fixes some bugs in the process.

See individual commits for details.

Fixes #104928.
Fixes #110051.
Epic: CRDB-26091

knz added 6 commits September 8, 2023 09:34
Ever since cockroachdb#104974 we have decided that the exclusive end key of a
virtual keyspace is at "current tenant ID + 1".

For this to be possible without special cases, it's important that
this key is always computable. For this, we must ensure that no tenant
can ever use an ID equal to MaxUint64.

This patch achieves it.

Release note: None
Prior to this patch, the RPC authz code was restricting
requests from secondary tenants to the range

   /Tenant/ID ... /Tenant/ID/PrefixEnd

However, meanwhile, ever since cockroachdb#104974 was merged,
the tenant keyspace is actually defined as the range

   /Tenant/ID ... /Tenant/<ID+1>

Under the hood, ".PrefixEnd()" computes the "prefix end" key by
incrementing the last byte. Therefore, for most integer values of ID,
/Tenant/ID/PrefixEnd and /Tenant/<ID+1> are equivalent. So the
mismatch above goes unnoticed until ... we run into an ID value for
which they are not equivalent.

An example such value is ID 511. This ID encodes to a sequence of
bytes such that the .PrefixEnd() key is larger than the /Tenant/511
prefix, but strictly smaller than /Tenant/512, which is the actual
exclusive end key for that virtual cluster.

Because of this mismatch, the RPC code would reject any request
from tenant 511 where the end key was /Tenant/512, even though it
should have accepted them.

This patch fixes it.

Release note: None
Prior to this patch, the streaming code was using `.PrefixEnd()` to
compute the exclusive end key of a tenant's keyspace.

As established earlier, this is incorrect, the actual end key is
`/Tenant/<ID+1>`. When the tenant ID is 109, 511 or other values for
which the value encoding needs escape bytes, `.PrefixEnd()` compares
smaller than the keyspace end key.

(We are not currently putting any data in the range
/Tenant/ID/PrefixEnd ... /Tenant/<ID+1> so this did not actually
result in a replication bug. There is no release note needed.)

Release note: None
Prior to this patch, TenantRanges was incorrectly scanning up to
tenantPrefix.PrefixEnd() to collect range descriptors.
Since the actual end key may be different, this resulted
in the endpoint potentially missing out on the last range
of a tenant keyspace, between /Tenant/ID/PrefixEnd and
/Tenant/<ID+1>.

Thankfully, as of this writing no data is ever written in this part of
the keyspace so this bug was not visible in practice.

Release note: None
Prior to this patch, the GC logic was incorrectly scanning up to
tenantPrefix.PrefixEnd() to delete tenant data.

Since the actual end key may be different, this resulted in GC
potentially failing to delete the last portion of a tenant keyspace,
between /Tenant/ID/PrefixEnd and /Tenant/<ID+1>.

Thankfully, as of this writing no data is ever written in this part of
the keyspace so this bug was not visible in practice.

Release note: None
These are the last few mistaken uses of .PrefixEnd().

Release note: None
@knz knz requested review from a team as code owners September 8, 2023 08:35
@knz knz requested a review from a team September 8, 2023 08:36
@knz knz requested a review from a team as a code owner September 8, 2023 08:36
@knz knz requested a review from a team September 8, 2023 08:36
@knz knz requested a review from a team as a code owner September 8, 2023 08:36
@knz knz requested review from a team and Santamaura and removed request for a team September 8, 2023 08:36
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@knz knz added the backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only label Sep 8, 2023
Copy link
Copy Markdown
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.

Thanks for the very clear commit messages and for cleaning this up.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Sep 8, 2023

TFYR!

bors r=stevendanna

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Sep 8, 2023

Build succeeded:

@craig craig bot merged commit b7840ea into cockroachdb:master Sep 8, 2023
@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Sep 8, 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 62399d7 to blathers/backport-release-23.1-110241: 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.


🦉 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.

SHOW RANGES FROM DATABASE fails with insufficient bytes to decode uvarint sql: audit usages of tenantPrefix.PrefixEnd() and codec.TenantSpan()

3 participants