policy: improve target block policy parsing and fix panics.

policystorage/nomad.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strings"
 
+	multierror "github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/nomad-autoscaler/plugins"
 	"github.com/hashicorp/nomad/api"
 )
@@ -43,6 +44,12 @@ func (n *Nomad) Get(ID string) (*Policy, error) {
 	if fromPolicy.Policy["source"] == nil {
 		fromPolicy.Policy["source"] = ""
 	}
+
+	target, err := parseTarget(fromPolicy.Policy["target"])
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse policy target: %v", err)
+	}
+
 	toPolicy := &Policy{
 		ID:       fromPolicy.ID,
 		Min:      *fromPolicy.Min,
@@ -51,7 +58,7 @@ func (n *Nomad) Get(ID string) (*Policy, error) {
 		Query:    fromPolicy.Policy["query"].(string),
 		Enabled:  *fromPolicy.Enabled,
 		Strategy: parseStrategy(fromPolicy.Policy["strategy"]),
-		Target:   parseTarget(fromPolicy.Policy["target"]),
+		Target:   target,
 	}
 	canonicalize(fromPolicy, toPolicy)
 	return toPolicy, nil
@@ -123,26 +130,111 @@ func parseStrategy(s interface{}) *Strategy {
 	}
 }
 
-func parseTarget(t interface{}) *Target {
-	if t == nil {
-		return &Target{}
+// parseTarget is used to process the target block from within a scaling
+// policy.
+func parseTarget(target interface{}) (*Target, error) {
+
+	// The Autoscaler policy allows for a non-defined target which means we
+	// default to Nomad.
+	if target == nil {
+		return &Target{}, nil
 	}
 
-	targetMap := t.([]interface{})[0].(map[string]interface{})
-	if targetMap == nil {
-		return &Target{}
+	targetMap, err := parseGenericBlock(target)
+	if err != nil {
+		return nil, fmt.Errorf("target %v", err)
 	}
 
-	var configMapString map[string]string
-	if targetMap["config"] != nil {
-		configMap := targetMap["config"].([]interface{})[0].(map[string]interface{})
-		configMapString = make(map[string]string)
-		for k, v := range configMap {
-			configMapString[k] = fmt.Sprintf("%v", v)
+	targetCfg, err := parseConfig(targetMap["config"])
+	if err != nil {
+		return nil, fmt.Errorf("target %v", err)
+	}
+
+	var name string
+
+	// Parse the name parameter of the target configuration. If we do not find
+	// a map entry the name can be set to an empty string. This means we will
+	// default to the Nomad target. If we find the map entry, but are unable to
+	// convert this to a string, pass an error back to the caller. This
+	// indicates the user has attempted to configure a name, but made a
+	// mistake. In this situation we should let them know, rather than apply a
+	// default.
+	nameInterface, ok := targetMap["name"]
+	if ok {
+		if name, ok = nameInterface.(string); !ok {
+			return nil, fmt.Errorf("target name is %T not string", nameInterface)
 		}
 	}
+
 	return &Target{
-		Name:   targetMap["name"].(string),
-		Config: configMapString,
+		Name:   name,
+		Config: targetCfg,
+	}, nil
+}
+
+// parseConfig processes the config block from within a scaling target or
+// strategy scaling block. It safely unpacks the decoded object, iterating all
+// provided config keys.
+func parseConfig(config interface{}) (map[string]string, error) {
+
+	// Define our output map.
+	cfg := make(map[string]string)
+
+	// Protect against nil config and return quickly.
+	if config == nil {
+		return cfg, nil
 	}
+
+	configMap, err := parseGenericBlock(config)
+	if err != nil {
+		return nil, fmt.Errorf("config %v", err)
+	}
+
+	// Use multierror so we gather all errors from iterating the config map in
+	// a single pass.
+	var errResult *multierror.Error
+
+	for k, v := range configMap {
+
+		// Assert that the config value is a string. If we are unable to do
+		// this add an error to the multierror list and move to the next item.
+		stringVal, ok := v.(string)
+		if !ok {
+			errResult = multierror.Append(errResult,
+				fmt.Errorf("config key %s value is %T not string", k, v))
+			continue
+		}
+
+		// Update the config mapping with our successfully asserted string key
+		// and value.
+		cfg[k] = stringVal
+	}
+
+	// Return the config and any errors. ErrorOrNil() performs a nil check so
+	// this is safe to call.
+	return cfg, errResult.ErrorOrNil()
+}
+
+// parseGenericBlock is a helper function to assert the decoded HCL block type
+// and return the map[string]interface{} configured object. The function does
+// not protect against nil inputs, this should be done by the caller and
+// appropriate action taken there.
+func parseGenericBlock(input interface{}) (map[string]interface{}, error) {
+
+	// Validate the input. This is opaque to Nomad on job registration so it is
+	// prone to mistakes and misconfiguration.
+	listInput, ok := input.([]interface{})
+	if !ok {
+		return nil, fmt.Errorf("block is %T not interface slice", input)
+	}
+
+	// HCL decodes the block as a slice of interfaces so check the type on the
+	// first item. As described above, this is opaque to Nomad so does not go
+	// through validation on job registration.
+	mapInput, ok := listInput[0].(map[string]interface{})
+	if !ok {
+		return nil, fmt.Errorf("block first item is %T not map", listInput[0])
+	}
+
+	return mapInput, nil
 }

policystorage/nomad_test.go

@@ -5,7 +5,9 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/nomad/api"
+	"github.com/stretchr/testify/assert"
 )
 
 func Test_storagNomad_Validate(t *testing.T) {
@@ -15,11 +17,11 @@ func Test_storagNomad_Validate(t *testing.T) {
 		want   []error
 	}{
 		{
-			name:   "validate missing",
+			name: "validate missing",
 			policy: &api.ScalingPolicy{
 				Policy: map[string]interface{}{},
 			},
-			want:   []error{fmt.Errorf("Policy.strategy (<nil>) is not a []interface{}")},
+			want: []error{fmt.Errorf("Policy.strategy (<nil>) is not a []interface{}")},
 		},
 	}
 	for _, tt := range tests {
@@ -29,4 +31,152 @@ func Test_storagNomad_Validate(t *testing.T) {
 			}
 		})
 	}
-}
+}
+
+func Test_parseTarget(t *testing.T) {
+	testCases := []struct {
+		inputTarget    interface{}
+		expectedOutput *Target
+		expectedError  error
+		name           string
+	}{
+		{
+			inputTarget:    nil,
+			expectedOutput: &Target{},
+			expectedError:  nil,
+			name:           "input target is nil",
+		},
+		{
+			inputTarget:    []interface{}{map[string]interface{}{}},
+			expectedOutput: &Target{Config: map[string]string{}},
+			expectedError:  nil,
+			name:           "input target is empty slice of maps",
+		},
+		{
+			inputTarget: []interface{}{map[string]interface{}{
+				"config": []interface{}{map[string]interface{}{"dry-run": "true"}},
+			}},
+			expectedOutput: &Target{Config: map[string]string{"dry-run": "true"}},
+			expectedError:  nil,
+			name:           "input target with config but without name",
+		},
+		{
+			inputTarget: []interface{}{map[string]interface{}{
+				"name": map[string]string{"foo": "bar"},
+			}},
+			expectedOutput: nil,
+			expectedError:  fmt.Errorf("target name is map[string]string not string"),
+			name:           "input target with name of the wrong type",
+		},
+		{
+			inputTarget:    "foo",
+			expectedOutput: nil,
+			expectedError:  fmt.Errorf("target block is string not interface slice"),
+			name:           "input target with wrong type",
+		},
+
+		{
+			inputTarget:    []interface{}{[]string{}},
+			expectedOutput: nil,
+			expectedError:  fmt.Errorf("target block first item is []string not map"),
+			name:           "input target interface slice is wrong type",
+		},
+	}
+
+	for _, tc := range testCases {
+		actualOutput, actualError := parseTarget(tc.inputTarget)
+		assert.Equal(t, tc.expectedError, actualError, tc.name)
+		assert.Equal(t, tc.expectedOutput, actualOutput, tc.name)
+	}
+}
+
+func Test_parseConfig(t *testing.T) {
+	testCases := []struct {
+		inputConfig    interface{}
+		expectedOutput map[string]string
+		expectedError  error
+		name           string
+	}{
+		{
+			inputConfig:    nil,
+			expectedOutput: map[string]string{},
+			expectedError:  nil,
+			name:           "input config is nil",
+		},
+		{
+			inputConfig:    "foo",
+			expectedOutput: nil,
+			expectedError:  fmt.Errorf("config block is string not interface slice"),
+			name:           "input config with wrong type",
+		},
+		{
+			inputConfig:    []interface{}{[]string{}},
+			expectedOutput: nil,
+			expectedError:  fmt.Errorf("config block first item is []string not map"),
+			name:           "input config interface slice is wrong type",
+		},
+		{
+			inputConfig: []interface{}{map[string]interface{}{
+				"dry-run": []string{"oh-no", "this-is-wrong"},
+				"feature": []int{13, 04}},
+			},
+			expectedOutput: map[string]string{},
+			expectedError: &multierror.Error{Errors: []error{
+				fmt.Errorf("config key dry-run value is []string not string"),
+				fmt.Errorf("config key feature value is []int not string")},
+			},
+			name: "input config parsable but incorrect map value types",
+		},
+		{
+			inputConfig: []interface{}{map[string]interface{}{
+				"dry-run": "true",
+				"feature": "fancy-feature-toggle"},
+			},
+			expectedOutput: map[string]string{"dry-run": "true", "feature": "fancy-feature-toggle"},
+			expectedError:  nil,
+			name:           "input config parsable and correctly formatted",
+		},
+	}
+
+	for _, tc := range testCases {
+		actualOutput, actualError := parseConfig(tc.inputConfig)
+		assert.Equal(t, tc.expectedError, actualError, tc.name)
+		assert.Equal(t, tc.expectedOutput, actualOutput, tc.name)
+	}
+}
+
+func Test_parseGenericBlock(t *testing.T) {
+	testCases := []struct {
+		input          interface{}
+		expectedOutput map[string]interface{}
+		expectedError  error
+		name           string
+	}{
+		{
+			input:          "foo",
+			expectedOutput: nil,
+			expectedError:  fmt.Errorf("block is string not interface slice"),
+			name:           "input with wrong type",
+		},
+		{
+			input:          []interface{}{[]string{}},
+			expectedOutput: nil,
+			expectedError:  fmt.Errorf("block first item is []string not map"),
+			name:           "input interface slice is wrong type",
+		},
+		{
+			input: []interface{}{map[string]interface{}{
+				"config": []interface{}{map[string]interface{}{"dry-run": "true"}},
+			}},
+			expectedOutput: map[string]interface{}{"config": []interface{}{map[string]interface{}{"dry-run": "true"}}},
+			expectedError:  nil,
+			name:           "input is parsable and correctly formatted",
+		},
+	}
+
+	for _, tc := range testCases {
+		actualOutput, actualError := parseGenericBlock(tc.input)
+		assert.Equal(t, tc.expectedError, actualError, tc.name)
+		assert.Equal(t, tc.expectedOutput, actualOutput, tc.name)
+	}
+}