Merge pull request #7964 from hashicorp/dnephin/remove-patch-slice-of-maps-forward-compat

config: Use HookWeakDecodeFromSlice in place of PatchSliceOfMaps

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,
+		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)
-			}
-		})
-	}
-}