Merge branch 'master' into autopilot

command/agent/config_test.go

@@ -1,6 +1,7 @@
 package agent
 
 import (
+	"fmt"
 	"io/ioutil"
 	"net"
 	"os"
@@ -9,6 +10,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/consul/lib/freeport"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/nomad/structs/config"
 )
@@ -541,7 +543,8 @@ func TestConfig_Listener(t *testing.T) {
 	}
 
 	// Works with valid inputs
-	ln, err := config.Listener("tcp", "127.0.0.1", 24000)
+	ports := freeport.GetT(t, 2)
+	ln, err := config.Listener("tcp", "127.0.0.1", ports[0])
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -550,20 +553,22 @@ func TestConfig_Listener(t *testing.T) {
 	if net := ln.Addr().Network(); net != "tcp" {
 		t.Fatalf("expected tcp, got: %q", net)
 	}
-	if addr := ln.Addr().String(); addr != "127.0.0.1:24000" {
-		t.Fatalf("expected 127.0.0.1:4646, got: %q", addr)
+	want := fmt.Sprintf("127.0.0.1:%d", ports[0])
+	if addr := ln.Addr().String(); addr != want {
+		t.Fatalf("expected %q, got: %q", want, addr)
 	}
 
 	// Falls back to default bind address if non provided
 	config.BindAddr = "0.0.0.0"
-	ln, err = config.Listener("tcp4", "", 24000)
+	ln, err = config.Listener("tcp4", "", ports[1])
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
 	ln.Close()
 
-	if addr := ln.Addr().String(); addr != "0.0.0.0:24000" {
-		t.Fatalf("expected 0.0.0.0:24000, got: %q", addr)
+	want = fmt.Sprintf("0.0.0.0:%d", ports[1])
+	if addr := ln.Addr().String(); addr != want {
+		t.Fatalf("expected %q, got: %q", want, addr)
 	}
 }
 

command/agent/consul/client.go

@@ -663,7 +663,7 @@ func (c *ServiceClient) checkRegs(ops *operations, allocID, serviceID string, se
 
 		ip, port, err := getAddress(addrMode, portLabel, task.Resources.Networks, net)
 		if err != nil {
-			return nil, fmt.Errorf("unable to get address for check %q: %v", check.Name, err)
+			return nil, fmt.Errorf("error getting address for check %q: %v", check.Name, err)
 		}
 
 		checkReg, err := createCheckReg(serviceID, checkID, check, ip, port)
@@ -1036,6 +1036,11 @@ func createCheckReg(serviceID, checkID string, check *structs.ServiceCheck, host
 	chkReg.Timeout = check.Timeout.String()
 	chkReg.Interval = check.Interval.String()
 
+	// Require an address for http or tcp checks
+	if port == 0 && check.RequiresPort() {
+		return nil, fmt.Errorf("%s checks require an address", check.Type)
+	}
+
 	switch check.Type {
 	case structs.ServiceCheckHTTP:
 		proto := check.Protocol
@@ -1089,9 +1094,15 @@ func isOldNomadService(id string) bool {
 	return strings.HasPrefix(id, prefix)
 }
 
-// getAddress returns the ip and port to use for a service or check. An error
-// is returned if an ip and port cannot be determined.
+// getAddress returns the IP and port to use for a service or check. If no port
+// label is specified (an empty value), zero values are returned because no
+// address could be resolved.
 func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet *cstructs.DriverNetwork) (string, int, error) {
+	// No port label specified, no address can be assembled
+	if portLabel == "" {
+		return "", 0, nil
+	}
+
 	switch addrMode {
 	case structs.AddressModeAuto:
 		if driverNet.Advertise() {

command/agent/consul/int_test.go

@@ -87,8 +87,17 @@ func TestConsul_Integration(t *testing.T) {
 	task.Config = map[string]interface{}{
 		"run_for": "1h",
 	}
+
 	// Choose a port that shouldn't be in use
-	task.Resources.Networks[0].ReservedPorts = []structs.Port{{Label: "http", Value: 3}}
+	netResource := &structs.NetworkResource{
+		Device:        "eth0",
+		IP:            "127.0.0.1",
+		MBits:         50,
+		ReservedPorts: []structs.Port{{Label: "http", Value: 3}},
+	}
+	alloc.Resources.Networks[0] = netResource
+	alloc.TaskResources["web"].Networks[0] = netResource
+	task.Resources.Networks[0] = netResource
 	task.Services = []*structs.Service{
 		{
 			Name:      "httpd",
@@ -96,13 +105,12 @@ func TestConsul_Integration(t *testing.T) {
 			Tags:      []string{"nomad", "test", "http"},
 			Checks: []*structs.ServiceCheck{
 				{
-					Name:      "httpd-http-check",
-					Type:      "http",
-					Path:      "/",
-					Protocol:  "http",
-					PortLabel: "http",
-					Interval:  9000 * time.Hour,
-					Timeout:   1, // fail as fast as possible
+					Name:     "httpd-http-check",
+					Type:     "http",
+					Path:     "/",
+					Protocol: "http",
+					Interval: 9000 * time.Hour,
+					Timeout:  1, // fail as fast as possible
 				},
 				{
 					Name:     "httpd-script-check",

command/agent/consul/unit_test.go

@@ -1566,8 +1566,13 @@ func TestGetAddress(t *testing.T) {
 		{
 			Name:        "InvalidMode",
 			Mode:        "invalid-mode",
+			PortLabel:   "80",
 			ErrContains: "invalid address mode",
 		},
+		{
+			Name: "EmptyIsOk",
+			Mode: structs.AddressModeHost,
+		},
 	}
 
 	for _, tc := range cases {

command/agent/http.go

@@ -11,6 +11,7 @@ import (
 	"net/http/pprof"
 	"os"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/NYTimes/gziphandler"
@@ -284,17 +285,22 @@ func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Reque
 		if err != nil {
 			s.logger.Printf("[ERR] http: Request %v, error: %v", reqURL, err)
 			code := 500
+			errMsg := err.Error()
 			if http, ok := err.(HTTPCodedError); ok {
 				code = http.Code()
 			} else {
-				switch err.Error() {
-				case structs.ErrPermissionDenied.Error(), structs.ErrTokenNotFound.Error():
+				// RPC errors get wrapped, so manually unwrap by only looking at their suffix
+				if strings.HasSuffix(errMsg, structs.ErrPermissionDenied.Error()) {
+					errMsg = structs.ErrPermissionDenied.Error()
+					code = 403
+				} else if strings.HasSuffix(errMsg, structs.ErrTokenNotFound.Error()) {
+					errMsg = structs.ErrTokenNotFound.Error()
 					code = 403
 				}
 			}
 
 			resp.WriteHeader(code)
-			resp.Write([]byte(err.Error()))
+			resp.Write([]byte(errMsg))
 			return
 		}
 

command/agent/http_test.go

@@ -225,15 +225,28 @@ func TestPermissionDenied(t *testing.T) {
 	})
 	defer s.Shutdown()
 
-	resp := httptest.NewRecorder()
-	handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-		return nil, structs.ErrPermissionDenied
+	{
+		resp := httptest.NewRecorder()
+		handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
+			return nil, structs.ErrPermissionDenied
+		}
+
+		req, _ := http.NewRequest("GET", "/v1/job/foo", nil)
+		s.Server.wrap(handler)(resp, req)
+		assert.Equal(t, resp.Code, 403)
 	}
 
-	urlStr := "/v1/job/foo"
-	req, _ := http.NewRequest("GET", urlStr, nil)
-	s.Server.wrap(handler)(resp, req)
-	assert.Equal(t, resp.Code, 403)
+	// When remote RPC is used the errors have "rpc error: " prependend
+	{
+		resp := httptest.NewRecorder()
+		handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
+			return nil, fmt.Errorf("rpc error: %v", structs.ErrPermissionDenied)
+		}
+
+		req, _ := http.NewRequest("GET", "/v1/job/foo", nil)
+		s.Server.wrap(handler)(resp, req)
+		assert.Equal(t, resp.Code, 403)
+	}
 }
 
 func TestTokenNotFound(t *testing.T) {

command/agent/job_endpoint_test.go

@@ -1212,6 +1212,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 								Name:      "serviceA",
 								Tags:      []string{"1", "2"},
 								PortLabel: "foo",
+								CheckRestart: &api.CheckRestart{
+									Limit: 4,
+									Grace: helper.TimeToPtr(11 * time.Second),
+								},
 								Checks: []api.ServiceCheck{
 									{
 										Id:            "hello",
@@ -1228,10 +1232,17 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 										InitialStatus: "ok",
 										CheckRestart: &api.CheckRestart{
 											Limit:          3,
-											Grace:          helper.TimeToPtr(10 * time.Second),
 											IgnoreWarnings: true,
 										},
 									},
+									{
+										Id:        "check2id",
+										Name:      "check2",
+										Type:      "tcp",
+										PortLabel: "foo",
+										Interval:  4 * time.Second,
+										Timeout:   2 * time.Second,
+									},
 								},
 							},
 						},
@@ -1425,10 +1436,21 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 										InitialStatus: "ok",
 										CheckRestart: &structs.CheckRestart{
 											Limit:          3,
-											Grace:          10 * time.Second,
+											Grace:          11 * time.Second,
 											IgnoreWarnings: true,
 										},
 									},
+									{
+										Name:      "check2",
+										Type:      "tcp",
+										PortLabel: "foo",
+										Interval:  4 * time.Second,
+										Timeout:   2 * time.Second,
+										CheckRestart: &structs.CheckRestart{
+											Limit: 4,
+											Grace: 11 * time.Second,
+										},
+									},
 								},
 							},
 						},