sql: Add system table join_tokens, and create_join_tokens() builtin function #62053
Conversation
|
This change is |
|
There are many SQL bits here so it'd rather have this get some more SQL eyes. That said, the first and third commits might have an impact on some of your work @aaron-crl , as it moves around some |
itsbilal
force-pushed
the
create-jointoken-sql-move-system-table
branch
3 times, most recently
from
699e7c4 to
49609c2
Compare
There was a problem hiding this comment.
1st commit LGTM
2nd commit not quite so. I think this work does not justify a fully fledged new SQL statement. (Moreover, CREATE is someone earmarked for DDL / SQL metaschema changes.) I would recommend a SQL built-in function instead. We already have crdb_internal.create_tenant and I believe we can follow this model here.
3rd commit LGTM
4th commit code OK, but when you connect the create function to the cert manager, don't do this in the sql/sem/builtins package. Instead add a function in the sql package and make it an interface. Again, you can follow the example of create_tenant.
5th commit LGTM
Reviewed 3 of 3 files at r1, 11 of 11 files at r2, 4 of 4 files at r3, 9 of 9 files at r4, 5 of 5 files at r5.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl and @dt)
itsbilal
force-pushed
the
create-jointoken-sql-move-system-table
branch
from
49609c2 to
790d72e
Compare
itsbilal
changed the title
itsbilal
force-pushed
the
create-jointoken-sql-move-system-table
branch
from
790d72e to
56df4ad
Compare
|
TFTR! I've moved over to a sql builtin, and updated the PR accordingly. Also folded the other PR into this one. |
There was a problem hiding this comment.
Reviewed 26 of 26 files at r6, 4 of 4 files at r7, 4 of 4 files at r8, 13 of 13 files at r9, 5 of 5 files at r10.
Reviewable status:
pkg/sql/join_token_test.go, line 87 at r7 (raw file):
defer s.Stopper().Stop(context.Background()) rows, err := sqldb.Query("CREATE JOINTOKEN;")
2nd commit: CREATE JOINTOKEN doesn't seem relevant to the PR any more. Maybe you can copy the query from the 4th commit into it.
Previously, the generation of join tokens bypassed the certificate manager when loading CA certificates. This change updates that code path and associated tests to use the certificate manager, to make testing easier. Release note: None.
Adds a create_join_token builtin function for use in TLS auto-joins. This function, when run on a self-hosted single-tenant Cockroach node, creates and returns a new join token. This join token can then be copy-pasted to new nodes and used to give them the set of certificates for secure auto TLS initialization. See RFC cockroachdb#51991. Part of cockroachdb#60632. Release note: None.
As we're going with a system table based appraoch for saving join tokens, it makes more sense to move the underlying struct to the security package so that it can be used in the sql subsystem. The reason why it was in server was so we could access Gossip in the same code as the JoinToken struct itself. Also remove the status server method added to generate/gossip join token. Release note: None.
itsbilal
force-pushed
the
create-jointoken-sql-move-system-table
branch
from
56df4ad to
f7b3b84
Compare
There was a problem hiding this comment.
TFTR! Updated the failing tests, and fixed lint issues too.
Reviewable status:
pkg/sql/join_token_test.go, line 87 at r7 (raw file):
Previously, knz (kena) wrote…
2nd commit: CREATE JOINTOKEN doesn't seem relevant to the PR any more. Maybe you can copy the query from the 4th commit into it.
Done. Moved this entire file to the 4th commit to reduce confusion, instead of having a half-working test.
This change adds a new system table, `join_tokens`, for the exclusive use of storing join tokens. This is necessary as we need guaranteed at-most-once semantics with these, which transactions give us pretty easily. This change also updates the `create_join_token()` function to create and store a join token in that table. Currently, there's no mechanism to remove expired join tokens. Release note (general change): Add `crdb_internal.create_join_token()` sql builtin function to create join tokens for use when joining new nodes to a secure cluster. This functionality is hidden behind a feature flag.
This change adds a SQL migration (gated on cluster versions) for the join_tokens table added in a previous commit. Release note: None.
itsbilal
force-pushed
the
create-jointoken-sql-move-system-table
branch
from
f7b3b84 to
068ad3d
Compare
A lot of tests in the cli and sql packages assert on system table IDs, range numbers, etc., that were failing due to the addition of the `join_tokens` table. This change updates all those tests to account for that table. Release note: None.
itsbilal
force-pushed
the
create-jointoken-sql-move-system-table
branch
from
068ad3d to
3a44b45
Compare
|
bors r=knz |
|
Build succeeded: |
| @@ -1756,6 +1764,49 @@ var ( | |||
| FormatVersion: descpb.InterleavedFormatVersion, | |||
| NextMutationID: 1, | |||
| }) | |||
|
|
|||
| // MigrationsTable is the descriptor for the migrations table. It stores facts | |||
There was a problem hiding this comment.
PS I think this comment could do with fixing, it's referring to the table before.
This change adds a new system table,
join_tokens, for theexclusive use of storing join tokens. This is necessary as
we need guaranteed at-most-once semantics with these, which
transactions give us pretty easily. A related migration is also added
to create said table
This change also adds a new builtin function,
crdb_internal.create_join_token()that creates and persists a join token in that table.
Currently, there's no mechanism to remove expired join tokens.
See RFC #51991. Part of #60632.
Release note (general change): Add
crdb_internal.create_join_token()sql builtin function to create join tokens for use when joining
new nodes to a secure cluster. This functionality is hidden behind
a feature flag.