Replace Consul SkipVerifyTLS handling

CHANGELOG.md

@@ -25,6 +25,8 @@ IMPROVEMENTS:
    [[GH-3781](https://github.com/hashicorp/nomad/issues/3781)]
  * discovery: Allow `check_restart` to be specified in the `service` stanza.
    [[GH-3718](https://github.com/hashicorp/nomad/issues/3718)]
+ * discovery: Only log if Consul does not support TLSSkipVerify instead of
+   dropping checks which relied on it. Consul has had this feature since 0.7.2. [[GH-3983](https://github.com/hashicorp/nomad/issues/3983)]
  * driver/docker: Support hard CPU limits [[GH-3825](https://github.com/hashicorp/nomad/issues/3825)]
  * driver/docker: Support advertising IPv6 addresses [[GH-3790](https://github.com/hashicorp/nomad/issues/3790)]
  * driver/docker; Support overriding image entrypoint [[GH-3788](https://github.com/hashicorp/nomad/issues/3788)]

client/task_runner_test.go

@@ -143,7 +143,7 @@ func testTaskRunnerFromAlloc(t *testing.T, restarts bool, alloc *structs.Allocat
 
 	vclient := vaultclient.NewMockVaultClient()
 	cclient := consul.NewMockAgent()
-	serviceClient := consul.NewServiceClient(cclient, true, logger)
+	serviceClient := consul.NewServiceClient(cclient, logger)
 	go serviceClient.Run()
 	tr := NewTaskRunner(logger, conf, db, upd.Update, taskDir, alloc, task, vclient, serviceClient)
 	if !restarts {
@@ -1854,7 +1854,7 @@ func TestTaskRunner_CheckWatcher_Restart(t *testing.T) {
 	// backed by a mock consul whose checks are always unhealthy.
 	consulAgent := consul.NewMockAgent()
 	consulAgent.SetStatus("critical")
-	consulClient := consul.NewServiceClient(consulAgent, true, ctx.tr.logger)
+	consulClient := consul.NewServiceClient(consulAgent, ctx.tr.logger)
 	go consulClient.Run()
 	defer consulClient.Shutdown()
 

command/agent/agent.go

@@ -15,7 +15,6 @@ import (
 
 	metrics "github.com/armon/go-metrics"
 	"github.com/hashicorp/consul/api"
-	version "github.com/hashicorp/go-version"
 	"github.com/hashicorp/nomad/client"
 	clientconfig "github.com/hashicorp/nomad/client/config"
 	"github.com/hashicorp/nomad/command/agent/consul"
@@ -57,10 +56,6 @@ type Agent struct {
 	// consulCatalog is the subset of Consul's Catalog API Nomad uses.
 	consulCatalog consul.CatalogAPI
 
-	// consulSupportsTLSSkipVerify flags whether or not Nomad can register
-	// checks with TLSSkipVerify
-	consulSupportsTLSSkipVerify bool
-
 	client *client.Client
 
 	server *nomad.Server
@@ -592,10 +587,6 @@ func (a *Agent) agentHTTPCheck(server bool) *structs.ServiceCheck {
 		// No HTTPS, return a plain http check
 		return &check
 	}
-	if !a.consulSupportsTLSSkipVerify {
-		a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because it requires Consul>=0.7.2")
-		return nil
-	}
 	if a.config.TLSConfig.VerifyHTTPSClient {
 		a.logger.Printf("[WARN] agent: not registering Nomad HTTPS Health Check because verify_https_client enabled")
 		return nil
@@ -837,55 +828,14 @@ func (a *Agent) setupConsul(consulConfig *config.ConsulConfig) error {
 	}
 
 	// Determine version for TLSSkipVerify
-	if self, err := client.Agent().Self(); err == nil {
-		a.consulSupportsTLSSkipVerify = consulSupportsTLSSkipVerify(self)
-	}
 
 	// Create Consul Catalog client for service discovery.
 	a.consulCatalog = client.Catalog()
 
 	// Create Consul Service client for service advertisement and checks.
-	a.consulService = consul.NewServiceClient(client.Agent(), a.consulSupportsTLSSkipVerify, a.logger)
+	a.consulService = consul.NewServiceClient(client.Agent(), a.logger)
 
 	// Run the Consul service client's sync'ing main loop
 	go a.consulService.Run()
 	return nil
 }
-
-var consulTLSSkipVerifyMinVersion = version.Must(version.NewVersion("0.7.2"))
-
-// consulSupportsTLSSkipVerify returns true if Consul supports TLSSkipVerify.
-func consulSupportsTLSSkipVerify(self map[string]map[string]interface{}) bool {
-	member, ok := self["Member"]
-	if !ok {
-		return false
-	}
-	tagsI, ok := member["Tags"]
-	if !ok {
-		return false
-	}
-	tags, ok := tagsI.(map[string]interface{})
-	if !ok {
-		return false
-	}
-	buildI, ok := tags["build"]
-	if !ok {
-		return false
-	}
-	build, ok := buildI.(string)
-	if !ok {
-		return false
-	}
-	parts := strings.SplitN(build, ":", 2)
-	if len(parts) != 2 {
-		return false
-	}
-	v, err := version.NewVersion(parts[0])
-	if err != nil {
-		return false
-	}
-	if v.LessThan(consulTLSSkipVerifyMinVersion) {
-		return false
-	}
-	return true
-}

command/agent/agent_test.go

@@ -1,7 +1,6 @@
 package agent
 
 import (
-	"encoding/json"
 	"io/ioutil"
 	"log"
 	"os"
@@ -379,9 +378,8 @@ func TestAgent_HTTPCheck(t *testing.T) {
 		}
 	})
 
-	t.Run("HTTPS + consulSupportsTLSSkipVerify", func(t *testing.T) {
+	t.Run("HTTPS", func(t *testing.T) {
 		a := agent()
-		a.consulSupportsTLSSkipVerify = true
 		a.config.TLSConfig.EnableHTTP = true
 
 		check := a.agentHTTPCheck(false)
@@ -396,19 +394,8 @@ func TestAgent_HTTPCheck(t *testing.T) {
 		}
 	})
 
-	t.Run("HTTPS w/o TLSSkipVerify", func(t *testing.T) {
-		a := agent()
-		a.consulSupportsTLSSkipVerify = false
-		a.config.TLSConfig.EnableHTTP = true
-
-		if check := a.agentHTTPCheck(false); check != nil {
-			t.Fatalf("expected nil check not: %#v", check)
-		}
-	})
-
 	t.Run("HTTPS + VerifyHTTPSClient", func(t *testing.T) {
 		a := agent()
-		a.consulSupportsTLSSkipVerify = true
 		a.config.TLSConfig.EnableHTTP = true
 		a.config.TLSConfig.VerifyHTTPSClient = true
 
@@ -418,111 +405,6 @@ func TestAgent_HTTPCheck(t *testing.T) {
 	})
 }
 
-func TestAgent_ConsulSupportsTLSSkipVerify(t *testing.T) {
-	t.Parallel()
-	assertSupport := func(expected bool, blob string) {
-		self := map[string]map[string]interface{}{}
-		if err := json.Unmarshal([]byte("{"+blob+"}"), &self); err != nil {
-			t.Fatalf("invalid json: %v", err)
-		}
-		actual := consulSupportsTLSSkipVerify(self)
-		if actual != expected {
-			t.Errorf("expected %t but got %t for:\n%s\n", expected, actual, blob)
-		}
-	}
-
-	// 0.6.4
-	assertSupport(false, `"Member": {
-        "Addr": "127.0.0.1",
-        "DelegateCur": 4,
-        "DelegateMax": 4,
-        "DelegateMin": 2,
-        "Name": "rusty",
-        "Port": 8301,
-        "ProtocolCur": 2,
-        "ProtocolMax": 3,
-        "ProtocolMin": 1,
-        "Status": 1,
-        "Tags": {
-            "build": "0.6.4:26a0ef8c",
-            "dc": "dc1",
-            "port": "8300",
-            "role": "consul",
-            "vsn": "2",
-            "vsn_max": "3",
-            "vsn_min": "1"
-        }}`)
-
-	// 0.7.0
-	assertSupport(false, `"Member": {
-        "Addr": "127.0.0.1",
-        "DelegateCur": 4,
-        "DelegateMax": 4,
-        "DelegateMin": 2,
-        "Name": "rusty",
-        "Port": 8301,
-        "ProtocolCur": 2,
-        "ProtocolMax": 4,
-        "ProtocolMin": 1,
-        "Status": 1,
-        "Tags": {
-            "build": "0.7.0:'a189091",
-            "dc": "dc1",
-            "port": "8300",
-            "role": "consul",
-            "vsn": "2",
-            "vsn_max": "3",
-            "vsn_min": "2"
-        }}`)
-
-	// 0.7.2
-	assertSupport(true, `"Member": {
-        "Addr": "127.0.0.1",
-        "DelegateCur": 4,
-        "DelegateMax": 4,
-        "DelegateMin": 2,
-        "Name": "rusty",
-        "Port": 8301,
-        "ProtocolCur": 2,
-        "ProtocolMax": 5,
-        "ProtocolMin": 1,
-        "Status": 1,
-        "Tags": {
-            "build": "0.7.2:'a9afa0c",
-            "dc": "dc1",
-            "port": "8300",
-            "role": "consul",
-            "vsn": "2",
-            "vsn_max": "3",
-            "vsn_min": "2"
-        }}`)
-
-	// 0.8.1
-	assertSupport(true, `"Member": {
-        "Addr": "127.0.0.1",
-        "DelegateCur": 4,
-        "DelegateMax": 5,
-        "DelegateMin": 2,
-        "Name": "rusty",
-        "Port": 8301,
-        "ProtocolCur": 2,
-        "ProtocolMax": 5,
-        "ProtocolMin": 1,
-        "Status": 1,
-        "Tags": {
-            "build": "0.8.1:'e9ca44d",
-            "dc": "dc1",
-            "id": "3ddc1b59-460e-a100-1d5c-ce3972122664",
-            "port": "8300",
-            "raft_vsn": "2",
-            "role": "consul",
-            "vsn": "2",
-            "vsn_max": "3",
-            "vsn_min": "2",
-            "wan_join_port": "8302"
-        }}`)
-}
-
 // TestAgent_HTTPCheckPath asserts clients and servers use different endpoints
 // for healthchecks.
 func TestAgent_HTTPCheckPath(t *testing.T) {

command/agent/consul/catalog_testing.go

@@ -61,6 +61,27 @@ func (c *MockAgent) SetStatus(s string) string {
 	return old
 }
 
+func (c *MockAgent) Self() (map[string]map[string]interface{}, error) {
+	s := map[string]map[string]interface{}{
+		"Member": map[string]interface{}{
+			"Addr":        "127.0.0.1",
+			"DelegateCur": 4,
+			"DelegateMax": 5,
+			"DelegateMin": 2,
+			"Name":        "rusty",
+			"Port":        8301,
+			"ProtocolCur": 2,
+			"ProtocolMax": 5,
+			"ProtocolMin": 1,
+			"Status":      1,
+			"Tags": map[string]interface{}{
+				"build": "0.8.1:'e9ca44d",
+			},
+		},
+	}
+	return s, nil
+}
+
 func (c *MockAgent) Services() (map[string]*api.AgentService, error) {
 	c.mu.Lock()
 	defer c.mu.Unlock()

command/agent/consul/client.go

@@ -70,6 +70,7 @@ type AgentAPI interface {
 	Checks() (map[string]*api.AgentCheck, error)
 	CheckRegister(check *api.AgentCheckRegistration) error
 	CheckDeregister(checkID string) error
+	Self() (map[string]map[string]interface{}, error)
 	ServiceRegister(service *api.AgentServiceRegistration) error
 	ServiceDeregister(serviceID string) error
 	UpdateTTL(id, output, status string) error
@@ -190,9 +191,6 @@ type ServiceClient struct {
 	retryInterval    time.Duration
 	maxRetryInterval time.Duration
 
-	// skipVerifySupport is true if the local Consul agent supports TLSSkipVerify
-	skipVerifySupport bool
-
 	// exitCh is closed when the main Run loop exits
 	exitCh chan struct{}
 
@@ -231,10 +229,9 @@ type ServiceClient struct {
 
 // NewServiceClient creates a new Consul ServiceClient from an existing Consul API
 // Client and logger.
-func NewServiceClient(consulClient AgentAPI, skipVerifySupport bool, logger *log.Logger) *ServiceClient {
+func NewServiceClient(consulClient AgentAPI, logger *log.Logger) *ServiceClient {
 	return &ServiceClient{
 		client:             consulClient,
-		skipVerifySupport:  skipVerifySupport,
 		logger:             logger,
 		retryInterval:      defaultRetryInterval,
 		maxRetryInterval:   defaultMaxRetryInterval,
@@ -273,19 +270,48 @@ func (c *ServiceClient) hasSeen() bool {
 func (c *ServiceClient) Run() {
 	defer close(c.exitCh)
 
-	// start checkWatcher
-	ctx, cancelWatcher := context.WithCancel(context.Background())
-	defer cancelWatcher()
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	// init will be closed when Consul has been contacted
+	init := make(chan struct{})
+	go checkConsulTLSSkipVerify(ctx, c.logger, c.client, init)
+
+	// Process operations while waiting for initial contact with Consul but
+	// do not sync until contact has been made.
+	hasOps := false
+INIT:
+	for {
+		select {
+		case <-init:
+			c.markSeen()
+			break INIT
+		case <-c.shutdownCh:
+			return
+		case ops := <-c.opCh:
+			hasOps = true
+			c.merge(ops)
+		}
+	}
+	c.logger.Printf("[TRACE] consul.sync: able to contact Consul")
+
+	// Block until contact with Consul has been established
+	// Start checkWatcher
 	go c.checkWatcher.Run(ctx)
 
 	retryTimer := time.NewTimer(0)
-	<-retryTimer.C // disabled by default
+	if !hasOps {
+		// No pending operations so don't immediately sync
+		<-retryTimer.C
+	}
+
 	failures := 0
 	for {
 		select {
 		case <-retryTimer.C:
 		case <-c.shutdownCh:
-			cancelWatcher()
+			// Cancel check watcher but sync one last time
+			cancel()
 		case ops := <-c.opCh:
 			c.merge(ops)
 		}
@@ -475,9 +501,6 @@ func (c *ServiceClient) sync() error {
 		}
 	}
 
-	// A Consul operation has succeeded, mark Consul as having been seen
-	c.markSeen()
-
 	c.logger.Printf("[DEBUG] consul.sync: registered %d services, %d checks; deregistered %d services, %d checks",
 		sreg, creg, sdereg, cdereg)
 	return nil
@@ -625,11 +648,6 @@ func (c *ServiceClient) checkRegs(ops *operations, allocID, serviceID string, se
 
 	checkIDs := make([]string, 0, numChecks)
 	for _, check := range service.Checks {
-		if check.TLSSkipVerify && !c.skipVerifySupport {
-			c.logger.Printf("[WARN] consul.sync: skipping check %q for task %q alloc %q because Consul doesn't support tls_skip_verify. Please upgrade to Consul >= 0.7.2.",
-				check.Name, task.Name, allocID)
-			continue
-		}
 		checkID := makeCheckID(serviceID, check)
 		checkIDs = append(checkIDs, checkID)
 		if check.Type == structs.ServiceCheckScript {