cli: Add advertise-host flag to start command

[a-robinson]

Enables overriding the interface(s) that the server is listening on
for the purposes of picking an address to advertise via the gossip
network.

Could reasonably be expanded with advertise-port if anyone hits a case
where port forwarding causes issues (could potentially happen in some
docker deployments).

For what it's worth, it's a little weird that we have --host and
--port to contrast with --http-addr and --http-port. Should we
try to phase out --http-addr and replace it with --http-host?

Fixes #1008

@tamird @sploiselle

---

This change is

[sploiselle]

CC @mberhault re: http--addr

[tamird]

I'd be in favour of replacing http-addr with http-host.

---

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

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

var connURL string
var connUser, connHost, connPort, advertiseHost, httpPort, httpAddr, connDBName, zoneConfig string
var zoneDisableReplication bool

what's this?

---

cli/start.go, line 135 at r1 (raw file):

  } else {
      serverCtx.Addr = net.JoinHostPort("localhost", connPort)
      serverCtx.AdvertiseAddr = net.JoinHostPort("localhost", connPort)

serverCtx.AdvertiseAddr = serverCtx.Addr? This is what's done elsewhere.

---

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

      Name: "host",
      Description: `
The address to listen on. The node will also advertise itself using this

s/address/hostname/?

---

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

      Name: "advertise-host",
      Description: `
The address to advertise to other CockroachDB nodes for intra-cluster

ditto

---

server/server.go, line 498 at r1 (raw file):

  log.Infof(s.Ctx(), "starting %s server at %s", s.ctx.HTTPRequestScheme(), unresolvedHTTPAddr)
  log.Infof(s.Ctx(), "starting grpc/postgres server at %s", unresolvedListenAddr)
  log.Infof(s.Ctx(), "advertising grpc/postgres server at %s", unresolvedAdvertAddr)

s,grpc/postgres server, CockroachDB node,

---

Comments from Reviewable

[bdarnell]

Agreed that http-addr should be changed to http-host. We should either always use separate flags for host and port or always use the combined address form.

---

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

cli/kv.go, line 48 at r1 (raw file):

  }
  sender, err := client.NewSender(
      rpc.NewContext(context.TODO(), ctx, nil, stopper), baseCtx.AdvertiseAddr)

I think this still wants to use baseCtx.Addr. This is for client-side CLI commands, where you'd pass --host=advertised-host, not --advertise-host=advertised-host.

---

server/testserver.go, line 85 at r1 (raw file):

  // full address (including bound port).
  ctx.Addr = util.TestAddr.String()
  ctx.AdvertiseAddr = util.TestAddr.String()

TestAddr is a port 0 address which will be assigned by the kernel when we start listening. We can't assign these separately to port 0; we have to bind to port 0 and then set both Addr and AdvertiseAddr to whatever address we get.

I think that instead of saying if AdvertiseAddr == nil { AdvertiseAddr = Addr } early in the CLI package we need to leave AdvertiseAddr unspecified for as long as possible, and fill it in with the default after we have bound to our port.

---

Comments from Reviewable

[a-robinson]

Responding directly since I haven't been able to get reviewable to work on this wifi.

I'd be in favour of replacing http-addr with http-host.

Opened #9516 to discuss/track.

what's this?

Bad cherry-pick from when I first did this on top of develop. Removed.

serverCtx.AdvertiseAddr = serverCtx.Addr? This is what's done elsewhere.

Done.

s/address/hostname/?

Done.

ditto

Done.

s,grpc/postgres server, CockroachDB node,

Done.

I think this still wants to use baseCtx.Addr. This is for client-side CLI commands, where you'd pass --host=advertised-host, not --advertise-host=advertised-host.

Done. It was effectively equivalent due to our setting of AdvertiseAddr to Addr, but makes more sense as just Addr.

TestAddr is a port 0 address which will be assigned by the kernel when we start listening. We can't assign these separately to port 0; we have to bind to port 0 and then set both Addr and AdvertiseAddr to whatever address we get.

I think that instead of saying if AdvertiseAddr == nil { AdvertiseAddr = Addr } early in the CLI package we need to leave AdvertiseAddr unspecified for as long as possible, and fill it in with the default after we have bound to our port.

It's being filled in with whatever we're actually listening on by server/server.go in the unresolvedAdvertAddr logic, the same as how we update Addr based on whatever address we actually use. Not setting it here causes a lot of breakages.

[tamird]

Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

server/server.go, line 497 at r1 (raw file):

  log.Infof(s.Ctx(), "starting %s server at %s", s.ctx.HTTPRequestScheme(), unresolvedHTTPAddr)
  log.Infof(s.Ctx(), "starting grpc/postgres server at %s", unresolvedListenAddr)

leave this one as before, it actually matters (because we might coalesce ports further in the future, and for parity with the HTTP{,S} log line above)

---

server/testserver.go, line 85 at r1 (raw file):
@bdarnell from @a-robinson's email reply:

It's being filled in with whatever we're actually listening on by server/server.go in the unresolvedAdvertAddr logic, the same as how we update Addr based on whatever address we actually use. Not setting it here causes a lot of breakages.

---

Comments from Reviewable

[a-robinson]

So apparently it wasn't the bad internet causing reviewable to break, it's something about this branch that reviewable doesn't like.

leave this one as before, it actually matters (because we might coalesce ports further in the future, and for parity with the HTTP{,S} log line above)

Done.

[tamird]

lgtm