Merge pull request #4381 from hashicorp/proxy-check-default

Proxy check default

agent/agent.go

@@ -2139,13 +2139,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)
@@ -2162,6 +2166,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

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,83 @@ 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{
+				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 +2771,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)
 		})
 	}
 }

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.
+
 * <a name="bind_address"></a><a href="#bind_address">`bind_address`</a> -
   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.
 
+* <a name="tcp_check_address"></a><a
+  href="#tcp_check_address">`tcp_check_address`</a> - 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 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
+  localhost or it's publicly advertised port. The check always uses the same
+  port that the proxy is bound to.
+
+* <a name="disable_tcp_check"></a><a
+  href="#disable_tcp_check">`disable_tcp_check`</a> - If true, this disables a
+  TCP check being setup for the proxy. Default is false.
+
 * <a name="local_service_address"></a><a href="#local_service_address">`local_service_address`</a> - 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