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

cli: IPv6-related fixes #38924

Merged
merged 2 commits into from Jul 17, 2019
Merged

cli: IPv6-related fixes #38924

merged 2 commits into from Jul 17, 2019

Conversation

knz
Copy link
Contributor

@knz knz commented Jul 17, 2019

Fixes #38882.

@knz knz requested review from bdarnell and tbg July 17, 2019 08:21
@knz knz requested a review from a team as a code owner July 17, 2019 08:21
@knz knz requested a review from a team July 17, 2019 08:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20190717-ipv6 branch 2 times, most recently from b14b543 to 5c9ac90 Compare July 17, 2019 09:34
Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 11 of 11 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @knz)


pkg/base/store_spec.go, line 413 at r1 (raw file):

			continue
		}
		addr, port, err := netutil.SplitHostPort(v, "")

Add a comment, it seems confusing that you'd split just to immediately join again.


pkg/util/netutil/addr.go, line 23 at r1 (raw file):

address that is provided

Also is this comment right? It looks like you require the [] now.

knz added 2 commits July 17, 2019 14:14
Prior to this patch, there was a complex dance of supported formats
between the CLI-level handling of "flag
variables" (`base.JoinListType`) an the config-level handling of "join
addresses" (`server.parseGossipBootstrapResolvers`). Due to
imperfections throughout this complexity, the logic was unable to
handle IPv6 addresses properly.

This patch fixes this as follows:

- it factors the handling of the host/port split between the
  `--host`/`--listen-addr` and `--join` flags.  This introduces a new
  `netutil.SplitHostPort` which is now used in lieu of
  `net.SplitHostPort` for command-line handling.

- it checks the format of addresses (and normalizes them) upfront
  during command-line parsing of `--join`, instead of waiting until
  the list of boostrap resolvers is computed.

- it simplifies the computation of bootstrap resolvers accordingly,
  changing the API to specify that `server.Config.JoinList` must now
  contain just one address per value.

- it adds the missing unit tests at the various levels.

Release note (cli change): CockroachDB now requires square brackets to
specify IPv6 addresses in `--join`, `--host`, `--listen-addr` and
similar flags, for example `--listen-addr=[::1]`. The syntax without
square brackets had been deprecated since CockroachDB 2.1.

Release note (bug fix): CockroachDB now recognizes IPv6 addresses
properly in the `--join` flag.
Before:

```
CockroachDB node starting...
client flags:        /usr/bin/cockroach <client cmd> --host=2607:a600:124:755c::16:26257
```

After:

```
CockroachDB node starting...
client flags:        /usr/bin/cockroach <client cmd> --host=[2607:a600:124:755c::16]:26257
```

Release note: None
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.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @tbg)


pkg/base/store_spec.go, line 413 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Add a comment, it seems confusing that you'd split just to immediately join again.

Done.


pkg/util/netutil/addr.go, line 23 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

address that is provided

Also is this comment right? It looks like you require the [] now.

Done.

@knz
Copy link
Contributor Author

knz commented Jul 17, 2019

TFYR!

bors r+

craig bot pushed a commit that referenced this pull request Jul 17, 2019
38924: cli: IPv6-related fixes r=knz a=knz

Fixes #38882.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@knz knz added this to To do in DB Server & Security via automation Jul 17, 2019
@knz
Copy link
Contributor Author

knz commented Jul 17, 2019

@bdarnell should I backport?

@knz
Copy link
Contributor Author

knz commented Jul 17, 2019

nvm it's a feature change so no backport

@craig
Copy link
Contributor

craig bot commented Jul 17, 2019

Build succeeded

@craig craig bot merged commit 3e809df into cockroachdb:master Jul 17, 2019
DB Server & Security automation moved this from To do to Done Jul 17, 2019
@knz knz moved this from Archive to Done 19.2 in DB Server & Security Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cockroach start with --join that only has IPv6 addrs using [ ... ] notation fails
3 participants