Skip to content

Commit

Permalink
code review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@chelseakomlo
chelseakomlo committed Feb 7, 2018
1 parent ca3b28c commit 1a26950
Show file tree
Hide file tree
Showing 4 changed files with 459 additions and 341 deletions.
103 changes: 9 additions & 94 deletions client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ import (
multierror "github.com/hashicorp/go-multierror"
"github.com/hashicorp/nomad/client/allocdir"
"github.com/hashicorp/nomad/client/config"
"github.com/hashicorp/nomad/client/driver"
"github.com/hashicorp/nomad/client/fingerprint"
"github.com/hashicorp/nomad/client/stats"
cstructs "github.com/hashicorp/nomad/client/structs"
"github.com/hashicorp/nomad/client/vaultclient"
Expand Down Expand Up @@ -230,28 +228,14 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic
return nil, fmt.Errorf("node setup failed: %v", err)
}

fingerprintManager := &FingerprintManager{
getConfig: func() *config.Config {
c.configLock.Lock()
defer c.configLock.Unlock()
return c.config
},
node: c.config.Node,
updateNode: c.updateNodeFromFingerprint,
shutdownCh: c.shutdownCh,
logger: c.logger,
}
fingerprintManager := NewFingerprintManager(c.GetConfig, c.config.Node,
c.shutdownCh, c.updateNodeFromFingerprint, c.logger)

// Fingerprint the node
if err := c.fingerprint(fingerprintManager); err != nil {
// Fingerprint the node and scan for drivers
if err := fingerprintManager.Run(); err != nil {
return nil, fmt.Errorf("fingerprinting failed: %v", err)
}

// Scan for drivers
if err := c.setupDrivers(fingerprintManager); err != nil {
return nil, fmt.Errorf("driver setup failed: %v", err)
}

// Setup the reserved resources
c.reservePorts()

Expand Down Expand Up @@ -412,8 +396,10 @@ func (c *Client) Leave() error {
return nil
}

// GetConfig returns the config of the client for testing purposes only
// GetConfig returns the config of the client
func (c *Client) GetConfig() *config.Config {
c.configLock.Lock()
defer c.configLock.Unlock()
return c.config
}

Expand Down Expand Up @@ -936,81 +922,9 @@ func (c *Client) reservePorts() {
}
}

// fingerprint is used to fingerprint the client and setup the node
func (c *Client) fingerprint(fingerprintManager *FingerprintManager) error {
whitelist := c.config.ReadStringListToMap("fingerprint.whitelist")
whitelistEnabled := len(whitelist) > 0
blacklist := c.config.ReadStringListToMap("fingerprint.blacklist")

c.logger.Printf("[DEBUG] client: built-in fingerprints: %v", fingerprint.BuiltinFingerprints())

var availableFingerprints []string
var skippedFingerprints []string
for _, name := range fingerprint.BuiltinFingerprints() {
// Skip modules that are not in the whitelist if it is enabled.
if _, ok := whitelist[name]; whitelistEnabled && !ok {
skippedFingerprints = append(skippedFingerprints, name)
continue
}
// Skip modules that are in the blacklist
if _, ok := blacklist[name]; ok {
skippedFingerprints = append(skippedFingerprints, name)
continue
}

availableFingerprints = append(availableFingerprints, name)
}

if err := fingerprintManager.SetupFingerprinters(availableFingerprints); err != nil {
return err
}

if len(skippedFingerprints) != 0 {
c.logger.Printf("[DEBUG] client: fingerprint modules skipped due to white/blacklist: %v", skippedFingerprints)
}
return nil
}

