From bb9a5c703b1a53248e59feca0073fbeeb90396b8 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Thu, 12 Jul 2018 12:57:10 +0100 Subject: [PATCH 1/3] Default managed proxy TCP check address sanely when proxy is bound to 0.0.0.0. This also provides a mechanism to configure custom address or disable the check entirely from managed proxy config. --- agent/agent.go | 56 ++++++++++++++++--- agent/agent_test.go | 128 ++++++++++++++++++++++++++++---------------- 2 files changed, 132 insertions(+), 52 deletions(-) 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) - - // 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" + 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) + 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) }) } } From 8405b41f2b67038388a2e3f6544f748c82448865 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Thu, 12 Jul 2018 13:07:48 +0100 Subject: [PATCH 2/3] Update proxy config docs and add test for ipv6 --- agent/agent_test.go | 14 +++++++++++++ .../source/docs/connect/configuration.html.md | 20 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/agent/agent_test.go b/agent/agent_test.go index 470a5498af..f22fc88ff7 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -2676,6 +2676,20 @@ func TestAgent_AddProxy(t *testing.T) { wantTCPCheck: "127.10.10.10:1234", wantErr: false, }, + { + desc: "managed proxy with overridden but unspecified ipv6 bind address", + proxy: &structs.ConnectManagedProxy{ + ExecMode: structs.ProxyExecModeDaemon, + Command: []string{"consul", "connect", "proxy"}, + Config: map[string]interface{}{ + "foo": "bar", + "bind_address": "[::]", + }, + TargetServiceID: "web", + }, + wantTCPCheck: "127.0.0.1:20000", + wantErr: false, + }, { desc: "managed proxy with overridden check address", proxy: &structs.ConnectManagedProxy{ diff --git a/website/source/docs/connect/configuration.html.md b/website/source/docs/connect/configuration.html.md index 3a08348810..b7697a7508 100644 --- a/website/source/docs/connect/configuration.html.md +++ b/website/source/docs/connect/configuration.html.md @@ -62,6 +62,8 @@ described here, the rest of the service definition is shown for context and is "config": { "bind_address": "0.0.0.0", "bind_port": 20000, + "tcp_check_address": "192.168.0.1", + "disable_tcp_check": false, "local_service_address": "127.0.0.1:1234", "local_connect_timeout_ms": 1000, "handshake_timeout_ms": 10000, @@ -84,6 +86,8 @@ described here, the rest of the service definition is shown for context and is #### Configuration Key Reference +All fields are optional with a sane default. + * `bind_address` - The address the proxy will bind it's _public_ mTLS listener to. It defaults to the same address the agent binds to. @@ -94,6 +98,22 @@ described here, the rest of the service definition is shown for context and is range](/docs/agent/options.html#proxy_min_port) if available. By default the range is [20000, 20255] and the port is selected at random from that range. +* `tcp_check_address` - The address the agent will + run a [TCP health check](/docs/agent/checks.html) against. By default this is + the same as the proxy's [bind address](#bind_address) except if the + bind_address is `0.0.0.0` or `[::]` in which case this defaults to `127.0.0.1` + and assumes agent can dial proxy over loopback. For more complex + configurations where agent and proxy communicate over a bridge for example, + this configuration can be used to specify a different _address_ (but not port) + for the agent to use for health checks if it can't talk to the proxy over + localhost or it's publicly advertised port. The check always uses the same + port that the proxy is bound to. + +* `disable_tcp_check` - If true, this disables a + TCP check being setup for the proxy. Default is false. + * `local_service_address` - The `[address]:port` that the proxy should use to connect to the local application instance. By default it assumes `127.0.0.1` as the address and takes the port From 43c7213fe9fa89ccb89c259e3cc8b82a71e988c1 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Thu, 12 Jul 2018 14:36:52 +0100 Subject: [PATCH 3/3] Grammar --- website/source/docs/connect/configuration.html.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/source/docs/connect/configuration.html.md b/website/source/docs/connect/configuration.html.md index b7697a7508..fdb92413be 100644 --- a/website/source/docs/connect/configuration.html.md +++ b/website/source/docs/connect/configuration.html.md @@ -103,7 +103,7 @@ All fields are optional with a sane default. run a [TCP health check](/docs/agent/checks.html) against. By default this is the same as the proxy's [bind address](#bind_address) except if the bind_address is `0.0.0.0` or `[::]` in which case this defaults to `127.0.0.1` - and assumes agent can dial proxy over loopback. For more complex + and assumes the agent can dial the proxy over loopback. For more complex configurations where agent and proxy communicate over a bridge for example, this configuration can be used to specify a different _address_ (but not port) for the agent to use for health checks if it can't talk to the proxy over