cli: enhance the `cockroach connect` command by knz · Pull Request #60854 · cockroachdb/cockroach

knz · 2021-02-21T18:09:30Z

Puts pieces together for #60632.

See individual commits for details.

Release justification: low risk, high benefit changes to existing functionality

cockroach-teamcity · 2021-02-21T18:09:43Z

This change is

knz · 2021-02-21T18:10:38Z

@itsbilal this PR is not necessarily meant for merging separately from yours. Feel free to absorb it into #60766 if it makes your life easier. But I need to start from there to connect the start command to the logic here.

itsbilal

thanks!

Reviewed 5 of 5 files at r1, 5 of 5 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aaron-crl)

aaron-crl

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

pkg/base/config.go, line 172 at r2 (raw file):

	// InitToken is a shared initialization token for generating TLS certificates
	// across multiple nodes.
	InitToken string

Looks like this might clobber the InitToken in the Config struct. Is that ok?

itsbilal · 2021-02-26T00:00:14Z

pkg/base/config.go, line 172 at r2 (raw file):

Previously, aaron-crl wrote…

Looks like this might clobber the InitToken in the Config struct. Is that ok?

That got moved to startCtx as it is probably a more appropriate place for it anyway.

aaron-crl

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @itsbilal)

pkg/base/config.go, line 172 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

That got moved to startCtx as it is probably a more appropriate place for it anyway.

Thanks! I missed that.

aaron-crl

Thank you!

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @itsbilal)

…ands This PR ensures that the `connect` command recognizes the same networking configuration flags as `start`. It also makes the `start` command recognize the `--init-token` flag (although it currently does nothing yet). Release justification: bug fixes and low-risk updates to new functionality Release note: None

Release justification: bug fixes and low-risk updates to new functionality Release note: None

Prior to this commit, the cert generation code improperly passed the value of the `--listen-addr` directly as SAN into the generated certificates, and failed to populate the CN. This caused multiple problems: - the node certs were outright invalid: they require CN=node. Also the UI cert require CN to be set to the advertised hostname. Fixing this required extending the interfaces with a commonName argument. - all the certs had broken SANs: the addresses in the command line are hostname + portnumber, but SANs must contain only hostnames. Fixing this required splitting host/port again inside the init function. - the SAN fields were invalid if the `--listen-addr` was left to its default, which is an empty string. To fix this requires opting into the semi-complicated dance around configured / actual addresses, specifically the sequence of the following 3 calls: - `baseCfg.ValidateAddrs` - populates defaults, resolves numeric port numbers. - `server.ListenAndUpdateAddrs` - performs the `net.Listen` calls and then back-propagates the actual listen address into the Addr fields, then populates the `Advertise` addresses from that if no advertise address was initially populated. - use the resulting advertised addr to configure the SAN field for each service. Release justification: bug fixes and low-risk updates to new functionality Release note: None

Release justification: low risk, high benefit changes to existing functionality Release note (cli change): The new `cockroach connect` command now recognizes `--single-node` to prepare a TLS configuration suitable for e.g. a subsequent `start-single-node` command. Additionally, the command checks that either `--single-node` is specified, or both `--init-token` and `--num-expected-peers`.

Release justification: low risk, high benefit changes to existing functionality Release note: None

This makes them look nicer in logging. Release justification: bug fixes and low-risk updates to new functionality Release note: None

This commit adds `context.Context` arguments in the right places and invokes logging to describe progress. Release justification: low risk, high benefit changes to existing functionality Release note: None

Release justification: bug fixes and low-risk updates to new functionality Release note: None

Release justification: non-production code changes Release note: None

Release justification: bug fixes and low-risk updates to new functionality Release note: None

…mmand Release justification: non-production code changes Release note: None

knz · 2021-03-01T18:51:51Z

TFYR!

I even added a (preliminary) unit test using TCL that exercises the new connect command.

bors r=aaron-crl,itsbilal

craig · 2021-03-01T20:38:26Z

Build succeeded:

GitHub CI (Cockroach)

knz requested a review from itsbilal February 21, 2021 18:09

knz requested a review from a team as a code owner February 21, 2021 18:09

knz requested a review from aaron-crl February 21, 2021 18:09

knz mentioned this pull request Feb 21, 2021

server: Implement auto-init handshake to negotiate CA certs between nodes #60766

Merged

2 tasks