// setupDrivers is used to find the available drivers
func (c *Client) setupDrivers(fingerprintManager *FingerprintManager) error {
// Build the white/blacklists of drivers.
whitelist := c.config.ReadStringListToMap("driver.whitelist")
whitelistEnabled := len(whitelist) > 0
blacklist := c.config.ReadStringListToMap("driver.blacklist")

var availDrivers []string
var skippedDrivers []string

for name := range driver.BuiltinDrivers {
// Skip fingerprinting drivers that are not in the whitelist if it is
// enabled.
if _, ok := whitelist[name]; whitelistEnabled && !ok {
skippedDrivers = append(skippedDrivers, name)
continue
}
// Skip fingerprinting drivers that are in the blacklist
if _, ok := blacklist[name]; ok {
skippedDrivers = append(skippedDrivers, name)
continue
}

availDrivers = append(availDrivers, name)
}

if err := fingerprintManager.SetupDrivers(availDrivers); err != nil {
return err
}

if len(skippedDrivers) > 0 {
c.logger.Printf("[DEBUG] client: drivers skipped due to white/blacklist: %v", skippedDrivers)
}

return nil
}

// updateNodeFromFingerprint updates the node with the result of
// fingerprinting the node from the diff that was created
func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintResponse) {
func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintResponse) *structs.Node {
c.configLock.Lock()
defer c.configLock.Unlock()
for name, val := range response.Attributes {
Expand All @@ -1034,6 +948,7 @@ func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintRespons
if response.Resources != nil {
c.config.Node.Resources.Merge(response.Resources)
}
return c.config.Node
}

// retryIntv calculates a retry interval value given the base
Expand Down
179 changes: 10 additions & 169 deletions client/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/hashicorp/nomad/testutil"
"github.com/mitchellh/hashstructure"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

ctestutil "github.com/hashicorp/nomad/client/testutil"
)
Expand Down Expand Up @@ -208,17 +209,19 @@ func TestClient_RPC_Passthrough(t *testing.T) {

func TestClient_Fingerprint(t *testing.T) {
t.Parallel()
require := require.New(t)
if _, ok := driver.BuiltinDrivers["mock_driver"]; !ok {
t.Skip(`test requires mock_driver; run with "-tags nomad_test"`)
}

c := testClient(t, nil)
defer c.Shutdown()

// Ensure kernel and arch are always present
// Ensure default values are present
node := c.Node()
if node.Attributes["kernel.name"] == "" {
t.Fatalf("missing kernel.name")
}
if node.Attributes["cpu.arch"] == "" {
t.Fatalf("missing cpu arch")
}
require.NotEqual("", node.Attributes["kernel.name"])
require.NotEqual("", node.Attributes["cpu.arch"])
require.NotEqual("", node.Attributes["driver.mock_driver"])
}

func TestClient_HasNodeChanged(t *testing.T) {
Expand Down Expand Up @@ -295,168 +298,6 @@ func TestClient_Fingerprint_Periodic(t *testing.T) {
})
}

func TestClient_Fingerprint_InWhitelist(t *testing.T) {
t.Parallel()
c := testClient(t, func(c *config.Config) {
if c.Options == nil {
c.Options = make(map[string]string)
}

// Weird spacing to test trimming. Whitelist all modules expect cpu.
c.Options["fingerprint.whitelist"] = " arch, consul,cpu,env_aws,env_gce,host,memory,network,storage,foo,bar "
})
defer c.Shutdown()

node := c.Node()
if node.Attributes["cpu.frequency"] == "" {
t.Fatalf("missing cpu fingerprint module")
}
}

func TestClient_Fingerprint_InBlacklist(t *testing.T) {
t.Parallel()
c := testClient(t, func(c *config.Config) {
if c.Options == nil {
c.Options = make(map[string]string)
}

// Weird spacing to test trimming. Blacklist cpu.
c.Options["fingerprint.blacklist"] = " cpu "
})
defer c.Shutdown()

node := c.Node()
if node.Attributes["cpu.frequency"] != "" {
t.Fatalf("cpu fingerprint module loaded despite blacklisting")
}
}

