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.
pull/8463/head
Daniel Nephin 2020-08-07 19:24:51 -04:00
parent 0738eb8596
commit 875d8bde42
3 changed files with 126 additions and 164 deletions

View File

@ -510,8 +510,9 @@ func New(options ...AgentOption) (*Agent, error) {
// Retrieve or generate the node ID before setting up the rest of the // Retrieve or generate the node ID before setting up the rest of the
// agent, which depends on it. // 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 // We used to do this in the Start method. However it doesn't need to go

View File

@ -11,70 +11,55 @@ import (
"github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/config"
"github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/lib"
"github.com/hashicorp/consul/types" "github.com/hashicorp/consul/types"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-uuid" "github.com/hashicorp/go-uuid"
"github.com/shirou/gopsutil/host" "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. // and persist it.
// FIXME: move to a different file. func newNodeIDFromConfig(config *config.RuntimeConfig, logger hclog.Logger) (types.NodeID, error) {
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 != "" { if config.NodeID != "" {
config.NodeID = types.NodeID(strings.ToLower(string(config.NodeID))) nodeID := strings.ToLower(string(config.NodeID))
if _, err := uuid.ParseUUID(string(config.NodeID)); err != nil { if _, err := uuid.ParseUUID(nodeID); err != nil {
return err return "", fmt.Errorf("specified NodeID is invalid: %w", err)
} }
return types.NodeID(nodeID), nil
return nil
} }
// For dev mode we have no filesystem access so just make one. // For dev mode we have no filesystem access so just make one.
if a.config.DataDir == "" { if config.DataDir == "" {
id, err := a.makeNodeID() id, err := makeNodeID(logger, config.DisableHostNodeID)
if err != nil { return types.NodeID(id), err
return err
}
config.NodeID = types.NodeID(id)
return nil
} }
// Load saved state, if any. Since a user could edit this, we also // Load saved state, if any. Since a user could edit this, we also validate it.
// validate it. filename := filepath.Join(config.DataDir, "node-id")
fileID := filepath.Join(config.DataDir, "node-id") if _, err := os.Stat(filename); err == nil {
if _, err := os.Stat(fileID); err == nil { rawID, err := ioutil.ReadFile(filename)
rawID, err := ioutil.ReadFile(fileID)
if err != nil { if err != nil {
return err return "", err
} }
nodeID := strings.TrimSpace(string(rawID)) nodeID := strings.TrimSpace(string(rawID))
nodeID = strings.ToLower(nodeID) nodeID = strings.ToLower(nodeID)
if _, err := uuid.ParseUUID(nodeID); err != nil { if _, err = uuid.ParseUUID(nodeID); err != nil {
return err return "", fmt.Errorf("NodeID in %s is invalid: %w", filename, err)
} }
return types.NodeID(nodeID), nil
config.NodeID = types.NodeID(nodeID)
} }
// If we still don't have a valid node ID, make one. id, err := makeNodeID(logger, config.DisableHostNodeID)
if config.NodeID == "" { if err != nil {
id, err := a.makeNodeID() return "", fmt.Errorf("failed to create a NodeID: %w", err)
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 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 // 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 // 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 // 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. // gopsutil change implementations without affecting in-place upgrades of nodes.
func (a *Agent) makeNodeID() (string, error) { func makeNodeID(logger hclog.Logger, disableHostNodeID bool) (string, error) {
// If they've disabled host-based IDs then just make a random one. if disableHostNodeID {
if a.config.DisableHostNodeID { return uuid.GenerateUUID()
return a.makeRandomID()
} }
// Try to get a stable ID associated with the host itself. // Try to get a stable ID associated with the host itself.
info, err := host.Info() info, err := host.Info()
if err != nil { if err != nil {
a.logger.Debug("Couldn't get a unique ID from the host", "error", err) logger.Debug("Couldn't get a unique ID from the host", "error", err)
return a.makeRandomID() return uuid.GenerateUUID()
} }
// Make sure the host ID parses as a UUID, since we don't have complete // Make sure the host ID parses as a UUID, since we don't have complete
// control over this process. // control over this process.
id := strings.ToLower(info.HostID) 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 { 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, "id", id,
"error", err, "error", err,
) )
return a.makeRandomID() return uuid.GenerateUUID()
} }
// Hash the input to make it well distributed. The reported Host UUID may be // 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[8:10],
buf[10:16]) buf[10:16])
a.logger.Debug("Using unique ID from host as node ID", "id", id) 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)
return id, nil return id, nil
} }

View File

