From 17688c572958214ec92cd2cd62904f82d5685bc0 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 14 Feb 2024 10:09:06 -0500 Subject: [PATCH] address comments from code review --- client/fingerprint/consul.go | 57 +++++++++++++++++++------------ client/fingerprint/consul_test.go | 17 ++------- 2 files changed, 37 insertions(+), 37 deletions(-) diff --git a/client/fingerprint/consul.go b/client/fingerprint/consul.go index 59d44b2b384..be7230f37e7 100644 --- a/client/fingerprint/consul.go +++ b/client/fingerprint/consul.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/go-hclog" log "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-multierror" + "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/go-version" agentconsul "github.com/hashicorp/nomad/command/agent/consul" "github.com/hashicorp/nomad/helper" @@ -313,16 +314,8 @@ func (cfs *consulFingerprintState) dnsPort(info agentconsul.Self) (string, bool) func (cfs *consulFingerprintState) dnsAddr(logger hclog.Logger) func(info agentconsul.Self) (string, bool) { return func(info agentconsul.Self) (string, bool) { - // only addresses we can use for an iptables rule from a container to the - // host will be fingerprinted - isValidForTaskUse := func(addr netip.Addr) (string, bool) { - if !addr.IsLoopback() && !addr.IsUnspecified() && addr.IsValid() { - return addr.String(), true - } - return "", false - } + var listenOnEveryIP bool - // first try to find an explicitly configured address dnsAddrs, ok := info["DebugConfig"]["DNSAddrs"].([]string) if ok { for _, dnsAddr := range dnsAddrs { @@ -330,30 +323,50 @@ func (cfs *consulFingerprintState) dnsAddr(logger hclog.Logger) func(info agentc dnsAddr = strings.TrimPrefix(dnsAddr, "udp://") parsed, err := netip.ParseAddrPort(dnsAddr) - if err != nil { + if err != nil || !parsed.IsValid() { logger.Warn("could not parse Consul addresses.dns config", "value", dnsAddr) - return "", false + return "", false // response is somehow malformed + } + + // only addresses we can use for an iptables rule from a + // container to the host will be fingerprinted + if parsed.Addr().IsUnspecified() { + listenOnEveryIP = true + break } - if val, ok := isValidForTaskUse(parsed.Addr()); ok { - return val, true + if !parsed.Addr().IsLoopback() { + return parsed.Addr().String(), true } } } - // fallback to the bind address - bindAddr, ok := info["DebugConfig"]["BindAddr"].(string) - if ok { - parsed, err := netip.ParseAddr(bindAddr) + // if Consul DNS is bound on 0.0.0.0, we want to fingerprint the private + // IP (or at worst, the public IP) of the host so that we have a valid + // IP address for the iptables rule + if listenOnEveryIP { + + privateIP, err := sockaddr.GetPrivateIP() + if err != nil { + logger.Warn("could not query network interfaces", "error", err) + return "", false // something is very wrong, so bail out + } + if privateIP != "" { + return privateIP, true + } + publicIP, err := sockaddr.GetPublicIP() if err != nil { - logger.Warn("could not parse Consul bind_addr config", "value", bindAddr) - return "", false + logger.Warn("could not query network interfaces", "error", err) + return "", false // something is very wrong, so bail out } - if val, ok := isValidForTaskUse(parsed); ok { - return val, true + if publicIP != "" { + return publicIP, true } } - return "", true // we can't fingerprint a useful value + // if we've hit here, Consul is bound on localhost and we won't be able + // to configure container DNS to use it, but we also don't want to have + // the fingerprinter return an error + return "", true } } diff --git a/client/fingerprint/consul_test.go b/client/fingerprint/consul_test.go index b1fdd11e66a..045a2f479fb 100644 --- a/client/fingerprint/consul_test.go +++ b/client/fingerprint/consul_test.go @@ -528,37 +528,24 @@ func TestConsulFingerprint_dns(t *testing.T) { must.Eq(t, "0", port) }) - t.Run("bind on 0.0.0.0", func(t *testing.T) { - addr, ok := cfs.dnsAddr(testlog.HCLogger(t))(agentconsul.Self{ - "DebugConfig": { - "DNSAddrs": []string{"tcp://0.0.0.0:8601", "udp://0.0.0.0:8601"}, - "BindAddr": "0.0.0.0", - }, - }) - must.True(t, ok) - must.Eq(t, "", addr) - }) - t.Run("get first IP", func(t *testing.T) { addr, ok := cfs.dnsAddr(testlog.HCLogger(t))(agentconsul.Self{ "DebugConfig": { "DNSAddrs": []string{"tcp://192.168.1.170:8601", "udp://192.168.1.171:8601"}, - "BindAddr": "192.168.1.172", }, }) must.True(t, ok) must.Eq(t, "192.168.1.170", addr) }) - t.Run("fallback to bind_addr", func(t *testing.T) { + t.Run("fallback to private or public IP", func(t *testing.T) { addr, ok := cfs.dnsAddr(testlog.HCLogger(t))(agentconsul.Self{ "DebugConfig": { "DNSAddrs": []string{"tcp://0.0.0.0:8601", "udp://0.0.0.0:8601"}, - "BindAddr": "192.168.1.172", }, }) must.True(t, ok) - must.Eq(t, "192.168.1.172", addr) + must.NotEq(t, "", addr) }) }