func TestClient_Fingerprint_OutOfWhitelist(t *testing.T) {
t.Parallel()
c := testClient(t, func(c *config.Config) {
if c.Options == nil {
c.Options = make(map[string]string)
}

c.Options["fingerprint.whitelist"] = "arch,consul,env_aws,env_gce,host,memory,network,storage,foo,bar"
})
defer c.Shutdown()

node := c.Node()
if node.Attributes["cpu.frequency"] != "" {
t.Fatalf("found cpu fingerprint module")
}
}

func TestClient_Fingerprint_WhitelistBlacklistCombination(t *testing.T) {
t.Parallel()
c := testClient(t, func(c *config.Config) {
if c.Options == nil {
c.Options = make(map[string]string)
}

// With both white- and blacklist, should return the set difference of modules (arch, cpu)
c.Options["fingerprint.whitelist"] = "arch,memory,cpu"
c.Options["fingerprint.blacklist"] = "memory,nomad"
})
defer c.Shutdown()

node := c.Node()
// Check expected modules are present
if node.Attributes["cpu.frequency"] == "" {
t.Fatalf("missing cpu fingerprint module")
}
if node.Attributes["cpu.arch"] == "" {
t.Fatalf("missing arch fingerprint module")
}
// Check remainder _not_ present
if node.Attributes["memory.totalbytes"] != "" {
t.Fatalf("found memory fingerprint module")
}
if node.Attributes["nomad.version"] != "" {
t.Fatalf("found nomad fingerprint module")
}
}

func TestClient_Drivers_InWhitelist(t *testing.T) {
t.Parallel()
c := testClient(t, func(c *config.Config) {
if c.Options == nil {
c.Options = make(map[string]string)
}

// Weird spacing to test trimming
c.Options["driver.raw_exec.enable"] = "1"
c.Options["driver.whitelist"] = " raw_exec , foo "
})
defer c.Shutdown()

node := c.Node()
if node.Attributes["driver.raw_exec"] == "" {
t.Fatalf("missing raw_exec driver")
}
}

func TestClient_Drivers_InBlacklist(t *testing.T) {
t.Parallel()
c := testClient(t, func(c *config.Config) {
if c.Options == nil {
c.Options = make(map[string]string)
}

// Weird spacing to test trimming
c.Options["driver.raw_exec.enable"] = "1"
c.Options["driver.blacklist"] = " raw_exec , foo "
})
defer c.Shutdown()

node := c.Node()
if node.Attributes["driver.raw_exec"] != "" {
t.Fatalf("raw_exec driver loaded despite blacklist")
}
}

func TestClient_Drivers_OutOfWhitelist(t *testing.T) {
t.Parallel()
c := testClient(t, func(c *config.Config) {
if c.Options == nil {
c.Options = make(map[string]string)
}

c.Options["driver.whitelist"] = "foo,bar,baz"
})
defer c.Shutdown()

node := c.Node()
if node.Attributes["driver.exec"] != "" {
t.Fatalf("found exec driver")
}
}

func TestClient_Drivers_WhitelistBlacklistCombination(t *testing.T) {
t.Parallel()
c := testClient(t, func(c *config.Config) {
if c.Options == nil {
c.Options = make(map[string]string)
}

// Expected output is set difference (raw_exec)
c.Options["driver.whitelist"] = "raw_exec,exec"
c.Options["driver.blacklist"] = "exec"
})
defer c.Shutdown()

node := c.Node()
// Check expected present
if node.Attributes["driver.raw_exec"] == "" {
t.Fatalf("missing raw_exec driver")
}
// Check expected absent
if node.Attributes["driver.exec"] != "" {
t.Fatalf("exec driver loaded despite blacklist")
}
}

// TestClient_MixedTLS asserts that when a server is running with TLS enabled
// it will reject any RPC connections from clients that lack TLS. See #2525
func TestClient_MixedTLS(t *testing.T) {
Expand Down
Loading

0 comments on commit 1a26950

Please sign in to comment.