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

roachpb,sql: limit the syntax of tenant names #93269

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Dec 8, 2022

Fixes #92613.

This commit limits the lexicographical structure of tenant names to be that of DNS hostnames: start with a letter, followed by a combination of letters, digits or hyphens. In particular periods and underscores are not allowed.

Release note: None

@knz knz requested review from stevendanna and ecwall December 8, 2022 18:20
@knz knz requested a review from a team as a code owner December 8, 2022 18:20
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz added the T-multitenant Issues owned by the multi-tenant virtual team label Dec 8, 2022
@knz
Copy link
Contributor Author

knz commented Dec 8, 2022

cc @adityamaru

@knz knz force-pushed the 20221208-tenant-names branch from 8c031f9 to da2765f Compare December 8, 2022 19:21
@knz knz requested a review from adityamaru December 8, 2022 19:22
@knz knz added A-multitenancy Related to multi-tenancy and removed T-multitenant Issues owned by the multi-tenant virtual team labels Dec 9, 2022
@knz knz force-pushed the 20221208-tenant-names branch from da2765f to 7d66012 Compare December 12, 2022 18:25
Copy link
Contributor

@ecwall ecwall 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! 0 of 0 LGTMs obtained (waiting on @adityamaru, @knz, and @stevendanna)


pkg/roachpb/tenant.go line 153 at r1 (raw file):

// to a database name in connection strings. However there cannot
// be a hyphen at the end.
var tenantNameRe = regexp.MustCompile(`^[a-z0-9]([a-z0-9---]{0,98}[a-z0-9])?$`)

What does the --- do in [a-z0-9---]? Is it different from just [a-z0-9-]?

@knz
Copy link
Contributor Author

knz commented Dec 12, 2022

What does the --- do in [a-z0-9---]? Is it different from just [a-z0-9-]?

A standalone - in a regexp [...] pattern is dangerous: if someone ever decides to add just 1 letter to the pattern, for example [a-z0-9X-] suddenly the pattern changes meaning.

By writing --- it becomes impervious to adding characters on the left in the future.

Copy link
Contributor

@ecwall ecwall left a comment

Choose a reason for hiding this comment

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

Looks good, just a request for some more test cases.
:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @knz, and @stevendanna)


pkg/sql/logictest/testdata/logic_test/tenant line 16 at r1 (raw file):

CREATE TENANT "two"

statement error invalid tenant name

Can you add test cases for length boundaries
0 -> invalid
1 -> valid
100 -> valid
101 -> valid

Copy link
Contributor

@ecwall ecwall 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 @adityamaru, @knz, and @stevendanna)


pkg/sql/logictest/testdata/logic_test/tenant line 16 at r1 (raw file):

Previously, ecwall (Evan Wall) wrote…

Can you add test cases for length boundaries
0 -> invalid
1 -> valid
100 -> valid
101 -> valid

  • 101 -> invalid

This commit limits the lexicographical structure of tenant names to be
that of DNS hostnames: start with a letter or digit, followed by a
combination of letters, digits or hyphens. In particular periods and
underscores are not allowed.

There is a minimum length of 1 character and a maximum of 100, like in
CC serverless.

Release note: None
@knz knz force-pushed the 20221208-tenant-names branch from 7d66012 to d6c3d5d Compare December 13, 2022 20:52
Copy link
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.

TFYR!

bors r=ecwall

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ecwall, and @stevendanna)


pkg/sql/logictest/testdata/logic_test/tenant line 16 at r1 (raw file):

Previously, ecwall (Evan Wall) wrote…
  • 101 -> invalid

Done.

@craig
Copy link
Contributor

craig bot commented Dec 14, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Dec 14, 2022

Build succeeded:

@craig craig bot merged commit 56c73a4 into cockroachdb:master Dec 14, 2022
@knz knz deleted the 20221208-tenant-names branch December 14, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql,server: add a validation to tenant names to mandate a specific lexical structure
3 participants