Fix 480. (#484)

sdk/config/config.go

@@ -137,7 +137,7 @@ func (loader *Loader) AddSearchPaths(paths ...string) {
 // configuration file is required or not. In cases where it is required and not
 // found, Load will return an error. If the config is optional and not found, no
 // error will be returned.
-func (loader *Loader) Load(pol policy.Policy) error {
+func (loader *Loader) Load(pol policy.Policy) (err error) {
 	log.WithFields(log.Fields{
 		"loader": loader.Name,
 		"paths":  loader.SearchPaths,
@@ -148,23 +148,29 @@ func (loader *Loader) Load(pol policy.Policy) error {
 
 	loader.policy = pol
 
-	if err := loader.checkOverrides(); err != nil {
+	if err = loader.checkOverrides(); err != nil {
 		return err
 	}
 
-	if err := loader.search(pol); err != nil {
+	if err = loader.search(pol); err != nil {
 		return err
 	}
 
-	if err := loader.read(pol); err != nil {
+	if err = loader.read(pol); err != nil {
 		return err
 	}
 
-	if err := loader.loadEnv(); err != nil {
+	if err = loader.loadEnv(); err != nil {
 		return err
 	}
 
-	if err := loader.merge(); err != nil {
+	if err = loader.merge(); err != nil {
+		return err
+	}
+
+	var redacted interface{}
+	redacted, err = utils.RedactPasswords(loader.merged)
+	if err != nil {
 		return err
 	}
 
@@ -174,7 +180,7 @@ func (loader *Loader) Load(pol policy.Policy) error {
 		"name":   loader.FileName,
 		"ext":    loader.Ext,
 		"policy": loader.policy,
-		"data":   utils.RedactPasswords(loader.merged),
+		"data": redacted,
 	}).Info("[config] successfully loaded configuration data")
 	return nil
 }
@@ -421,9 +427,16 @@ func (loader *Loader) read(pol policy.Policy) error {
 				log.WithField("error", err).Error("[config] failed to unmarshal config data")
 				return err
 			}
+
+			var redacted interface{}
+			redacted, err = utils.RedactPasswords(loader.merged)
+			if err != nil {
+				return err
+			}
+
 			log.WithFields(log.Fields{
 				"file": path,
-				"data": utils.RedactPasswords(res),
+				"data": redacted,
 			}).Debug("[config] loaded configuration from file")
 			loader.data = append(loader.data, res)
 		default:

sdk/config/config_test.go

@@ -854,3 +854,16 @@ func TestLoader_Load2(t *testing.T) {
 	assert.Equal(t, 1, d.Foo)
 	assert.Equal(t, 2, d.Bar)
 }
+
+// Sad case tests.
+
+// TestNilInterfaceValue tests an untyped nil interface, which used to cause a panic.
+// This happens on yaml as in ./testdata/nil/nil_interface.yaml. See that file for where.
+// We should get a reasonable error message in this case.
+func TestNilInterfaceValue(t *testing.T) {
+	loader := NewYamlLoader("sad-loader-nil")
+	loader.AddSearchPaths("./testdata/nil")
+	err := loader.Load(policy.Required)
+	assert.Error(t, err)
+	assert.Equal(t, "Missing value after width", err.Error())
+}

sdk/config/plugin.go

@@ -411,13 +411,21 @@ type DynamicRegistrationSettings struct {
 }
 
 // Log logs out the config at INFO level.
-func (conf *DynamicRegistrationSettings) Log() {
+func (conf *DynamicRegistrationSettings) Log() (err error) {
 	if conf == nil {
 		log.Infof("  DynamicRegistration: nil")
 	} else {
+
+		var redacted interface{}
+		redacted, err = utils.RedactPasswords(conf.Config)
+		if err != nil {
+			return err
+		}
+
 		log.Infof("  DynamicRegistration:")
-		log.Infof("    Config: %v", utils.RedactPasswords(conf.Config))
+		log.Infof("    Config: %v", redacted)
 	}
+	return
 }
 
 // HealthSettings are the settings for plugin health.

sdk/config/testdata/nil/nil_interface.yaml

@@ -0,0 +1,53 @@
+version: 3
+devices:
+  - type: current
+    handler: read_only_holding_register
+    data:
+      host: "10.193.12.210"
+      port: 502
+      timeout: 10s
+      failOnError: false
+      type: u16
+      width: 1
+    context:
+      rack: r1
+      model: AC CPM
+    tags:
+      - 'vapor/rack:r1'
+    instances:
+      - info: Infeed L1 Current Low Resolution
+        output: electric-current
+        data:
+          address: 1
+      - info: Infeed L2 Current Low Resolution
+        output: electric-current
+        data:
+          address: 2
+      - info: Infeed L3 Current Low Resolution
+        output: electric-current
+        data:
+          address: 3
+      - info: Infeed Neutral Current Low Resolution
+        output: electric-current
+        data:
+          address: 4
+  - type: macAddressWide
+    handler: read_only_holding_register
+    data:
+      host: "10.193.12.210"
+      port: 502
+      timeout: 10s
+      failOnError: false
+      type: macaddresswide
+      # This is the problem here. This width has an untyped nil interface value.
+      width:
+    context:
+      rack: r1
+      model: AC CPM
+    tags:
+      - 'vapor/rack:r1'
+    instances:
+      - info: LAN Mac Address
+        output: macAddressWide
+        data:
+          address: 5

sdk/utils/redact.go

@@ -17,6 +17,7 @@
 package utils
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 )
@@ -33,23 +34,27 @@ import (
 // This function is likely very inefficient due to the interface casting and
 // the need to copy the lists/maps provided so that it does not overwrite the
 // original list/map.
-func RedactPasswords(obj interface{}) interface{} {
+func RedactPasswords(obj interface{}) (redacted interface{}, err error) {
 	// Wrap the original obj in a reflect.Value
 	original := reflect.ValueOf(obj)
 
 	if !original.IsValid() {
-		return obj
+		return obj, nil
 	}
 
 	// Create a new copy of the object type
 	copied := reflect.New(original.Type()).Elem()
-	redactRecursive(copied, original)
+	var lastMapKey string // The last map key name we have checked.
+	err = redactRecursive(copied, original, lastMapKey)
+	if err != nil {
+		return
+	}
 
 	// Return the value of the copy
-	return copied.Interface()
+	return copied.Interface(), nil
 }
 
-func redactRecursive(copied, original reflect.Value) {
+func redactRecursive(copied, original reflect.Value, lastMapKey string) (err error) {
 	switch original.Kind() {
 
 	// If a pointer, unwrap and call again.
@@ -61,7 +66,10 @@ func redactRecursive(copied, original reflect.Value) {
 		}
 		// Create a new object and set the pointer to it, then recurse.
 		copied.Set(reflect.New(originalValue.Type()))
-		redactRecursive(copied.Elem(), originalValue)
+		err = redactRecursive(copied.Elem(), originalValue, lastMapKey)
+		if err != nil {
+			return
+		}
 
 	// If an interface, unwrap the interface and recurse.
 	case reflect.Interface:
@@ -70,24 +78,35 @@ func redactRecursive(copied, original reflect.Value) {
 		// Create a new object. New gives us a pointer which we don't want,
 		// so call Elem to dereference the pointer.
 		if !originalValue.IsValid() {
-			copied.Set(originalValue)
-		} else {
-			copyValue := reflect.New(originalValue.Type()).Elem()
-			redactRecursive(copyValue, originalValue)
-			copied.Set(copyValue)
+			// This is an untyped nil interface. We cannot set to the originalValue.
+			// See https://github.com/vapor-ware/synse-sdk/issues/480.
+			// Unfortunately we do not have line number and column here,
+			// but hopefully this message is useful.
+			return fmt.Errorf("Missing value after %v", lastMapKey)
 		}
 
+		copyValue := reflect.New(originalValue.Type()).Elem()
+		err = redactRecursive(copyValue, originalValue, lastMapKey)
+		if err != nil {
+			return
+		}
+		copied.Set(copyValue)
+
 	// If a slice, create a new slice and check each element in the slice.
 	case reflect.Slice:
 		copied.Set(reflect.MakeSlice(original.Type(), original.Len(), original.Cap()))
 		for i := 0; i < original.Len(); i++ {
-			redactRecursive(copied.Index(i), original.Index(i))
+			err = redactRecursive(copied.Index(i), original.Index(i), lastMapKey)
+			if err != nil {
+				return
+			}
 		}
 
 	// If a map, create a new map and check each element in the map.
 	case reflect.Map:
 		copied.Set(reflect.MakeMap(original.Type()))
 		for _, key := range original.MapKeys() {
+			lastMapKey = key.Interface().(string) // Save this for the message in case of error.
 			originalValue := original.MapIndex(key)
 
 			// First, check that the key is a string, and if so, that it contains the
@@ -124,13 +143,19 @@ func redactRecursive(copied, original reflect.Value) {
 						copied.SetMapIndex(key, originalValue)
 					} else {
 						copyValue := reflect.New(originalValue.Type()).Elem()
-						redactRecursive(copyValue, originalValue)
+						err = redactRecursive(copyValue, originalValue, lastMapKey)
+						if err != nil {
+							return
+						}
 						copied.SetMapIndex(key, copyValue)
 					}
 				}
 			} else {
 				copyValue := reflect.New(originalValue.Type()).Elem()
-				redactRecursive(copyValue, originalValue)
+				err = redactRecursive(copyValue, originalValue, lastMapKey)
+				if err != nil {
+					return
+				}
 				copied.SetMapIndex(key, copyValue)
 			}
 		}
@@ -139,4 +164,5 @@ func redactRecursive(copied, original reflect.Value) {
 	default:
 		copied.Set(original)
 	}
+	return
 }

sdk/utils/redact_test.go

@@ -189,7 +189,8 @@ func TestRedactPasswords(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			redacted := RedactPasswords(test.input)
+			redacted, err := RedactPasswords(test.input)
+			assert.NoError(t, err)
 			assert.Equal(t, test.expected, redacted)
 		})
 	}
@@ -208,7 +209,8 @@ func TestRedactPasswords_NoMutate_1(t *testing.T) {
 		"pass": "REDACTED",
 	}
 
-	redacted := RedactPasswords(input)
+	redacted, err := RedactPasswords(input)
+	assert.NoError(t, err)
 	assert.Equal(t, expected, redacted)
 
 	// Verify that the input password was not mutated.
@@ -240,7 +242,8 @@ func TestRedactPasswords_NoMutate_2(t *testing.T) {
 		},
 	}
 
-	redacted := RedactPasswords(input)
+	redacted, err := RedactPasswords(input)
+	assert.NoError(t, err)
 	assert.Equal(t, expected, redacted)
 
 	// Verify that the input password was not mutated.
@@ -273,7 +276,8 @@ func TestRedactPasswords_NoMutate_3(t *testing.T) {
 		},
 	}
 
-	redacted := RedactPasswords(input)
+	redacted, err := RedactPasswords(input)
+	assert.NoError(t, err)
 	assert.Equal(t, expected, redacted)
 
 	// Verify that the input password was not mutated.