config: add HookWeakDecodeFromSlice

Currently opaque config blocks (config entries, and CA provider config) are
modified by PatchSliceOfMaps, making it impossible for these opaque
config sections to contain slices of maps.

In order to fix this problem, any lazy-decoding of these blocks needs to support
weak decoding of []map[string]interface{} to a struct type before
PatchSliceOfMaps is replaces. This is necessary because these config
blobs are persisted, and during an upgrade an older version of Consul
could read one of the new configuration values, which would cause an error.

To support the upgrade path, this commit first introduces the new hooks
for weak decoding of []map[string]interface{} and uses them only in the
lazy-decode paths. That way, in a future release, new style
configuration will be supported by the older version of Consul.

This decode hook has a number of advantages:

1. It no longer panics. It allows mapstructure to report the error
2. It no longer requires the user to declare which fields are slices of
   structs. It can deduce that information from the 'to' value.
3. It will make it possible to preserve opaque configuration, allowing
   for structured opaque config.

agent/config/config.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/hashicorp/consul/lib"
 	"github.com/hashicorp/consul/lib/decode"
 	"github.com/hashicorp/hcl"
 	"github.com/mitchellh/mapstructure"
@@ -48,75 +47,23 @@ func Parse(data string, format string) (c Config, md mapstructure.Metadata, err
 		return Config{}, mapstructure.Metadata{}, err
 	}
 
-	// We want to be able to report fields which we cannot map as an
-	// error so that users find typos in their configuration quickly. To
-	// achieve this we use the mapstructure library which maps a a raw
-	// map[string]interface{} to a nested structure and reports unused
-	// fields. The input for a mapstructure.Decode expects a
-	// map[string]interface{} as produced by encoding/json.
-	//
-	// The HCL language allows to repeat map keys which forces it to
-	// store nested structs as []map[string]interface{} instead of
-	// map[string]interface{}. This is an ambiguity which makes the
-	// generated structures incompatible with a corresponding JSON
-	// struct. It also does not work well with the mapstructure library.
-	//
-	// In order to still use the mapstructure library to find unused
-	// fields we patch instances of []map[string]interface{} to a
-	// map[string]interface{} before we decode that into a Config
-	// struct.
-	//
-	// However, Config has some fields which are either
-	// []map[string]interface{} or are arrays of structs which
-	// encoding/json will decode to []map[string]interface{}. Therefore,
-	// we need to be able to specify exceptions for this mapping. The
-	// PatchSliceOfMaps() implements that mapping. All fields of type
-	// []map[string]interface{} are mapped to map[string]interface{} if
-	// it contains at most one value. If there is more than one value it
-	// panics. To define exceptions one can specify the nested field
-	// names in dot notation.
-	//
-	// todo(fs): There might be an easier way to achieve the same thing
-	// todo(fs): but this approach works for now.
-	m := lib.PatchSliceOfMaps(raw, []string{
-		"checks",
-		"segments",
-		"service.checks",
-		"services",
-		"services.checks",
-		"watches",
-		"service.connect.proxy.config.upstreams", // Deprecated
-		"services.connect.proxy.config.upstreams", // Deprecated
-		"service.connect.proxy.upstreams",
-		"services.connect.proxy.upstreams",
-		"service.connect.proxy.expose.paths",
-		"services.connect.proxy.expose.paths",
-		"service.proxy.upstreams",
-		"services.proxy.upstreams",
-		"service.proxy.expose.paths",
-		"services.proxy.expose.paths",
-		"acl.tokens.managed_service_provider",
-
-		// Need all the service(s) exceptions also for nested sidecar service.
-		"service.connect.sidecar_service.checks",
-		"services.connect.sidecar_service.checks",
-		"service.connect.sidecar_service.proxy.upstreams",
-		"services.connect.sidecar_service.proxy.upstreams",
-		"service.connect.sidecar_service.proxy.expose.paths",
-		"services.connect.sidecar_service.proxy.expose.paths",
-	}, []string{
-		"config_entries.bootstrap", // completely ignore this tree (fixed elsewhere)
-	})
-
 	d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
