From 9c9b8785eabca76730ceaab72fb931e939828c5c Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 8 Aug 2018 07:36:03 -0400 Subject: [PATCH] cli: make --host/--{listen,advertise,http}-addr recognize port numbers Prior to this patch, the various `cockroach` sub-commands would take separate flags to specify an address/hostanme and to specify a port number. Meanwhile: 1. `--join` would recognize the syntax `host:port`. 2. the web UI, docs and other places often refer to a "server address" as the pair hostname:portnr. For user convenience, it is thus important to make the interface more straightforward/regular. This patch achieves this as follows: - the flags `--listen-addr`/`--advertise-addr`/`--http-addr` (server-side) and `--host` (client-side) now recognize the syntax `host/addr:port`. - the server-side `--port` flags are still recognized for backward compatibility but are marked as deprecated. The client-side `--port` is still recognized and not deprecated for now, but hidden from the contextual help. As a side-effect of recognizing the port number inside the same flag, the syntax with square brackets for IPv6 addresses now becomes necessary when specifying also a port number. The syntax without square brackets (and without port number) is temporarily still recognized for backward compatibility, but is also marked as deprecated. Finally the shorthand flag `-s` is now recognized as an alias for `--host` client-side. Release note (cli change): The flag `-s` can now be used as alias to `--host` for `cockroach` client sub-commands. Release note (cli change): the server-side command line flag `--listen-addr`, which replaces the previous `--host` flag, is now equipped to recognize both a hostname/address and port number. The `--port` flag is deprecated as a result. Release note (cli change): the server-side command line flag `--http-addr`, which replaces the previous `--http-host` flag, is now equipped to recognize both a hostname/address and port number. The `--http-port` flag is deprecated as a result. Release note (cli change): the server-side command line flag `--advertise-addr`, which replaces the previous `--advertise-host` flag, is now equipped to recognize both a hostname/address and port number. The `--advertise-port` flag is deprecated as a result. Release note (cli change): the client-side command line flag `--host` is now equipped to recognize both a hostname/address and port number. The client-side `--port` flag is still recognized, but not documented any more; `--host` is now preferred. Release note (cli change): the environment variable COCKROACH_PORT that specifies the port number to use for client commands is now deprecated. The port number can be placed in the COCKROACH_HOST environment variable instead. Release note (cli change): The syntax to specify IPv6 addresses with the client-side command line flag `--host` is changed to use square brackets, for example `--host=[::1]` instead of just `--host=::1` previously. The previous syntax is still recognized for backward compatibility but is deprecated. Release note (cli change): the flag `--listen-port` which was introduced in a recent change is now removed. (DOCS NOTE: remove both this release note and the previous one that introduced --listen-port) --- pkg/acceptance/cli_test.go | 6 +- pkg/acceptance/cluster/cluster.go | 2 +- pkg/acceptance/cluster/dockercluster.go | 2 +- pkg/acceptance/localcluster/localcluster.go | 2 +- pkg/ccl/cliccl/cli_test.go | 2 +- pkg/cli/cli_test.go | 3 +- pkg/cli/cliflags/flags.go | 65 ++++++++-------- pkg/cli/flags.go | 64 +++++++++++++--- pkg/cli/flags_test.go | 83 ++++++++++++++------- pkg/cli/interactive_tests/test_flags.tcl | 13 +++- pkg/cli/start.go | 11 +-- pkg/cmd/roachtest/debug.go | 2 +- pkg/cmd/roachtest/decommission.go | 4 +- pkg/cmd/roachtest/encryption.go | 2 +- pkg/cmd/roachtest/kv.go | 2 +- pkg/cmd/roachtest/upgrade.go | 4 +- pkg/cmd/roachtest/version.go | 2 +- 17 files changed, 176 insertions(+), 93 deletions(-) diff --git a/pkg/acceptance/cli_test.go b/pkg/acceptance/cli_test.go index dc6fcdce565a..fd151bd1972a 100644 --- a/pkg/acceptance/cli_test.go +++ b/pkg/acceptance/cli_test.go @@ -117,9 +117,9 @@ function finish() { trap finish EXIT HOST=$(hostname -f) -$bin start --logtostderr=INFO --background --insecure --listen-addr="${HOST}" --listen-port=12345 &> out -$bin sql --insecure --host="${HOST}" --port=12345 -e "show databases" -$bin quit --insecure --host="${HOST}" --port=12345 +$bin start --logtostderr=INFO --background --insecure --listen-addr="${HOST}":12345 &> out +$bin sql --insecure --host="${HOST}":12345 -e "show databases" +$bin quit --insecure --host="${HOST}":12345 ` containerConfig.Cmd = []string{"/bin/bash", "-c", script} if err := testDockerOneShot(ctx, t, "start_flags_test", containerConfig); err != nil { diff --git a/pkg/acceptance/cluster/cluster.go b/pkg/acceptance/cluster/cluster.go index 1ee7f8fcf6b5..017600abaa6e 100644 --- a/pkg/acceptance/cluster/cluster.go +++ b/pkg/acceptance/cluster/cluster.go @@ -43,7 +43,7 @@ type Cluster interface { // dismantle the cluster. AssertAndStop(context.Context, testing.TB) // ExecCLI runs `./cockroach `, while filling in required flags such as - // --insecure, --certs-dir, --host or --port. + // --insecure, --certs-dir, --host. // // Returns stdout, stderr, and an error. ExecCLI(ctx context.Context, i int, args []string) (string, string, error) diff --git a/pkg/acceptance/cluster/dockercluster.go b/pkg/acceptance/cluster/dockercluster.go index 545aaed37308..fe72120dc9ae 100644 --- a/pkg/acceptance/cluster/dockercluster.go +++ b/pkg/acceptance/cluster/dockercluster.go @@ -541,7 +541,7 @@ func (l *DockerCluster) startNode(ctx context.Context, node *testNode) { pprof: docker exec -it %[4]s pprof https+insecure://$(hostname):%[5]s/debug/pprof/heap cockroach: %[6]s - cli-env: COCKROACH_INSECURE=false COCKROACH_CERTS_DIR=%[7]s COCKROACH_HOST=%s COCKROACH_PORT=%d`, + cli-env: COCKROACH_INSECURE=false COCKROACH_CERTS_DIR=%[7]s COCKROACH_HOST=%s:%d`, node.Name(), "https://"+httpAddr.String(), localLogDir, node.Container.id[:5], base.DefaultHTTPPort, cmd, certsDir, httpAddr.IP, httpAddr.Port) } diff --git a/pkg/acceptance/localcluster/localcluster.go b/pkg/acceptance/localcluster/localcluster.go index 1ebe678fd7fb..05d78cb8a6b2 100644 --- a/pkg/acceptance/localcluster/localcluster.go +++ b/pkg/acceptance/localcluster/localcluster.go @@ -77,7 +77,7 @@ func (b *LocalCluster) AssertAndStop(ctx context.Context, t testing.TB) { // ExecCLI implements cluster.Cluster. func (b *LocalCluster) ExecCLI(ctx context.Context, i int, cmd []string) (string, string, error) { cmd = append([]string{b.Cfg.Binary}, cmd...) - cmd = append(cmd, "--insecure", "--port", b.Port(ctx, i)) + cmd = append(cmd, "--insecure", "--host", ":"+b.Port(ctx, i)) c := exec.CommandContext(ctx, cmd[0], cmd[1:]...) var o, e bytes.Buffer c.Stdout, c.Stderr = &o, &e diff --git a/pkg/ccl/cliccl/cli_test.go b/pkg/ccl/cliccl/cli_test.go index f42833ebf256..8a7599eaf1a4 100644 --- a/pkg/ccl/cliccl/cli_test.go +++ b/pkg/ccl/cliccl/cli_test.go @@ -42,7 +42,7 @@ func newCLITest(args base.TestServerArgs) cliTest { } return cliTest{ TestServer: s.(*server.TestServer), - connArgs: []string{"--insecure", "--host=" + host, "--port=" + port}, + connArgs: []string{"--insecure", "--host=" + host + ":" + port}, } } diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go index c55de02f753f..e393ca243bfe 100644 --- a/pkg/cli/cli_test.go +++ b/pkg/cli/cli_test.go @@ -290,8 +290,7 @@ func (c cliTest) RunWithArgs(origArgs []string) { args = append(args, "--insecure=false") args = append(args, fmt.Sprintf("--certs-dir=%s", c.certsDir)) } - args = append(args, fmt.Sprintf("--host=%s", h)) - args = append(args, fmt.Sprintf("--port=%s", p)) + args = append(args, fmt.Sprintf("--host=%s:%s", h, p)) } args = append(args, origArgs[1:]...) diff --git a/pkg/cli/cliflags/flags.go b/pkg/cli/cliflags/flags.go index 505a8c84d96b..2f20c48603c2 100644 --- a/pkg/cli/cliflags/flags.go +++ b/pkg/cli/cliflags/flags.go @@ -206,16 +206,23 @@ can also be specified (e.g. .25).`, } ClientHost = FlagInfo{ - Name: "host", - EnvVar: "COCKROACH_HOST", - Description: `Database server host to connect to.`, + Name: "host", + Shorthand: "s", + EnvVar: "COCKROACH_HOST", + Description: ` +CockroachDB node to connect to. +This can be specified either as an address/hostname, or +together with a port number as in -s myhost:26257. +If the port number is left unspecified, it defaults to 26257. +An IPv6 address can also be specified with the notation [...], for +example [::1]:26257 or [fe80::f6f2:::]:26257.`, } ClientPort = FlagInfo{ Name: "port", Shorthand: "p", EnvVar: "COCKROACH_PORT", - Description: `Database server port to connect to.`, + Description: `Deprecated. Use --host=:.`, } Database = FlagInfo{ @@ -267,8 +274,7 @@ sql_safe_updates = FALSE.`, } Set = FlagInfo{ - Name: "set", - Shorthand: "s", + Name: "set", Description: ` Set a client-side configuration parameter before running the SQL shell. This flag may be specified multiple times.`, @@ -327,9 +333,12 @@ or both forms can be used together, for example: ListenAddr = FlagInfo{ Name: "listen-addr", Description: ` -The address or hostname to listen on. An IPv6 address can also be -specified with the notation [...], for example [::1] or -[fe80::f6f2:::]. +The address/hostname and port to listen on, for example +--listen-addr=myhost:26257 or --listen-addr=:26257 (listen on all +interfaces). If the port part is left unspecified, it defaults +to 26257. +An IPv6 address can also be specified with the notation [...], for +example [::1]:26257 or [fe80::f6f2:::]:26257. If --advertise-addr is left unspecified, the node will also announce this address for use by other nodes. It is strongly recommended to use --advertise-addr in cloud and container deployments or any setup where @@ -341,16 +350,6 @@ NAT is present between cluster nodes.`, Description: `Alias for --listen-addr. Deprecated.`, } - ListenPort = FlagInfo{ - Name: "listen-port", - Shorthand: "p", - Description: ` -The port to bind to. -If --advertise-port is left unspecified, the node will also announce -this port for use by other nodes. If a router implements NAT or N:M -port forwarding, be sure to also use --advertise-port.`, - } - ServerPort = FlagInfo{ Name: "port", Description: `Alias for --listen-port. Deprecated.`, @@ -359,10 +358,14 @@ port forwarding, be sure to also use --advertise-port.`, AdvertiseAddr = FlagInfo{ Name: "advertise-addr", Description: ` -The address or hostname to advertise to other CockroachDB nodes for intra-cluster -communication; it must resolve and be routable from other nodes in the cluster. +The address/hostname and port to advertise to other CockroachDB nodes +for intra-cluster communication. It must resolve and be routable from +other nodes in the cluster. +If left unspecified, it defaults to the setting of --listen-addr. An IPv6 address can also be specified with the notation [...], for -example [::1] or [fe80::f6f2:::].`, +example [::1]:26257 or [fe80::f6f2:::]:26257. +The port number should be the same as in --listen-addr unless port +forwarding is set up on an intermediate firewall/router.`, } AdvertiseHost = FlagInfo{ @@ -371,16 +374,18 @@ example [::1] or [fe80::f6f2:::].`, } AdvertisePort = FlagInfo{ - Name: "advertise-port", - Description: ` -The port to advertise to other CockroachDB nodes for intra-cluster -communication. This should not be set differently from --listen-port -unless port forwarding is set up on an intermediate firewall/router.`, + Name: "advertise-port", + Description: `Deprecated. Use --advertise-addr=:.`, } ListenHTTPAddr = FlagInfo{ - Name: "http-addr", - Description: `The hostname or IP address to bind to for HTTP requests.`, + Name: "http-addr", + Description: ` +The hostname or IP address to bind to for HTTP requests. +If left unspecified, the address part defaults to the setting of +--listen-addr. The port number defaults to 8080. +An IPv6 address can also be specified with the notation [...], for +example [::1]:8080 or [fe80::f6f2:::]:8080.`, } ListenHTTPAddrAlias = FlagInfo{ @@ -390,7 +395,7 @@ unless port forwarding is set up on an intermediate firewall/router.`, ListenHTTPPort = FlagInfo{ Name: "http-port", - Description: `The port to bind to for HTTP requests.`, + Description: `Deprecated. Use --http-addr=:.`, } ListeningURLFile = FlagInfo{ diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index 4cc518104e8d..7a220ec80d05 100644 --- a/pkg/cli/flags.go +++ b/pkg/cli/flags.go @@ -16,6 +16,7 @@ package cli import ( "flag" + "fmt" "net" "strings" "time" @@ -151,6 +152,52 @@ func (a aliasStrVar) Set(v string) error { // Type implements the pflag.Value interface. func (a aliasStrVar) Type() string { return "string" } +// addrSetter wraps a address/port configuration option pair and +// enables setting them both with a single command-line flag. +type addrSetter struct { + addr *string + port *string +} + +// String implements the pflag.Value interface. +func (a addrSetter) String() string { + return net.JoinHostPort(*a.addr, *a.port) +} + +// Type implements the pflag.Value interface +func (a addrSetter) Type() string { return "[:]" } + +// Set implement the pflag.Value interface. +func (a addrSetter) Set(v string) error { + addr, port, err := net.SplitHostPort(v) + if err != nil { + if aerr, ok := err.(*net.AddrError); ok { + if strings.HasPrefix(aerr.Err, "too many colons") { + // Maybe this was an IPv6 address using the deprecated syntax + // without '[...]'. Try that. + // Note: the following is valid even if *a.port is empty. + // (An empty port number is always a valid listen address.) + maybeAddr := "[" + v + "]" + *a.port + addr, port, err = net.SplitHostPort(maybeAddr) + if err == nil { + fmt.Fprintf(stderr, + "warning: the syntax \"%s\" for IPv6 addresses is deprecated; use \"[%s]\"\n", v, v) + } + } else if strings.HasPrefix(aerr.Err, "missing port") { + // It's inconvenient that SplitHostPort doesn't know how to ignore + // a missing port number. Oh well. + addr, port, err = net.SplitHostPort(v + ":" + *a.port) + } + } + } + if err != nil { + return err + } + *a.addr = addr + *a.port = port + return nil +} + func init() { initCLIDefaults() @@ -211,25 +258,23 @@ func init() { f := StartCmd.Flags() // Server flags. - StringFlag(f, &startCtx.serverListenAddr, cliflags.ListenAddr, startCtx.serverListenAddr) + VarFlag(f, addrSetter{&startCtx.serverListenAddr, &serverListenPort}, cliflags.ListenAddr) VarFlag(f, aliasStrVar{&startCtx.serverListenAddr}, cliflags.ServerHost) _ = f.MarkDeprecated(cliflags.ServerHost.Name, "use --listen-addr/--advertise-addr instead.") - - StringFlag(f, &serverListenPort, cliflags.ListenPort, serverListenPort) VarFlag(f, aliasStrVar{&serverListenPort}, cliflags.ServerPort) - _ = f.MarkDeprecated(cliflags.ServerPort.Name, "use --listen-port/--advertise-port instead.") + _ = f.MarkDeprecated(cliflags.ServerPort.Name, "use --listen-addr instead.") - StringFlag(f, &serverAdvertiseAddr, cliflags.AdvertiseAddr, serverAdvertiseAddr) + VarFlag(f, addrSetter{&serverAdvertiseAddr, &serverAdvertisePort}, cliflags.AdvertiseAddr) VarFlag(f, aliasStrVar{&serverAdvertiseAddr}, cliflags.AdvertiseHost) _ = f.MarkDeprecated(cliflags.AdvertiseHost.Name, "use --advertise-addr instead.") StringFlag(f, &serverAdvertisePort, cliflags.AdvertisePort, serverAdvertisePort) + _ = f.MarkDeprecated(cliflags.AdvertisePort.Name, "use --advertise-addr=...: instead.") - StringFlag(f, &serverHTTPAddr, cliflags.ListenHTTPAddr, serverHTTPAddr) - VarFlag(f, aliasStrVar{&serverHTTPAddr}, cliflags.ListenHTTPAddrAlias) + VarFlag(f, addrSetter{&serverHTTPAddr, &serverHTTPPort}, cliflags.ListenHTTPAddr) _ = f.MarkDeprecated(cliflags.ListenHTTPAddrAlias.Name, "use --http-addr instead.") - StringFlag(f, &serverHTTPPort, cliflags.ListenHTTPPort, serverHTTPPort) + _ = f.MarkDeprecated(cliflags.ListenHTTPPort.Name, "use --http-addr=...: instead.") StringFlag(f, &serverCfg.Attrs, cliflags.Attrs, serverCfg.Attrs) VarFlag(f, &serverCfg.Locality, cliflags.Locality) @@ -317,8 +362,9 @@ func init() { clientCmds = append(clientCmds, initCmd) for _, cmd := range clientCmds { f := cmd.PersistentFlags() - StringFlag(f, &clientConnHost, cliflags.ClientHost, clientConnHost) + VarFlag(f, addrSetter{&clientConnHost, &clientConnPort}, cliflags.ClientHost) StringFlag(f, &clientConnPort, cliflags.ClientPort, clientConnPort) + _ = f.MarkHidden(cliflags.ClientPort.Name) BoolFlag(f, &baseCfg.Insecure, cliflags.ClientInsecure, baseCfg.Insecure) diff --git a/pkg/cli/flags_test.go b/pkg/cli/flags_test.go index 30520b03e502..2b31677250e6 100644 --- a/pkg/cli/flags_test.go +++ b/pkg/cli/flags_test.go @@ -164,44 +164,60 @@ func TestServerConnSettings(t *testing.T) { {[]string{"start"}, ":" + base.DefaultPort, ":" + base.DefaultPort}, {[]string{"start", "--listen-addr", "127.0.0.1"}, "127.0.0.1:" + base.DefaultPort, "127.0.0.1:" + base.DefaultPort}, {[]string{"start", "--listen-addr", "192.168.0.111"}, "192.168.0.111:" + base.DefaultPort, "192.168.0.111:" + base.DefaultPort}, - {[]string{"start", "--listen-port", "12345"}, ":12345", ":12345"}, - {[]string{"start", "--listen-addr", "127.0.0.1", "--listen-port", "12345"}, "127.0.0.1:12345", "127.0.0.1:12345"}, + {[]string{"start", "--listen-addr", ":12345"}, ":12345", ":12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1:12345"}, "127.0.0.1:12345", "127.0.0.1:12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1:12345", "--port", "55555"}, "127.0.0.1:55555", "127.0.0.1:55555"}, {[]string{"start", "--advertise-addr", "192.168.0.111"}, ":" + base.DefaultPort, "192.168.0.111:" + base.DefaultPort}, - {[]string{"start", "--advertise-addr", "192.168.0.111", "--advertise-port", "12345"}, ":" + base.DefaultPort, "192.168.0.111:12345"}, + {[]string{"start", "--advertise-addr", "192.168.0.111:12345"}, ":" + base.DefaultPort, "192.168.0.111:12345"}, {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111"}, "127.0.0.1:" + base.DefaultPort, "192.168.0.111:" + base.DefaultPort}, - {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111", "--listen-port", "12345"}, "127.0.0.1:12345", "192.168.0.111:12345"}, - {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111", "--advertise-port", "12345"}, "127.0.0.1:" + base.DefaultPort, "192.168.0.111:12345"}, - {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111", "--listen-port", "54321", "--advertise-port", "12345"}, "127.0.0.1:54321", "192.168.0.111:12345"}, - {[]string{"start", "--advertise-addr", "192.168.0.111", "--listen-port", "12345"}, ":12345", "192.168.0.111:12345"}, - {[]string{"start", "--advertise-addr", "192.168.0.111", "--advertise-port", "12345"}, ":" + base.DefaultPort, "192.168.0.111:12345"}, - {[]string{"start", "--advertise-addr", "192.168.0.111", "--listen-port", "54321", "--advertise-port", "12345"}, ":54321", "192.168.0.111:12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1:12345", "--advertise-addr", "192.168.0.111"}, "127.0.0.1:12345", "192.168.0.111:12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111:12345"}, "127.0.0.1:" + base.DefaultPort, "192.168.0.111:12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1:54321", "--advertise-addr", "192.168.0.111:12345"}, "127.0.0.1:54321", "192.168.0.111:12345"}, + {[]string{"start", "--advertise-addr", "192.168.0.111", "--listen-addr", ":12345"}, ":12345", "192.168.0.111:12345"}, + {[]string{"start", "--advertise-addr", "192.168.0.111:12345", "--listen-addr", ":54321"}, ":54321", "192.168.0.111:12345"}, // confirm hostnames will work {[]string{"start", "--listen-addr", "my.host.name"}, "my.host.name:" + base.DefaultPort, "my.host.name:" + base.DefaultPort}, {[]string{"start", "--listen-addr", "myhostname"}, "myhostname:" + base.DefaultPort, "myhostname:" + base.DefaultPort}, // confirm IPv6 works too - {[]string{"start", "--listen-addr", "::1"}, "[::1]:" + base.DefaultPort, "[::1]:" + base.DefaultPort}, - {[]string{"start", "--listen-addr", "2622:6221:e663:4922:fc2b:788b:fadd:7b48"}, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort}, - // Backward-compatibility flags + {[]string{"start", "--listen-addr", "[::1]"}, "[::1]:" + base.DefaultPort, "[::1]:" + base.DefaultPort}, + {[]string{"start", "--listen-addr", "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]"}, + "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort}, + + // Backward-compatibility flag combinations. {[]string{"start", "--host", "192.168.0.111"}, "192.168.0.111:" + base.DefaultPort, "192.168.0.111:" + base.DefaultPort}, {[]string{"start", "--port", "12345"}, ":12345", ":12345"}, {[]string{"start", "--advertise-host", "192.168.0.111"}, ":" + base.DefaultPort, "192.168.0.111:" + base.DefaultPort}, + {[]string{"start", "--advertise-addr", "192.168.0.111", "--advertise-port", "12345"}, ":" + base.DefaultPort, "192.168.0.111:12345"}, + {[]string{"start", "--listen-addr", "::1"}, "[::1]:" + base.DefaultPort, "[::1]:" + base.DefaultPort}, + {[]string{"start", "--listen-addr", "2622:6221:e663:4922:fc2b:788b:fadd:7b48"}, + "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort}, + {[]string{"start", "--listen-addr", "127.0.0.1", "--port", "12345"}, "127.0.0.1:12345", "127.0.0.1:12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111", "--port", "12345"}, "127.0.0.1:12345", "192.168.0.111:12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111", "--port", "12345"}, "127.0.0.1:12345", "192.168.0.111:12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111", "--advertise-port", "12345"}, "127.0.0.1:" + base.DefaultPort, "192.168.0.111:12345"}, + {[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111", "--port", "54321", "--advertise-port", "12345"}, "127.0.0.1:54321", "192.168.0.111:12345"}, + {[]string{"start", "--advertise-addr", "192.168.0.111", "--port", "12345"}, ":12345", "192.168.0.111:12345"}, + {[]string{"start", "--advertise-addr", "192.168.0.111", "--advertise-port", "12345"}, ":" + base.DefaultPort, "192.168.0.111:12345"}, + {[]string{"start", "--advertise-addr", "192.168.0.111", "--port", "54321", "--advertise-port", "12345"}, ":54321", "192.168.0.111:12345"}, } for i, td := range testData { - initCLIDefaults() - if err := f.Parse(td.args); err != nil { - t.Fatalf("Parse(%#v) got unexpected error: %v", td.args, err) - } - - extraServerFlagInit() - if td.expectedAddr != serverCfg.Addr { - t.Errorf("%d. serverCfg.Addr expected '%s', but got '%s'. td.args was '%#v'.", - i, td.expectedAddr, serverCfg.Addr, td.args) - } - if td.expectedAdvertiseAddr != serverCfg.AdvertiseAddr { - t.Errorf("%d. serverCfg.AdvertiseAddr expected '%s', but got '%s'. td.args was '%#v'.", - i, td.expectedAdvertiseAddr, serverCfg.AdvertiseAddr, td.args) - } + t.Run(strings.Join(td.args, " "), func(t *testing.T) { + initCLIDefaults() + if err := f.Parse(td.args); err != nil { + t.Fatalf("Parse(%#v) got unexpected error: %v", td.args, err) + } + + extraServerFlagInit() + if td.expectedAddr != serverCfg.Addr { + t.Errorf("%d. serverCfg.Addr expected '%s', but got '%s'. td.args was '%#v'.", + i, td.expectedAddr, serverCfg.Addr, td.args) + } + if td.expectedAdvertiseAddr != serverCfg.AdvertiseAddr { + t.Errorf("%d. serverCfg.AdvertiseAddr expected '%s', but got '%s'. td.args was '%#v'.", + i, td.expectedAdvertiseAddr, serverCfg.AdvertiseAddr, td.args) + } + }) } } @@ -225,14 +241,23 @@ func TestClientConnSettings(t *testing.T) { {[]string{"quit"}, ":" + base.DefaultPort}, {[]string{"quit", "--host", "127.0.0.1"}, "127.0.0.1:" + base.DefaultPort}, {[]string{"quit", "--host", "192.168.0.111"}, "192.168.0.111:" + base.DefaultPort}, - {[]string{"quit", "--port", "12345"}, ":12345"}, - {[]string{"quit", "--host", "127.0.0.1", "--port", "12345"}, "127.0.0.1:12345"}, + {[]string{"quit", "--host", ":12345"}, ":12345"}, + {[]string{"quit", "--host", "127.0.0.1:12345"}, "127.0.0.1:12345"}, + {[]string{"quit", "-s", "127.0.0.1:12345"}, "127.0.0.1:12345"}, // confirm hostnames will work {[]string{"quit", "--host", "my.host.name"}, "my.host.name:" + base.DefaultPort}, {[]string{"quit", "--host", "myhostname"}, "myhostname:" + base.DefaultPort}, // confirm IPv6 works too + {[]string{"quit", "--host", "[::1]"}, "[::1]:" + base.DefaultPort}, + {[]string{"quit", "--host", "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]"}, + "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort}, + + // Deprecated syntax. + {[]string{"quit", "--port", "12345"}, ":12345"}, + {[]string{"quit", "--host", "127.0.0.1", "--port", "12345"}, "127.0.0.1:12345"}, {[]string{"quit", "--host", "::1"}, "[::1]:" + base.DefaultPort}, - {[]string{"quit", "--host", "2622:6221:e663:4922:fc2b:788b:fadd:7b48"}, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort}, + {[]string{"quit", "--host", "2622:6221:e663:4922:fc2b:788b:fadd:7b48"}, + "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort}, } for i, td := range testData { diff --git a/pkg/cli/interactive_tests/test_flags.tcl b/pkg/cli/interactive_tests/test_flags.tcl index 052a5d10aa99..e2b253a617af 100644 --- a/pkg/cli/interactive_tests/test_flags.tcl +++ b/pkg/cli/interactive_tests/test_flags.tcl @@ -46,9 +46,17 @@ interrupt eexpect ":/# " end_test -start_test "Check that --port causes a deprecation warning." +start_test "Check that server --port causes a deprecation warning." send "$argv start --insecure --port=26257\r" -eexpect "port has been deprecated, use --listen-port/--advertise-port instead." +eexpect "port has been deprecated, use --listen-addr instead." +eexpect "node starting" +interrupt +eexpect ":/# " +end_test + +start_test "Check that server --advertise-port causes a deprecation warning." +send "$argv start --insecure --advertise-port=12345\r" +eexpect "advertise-port has been deprecated, use --advertise-addr" eexpect "node starting" interrupt eexpect ":/# " @@ -62,7 +70,6 @@ interrupt eexpect ":/# " end_test - start_test {Check that the "failed running SUBCOMMAND" message does not consider a flag the subcommand} send "$argv --verbosity 2 start --garbage\r" eexpect {Failed running "start"} diff --git a/pkg/cli/start.go b/pkg/cli/start.go index 64275e15531d..1326ac97445d 100644 --- a/pkg/cli/start.go +++ b/pkg/cli/start.go @@ -534,9 +534,9 @@ func runStart(cmd *cobra.Command, args []string) error { if le, ok := err.(server.ListenError); ok { const errorPrefix = "consider changing the port via --" if le.Addr == serverCfg.Addr { - err = errors.Wrap(err, errorPrefix+cliflags.ListenPort.Name) + err = errors.Wrap(err, errorPrefix+cliflags.ListenAddr.Name) } else if le.Addr == serverCfg.HTTPAddr { - err = errors.Wrap(err, errorPrefix+cliflags.ListenHTTPPort.Name) + err = errors.Wrap(err, errorPrefix+cliflags.ListenHTTPAddr.Name) } } @@ -815,9 +815,7 @@ func clientFlags() string { flags := []string{os.Args[0]} host, port, err := net.SplitHostPort(serverCfg.AdvertiseAddr) if err == nil { - flags = append(flags, - "--host="+host, - "--port="+port) + flags = append(flags, "--host="+host+":"+port) } if startCtx.serverInsecure { flags = append(flags, "--insecure") @@ -1002,6 +1000,9 @@ func addrWithDefaultHost(addr string) (string, error) { if host == "" { host = "localhost" } + if strings.Contains(host, ":") { + host = "[" + host + "]" + } return net.JoinHostPort(host, port), nil } diff --git a/pkg/cmd/roachtest/debug.go b/pkg/cmd/roachtest/debug.go index 9fd4a967c1ba..c68784a6a123 100644 --- a/pkg/cmd/roachtest/debug.go +++ b/pkg/cmd/roachtest/debug.go @@ -33,7 +33,7 @@ func registerDebug(r *registry) { // present. debugExtractExist := func(node int, file string) error { port := fmt.Sprintf("{pgport:%d}", node) - if err := c.RunE(ctx, c.Node(node), "./cockroach debug zip "+file+" --insecure --port "+port); err != nil { + if err := c.RunE(ctx, c.Node(node), "./cockroach debug zip "+file+" --insecure --host=:"+port); err != nil { return err } diff --git a/pkg/cmd/roachtest/decommission.go b/pkg/cmd/roachtest/decommission.go index b4974e2eb64e..ebd7d8cfa778 100644 --- a/pkg/cmd/roachtest/decommission.go +++ b/pkg/cmd/roachtest/decommission.go @@ -155,13 +155,13 @@ func runDecommission(t *test, c *cluster, nodes int, duration time.Duration) { stop := func(node int) error { port := fmt.Sprintf("{pgport:%d}", node) defer time.Sleep(time.Second) // work around quit returning too early - return c.RunE(ctx, c.Node(node), "./cockroach quit --insecure --port "+port) + return c.RunE(ctx, c.Node(node), "./cockroach quit --insecure --host=:"+port) } decom := func(id string) error { port := fmt.Sprintf("{pgport:%d}", nodes) // always use last node t.Status("decommissioning node %s", id) - return c.RunE(ctx, c.Node(nodes), "./cockroach node decommission --insecure --wait=live --port "+port+" "+id) + return c.RunE(ctx, c.Node(nodes), "./cockroach node decommission --insecure --wait=live --host=:"+port+" "+id) } for tBegin, whileDown, node := timeutil.Now(), true, 1; timeutil.Since(tBegin) <= duration; whileDown, node = !whileDown, (node%numDecom)+1 { diff --git a/pkg/cmd/roachtest/encryption.go b/pkg/cmd/roachtest/encryption.go index 14bbaf116c09..f19a451c29f0 100644 --- a/pkg/cmd/roachtest/encryption.go +++ b/pkg/cmd/roachtest/encryption.go @@ -40,7 +40,7 @@ func registerEncryption(r *registry) { stop := func(node int) error { port := fmt.Sprintf("{pgport:%d}", node) - if err := c.RunE(ctx, c.Node(node), "./cockroach quit --insecure --port "+port); err != nil { + if err := c.RunE(ctx, c.Node(node), "./cockroach quit --insecure --host=:"+port); err != nil { return err } c.Stop(ctx, c.Node(node)) diff --git a/pkg/cmd/roachtest/kv.go b/pkg/cmd/roachtest/kv.go index c230eb5b303d..263f53c62f7b 100644 --- a/pkg/cmd/roachtest/kv.go +++ b/pkg/cmd/roachtest/kv.go @@ -153,7 +153,7 @@ func registerKVQuiescenceDead(r *registry) { run(kv+" --seed 1 {pgurl:1}", true) }) // Gracefully shut down third node (doesn't matter whether it's graceful or not). - c.Run(ctx, c.Node(nodes), "./cockroach quit --insecure --port {pgport:3}") + c.Run(ctx, c.Node(nodes), "./cockroach quit --insecure --host:{pgport:3}") c.Stop(ctx, c.Node(nodes)) // Measure qps with node down (i.e. without quiescence). qpsOneDown := qps(func() { diff --git a/pkg/cmd/roachtest/upgrade.go b/pkg/cmd/roachtest/upgrade.go index 404c0bf776f8..84a3a5e37d61 100644 --- a/pkg/cmd/roachtest/upgrade.go +++ b/pkg/cmd/roachtest/upgrade.go @@ -86,7 +86,7 @@ func registerUpgrade(r *registry) { stop := func(node int) error { port := fmt.Sprintf("{pgport:%d}", node) - if err := c.RunE(ctx, c.Node(node), "./cockroach quit --insecure --port "+port); err != nil { + if err := c.RunE(ctx, c.Node(node), "./cockroach quit --insecure --host=:"+port); err != nil { return err } c.Stop(ctx, c.Node(node)) @@ -95,7 +95,7 @@ func registerUpgrade(r *registry) { decommissionAndStop := func(node int) error { port := fmt.Sprintf("{pgport:%d}", node) - if err := c.RunE(ctx, c.Node(node), "./cockroach quit --decommission --insecure --port "+port); err != nil { + if err := c.RunE(ctx, c.Node(node), "./cockroach quit --decommission --insecure --host=:"+port); err != nil { return err } c.Stop(ctx, c.Node(node)) diff --git a/pkg/cmd/roachtest/version.go b/pkg/cmd/roachtest/version.go index 445ef7ff207a..db77094a1753 100644 --- a/pkg/cmd/roachtest/version.go +++ b/pkg/cmd/roachtest/version.go @@ -166,7 +166,7 @@ func registerVersion(r *registry) { stop := func(node int) error { l.printf("stopping node %d\n", node) port := fmt.Sprintf("{pgport:%d}", node) - if err := c.RunE(ctx, c.Node(node), "./cockroach quit --insecure --port "+port); err != nil { + if err := c.RunE(ctx, c.Node(node), "./cockroach quit --insecure --host=:"+port); err != nil { return err } // NB: we still call Stop to make sure the process is dead when we try