client/connect: ConsulProxy LocalServicePort/Address

Without a `LocalServicePort`, Connect services will try to use the mapped port even when delivering traffic locally. A user can override this behavior by pinning the port value in the `service` stanza but this prevents us from using the Consul service name to reach the service. This commits configures the Consul proxy with its `LocalServicePort` and `LocalServiceAddress` fields.
hashicorp · Sep 23, 2019 · 9a0f8a4 · 9a0f8a4
1 parent 05fd8a0
commit 9a0f8a4
Show file tree

Hide file tree

Showing 12 changed files with 114 additions and 17 deletions.
diff --git a/api/services.go b/api/services.go
@@ -165,8 +165,10 @@ type SidecarTask struct {
 
 // ConsulProxy represents a Consul Connect sidecar proxy jobspec stanza.
 type ConsulProxy struct {
-	Upstreams []*ConsulUpstream
-	Config    map[string]interface{}
+	LocalServiceAddress string `mapstructure:"local_service_address"`
+	LocalServicePort    int    `mapstructure:"local_service_port"`
+	Upstreams           []*ConsulUpstream
+	Config              map[string]interface{}
 }
 
 // ConsulUpstream represents a Consul Connect upstream jobspec stanza.

diff --git a/api/services_test.go b/api/services_test.go
@@ -54,3 +54,32 @@ func TestService_CheckRestart(t *testing.T) {
 	assert.Equal(t, *service.Checks[2].CheckRestart.Grace, 11*time.Second)
 	assert.True(t, service.Checks[2].CheckRestart.IgnoreWarnings)
 }
+
+// TestService_Connect asserts Service.Connect settings are properly
+// inherited by Checks.
+func TestService_Connect(t *testing.T) {
+	job := &Job{Name: stringToPtr("job")}
+	tg := &TaskGroup{Name: stringToPtr("group")}
+	task := &Task{Name: "task"}
+	service := &Service{
+		Connect: &ConsulConnect{
+			SidecarService: &ConsulSidecarService{
+				Proxy: &ConsulProxy{
+					Upstreams: []*ConsulUpstream{
+						{
+							DestinationName: "upstream",
+							LocalBindPort:   80,
+						},
+					},
+					LocalServicePort: 8000,
+				},
+			},
+		},
+	}
+
+	service.Canonicalize(task, tg, job)
+	proxy := service.Connect.SidecarService.Proxy
+	assert.Equal(t, proxy.Upstreams[0].LocalBindPort, 80)
+	assert.Equal(t, proxy.Upstreams[0].DestinationName, "upstream")
+	assert.Equal(t, proxy.LocalServicePort, 8000)
+}
diff --git a/command/agent/consul/client.go b/command/agent/consul/client.go
@@ -1459,8 +1459,14 @@ func newConnect(serviceName string, nc *structs.ConsulConnect, networks structs.
 
 	// Bind to netns IP(s):port
 	proxyConfig := map[string]interface{}{}
-	if nc.SidecarService.Proxy != nil && nc.SidecarService.Proxy.Config != nil {
-		proxyConfig = nc.SidecarService.Proxy.Config
+	localServiceAddress := ""
+	localServicePort := 0
+	if nc.SidecarService.Proxy != nil {
+		localServiceAddress = nc.SidecarService.Proxy.LocalServiceAddress
+		localServicePort = nc.SidecarService.Proxy.LocalServicePort
+		if nc.SidecarService.Proxy.Config != nil {
+			proxyConfig = nc.SidecarService.Proxy.Config
+		}
 	}
 	proxyConfig["bind_address"] = "0.0.0.0"
 	proxyConfig["bind_port"] = port.To
@@ -1473,7 +1479,9 @@ func newConnect(serviceName string, nc *structs.ConsulConnect, networks structs.
 		// Automatically configure the proxy to bind to all addresses
 		// within the netns.
 		Proxy: &api.AgentServiceConnectProxyConfig{
-			Config: proxyConfig,
+			LocalServiceAddress: localServiceAddress,
+			LocalServicePort:    localServicePort,
+			Config:              proxyConfig,
 		},
 	}
 

diff --git a/command/agent/consul/group_test.go b/command/agent/consul/group_test.go
@@ -65,7 +65,11 @@ func TestConsul_Connect(t *testing.T) {
 			Name:      "testconnect",
 			PortLabel: "9999",
 			Connect: &structs.ConsulConnect{
-				SidecarService: &structs.ConsulSidecarService{},
+				SidecarService: &structs.ConsulSidecarService{
+					Proxy: &structs.ConsulProxy{
+						LocalServicePort: 9000,
+					},
+				},
 			},
 		},
 	}
@@ -114,7 +118,7 @@ func TestConsul_Connect(t *testing.T) {
 		require.Equal(t, connectService.Proxy.DestinationServiceName, "testconnect")
 		require.Equal(t, connectService.Proxy.DestinationServiceID, serviceID)
 		require.Equal(t, connectService.Proxy.LocalServiceAddress, "127.0.0.1")
-		require.Equal(t, connectService.Proxy.LocalServicePort, 9999)
+		require.Equal(t, connectService.Proxy.LocalServicePort, 9000)
 		require.Equal(t, connectService.Proxy.Config, map[string]interface{}{
 			"bind_address": "0.0.0.0",
 			"bind_port":    float64(9998),

diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go
@@ -1068,7 +1068,9 @@ func ApiConsulConnectToStructs(in *api.ConsulConnect) *structs.ConsulConnect {
 		if in.SidecarService.Proxy != nil {
 
 			out.SidecarService.Proxy = &structs.ConsulProxy{
-				Config: in.SidecarService.Proxy.Config,
+				LocalServiceAddress: in.SidecarService.Proxy.LocalServiceAddress,
+				LocalServicePort:    in.SidecarService.Proxy.LocalServicePort,
+				Config:              in.SidecarService.Proxy.Config,
 			}
 
 			upstreams := make([]structs.ConsulUpstream, len(in.SidecarService.Proxy.Upstreams))

diff --git a/jobspec/parse_service.go b/jobspec/parse_service.go
@@ -315,6 +315,8 @@ func parseSidecarTask(item *ast.ObjectItem) (*api.SidecarTask, error) {
 
 func parseProxy(o *ast.ObjectItem) (*api.ConsulProxy, error) {
 	valid := []string{
+		"local_service_address",
+		"local_service_port",
 		"upstreams",
 		"config",
 	}

diff --git a/jobspec/test-fixtures/tg-network.hcl b/jobspec/test-fixtures/tg-network.hcl
@@ -1,12 +1,15 @@
 job "foo" {
   datacenters = ["dc1"]
+
   group "bar" {
     count = 3
+
     network {
       mode = "bridge"
+
       port "http" {
         static = 80
-        to = 8080
+        to     = 8080
       }
     }
 
@@ -19,15 +22,18 @@ job "foo" {
       connect {
         sidecar_service {
           proxy {
+            local_service_port = 8080
+
             upstreams {
               destination_name = "other-service"
               local_bind_port  = 4567
             }
           }
         }
+
         sidecar_task {
           resources {
-            cpu = 500
+            cpu    = 500
             memory = 1024
           }
 
@@ -42,13 +48,15 @@ job "foo" {
 
     task "bar" {
       driver = "raw_exec"
+
       config {
-         command = "bash"
-         args    = ["-c", "echo hi"]
+        command = "bash"
+        args    = ["-c", "echo hi"]
       }
+
       resources {
         network {
-            mbits = 10
+          mbits = 10
         }
       }
     }

diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go
@@ -2491,6 +2491,8 @@ func TestTaskGroupDiff(t *testing.T) {
 							SidecarService: &ConsulSidecarService{
 								Port: "http",
 								Proxy: &ConsulProxy{
+									LocalServiceAddress: "127.0.0.1",
+									LocalServicePort:    8080,
 									Upstreams: []ConsulUpstream{
 										{
 											DestinationName: "foo",
@@ -2674,6 +2676,19 @@ func TestTaskGroupDiff(t *testing.T) {
 											{
 												Type: DiffTypeAdded,
 												Name: "ConsulProxy",
+												Fields: []*FieldDiff{
+													{
+														Type: DiffTypeAdded,
+														Name: "LocalServiceAddress",
+														Old:  "",
+														New:  "127.0.0.1",
+													}, {
+														Type: DiffTypeAdded,
+														Name: "LocalServicePort",
+														Old:  "",
+														New:  "8080",
+													},
+												},
 												Objects: []*ObjectDiff{
 													{
 														Type: DiffTypeAdded,

diff --git a/nomad/structs/services.go b/nomad/structs/services.go
@@ -775,6 +775,17 @@ func (t *SidecarTask) MergeIntoTask(task *Task) {
 
 // ConsulProxy represents a Consul Connect sidecar proxy jobspec stanza.
 type ConsulProxy struct {
+
+	// LocalServiceAddress is the address the local service binds to.
+	// Usually 127.0.0.1 it is useful to customize in clusters with mixed
+	// Connect and non-Connect services.
+	LocalServiceAddress string
+
+	// LocalServicePort is the port the local service binds to. Usually
+	// the same as the parent service's port, it is useful to customize
+	// in clusters with mixed Connect and non-Connect services
+	LocalServicePort int
+
 	// Upstreams configures the upstream services this service intends to
 	// connect to.
 	Upstreams []ConsulUpstream
@@ -791,6 +802,8 @@ func (p *ConsulProxy) Copy() *ConsulProxy {
 	}
 
 	newP := ConsulProxy{}
+	newP.LocalServiceAddress = p.LocalServiceAddress
+	newP.LocalServicePort = p.LocalServicePort
 
 	if n := len(p.Upstreams); n > 0 {
 		newP.Upstreams = make([]ConsulUpstream, n)
@@ -817,6 +830,12 @@ func (p *ConsulProxy) Equals(o *ConsulProxy) bool {
 		return p == o
 	}
 
+	if p.LocalServiceAddress != o.LocalServiceAddress {
+		return false
+	}
+	if p.LocalServicePort != o.LocalServicePort {
+		return false
+	}
 	if len(p.Upstreams) != len(o.Upstreams) {
 		return false
 	}

diff --git a/nomad/structs/services_test.go b/nomad/structs/services_test.go
@@ -36,6 +36,8 @@ func TestConsulConnect_CopyEquals(t *testing.T) {
 		SidecarService: &ConsulSidecarService{
 			Port: "9001",
 			Proxy: &ConsulProxy{
+				LocalServiceAddress: "127.0.0.1",
+				LocalServicePort:    8080,
 				Upstreams: []ConsulUpstream{
 					{
 						DestinationName: "up1",

diff --git a/website/source/docs/job-specification/network.html.md b/website/source/docs/job-specification/network.html.md
@@ -74,7 +74,7 @@ job "docs" {
 - `port` <code>([Port](#port-parameters): nil)</code> - Specifies a TCP/UDP port
   allocation and can be used to specify both dynamic ports and reserved ports.
 
-- `mode`  `(string: "host")- Mode of the network. The following modes are available:
+- `mode`  `(string: "host")` - Mode of the network. The following modes are available:
 
  - “none” - Task group will have an isolated network without any network interfaces.
  - “bridge” - Task group will have an isolated network namespace with an interface
@@ -214,4 +214,4 @@ network {
 
 ### Limitations
 
-Only one `network` stanza can be specified, when it is defined at the task group level.
+Only one `network` stanza can be specified, when it is defined at the task group level.
diff --git a/website/source/docs/job-specification/proxy.html.md b/website/source/docs/job-specification/proxy.html.md
@@ -52,9 +52,14 @@ within the context of a `sidecar_service` stanza.
 
 ## `proxy` Parameters
 
-- `upstreams` <code>([upstreams][]: nil)</code> Used to configure details of each upstream service that
+- `local_service_address` `(string: "127.0.0.1")` - The address the local service binds to. Useful to
+  customize in clusters with mixed Connect and non-Connect services.
+- `local_service_port` <code>(int:[port][])</code> - The port the local service binds to.
+   Usually the same as the parent service's port, it is useful to customize in clusters with mixed
+   Connect and non-Connect services
+- `upstreams` <code>([upstreams][]: nil)</code> - Used to configure details of each upstream service that
   this sidecar proxy communicates with.
-- `config` - (map: nil)</code> - Proxy configuration that's opaque to Nomad and
+- `config` <code>(map: nil)</code> - Proxy configuration that's opaque to Nomad and
   passed directly to Consul. See [Consul Connect's
   documentation](https://www.consul.io/docs/connect/proxies/envoy.html#dynamic-configuration)
   for details.
@@ -81,3 +86,4 @@ The following example is a proxy specification that includes upstreams configura
 [interpolation]: /docs/runtime/interpolation.html "Nomad interpolation"
 [sidecar_service]: /docs/job-specification/sidecar_service.html "Nomad sidecar service Specification"
 [upstreams]: /docs/job-specification/upstreams.html "Nomad upstream config Specification"
+[port]: /docs/job-specification/network.html#port-parameters "Nomad network port configuration"