knz force-pushed the 20210221-secure-init branch 2 times, most recently from 5e3dc4e to 652c6de Compare February 25, 2021 16:42

itsbilal approved these changes Feb 25, 2021

View reviewed changes

aaron-crl reviewed Feb 25, 2021

View reviewed changes

aaron-crl approved these changes Feb 26, 2021

View reviewed changes

knz force-pushed the 20210221-secure-init branch 2 times, most recently from f451304 to ffce3a9 Compare February 28, 2021 15:05

knz mentioned this pull request Feb 28, 2021

cli: cockroach cert create-client fails after cockroach connect #61230

Open

knz changed the title ~~cli: rework the command-line flags for the 'connect' and 'start' commands~~ cli: enhance the cockroach connect command Feb 28, 2021

aaron-crl approved these changes Feb 28, 2021

View reviewed changes

knz force-pushed the 20210221-secure-init branch 4 times, most recently from 67e94ac to c6c8909 Compare March 1, 2021 09:30

aaron-crl mentioned this pull request Mar 1, 2021

server: integrate the TLS auto-negotiation inside the crdb code #60632

Closed

25 tasks

knz added 8 commits March 1, 2021 18:20

cli,server: update the InitHandshake interface

ca1484b

Release justification: bug fixes and low-risk updates to new functionality Release note: None

server,security: ensure the certs dir exists before creating files

d383ea4

Release justification: bug fixes and low-risk updates to new functionality Release note: None

cli: add some logging to the connect command

3dc9337

Release justification: low risk, high benefit changes to existing functionality Release note: None

cli,security,server: avoid doing work if node certs already exists

cae5cfb

Release justification: low risk, high benefit changes to existing functionality Release note: None

security: properly populate the issuer field in auto-gen certs

ab79ee7

Release justification: low risk, high benefit changes to existing functionality Release note: None

knz added 6 commits March 1, 2021 18:20

server: rename the service labels during cert auto-gen

2c9b43b

This makes them look nicer in logging. Release justification: bug fixes and low-risk updates to new functionality Release note: None

cli,server,security: add logging to the cert auto-generation

c9a423c

This commit adds `context.Context` arguments in the right places and invokes logging to describe progress. Release justification: low risk, high benefit changes to existing functionality Release note: None

security,server: ensure that node-node certs are also valid as client

e1b2f22

Release justification: bug fixes and low-risk updates to new functionality Release note: None

server: avoid long tests under the short flag

06667d6

Release justification: non-production code changes Release note: None

cli/connect: flush logs before exit

9d2641e

Release justification: bug fixes and low-risk updates to new functionality Release note: None

cli/interactive_tests: new preliminary testing for the new connect co…

ac625de

…mmand Release justification: non-production code changes Release note: None

knz force-pushed the 20210221-secure-init branch from c6c8909 to ac625de Compare March 1, 2021 18:11

craig bot merged commit 09514e9 into cockroachdb:master Mar 1, 2021

knz deleted the 20210221-secure-init branch March 1, 2021 20:38

jseldess mentioned this pull request Mar 23, 2021

cli: enhance the cockroach connect command cockroachdb/docs#10148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: enhance the `cockroach connect` command#60854

cli: enhance the `cockroach connect` command#60854
craig[bot] merged 14 commits intocockroachdb:masterfrom
knz:20210221-secure-init

knz commented Feb 21, 2021 •

edited

Loading

Uh oh!

cockroach-teamcity commented Feb 21, 2021

Uh oh!

knz commented Feb 21, 2021

Uh oh!

itsbilal left a comment

Uh oh!

aaron-crl left a comment

Uh oh!

itsbilal commented Feb 26, 2021

Uh oh!

aaron-crl left a comment

Uh oh!

aaron-crl left a comment

Uh oh!

knz commented Mar 1, 2021

Uh oh!

craig bot commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

knz commented Feb 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Feb 21, 2021

Uh oh!

knz commented Feb 21, 2021

Uh oh!

itsbilal left a comment

Choose a reason for hiding this comment

Uh oh!

aaron-crl left a comment

Choose a reason for hiding this comment

Uh oh!

itsbilal commented Feb 26, 2021

Uh oh!

aaron-crl left a comment

Choose a reason for hiding this comment

Uh oh!

aaron-crl left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Mar 1, 2021

Uh oh!

craig bot commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

knz commented Feb 21, 2021 •

edited

Loading