-		DecodeHook: decode.HookTranslateKeys,
-		Metadata:   &md,
-		Result:     &c,
+		DecodeHook: mapstructure.ComposeDecodeHookFunc(
+			// decode.HookWeakDecodeFromSlice is only necessary when reading from
+			// an HCL config file. In the future we could omit it when reading from
+			// JSON configs. It is left here for now to maintain backwards compat
+			// for the unlikely scenario that someone is using malformed JSON configs
+			// and expecting this behaviour to correct their config.
+			decode.HookWeakDecodeFromSlice,
+			decode.HookTranslateKeys,
+		),
+		Metadata: &md,
+		Result:   &c,
 	})
 	if err != nil {
 		return Config{}, mapstructure.Metadata{}, err
 	}
-	if err := d.Decode(m); err != nil {
+	if err := d.Decode(raw); err != nil {
 		return Config{}, mapstructure.Metadata{}, err
 	}
 

agent/discovery_chain_endpoint.go

@@ -8,7 +8,6 @@ import (
 
 	cachetype "github.com/hashicorp/consul/agent/cache-types"
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/lib"
 	"github.com/hashicorp/consul/lib/decode"
 	"github.com/mitchellh/mapstructure"
 )
@@ -102,13 +101,14 @@ type discoveryChainReadResponse struct {
 }
 
 func decodeDiscoveryChainReadRequest(raw map[string]interface{}) (*discoveryChainReadRequest, error) {
-	// lib.TranslateKeys doesn't understand []map[string]interface{} so we have
-	// to do this part first.
-	raw = lib.PatchSliceOfMaps(raw, nil, nil)
-
 	var apiReq discoveryChainReadRequest
+	// TODO(dnephin): at this time only JSON payloads are read, so it is unlikely
+	// that HookWeakDecodeFromSlice is necessary. It was added while porting
+	// from lib.PatchSliceOfMaps to decode.HookWeakDecodeFromSlice. It may be
+	// safe to remove in the future.
 	decodeConf := &mapstructure.DecoderConfig{
 		DecodeHook: mapstructure.ComposeDecodeHookFunc(
+			decode.HookWeakDecodeFromSlice,
 			decode.HookTranslateKeys,
 			mapstructure.StringToTimeDurationHookFunc(),
 		),

agent/structs/config_entry.go

@@ -284,18 +284,10 @@ func DecodeConfigEntry(raw map[string]interface{}) (ConfigEntry, error) {
 		return nil, fmt.Errorf("Kind value in payload is not a string")
 	}
 
-	skipWhenPatching, err := ConfigEntryDecodeRulesForKind(entry.GetKind())
-	if err != nil {
-		return nil, err
-	}
-
-	// lib.TranslateKeys doesn't understand []map[string]interface{} so we have
-	// to do this part first.
-	raw = lib.PatchSliceOfMaps(raw, skipWhenPatching, nil)
-
 	var md mapstructure.Metadata
 	decodeConf := &mapstructure.DecoderConfig{
 		DecodeHook: mapstructure.ComposeDecodeHookFunc(
+			decode.HookWeakDecodeFromSlice,
 			decode.HookTranslateKeys,
 			mapstructure.StringToTimeDurationHookFunc(),
 		),
@@ -326,54 +318,6 @@ func DecodeConfigEntry(raw map[string]interface{}) (ConfigEntry, error) {
 	return entry, nil
 }
 
-// ConfigEntryDecodeRulesForKind returns rules for 'fixing' config entry key
-// formats by kind. This is shared between the 'structs' and 'api' variations
-// of config entries.
-func ConfigEntryDecodeRulesForKind(kind string) (skipWhenPatching []string, err error) {
-	switch kind {
-	case ProxyDefaults:
-		return []string{
-			"expose.paths",
-			"Expose.Paths",
-		}, nil
-	case ServiceDefaults:
-		return []string{
-			"expose.paths",
-			"Expose.Paths",
-		}, nil
-	case ServiceRouter:
-		return []string{
-			"routes",
-			"Routes",
-			"routes.match.http.header",
-			"Routes.Match.HTTP.Header",
-			"routes.match.http.query_param",
-			"Routes.Match.HTTP.QueryParam",
-		}, nil
-	case ServiceSplitter:
-		return []string{
-			"splits",
-			"Splits",
-		}, nil
-	case ServiceResolver:
-		return nil, nil
-	case IngressGateway:
-		return []string{
-			"listeners",
-			"Listeners",
-			"listeners.services",
-			"Listeners.Services",
-		}, nil
-	case TerminatingGateway:
-		return []string{
-			"services",
-			"Services",
-		}, nil
-	default:
-		return nil, fmt.Errorf("kind %q should be explicitly handled here", kind)
-	}
-}
-
 type ConfigEntryOp string
 
 const (

agent/xds/config.go

@@ -58,7 +58,22 @@ type ProxyConfig struct {
 // allows caller to choose whether and how to report the error.
 func ParseProxyConfig(m map[string]interface{}) (ProxyConfig, error) {
 	var cfg ProxyConfig
-	err := mapstructure.WeakDecode(m, &cfg)
+	decodeConf := &mapstructure.DecoderConfig{
+		DecodeHook: mapstructure.ComposeDecodeHookFunc(
+			decode.HookWeakDecodeFromSlice,
+			decode.HookTranslateKeys,
+		),
+		Result:           &cfg,
+		WeaklyTypedInput: true,
+	}
+	decoder, err := mapstructure.NewDecoder(decodeConf)
+	if err != nil {
+		return cfg, err
+	}
+	if err := decoder.Decode(m); err != nil {
+		return cfg, err
+	}
+
 	// Set defaults (even if error is returned)
 	if cfg.Protocol == "" {
 		cfg.Protocol = "tcp"
@@ -103,7 +118,10 @@ type GatewayConfig struct {
 func ParseGatewayConfig(m map[string]interface{}) (GatewayConfig, error) {
 	var cfg GatewayConfig
 	d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
-		DecodeHook:       decode.HookTranslateKeys,
+		DecodeHook: mapstructure.ComposeDecodeHookFunc(
+			decode.HookWeakDecodeFromSlice,
+			decode.HookTranslateKeys,
+		),
 		Result:           &cfg,
 		WeaklyTypedInput: true,
 	})
@@ -204,7 +222,11 @@ func (p PassiveHealthCheck) AsOutlierDetection() *envoycluster.OutlierDetection
 func ParseUpstreamConfigNoDefaults(m map[string]interface{}) (UpstreamConfig, error) {
 	var cfg UpstreamConfig
 	config := &mapstructure.DecoderConfig{
-		DecodeHook:       mapstructure.StringToTimeDurationHookFunc(),
+		DecodeHook: mapstructure.ComposeDecodeHookFunc(
+			decode.HookWeakDecodeFromSlice,
+			decode.HookTranslateKeys,
+			mapstructure.StringToTimeDurationHookFunc(),
+		),
 		Result:           &cfg,
 		WeaklyTypedInput: true,
 	}

command/config/write/config_write.go

@@ -5,11 +5,9 @@ import (
 	"fmt"
 	"io"
 
-	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/command/flags"
 	"github.com/hashicorp/consul/command/helpers"
-	"github.com/hashicorp/consul/lib"
 	"github.com/hashicorp/consul/lib/decode"
 	"github.com/hashicorp/go-multierror"
 	"github.com/mitchellh/cli"
@@ -133,18 +131,10 @@ func newDecodeConfigEntry(raw map[string]interface{}) (api.ConfigEntry, error) {
 		return nil, fmt.Errorf("Kind value in payload is not a string")
 	}
 
-	skipWhenPatching, err := structs.ConfigEntryDecodeRulesForKind(entry.GetKind())
-	if err != nil {
-		return nil, err
-	}
-
-	// lib.TranslateKeys doesn't understand []map[string]interface{} so we have
-	// to do this part first.
-	raw = lib.PatchSliceOfMaps(raw, skipWhenPatching, nil)
-
 	var md mapstructure.Metadata
 	decodeConf := &mapstructure.DecoderConfig{
 		DecodeHook: mapstructure.ComposeDecodeHookFunc(
+			decode.HookWeakDecodeFromSlice,
 			decode.HookTranslateKeys,
 			mapstructure.StringToTimeDurationHookFunc(),
 		),

lib/decode/decode.go

@@ -91,3 +91,44 @@ func canonicalFieldKey(field reflect.StructField) string {
 	}
 	return parts[0]
 }
+
+// HookWeakDecodeFromSlice looks for []map[string]interface{} in the source
+// data. If the target is not a slice or array it attempts to unpack 1 item
+// out of the slice. If there are more items the source data is left unmodified,
+// allowing mapstructure to handle and report the decode error caused by
+// mismatched types.
+//
+// If this hook is being used on a "second pass" decode to decode an opaque
+// configuration into a type, the DecodeConfig should set WeaklyTypedInput=true,
+// (or another hook) to convert any scalar values into a slice of one value when
+// the target is a slice. This is necessary because this hook would have converted
+// the initial slices into single values on the first pass.
+//
+// Background
+//
+// HCL allows for repeated blocks which forces it to store structures
+// as []map[string]interface{} instead of map[string]interface{}. This is an
+// ambiguity which makes the generated structures incompatible with the
+// corresponding JSON data.
+//
+// This hook allows config to be read from the HCL format into a raw structure,
+// and later decoded into a strongly typed structure.
+func HookWeakDecodeFromSlice(from, to reflect.Type, data interface{}) (interface{}, error) {
+	if from.Kind() == reflect.Slice && (to.Kind() == reflect.Slice || to.Kind() == reflect.Array) {
+		return data, nil
+	}
+
+	switch d := data.(type) {
+	case []map[string]interface{}:
+		switch {
+		case len(d) == 0:
+			return nil, nil
+		case len(d) == 1:
+			return d[0], nil
+		default:
+			return data, nil
+		}
+	default:
+		return data, nil
+	}
+}

lib/decode/decode_test.go

@@ -4,6 +4,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/hashicorp/hcl"
 	"github.com/mitchellh/mapstructure"
 	"github.com/stretchr/testify/require"
 )
@@ -205,3 +206,69 @@ type nested struct {
 type Item struct {
 	Name string
 }
+
+func TestHookWeakDecodeFromSlice_DoesNotModifySliceTargets(t *testing.T) {
+	source := `
+slice {
+    name = "first"
+}
+slice {
+    name = "second"
+}
+`
+	target := &nested{}
+	err := decodeHCLToMapStructure(source, target)
+	require.NoError(t, err)
+
+	expected := &nested{
+		Slice: []Item{{Name: "first"}, {Name: "second"}},
+	}
+	require.Equal(t, target, expected)
+}
+
+func decodeHCLToMapStructure(source string, target interface{}) error {
+	raw := map[string]interface{}{}
+	err := hcl.Decode(&raw, source)
+	if err != nil {
+		return err
+	}
+
+	md := new(mapstructure.Metadata)
+	decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
+		DecodeHook: HookWeakDecodeFromSlice,
+		Metadata:   md,
+		Result:     target,
+	})
+	return decoder.Decode(&raw)
+}
+
+func TestHookWeakDecodeFromSlice_ErrorsWithMultipleNestedBlocks(t *testing.T) {
+	source := `
+item {
+    name = "first"
+}
+item {
+    name = "second"
+}
+`
+	target := &nested{}
+	err := decodeHCLToMapStructure(source, target)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "'Item' expected a map, got 'slice'")
+}
+
+func TestHookWeakDecodeFromSlice_UnpacksNestedBlocks(t *testing.T) {
+	source := `
+item {
+    name = "first"
+}
+`
+	target := &nested{}
+	err := decodeHCLToMapStructure(source, target)
+	require.NoError(t, err)
+
+	expected := &nested{
+		Item: Item{Name: "first"},
+	}
+	require.Equal(t, target, expected)
+}

lib/patch_hcl.go

@@ -1,91 +0,0 @@
-package lib
-
-import (
-	"fmt"
-	"strings"
-)
-
-func PatchSliceOfMaps(m map[string]interface{}, skip []string, skipTree []string) map[string]interface{} {
-	lowerSkip := make([]string, len(skip))
-	lowerSkipTree := make([]string, len(skipTree))
-
-	for i, val := range skip {
-		lowerSkip[i] = strings.ToLower(val)
-	}
-
-	for i, val := range skipTree {
-		lowerSkipTree[i] = strings.ToLower(val)
-	}
-
-	return patchValue("", m, lowerSkip, lowerSkipTree).(map[string]interface{})
-}
-
-func patchValue(name string, v interface{}, skip []string, skipTree []string) interface{} {
-	switch x := v.(type) {
-	case map[string]interface{}:
-		if len(x) == 0 {
-			return x
-		}
-		mm := make(map[string]interface{})
-		for k, v := range x {
-			key := k
-			if name != "" {
-				key = name + "." + k
-			}
-			mm[k] = patchValue(key, v, skip, skipTree)
-		}
-		return mm
-
-	case []interface{}:
-		if len(x) == 0 {
-			return nil
-		}
-		if strSliceContains(name, skipTree) {
-			return x
-		}
-		if strSliceContains(name, skip) {
-			for i, y := range x {
-				x[i] = patchValue(name, y, skip, skipTree)
-			}
-			return x
-		}
-		if _, ok := x[0].(map[string]interface{}); !ok {
-			return x
-		}
-		if len(x) > 1 {
-			panic(fmt.Sprintf("%s: []map[string]interface{} with more than one element not supported: %s", name, v))
-		}
-		return patchValue(name, x[0], skip, skipTree)
-
-	case []map[string]interface{}:
-		if len(x) == 0 {
-			return nil
-		}
-		if strSliceContains(name, skipTree) {
-			return x
-		}
-		if strSliceContains(name, skip) {
-			for i, y := range x {
-				x[i] = patchValue(name, y, skip, skipTree).(map[string]interface{})
-			}
-			return x
-		}
-		if len(x) > 1 {
-			panic(fmt.Sprintf("%s: []map[string]interface{} with more than one element not supported: %s", name, v))
-		}
-		return patchValue(name, x[0], skip, skipTree)
-
-	default:
-		return v
-	}
-}
-
-func strSliceContains(s string, v []string) bool {
-	lower := strings.ToLower(s)
-	for _, vv := range v {
-		if lower == vv {
-			return true
-		}
-	}
-	return false
-}

lib/patch_hcl_test.go

@@ -1,123 +0,0 @@
-package lib
-
-import (
-	"encoding/json"
-	"fmt"
-	"reflect"
-	"testing"
-)
-
-func parse(s string) map[string]interface{} {
-	var m map[string]interface{}
-	if err := json.Unmarshal([]byte(s), &m); err != nil {
-		panic(s + ":" + err.Error())
-	}
-	return m
-}
-
-func TestPatchSliceOfMaps(t *testing.T) {
-	tests := []struct {
-		in, out  string
-		skip     []string
-		skipTree []string
-	}{
-		{
-			in:  `{"a":{"b":"c"}}`,
-			out: `{"a":{"b":"c"}}`,
-		},
-		{
-			in:  `{"a":[{"b":"c"}]}`,
-			out: `{"a":{"b":"c"}}`,
-		},
-		{
-			in:  `{"a":[{"b":[{"c":"d"}]}]}`,
-			out: `{"a":{"b":{"c":"d"}}}`,
-		},
-		{
-			in:   `{"a":[{"b":"c"}]}`,
-			out:  `{"a":[{"b":"c"}]}`,
-			skip: []string{"a"},
-		},
-		{
-			in: `{
-				"Services": [
-					{
-						"checks": [
-							{
-								"header": [
-									{"a":"b"}
-								]
-							}
-						]
-					}
-				]
-			}`,
-			out: `{
-				"Services": [
-					{
-						"checks": [
-							{
-								"header": {"a":"b"}
-							}
-						]
-					}
-				]
-			}`,
-			skip: []string{"services", "services.checks"},
-		},
-		{
-			// inspired by the 'config_entries.bootstrap.*' structure for configs
-			in: `
-			{
-				"a": [
-					{
-						"b": [
-							{
-								"c": "val1",
-								"d": {
-									"foo": "bar"
-								},
-								"e": [
-									{
-										"super": "duper"
-									}
-								]
-							}
-						]
-					}
-				]
-			}
-			`,
-			out: `
-			{
-				"a": {
-					"b": [
-						{
-							"c": "val1",
-							"d": {
-								"foo": "bar"
-							},
-							"e": [
-								{
-									"super": "duper"
-								}
-							]
-						}
-					]
-				}
-			}
-			`,
-			skipTree: []string{"a.b"},
-		},
-	}
-
-	for i, tt := range tests {
-		desc := fmt.Sprintf("%02d: %s -> %s skip: %v", i, tt.in, tt.out, tt.skip)
-		t.Run(desc, func(t *testing.T) {
-			out := PatchSliceOfMaps(parse(tt.in), tt.skip, tt.skipTree)
-			if got, want := out, parse(tt.out); !reflect.DeepEqual(got, want) {
-				t.Fatalf("\ngot  %#v\nwant %#v", got, want)
-			}
-		})
-	}
-}