@ -1,125 +1,112 @@
package agent package agent
import ( import (
"fmt"
"io/ioutil" "io/ioutil"
"os"
"path/filepath" "path/filepath"
"strings" "strings"
"testing" "testing"
"github.com/hashicorp/consul/agent/config"
"github.com/hashicorp/consul/sdk/testutil"
"github.com/hashicorp/consul/types" "github.com/hashicorp/consul/types"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-uuid" "github.com/hashicorp/go-uuid"
"github.com/stretchr/testify/require"
) )
func TestSetupNodeID(t *testing.T) { func TestNewNodeIDFromConfig(t *testing.T) {
t.Parallel() logger := hclog.New(nil)
a := NewTestAgent(t, ` tmpDir := testutil.TempDir(t, "")
node_id = "" t.Cleanup(func() {
`) os.RemoveAll(tmpDir)
defer a.Shutdown() })
cfg := &config.RuntimeConfig{
cfg := a.config DataDir: tmpDir,
// 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). var randomNodeID types.NodeID
cfg.NodeID = "" t.Run("a new ID is generated when none is specified", func(t *testing.T) {
if err := a.setupNodeID(cfg); err != nil { var err error
t.Fatalf("err: %v", err) randomNodeID, err = newNodeIDFromConfig(cfg, logger)
} require.NoError(t, err)
if newID := a.consulConfig().NodeID; id != newID {
t.Fatalf("bad: %q vs %q", id, newID)
}
// Set an invalid ID via.Config. _, err = uuid.ParseUUID(string(randomNodeID))
cfg.NodeID = types.NodeID("nope") require.NoError(t, err)
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. t.Run("running again should get the NodeID that was persisted to disk", func(t *testing.T) {
newID, err := uuid.GenerateUUID() nodeID, err := newNodeIDFromConfig(cfg, logger)
if err != nil { require.NoError(t, err)
t.Fatalf("err: %v", err) require.NotEqual(t, nodeID, "")
} require.Equal(t, nodeID, randomNodeID)
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. t.Run("invalid NodeID in config", func(t *testing.T) {
fileID := filepath.Join(cfg.DataDir, "node-id") cfg.NodeID = "nope"
if err := ioutil.WriteFile(fileID, []byte("adf4238a!882b!9ddc!4a9d!5b6758e4159e"), 0600); err != nil { _, err := newNodeIDFromConfig(cfg, logger)
t.Fatalf("err: %v", err) require.Error(t, err)
} require.Contains(t, err.Error(), "specified NodeID is invalid")
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. t.Run("valid NodeID in config", func(t *testing.T) {
if err := ioutil.WriteFile(fileID, []byte("ADF4238a-882b-9ddc-4a9d-5b6758e4159e"), 0600); err != nil { newID, err := uuid.GenerateUUID()
t.Fatalf("err: %v", err) require.NoError(t, err)
}
cfg.NodeID = "" cfg.NodeID = types.NodeID(strings.ToUpper(newID))
if err := a.setupNodeID(cfg); err != nil { nodeID, err := newNodeIDFromConfig(cfg, logger)
t.Fatalf("err: %v", err) require.NoError(t, err)
} require.Equal(t, string(nodeID), newID)
if id := a.consulConfig().NodeID; string(id) != "adf4238a-882b-9ddc-4a9d-5b6758e4159e" { })
t.Fatalf("bad: %q vs. %q", id, 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) { func TestMakeNodeID(t *testing.T) {
t.Parallel() logger := hclog.New(nil)
a := NewTestAgent(t, `
node_id = ""
`)
defer a.Shutdown()
// We should get a valid host-based ID initially. var randomID string
id, err := a.makeNodeID() t.Run("Random ID when HostNodeID is disabled", func(t *testing.T) {
if err != nil { var err error
t.Fatalf("err: %v", err) randomID, err = makeNodeID(logger, true)
} require.NoError(t, err)
if _, err := uuid.ParseUUID(id); err != nil {
t.Fatalf("err: %v", err)
}
// Calling again should yield a random ID by default. _, err = uuid.ParseUUID(randomID)
another, err := a.makeNodeID() require.NoError(t, err)
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 another, err := makeNodeID(logger, true)
// each time (and a different one from the random one above). require.NoError(t, err)
a.GetConfig().DisableHostNodeID = false require.NotEqual(t, randomID, another)
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. t.Run("host-based ID when HostNodeID is enabled", func(t *testing.T) {
another, err = a.makeNodeID() id, err := makeNodeID(logger, false)
if err != nil { require.NoError(t, err)
t.Fatalf("err: %v", err) require.NotEqual(t, randomID, id)
}
if id != another { another, err := makeNodeID(logger, false)
t.Fatalf("bad: %s vs %s", id, another) require.NoError(t, err)
} require.Equal(t, id, another)
})
} }