From 7b99d9a25deba3bef735150b04acf53dcd57f299 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 27 May 2020 13:02:22 -0400 Subject: [PATCH] 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 | 77 +++-------------- agent/discovery_chain_endpoint.go | 10 +-- agent/structs/config_entry.go | 58 +------------ agent/xds/config.go | 28 +++++- command/config/write/config_write.go | 12 +-- lib/decode/decode.go | 41 +++++++++ lib/decode/decode_test.go | 67 +++++++++++++++ lib/patch_hcl.go | 91 -------------------- lib/patch_hcl_test.go | 123 --------------------------- 9 files changed, 152 insertions(+), 355 deletions(-) delete mode 100644 lib/patch_hcl.go delete mode 100644 lib/patch_hcl_test.go diff --git a/agent/config/config.go b/agent/config/config.go index 6d1da49f2..165594d6f 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 01bdf81eb..3f7e55cfc 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 0cacafef3..922076a82 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 514ebd787..4dcc6a195 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 81deeb89e..8691ab8ea 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 f6b81911d..b098e4313 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 e70144547..cc2ca1bfb 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 bd4431421..000000000 --- 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 f9ce78b2d..000000000 --- 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) - } - }) - } -}