Merge pull request #8463 from hashicorp/dnephin/unmethod-make-node-id

agent: convert NodeID methods to functions

agent/agent.go

@@ -2,7 +2,6 @@ package agent
 
 import (
 	"context"
-	"crypto/sha512"
 	"crypto/tls"
 	"encoding/json"
 	"fmt"
@@ -52,11 +51,9 @@ import (
 	"github.com/hashicorp/consul/tlsutil"
 	"github.com/hashicorp/consul/types"
 	"github.com/hashicorp/go-multierror"
-	"github.com/hashicorp/go-uuid"
 	"github.com/hashicorp/memberlist"
 	"github.com/hashicorp/raft"
 	"github.com/hashicorp/serf/serf"
-	"github.com/shirou/gopsutil/host"
 	"golang.org/x/net/http2"
 )
 
@@ -513,8 +510,9 @@ func New(options ...AgentOption) (*Agent, error) {
 
 	// Retrieve or generate the node ID before setting up the rest of the
 	// agent, which depends on it.
-	if err := a.setupNodeID(a.config); err != nil {
+	config.NodeID, err = newNodeIDFromConfig(a.config, a.logger)
-		return nil, fmt.Errorf("Failed to setup node ID: %v", err)
+	if err != nil {
+		return nil, fmt.Errorf("failed to setup node ID: %w", err)
 	}
 
 	// We used to do this in the Start method. However it doesn't need to go
@@ -1562,122 +1560,6 @@ func (a *Agent) segmentConfig() ([]consul.NetworkSegment, error) {
 	return segments, nil
 }
 
-// makeRandomID will generate a random UUID for a node.
-func (a *Agent) makeRandomID() (string, error) {
-	id, err := uuid.GenerateUUID()
-	if err != nil {
-		return "", err
-	}
-
-	a.logger.Debug("Using random ID as node ID", "id", id)
-	return id, nil
-}
-
-// makeNodeID will try to find a host-specific ID, or else will generate a
-// random ID. The returned ID will always be formatted as a GUID. We don't tell
-// the caller whether this ID is random or stable since the consequences are
-// high for us if this changes, so we will persist it either way. This will let
-// gopsutil change implementations without affecting in-place upgrades of nodes.
-func (a *Agent) makeNodeID() (string, error) {
-	// If they've disabled host-based IDs then just make a random one.
-	if a.config.DisableHostNodeID {
-		return a.makeRandomID()
-	}
-
-	// Try to get a stable ID associated with the host itself.
-	info, err := host.Info()
-	if err != nil {
-		a.logger.Debug("Couldn't get a unique ID from the host", "error", err)
-		return a.makeRandomID()
-	}
-
-	// Make sure the host ID parses as a UUID, since we don't have complete
-	// control over this process.
-	id := strings.ToLower(info.HostID)
-	if _, err := uuid.ParseUUID(id); err != nil {
-		a.logger.Debug("Unique ID from host isn't formatted as a UUID",
-			"id", id,
-			"error", err,
-		)
-		return a.makeRandomID()
-	}
-
-	// Hash the input to make it well distributed. The reported Host UUID may be
-	// similar across nodes if they are on a cloud provider or on motherboards
-	// created from the same batch.
-	buf := sha512.Sum512([]byte(id))
-	id = fmt.Sprintf("%08x-%04x-%04x-%04x-%12x",
-		buf[0:4],
-		buf[4:6],
-		buf[6:8],
-		buf[8:10],
-		buf[10:16])
-
-	a.logger.Debug("Using unique ID from host as node ID", "id", id)
-	return id, nil
-}
-
-// setupNodeID will pull the persisted node ID, if any, or create a random one
-// and persist it.
-func (a *Agent) setupNodeID(config *config.RuntimeConfig) error {
-	// If they've configured a node ID manually then just use that, as
-	// long as it's valid.
-	if config.NodeID != "" {
-		config.NodeID = types.NodeID(strings.ToLower(string(config.NodeID)))
-		if _, err := uuid.ParseUUID(string(config.NodeID)); err != nil {
-			return err
-		}
-
-		return nil
-	}
-
-	// For dev mode we have no filesystem access so just make one.
-	if a.config.DataDir == "" {
-		id, err := a.makeNodeID()
-		if err != nil {
-			return err
-		}
-
-		config.NodeID = types.NodeID(id)
-		return nil
-	}
-
-	// Load saved state, if any. Since a user could edit this, we also
-	// validate it.
-	fileID := filepath.Join(config.DataDir, "node-id")
-	if _, err := os.Stat(fileID); err == nil {
-		rawID, err := ioutil.ReadFile(fileID)
-		if err != nil {
-			return err
-		}
-
-		nodeID := strings.TrimSpace(string(rawID))
-		nodeID = strings.ToLower(nodeID)
-		if _, err := uuid.ParseUUID(nodeID); err != nil {
-			return err
-		}
-
-		config.NodeID = types.NodeID(nodeID)
-	}
-
-	// If we still don't have a valid node ID, make one.
-	if config.NodeID == "" {
-		id, err := a.makeNodeID()
-		if err != nil {
-			return err
-		}
-		if err := lib.EnsurePath(fileID, false); err != nil {
-			return err
-		}
-		if err := ioutil.WriteFile(fileID, []byte(id), 0600); err != nil {
-			return err
-		}
-
-		config.NodeID = types.NodeID(id)
-	}
-	return nil
-}
-
 // setupBaseKeyrings configures the LAN and WAN keyrings.
 func (a *Agent) setupBaseKeyrings(config *consul.Config) error {
 	// If the keyring file is disabled then just poke the provided key

agent/agent_test.go

@@ -39,7 +39,6 @@ import (
 	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/consul/testrpc"
 	"github.com/hashicorp/consul/types"
-	"github.com/hashicorp/go-uuid"
 	"github.com/hashicorp/serf/coordinate"
 	"github.com/hashicorp/serf/serf"
 	"github.com/stretchr/testify/assert"
@@ -252,120 +251,6 @@ func TestAgent_ReconnectConfigWanDisabled(t *testing.T) {
 	require.Nil(t, a.consulConfig().SerfWANConfig)
 }
 
-func TestAgent_setupNodeID(t *testing.T) {
-	t.Parallel()
-	a := NewTestAgent(t, `
-		node_id = ""
-	`)
-	defer a.Shutdown()
-
-	cfg := a.config
-
-	// The auto-assigned ID should be valid.
-	id := a.consulConfig().NodeID
-	if _, err := uuid.ParseUUID(string(id)); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-
-	// Running again should get the same ID (persisted in the file).
-	cfg.NodeID = ""
-	if err := a.setupNodeID(cfg); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if newID := a.consulConfig().NodeID; id != newID {
-		t.Fatalf("bad: %q vs %q", id, newID)
-	}
-
-	// Set an invalid ID via.Config.
-	cfg.NodeID = types.NodeID("nope")
-	err := a.setupNodeID(cfg)
-	if err == nil || !strings.Contains(err.Error(), "uuid string is wrong length") {
-		t.Fatalf("err: %v", err)
-	}
-
-	// Set a valid ID via.Config.
-	newID, err := uuid.GenerateUUID()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	cfg.NodeID = types.NodeID(strings.ToUpper(newID))
-	if err := a.setupNodeID(cfg); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if id := a.consulConfig().NodeID; string(id) != newID {
-		t.Fatalf("bad: %q vs. %q", id, newID)
-	}
-
-	// Set an invalid ID via the file.
-	fileID := filepath.Join(cfg.DataDir, "node-id")
-	if err := ioutil.WriteFile(fileID, []byte("adf4238a!882b!9ddc!4a9d!5b6758e4159e"), 0600); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	cfg.NodeID = ""
-	err = a.setupNodeID(cfg)
-	if err == nil || !strings.Contains(err.Error(), "uuid is improperly formatted") {
-		t.Fatalf("err: %v", err)
-	}
-
-	// Set a valid ID via the file.
-	if err := ioutil.WriteFile(fileID, []byte("ADF4238a-882b-9ddc-4a9d-5b6758e4159e"), 0600); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	cfg.NodeID = ""
-	if err := a.setupNodeID(cfg); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if id := a.consulConfig().NodeID; string(id) != "adf4238a-882b-9ddc-4a9d-5b6758e4159e" {
-		t.Fatalf("bad: %q vs. %q", id, newID)
-	}
-}
-
-func TestAgent_makeNodeID(t *testing.T) {
-	t.Parallel()
-	a := NewTestAgent(t, `
-		node_id = ""
-	`)
-	defer a.Shutdown()
-
-	// We should get a valid host-based ID initially.
-	id, err := a.makeNodeID()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if _, err := uuid.ParseUUID(id); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-
-	// Calling again should yield a random ID by default.
-	another, err := a.makeNodeID()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if id == another {
-		t.Fatalf("bad: %s vs %s", id, another)
-	}
-
-	// Turn on host-based IDs and try again. We should get the same ID
-	// each time (and a different one from the random one above).
-	a.GetConfig().DisableHostNodeID = false
-	id, err = a.makeNodeID()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if id == another {
-		t.Fatalf("bad: %s vs %s", id, another)
-	}
-
-	// Calling again should yield the host-based ID.
-	another, err = a.makeNodeID()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if id != another {
-		t.Fatalf("bad: %s vs %s", id, another)
-	}
-}
-
 func TestAgent_AddService(t *testing.T) {
 	t.Run("normal", func(t *testing.T) {
 		t.Parallel()

agent/nodeid.go

@@ -0,0 +1,95 @@
+package agent
+
+import (
+	"crypto/sha512"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
+
+	"github.com/hashicorp/consul/agent/config"
+	"github.com/hashicorp/consul/lib"
+	"github.com/hashicorp/consul/types"
+	"github.com/hashicorp/go-hclog"
+	"github.com/hashicorp/go-uuid"
+	"github.com/shirou/gopsutil/host"
+)
+
+// newNodeIDFromConfig will pull the persisted node ID, if any, or create a random one
+// and persist it.
+func newNodeIDFromConfig(config *config.RuntimeConfig, logger hclog.Logger) (types.NodeID, error) {
+	if config.NodeID != "" {
+		nodeID := strings.ToLower(string(config.NodeID))
+		if _, err := uuid.ParseUUID(nodeID); err != nil {
+			return "", fmt.Errorf("specified NodeID is invalid: %w", err)
+		}
+		return types.NodeID(nodeID), nil
+	}
+
+	// For dev mode we have no filesystem access so just make one.
+	if config.DataDir == "" {
+		id, err := makeNodeID(logger, config.DisableHostNodeID)
+		return types.NodeID(id), err
+	}
+
+	// Load saved state, if any. Since a user could edit this, we also validate it.
+	filename := filepath.Join(config.DataDir, "node-id")
+	if _, err := os.Stat(filename); err == nil {
+		rawID, err := ioutil.ReadFile(filename)
+		if err != nil {
+			return "", err
+		}
+
+		nodeID := strings.TrimSpace(string(rawID))
+		nodeID = strings.ToLower(nodeID)
+		if _, err = uuid.ParseUUID(nodeID); err != nil {
+			return "", fmt.Errorf("NodeID in %s is invalid: %w", filename, err)
+		}
+		return types.NodeID(nodeID), nil
+	}
+
+	id, err := makeNodeID(logger, config.DisableHostNodeID)
+	if err != nil {
+		return "", fmt.Errorf("failed to create a NodeID: %w", err)
+	}
+	if err := lib.EnsurePath(filename, false); err != nil {
+		return "", err
+	}
+	if err := ioutil.WriteFile(filename, []byte(id), 0600); err != nil {
+		return "", fmt.Errorf("failed to write NodeID to disk: %w", err)
+	}
+	return types.NodeID(id), nil
+}
+
+// makeNodeID will try to find a host-specific ID, or else will generate a
+// random ID. The returned ID will always be formatted as a GUID. We don't tell
+// the caller whether this ID is random or stable since the consequences are
+// high for us if this changes, so we will persist it either way. This will let
+// gopsutil change implementations without affecting in-place upgrades of nodes.
+func makeNodeID(logger hclog.Logger, disableHostNodeID bool) (string, error) {
+	if disableHostNodeID {
+		return uuid.GenerateUUID()
+	}
+
+	// Try to get a stable ID associated with the host itself.
+	info, err := host.Info()
+	if err != nil {
+		logger.Debug("Couldn't get a unique ID from the host", "error", err)
+		return uuid.GenerateUUID()
+	}
+
+	// Hash the input to make it well distributed. The reported Host UUID may be
+	// similar across nodes if they are on a cloud provider or on motherboards
+	// created from the same batch.
+	buf := sha512.Sum512([]byte(strings.ToLower(info.HostID)))
+	id := fmt.Sprintf("%08x-%04x-%04x-%04x-%12x",
+		buf[0:4],
+		buf[4:6],
+		buf[6:8],
+		buf[8:10],
+		buf[10:16])
+
+	logger.Debug("Using unique ID from host as node ID", "id", id)
+	return id, nil
+}

agent/nodeid_test.go

@@ -0,0 +1,112 @@
+package agent
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/hashicorp/consul/agent/config"
+	"github.com/hashicorp/consul/sdk/testutil"
+	"github.com/hashicorp/consul/types"
+	"github.com/hashicorp/go-hclog"
+	"github.com/hashicorp/go-uuid"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewNodeIDFromConfig(t *testing.T) {
+	logger := hclog.New(nil)
+	tmpDir := testutil.TempDir(t, "")
+	t.Cleanup(func() {
+		os.RemoveAll(tmpDir)
+	})
+	cfg := &config.RuntimeConfig{
+		DataDir: tmpDir,
+	}
+
+	var randomNodeID types.NodeID
+	t.Run("a new ID is generated when none is specified", func(t *testing.T) {
+		var err error
+		randomNodeID, err = newNodeIDFromConfig(cfg, logger)
+		require.NoError(t, err)
+
+		_, err = uuid.ParseUUID(string(randomNodeID))
+		require.NoError(t, err)
+	})
+
+	t.Run("running again should get the NodeID that was persisted to disk", func(t *testing.T) {
+		nodeID, err := newNodeIDFromConfig(cfg, logger)
+		require.NoError(t, err)
+		require.NotEqual(t, nodeID, "")
+		require.Equal(t, nodeID, randomNodeID)
+	})
+
+	t.Run("invalid NodeID in config", func(t *testing.T) {
+		cfg.NodeID = "nope"
+		_, err := newNodeIDFromConfig(cfg, logger)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "specified NodeID is invalid")
+	})
+
+	t.Run("valid NodeID in config", func(t *testing.T) {
+		newID, err := uuid.GenerateUUID()
+		require.NoError(t, err)
+
+		cfg.NodeID = types.NodeID(strings.ToUpper(newID))
+		nodeID, err := newNodeIDFromConfig(cfg, logger)
+		require.NoError(t, err)
+		require.Equal(t, string(nodeID), newID)
+	})
+
+	t.Run("invalid NodeID in file", func(t *testing.T) {
+		cfg.NodeID = ""
+		filename := filepath.Join(cfg.DataDir, "node-id")
+		err := ioutil.WriteFile(filename, []byte("adf4238a!882b!9ddc!4a9d!5b6758e4159e"), 0600)
+		require.NoError(t, err)
+
+		_, err = newNodeIDFromConfig(cfg, logger)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), fmt.Sprintf("NodeID in %s is invalid", filename))
+	})
+
+	t.Run("valid NodeID in file", func(t *testing.T) {
+		cfg.NodeID = ""
+		filename := filepath.Join(cfg.DataDir, "node-id")
+		err := ioutil.WriteFile(filename, []byte("ADF4238a-882b-9ddc-4a9d-5b6758e4159e"), 0600)
+		require.NoError(t, err)
+
+		nodeID, err := newNodeIDFromConfig(cfg, logger)
+		require.NoError(t, err)
+		require.Equal(t, string(nodeID), "adf4238a-882b-9ddc-4a9d-5b6758e4159e")
+	})
+}
+
+func TestMakeNodeID(t *testing.T) {
+	logger := hclog.New(nil)
+
+	var randomID string
+	t.Run("Random ID when HostNodeID is disabled", func(t *testing.T) {
+		var err error
+		randomID, err = makeNodeID(logger, true)
+		require.NoError(t, err)
+
+		_, err = uuid.ParseUUID(randomID)
+		require.NoError(t, err)
+
+		another, err := makeNodeID(logger, true)
+		require.NoError(t, err)
+		require.NotEqual(t, randomID, another)
+	})
+
+	t.Run("host-based ID when HostNodeID is enabled", func(t *testing.T) {
+		id, err := makeNodeID(logger, false)
+		require.NoError(t, err)
+		require.NotEqual(t, randomID, id)
+
+		another, err := makeNodeID(logger, false)
+		require.NoError(t, err)
+		require.Equal(t, id, another)
+	})
+}