cli: make --host/--{listen,advertise,http}-addr recognize port numbers

[knz]

cc @jseldess @Amruta-Ranade
Fixes #23277.
Needed for #5816.

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.

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)

[cockroach-teamcity]

This change is 

[bdarnell]

Reviewed 16 of 17 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cli/cliflags/flags.go, line 210 at r1 (raw file):

	ClientHost = FlagInfo{
		Name:      "host",
		Shorthand: "s",

Why -s? I'd expect -h or -a (for "address"). (Oh, I see. We gave -h to --help. That's unfortunate)

---

pkg/cli/cliflags/flags.go, line 271 at r1 (raw file):

	Set = FlagInfo{
		Name:      "set",
		Shorthand: "s",

This wasn't present in 2.0, right?

---

pkg/cli/interactive_tests/test_flags.tcl, line 75 at r1 (raw file):

start_test "Check that client --port causes a deprecation warning."
send "$argv quit --insecure --port=26257\r"
eexpect "port has been deprecated, use --host"

It's weird that the client side still uses --host. Should we introduce a --addr flag for the client, or skip it and go all the way to using --url for everything on the client side?

This message should give the specific syntax to be used instead of just naming the --host flag. Or we should continue to support --port indefinitely and not deprecate it (I worry we might be too aggressive/annoying about deprecating things here. Better to support multiple redundant sets of flags than to force a lot of churn on users).

[a-robinson]

Reviewed 16 of 17 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cli/flags.go, line 176 at r1 (raw file):

		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

The fact that we have to handle this manually is mind-boggling. Thanks for doing it, though.

---

pkg/cli/flags.go, line 178 at r1 (raw file):

				// Maybe this was an IPv6 address using the deprecated syntax
				// without '[...]'. Try that.
				maybeAddr := "[" + v + "]:" + *a.port

It's a bit scary to just assume that a.port is non-nil, especially in a method whose intent is to set a.addr and a.port, but I guess this struct's usage is pretty limited so it's your call whether it's worth doing anything about.

---

pkg/cli/flags_test.go, line 168 at r1 (raw file):

		{[]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"},

There's still value in keeping tests around for deprecated features, I don't think we should remove all the --listen-port and --advertise-port tests yet.

---

pkg/cli/flags_test.go, line 191 at r1 (raw file):

		{[]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},

Should we have some test cases that verify the behavior when conflicting flags are set? e.g. --listen-addr=1.1.1.1:12345 --port=23456

---

pkg/cli/cliflags/flags.go, line 271 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This wasn't present in 2.0, right?

Looks like it wasn't. I don't love that -s will have a different meaning for cockroach start than for client commands, though.

---

pkg/cli/interactive_tests/test_flags.tcl, line 75 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

It's weird that the client side still uses --host. Should we introduce a --addr flag for the client, or skip it and go all the way to using --url for everything on the client side?

This message should give the specific syntax to be used instead of just naming the --host flag. Or we should continue to support --port indefinitely and not deprecate it (I worry we might be too aggressive/annoying about deprecating things here. Better to support multiple redundant sets of flags than to force a lot of churn on users).

I'd vote for keeping --port supported and not annoying people with a deprecation message. It's easy enough to keep working for existing users without much maintenance burden. We might want to hide it from the --help output, though.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cli/flags.go, line 178 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

It's a bit scary to just assume that a.port is non-nil, especially in a method whose intent is to set a.addr and a.port, but I guess this struct's usage is pretty limited so it's your call whether it's worth doing anything about.

It's better than that: the syntax with an empty string is a valid input for the net package so this will succeed no matter what. Added a comment to clarify.

---

pkg/cli/flags_test.go, line 168 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

There's still value in keeping tests around for deprecated features, I don't think we should remove all the --listen-port and --advertise-port tests yet.

Fair enough. I re-added them.

---

pkg/cli/flags_test.go, line 191 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Should we have some test cases that verify the behavior when conflicting flags are set? e.g. --listen-addr=1.1.1.1:12345 --port=23456

They are not conflicting -- the later flags override the earlier flags. This was already the case before. Added a test case to clarify.

---

pkg/cli/cliflags/flags.go, line 210 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Why -s? I'd expect -h or -a (for "address"). (Oh, I see. We gave -h to --help. That's unfortunate)

-h is relatively standard for help so I'm not keen on using it.

-s for "server". I could also live with "-c" (connect).

-a would be sad to use because I can imagine a command later wanting to use a for "all" of something.

---

pkg/cli/cliflags/flags.go, line 271 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Looks like it wasn't. I don't love that -s will have a different meaning for cockroach start than for client commands, though.

@a-robinson how so different? -s is not valid for cockroach start, in the same way that -p was not valid for it either.

---

pkg/cli/interactive_tests/test_flags.tcl, line 75 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

I'd vote for keeping --port supported and not annoying people with a deprecation message. It's easy enough to keep working for existing users without much maintenance burden. We might want to hide it from the --help output, though.

@bdarnell yes the plan is to use --url for every client command (and deprecate/hide --host + --port) but that is a separate issue #4984 which I'd like to tackle in a separate PR.

In the meantime I removed the deprecation warning for --port, but kept the flag hidden as Alex suggests.

[bdarnell]

Reviewed 4 of 4 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/cli/cliflags/flags.go, line 210 at r1 (raw file):

Previously, knz (kena) wrote…

-h is relatively standard for help so I'm not keen on using it.

-s for "server". I could also live with "-c" (connect).

-a would be sad to use because I can imagine a command later wanting to use a for "all" of something.

-h for help is far from universal - ls and grep (to name a couple off the top of my head) have -h with other meanings.

Since we want to move towards URLs for all client commands anyway, maybe this flag doesn't even need a short version.

---

pkg/cli/cliflags/flags.go, line 271 at r1 (raw file):

Previously, knz (kena) wrote…

@a-robinson how so different? -s is not valid for cockroach start, in the same way that -p was not valid for it either.

-s is --store for start

---

pkg/cli/interactive_tests/test_flags.tcl, line 75 at r1 (raw file):

Previously, knz (kena) wrote…

@bdarnell yes the plan is to use --url for every client command (and deprecate/hide --host + --port) but that is a separate issue #4984 which I'd like to tackle in a separate PR.

In the meantime I removed the deprecation warning for --port, but kept the flag hidden as Alex suggests.

I dislike hidden flags; I think they're more confusing than just having an option that most people won't use in the help output.

[a-robinson]

pending your last conversation or two with Ben finishing up.

Reviewed 1 of 17 files at r1, 4 of 4 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/cli/flags.go, line 178 at r1 (raw file):

Previously, knz (kena) wrote…

It's better than that: the syntax with an empty string is a valid input for the net package so this will succeed no matter what. Added a comment to clarify.

I was thinking of code like

var a addrSetter
a.Set("foo")

In which a.port isn't just an empty string, but a nil pointer. But again, since the use of addrSetter is pretty tightly controlled to this small package, it's not a big deal.

---

pkg/cli/flags_test.go, line 191 at r1 (raw file):

Previously, knz (kena) wrote…

They are not conflicting -- the later flags override the earlier flags. This was already the case before. Added a test case to clarify.

They're logically conflicting to someone who otherwise doesn't know how the implementation. Thanks for adding the test case!

---

pkg/cli/interactive_tests/test_flags.tcl, line 75 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I dislike hidden flags; I think they're more confusing than just having an option that most people won't use in the help output.

Hm, I prefer not having flags with overlapping behavior in the help output since we then need to explain how they interact with each other or how they differ in the help output. I don't feel that strongly though, I'll defer to you guys.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/cli/flags.go, line 178 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

I was thinking of code like

var a addrSetter
a.Set("foo")

In which a.port isn't just an empty string, but a nil pointer. But again, since the use of addrSetter is pretty tightly controlled to this small package, it's not a big deal.

Yeah ok I'll leave it this way then.

---

pkg/cli/flags_test.go, line 191 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

They're logically conflicting to someone who otherwise doesn't know how the implementation. Thanks for adding the test case!

You're welcome.

---

pkg/cli/cliflags/flags.go, line 210 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

-h for help is far from universal - ls and grep (to name a couple off the top of my head) have -h with other meanings.

Since we want to move towards URLs for all client commands anyway, maybe this flag doesn't even need a short version.

Yes I also had missed that -s is alias for --store. Let's remove the short flag altogether.

---

pkg/cli/cliflags/flags.go, line 271 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

-s is --store for start

Acknowledged. I had forgotten.

---

pkg/cli/interactive_tests/test_flags.tcl, line 75 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Hm, I prefer not having flags with overlapping behavior in the help output since we then need to explain how they interact with each other or how they differ in the help output. I don't feel that strongly though, I'll defer to you guys.

I'm with Alex and rather not list / document the flag.

[bdarnell]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

[knz]

Alex?

[a-robinson]

Weird, I thought you had my LGTM as of #28373 (review). Github has gotten more complicated.

[knz]

it has! Oh well. Thanks :)

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)