diff --git a/agent/agent.go b/agent/agent.go index c0d840947d..a7708646a6 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2132,13 +2132,17 @@ func (a *Agent) AddProxy(proxy *structs.ConnectManagedProxy, persist bool, if err != nil { return err } - chkTypes := []*structs.CheckType{ - &structs.CheckType{ - Name: "Connect Proxy Listening", - TCP: fmt.Sprintf("%s:%d", proxyCfg["bind_address"], - proxyCfg["bind_port"]), - Interval: 10 * time.Second, - }, + chkAddr := a.resolveProxyCheckAddress(proxyCfg) + chkTypes := []*structs.CheckType{} + if chkAddr != "" { + chkTypes = []*structs.CheckType{ + &structs.CheckType{ + Name: "Connect Proxy Listening", + TCP: fmt.Sprintf("%s:%d", chkAddr, + proxyCfg["bind_port"]), + Interval: 10 * time.Second, + }, + } } err = a.AddService(proxyService, chkTypes, persist, token) @@ -2155,6 +2159,44 @@ func (a *Agent) AddProxy(proxy *structs.ConnectManagedProxy, persist bool, return nil } +// resolveProxyCheckAddress returns the best address to use for a TCP check of +// the proxy's public listener. It expects the input to already have default +// values populated by applyProxyConfigDefaults. It may return an empty string +// indicating that the TCP check should not be created at all. +// +// By default this uses the proxy's bind address which in turn defaults to the +// agent's bind address. If the proxy bind address ends up being 0.0.0.0 we have +// to assume the agent can dial it over loopback which is usually true. +// +// In some topologies such as proxy being in a different container, the IP the +// agent used to dial proxy over a local bridge might not be the same as the +// container's public routable IP address so we allow a manual override of the +// check address in config "tcp_check_address" too. +// +// Finally the TCP check can be disabled by another manual override +// "disable_tcp_check" in cases where the agent will never be able to dial the +// proxy directly for some reason. +func (a *Agent) resolveProxyCheckAddress(proxyCfg map[string]interface{}) string { + // If user disabled the check return empty string + if disable, ok := proxyCfg["disable_tcp_check"].(bool); ok && disable { + return "" + } + + // If user specified a custom one, use that + if chkAddr, ok := proxyCfg["tcp_check_address"].(string); ok && chkAddr != "" { + return chkAddr + } + + // If we have a bind address and its diallable, use that + if bindAddr, ok := proxyCfg["bind_address"].(string); ok && + bindAddr != "" && bindAddr != "0.0.0.0" && bindAddr != "[::]" { + return bindAddr + } + + // Default to localhost + return "127.0.0.1" +} + // applyProxyConfigDefaults takes a *structs.ConnectManagedProxy and returns // it's Config map merged with any defaults from the Agent's config. It would be // nicer if this were defined as a method on structs.ConnectManagedProxy but we diff --git a/agent/agent_test.go b/agent/agent_test.go index 43478ef780..470a5498af 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -2567,30 +2567,6 @@ func TestAgent_reloadWatchesHTTPS(t *testing.T) { func TestAgent_AddProxy(t *testing.T) { t.Parallel() - a := NewTestAgent(t.Name(), ` - node_name = "node1" - - connect { - proxy_defaults { - exec_mode = "script" - daemon_command = ["foo", "bar"] - script_command = ["bar", "foo"] - } - } - - ports { - proxy_min_port = 20000 - proxy_max_port = 20000 - } - `) - defer a.Shutdown() - - // Register a target service we can use - reg := &structs.NodeService{ - Service: "web", - Port: 8080, - } - require.NoError(t, a.AddService(reg, nil, false, "")) tests := []struct { desc string @@ -2621,7 +2597,10 @@ func TestAgent_AddProxy(t *testing.T) { }, TargetServiceID: "web", }, - wantErr: false, + // Proxy will inherit agent's 0.0.0.0 bind address but we can't check that + // so we should default to localhost in that case. + wantTCPCheck: "127.0.0.1:20000", + wantErr: false, }, { desc: "default global exec mode", @@ -2634,7 +2613,8 @@ func TestAgent_AddProxy(t *testing.T) { Command: []string{"consul", "connect", "proxy"}, TargetServiceID: "web", }, - wantErr: false, + wantTCPCheck: "127.0.0.1:20000", + wantErr: false, }, { desc: "default daemon command", @@ -2647,7 +2627,8 @@ func TestAgent_AddProxy(t *testing.T) { Command: []string{"foo", "bar"}, TargetServiceID: "web", }, - wantErr: false, + wantTCPCheck: "127.0.0.1:20000", + wantErr: false, }, { desc: "default script command", @@ -2660,7 +2641,8 @@ func TestAgent_AddProxy(t *testing.T) { Command: []string{"bar", "foo"}, TargetServiceID: "web", }, - wantErr: false, + wantTCPCheck: "127.0.0.1:20000", + wantErr: false, }, { desc: "managed proxy with custom bind port", @@ -2677,7 +2659,6 @@ func TestAgent_AddProxy(t *testing.T) { wantTCPCheck: "127.10.10.10:1234", wantErr: false, }, - { // This test is necessary since JSON and HCL both will parse // numbers as a float64. @@ -2695,12 +2676,69 @@ func TestAgent_AddProxy(t *testing.T) { wantTCPCheck: "127.10.10.10:1234", wantErr: false, }, + { + desc: "managed proxy with overridden check address", + proxy: &structs.ConnectManagedProxy{ + ExecMode: structs.ProxyExecModeDaemon, + Command: []string{"consul", "connect", "proxy"}, + Config: map[string]interface{}{ + "foo": "bar", + "tcp_check_address": "127.20.20.20", + }, + TargetServiceID: "web", + }, + wantTCPCheck: "127.20.20.20:20000", + wantErr: false, + }, + { + desc: "managed proxy with disabled check", + proxy: &structs.ConnectManagedProxy{ + ExecMode: structs.ProxyExecModeDaemon, + Command: []string{"consul", "connect", "proxy"}, + Config: map[string]interface{}{ + "foo": "bar", + "disable_tcp_check": true, + }, + TargetServiceID: "web", + }, + wantTCPCheck: "", + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { require := require.New(t) + a := NewTestAgent(t.Name(), ` + node_name = "node1" + + # Explicit test because proxies inheriting this value must have a health + # check on a different IP. + bind_addr = "0.0.0.0" + + connect { + proxy_defaults { + exec_mode = "script" + daemon_command = ["foo", "bar"] + script_command = ["bar", "foo"] + } + } + + ports { + proxy_min_port = 20000 + proxy_max_port = 20000 + } + `) + defer a.Shutdown() + + // Register a target service we can use + reg := &structs.NodeService{ + Service: "web", + Port: 8080, + } + require.NoError(a.AddService(reg, nil, false, "")) + err := a.AddProxy(tt.proxy, false, "") if tt.wantErr { require.Error(err) @@ -2719,23 +2757,23 @@ func TestAgent_AddProxy(t *testing.T) { // Ensure a TCP check was created for the service. gotCheck := a.State.Check("service:web-proxy") - require.NotNil(gotCheck) - require.Equal("Connect Proxy Listening", gotCheck.Name) + if tt.wantTCPCheck == "" { + require.Nil(gotCheck) + } else { + require.NotNil(gotCheck) + require.Equal("Connect Proxy Listening", gotCheck.Name) - // Confusingly, a.State.Check("service:web-proxy") will return the state - // but it's Definition field will be empty. This appears to be expected - // when adding Checks as part of `AddService`. Notice how `AddService` - // tests in this file don't assert on that state but instead look at the - // agent's check state directly to ensure the right thing was registered. - // We'll do the same for now. - gotTCP, ok := a.checkTCPs["service:web-proxy"] - require.True(ok) - wantTCPCheck := tt.wantTCPCheck - if wantTCPCheck == "" { - wantTCPCheck = "127.0.0.1:20000" + // Confusingly, a.State.Check("service:web-proxy") will return the state + // but it's Definition field will be empty. This appears to be expected + // when adding Checks as part of `AddService`. Notice how `AddService` + // tests in this file don't assert on that state but instead look at the + // agent's check state directly to ensure the right thing was registered. + // We'll do the same for now. + gotTCP, ok := a.checkTCPs["service:web-proxy"] + require.True(ok) + require.Equal(tt.wantTCPCheck, gotTCP.TCP) + require.Equal(10*time.Second, gotTCP.Interval) } - require.Equal(wantTCPCheck, gotTCP.TCP) - require.Equal(10*time.Second, gotTCP.Interval) }) } }