From 875d8bde425e62a518938cdcb63faaba87d1d646 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 7 Aug 2020 19:24:51 -0400 Subject: [PATCH] agent: convert NodeID methods to functions Making these functions allows us to cleanup how an agent is initialized. They only make use of a config and a logger, so they do not need to be agent methods. Also cleanup the testing to use t.Run and require. --- agent/agent.go | 5 +- agent/nodeid.go | 100 +++++++++-------------- agent/nodeid_test.go | 185 ++++++++++++++++++++----------------------- 3 files changed, 126 insertions(+), 164 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 81f68b9f0f..0382297c0d 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -510,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 { - return nil, fmt.Errorf("Failed to setup node ID: %v", err) + config.NodeID, err = newNodeIDFromConfig(a.config, a.logger) + 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 diff --git a/agent/nodeid.go b/agent/nodeid.go index 06a9edaeda..40cba05d1e 100644 --- a/agent/nodeid.go +++ b/agent/nodeid.go @@ -11,70 +11,55 @@ import ( "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" ) -// setupNodeID will pull the persisted node ID, if any, or create a random one +// newNodeIDFromConfig will pull the persisted node ID, if any, or create a random one // and persist it. -// FIXME: move to a different file. -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. +func newNodeIDFromConfig(config *config.RuntimeConfig, logger hclog.Logger) (types.NodeID, error) { if config.NodeID != "" { - config.NodeID = types.NodeID(strings.ToLower(string(config.NodeID))) - if _, err := uuid.ParseUUID(string(config.NodeID)); err != nil { - return err + nodeID := strings.ToLower(string(config.NodeID)) + if _, err := uuid.ParseUUID(nodeID); err != nil { + return "", fmt.Errorf("specified NodeID is invalid: %w", err) } - - return nil + return types.NodeID(nodeID), 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 + 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. - fileID := filepath.Join(config.DataDir, "node-id") - if _, err := os.Stat(fileID); err == nil { - rawID, err := ioutil.ReadFile(fileID) + // 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 + return "", err } nodeID := strings.TrimSpace(string(rawID)) nodeID = strings.ToLower(nodeID) - if _, err := uuid.ParseUUID(nodeID); err != nil { - return err + if _, err = uuid.ParseUUID(nodeID); err != nil { + return "", fmt.Errorf("NodeID in %s is invalid: %w", filename, err) } - - config.NodeID = types.NodeID(nodeID) + return types.NodeID(nodeID), nil } - // 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) + id, err := makeNodeID(logger, config.DisableHostNodeID) + if err != nil { + return "", fmt.Errorf("failed to create a NodeID: %w", err) } - return nil + 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 @@ -82,28 +67,28 @@ func (a *Agent) setupNodeID(config *config.RuntimeConfig) error { // 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() +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 { - a.logger.Debug("Couldn't get a unique ID from the host", "error", err) - return a.makeRandomID() + logger.Debug("Couldn't get a unique ID from the host", "error", err) + return uuid.GenerateUUID() } // Make sure the host ID parses as a UUID, since we don't have complete // control over this process. id := strings.ToLower(info.HostID) + // TODO: why do we care if HostID is a uuid, if we are about to hash it? if _, err := uuid.ParseUUID(id); err != nil { - a.logger.Debug("Unique ID from host isn't formatted as a UUID", + logger.Debug("Unique ID from host isn't formatted as a UUID", "id", id, "error", err, ) - return a.makeRandomID() + return uuid.GenerateUUID() } // Hash the input to make it well distributed. The reported Host UUID may be @@ -117,17 +102,6 @@ func (a *Agent) makeNodeID() (string, error) { buf[8:10], buf[10:16]) - a.logger.Debug("Using unique ID from host as node ID", "id", id) - return id, 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) + logger.Debug("Using unique ID from host as node ID", "id", id) return id, nil } diff --git a/agent/nodeid_test.go b/agent/nodeid_test.go index aeb45da4f3..6ed2ccfae9 100644 --- a/agent/nodeid_test.go +++ b/agent/nodeid_test.go @@ -1,125 +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 TestSetupNodeID(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) +func TestNewNodeIDFromConfig(t *testing.T) { + logger := hclog.New(nil) + tmpDir := testutil.TempDir(t, "") + t.Cleanup(func() { + os.RemoveAll(tmpDir) + }) + cfg := &config.RuntimeConfig{ + DataDir: tmpDir, } - // 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) - } + 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) - // 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) - } + _, err = uuid.ParseUUID(string(randomNodeID)) + require.NoError(t, 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) - } + 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) + }) - // 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) - } + 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") + }) - // 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) - } + 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) { - t.Parallel() - a := NewTestAgent(t, ` - node_id = "" - `) - defer a.Shutdown() + logger := hclog.New(nil) - // 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) - } + 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) - // 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) - } + _, err = uuid.ParseUUID(randomID) + require.NoError(t, err) - // 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) - } + another, err := makeNodeID(logger, true) + require.NoError(t, err) + require.NotEqual(t, randomID, 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) - } + 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) + }) }