diff --git a/agent/config/config.go b/agent/config/config.go index 6d1da49f24..165594d6fd 100644 --- a/agent/config/config.go +++ b/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 } diff --git a/agent/discovery_chain_endpoint.go b/agent/discovery_chain_endpoint.go index 01bdf81eb9..3f7e55cfc6 100644 --- a/agent/discovery_chain_endpoint.go +++ b/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(), ), diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index 0cacafef38..922076a82d 100644 --- a/agent/structs/config_entry.go +++ b/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 ( diff --git a/agent/xds/config.go b/agent/xds/config.go index 514ebd7875..4dcc6a195c 100644 --- a/agent/xds/config.go +++ b/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, } diff --git a/command/config/write/config_write.go b/command/config/write/config_write.go index 81deeb89e3..8691ab8eaf 100644 --- a/command/config/write/config_write.go +++ b/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(), ), diff --git a/lib/decode/decode.go b/lib/decode/decode.go index f6b81911dc..b098e43135 100644 --- a/lib/decode/decode.go +++ b/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 + } +} diff --git a/lib/decode/decode_test.go b/lib/decode/decode_test.go index e701445471..cc2ca1bfb0 100644 --- a/lib/decode/decode_test.go +++ b/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) +} diff --git a/lib/patch_hcl.go b/lib/patch_hcl.go deleted file mode 100644 index bd4431421e..0000000000 --- a/lib/patch_hcl.go +++ /dev/null @@ -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 -} diff --git a/lib/patch_hcl_test.go b/lib/patch_hcl_test.go deleted file mode 100644 index f9ce78b2d1..0000000000 --- a/lib/patch_hcl_test.go +++ /dev/null @@ -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) - } - }) - } -}