server: resolve hostnames before use

[tamird]

It seems that configurations where the OS-provided hostname doesn't
actually resolve from the local machine are quite common and can cause
hard to diagnose problems for users. Avoid using the OS-provided host
if we're not even able to resolve it locally.

Updates #16173.

[cockroach-teamcity]

This change is 

[petermattis]

Channeling my inner Tamir: doesn't this deserve a test?

---

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

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

		if unresolvedHost != "" {
			// A host was provided, use it.
			return unresolvedHost

Shouldn't we try to resolve the hostname even when one was explicitly provided? This will probably require updating TestOfficializeAddr.

---

Comments from Reviewable

[tamird]

Why it certainly does! I don't know how to make os.Hostname return something predictably non-resolvable, though. Any ideas?

---

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

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

Previously, petermattis (Peter Mattis) wrote…

Shouldn't we try to resolve the hostname even when one was explicitly provided? This will probably require updating TestOfficializeAddr.

Well, if one was explicitly provided, it's conceivable that the user simply knows better than we do. This function is use to "officialize" 3 addresses:

• s.cfg.Addr
• s.cfg.AdvertiseAddr
• s.cfg.HTTPAddr

The user may very well know better than us in the case of the latter two (i.e. the user is giving us an address that is externally resolvable, despite not being locally resolvable).

s.cfg.Addr is used to make a loopback grpc connection for the grpc-gateway handlers, though. Still, if a user gives us a nonsense --addr value, I think we should do what they say rather than try to be clever.

---

Comments from Reviewable

[petermattis]

I don't know how to make os.Hostname return something predictably non-resolvable, though. Any ideas?

Pass os.Hostname to the method that uses it. In a test, pass a closure.

---

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

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

Previously, tamird (Tamir Duberstein) wrote…

Well, if one was explicitly provided, it's conceivable that the user simply knows better than we do. This function is use to "officialize" 3 addresses:

• s.cfg.Addr
• s.cfg.AdvertiseAddr
• s.cfg.HTTPAddr

The user may very well know better than us in the case of the latter two (i.e. the user is giving us an address that is externally resolvable, despite not being locally resolvable).

s.cfg.Addr is used to make a loopback grpc connection for the grpc-gateway handlers, though. Still, if a user gives us a nonsense --addr value, I think we should do what they say rather than try to be clever.

This is exactly the sort of misconfiguration we should be helping the user discover. Are there common deployment scenarios where a hostname is externally resolvable despite not being locally resolvable?

At the very least, we should be resolving the hostname and shouting a warning (but still using the hostname) if it is not resolvable.

---

Comments from Reviewable

[bdarnell]

That's half the problem. Finding a reliably non-resolvable hostname is tricky too. I've seen resolvers give results (the bogus ad-insertion kind) even for names containing spaces and other garbage. Fortunately, Go's resolver guarantees that an empty string will never resolve.

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

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

Previously, petermattis (Peter Mattis) wrote…

This is exactly the sort of misconfiguration we should be helping the user discover. Are there common deployment scenarios where a hostname is externally resolvable despite not being locally resolvable?

At the very least, we should be resolving the hostname and shouting a warning (but still using the hostname) if it is not resolvable.

There are common scenarios where a hostname resolves differently locally than it does remotely (i.e. it may resolve to a loopback IP instead of a routable one). But I think it's correct to require that it always resolve to something locally.

---

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

		// resolved address.
		if name, err := os.Hostname(); err != nil {
			log.ErrEventf(ctx, "unable to get hostname: %s", err)

Why are we logging this error (and below) instead of returning it? These errors should cause the server to fail to start instead of just logging quietly.

---

Comments from Reviewable

[tamird]

Updated, PTAL.

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

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

Previously, bdarnell (Ben Darnell) wrote…

There are common scenarios where a hostname resolves differently locally than it does remotely (i.e. it may resolve to a loopback IP instead of a routable one). But I think it's correct to require that it always resolve to something locally.

Done.

---

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

Previously, bdarnell (Ben Darnell) wrote…

Why are we logging this error (and below) instead of returning it? These errors should cause the server to fail to start instead of just logging quietly.

OK, changed to return errors.

---

Comments from Reviewable

[bdarnell]

---

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable