From 8844d8d40f9cd3e448f1b9d0a68b7aac8d5b9b85 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 27 May 2020 13:02:22 -0400 Subject: [PATCH 1/4] 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. --- lib/decode/decode.go | 35 ++++++++++++++++++++ lib/decode/decode_test.go | 67 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) diff --git a/lib/decode/decode.go b/lib/decode/decode.go index f6b81911dc03..ea4d22395dc9 100644 --- a/lib/decode/decode.go +++ b/lib/decode/decode.go @@ -91,3 +91,38 @@ 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. +// +// 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 e70144547150..cc2ca1bfb07d 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) +} From 7e7a67bf1a39056e5cf796eded6c513127a47ad2 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 8 May 2020 17:08:40 -0400 Subject: [PATCH 2/4] Add decode hooks to all lazy-decode parse functions So that if they encounter config accepted by a newer version of Consul they will know how to handle it. --- agent/xds/config.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/agent/xds/config.go b/agent/xds/config.go index d2a54e443a0c..11c1b9ba42b2 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" From 09f5c4878c58dcb0f2b1f07802b82c3fb36a2a68 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 27 May 2020 13:25:35 -0400 Subject: [PATCH 3/4] config: Use HookWeakDecodeFromSlice in place of PatchSliceOfMaps --- agent/config/config.go | 72 ++---------- agent/connect/ca/provider_aws.go | 7 +- agent/connect/ca/provider_consul_config.go | 7 +- agent/connect/ca/provider_vault.go | 7 +- agent/consul/authmethod/authmethods.go | 2 + agent/discovery_chain_endpoint.go | 6 +- agent/structs/config_entry.go | 58 +--------- agent/structs/connect_ca.go | 7 +- agent/xds/config.go | 11 +- command/config/write/config_write.go | 12 +- command/services/config.go | 9 +- lib/patch_hcl.go | 91 --------------- lib/patch_hcl_test.go | 123 --------------------- 13 files changed, 52 insertions(+), 360 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 df5bca3c72e7..08d8689fb6dd 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/go-multierror" "github.com/hashicorp/hcl" @@ -49,76 +48,19 @@ func Parse(data string, format string) (c Config, keys []string, err error) { return Config{}, nil, 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) - }) - var md mapstructure.Metadata d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ - DecodeHook: decode.HookTranslateKeys, - Metadata: &md, - Result: &c, + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookWeakDecodeFromSlice, // TODO: only apply when format is hcl + decode.HookTranslateKeys, + ), + Metadata: &md, + Result: &c, }) if err != nil { return Config{}, nil, err } - if err := d.Decode(m); err != nil { + if err := d.Decode(raw); err != nil { return Config{}, nil, err } diff --git a/agent/connect/ca/provider_aws.go b/agent/connect/ca/provider_aws.go index 1868ac54ec75..aafa64cb613e 100644 --- a/agent/connect/ca/provider_aws.go +++ b/agent/connect/ca/provider_aws.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/acmpca" + "github.com/hashicorp/consul/lib/decode" "github.com/mitchellh/mapstructure" "github.com/hashicorp/consul/agent/connect" @@ -690,7 +691,11 @@ func ParseAWSCAConfig(raw map[string]interface{}) (*structs.AWSCAProviderConfig, } decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: structs.ParseDurationFunc(), + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookWeakDecodeFromSlice, + decode.HookTranslateKeys, + structs.ParseDurationFunc(), + ), Result: &config, WeaklyTypedInput: true, } diff --git a/agent/connect/ca/provider_consul_config.go b/agent/connect/ca/provider_consul_config.go index 48faa0717b20..eccc61fb01e2 100644 --- a/agent/connect/ca/provider_consul_config.go +++ b/agent/connect/ca/provider_consul_config.go @@ -6,13 +6,18 @@ import ( "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/lib/decode" "github.com/mitchellh/mapstructure" ) func ParseConsulCAConfig(raw map[string]interface{}) (*structs.ConsulCAProviderConfig, error) { config := defaultConsulCAProviderConfig() decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: structs.ParseDurationFunc(), + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookWeakDecodeFromSlice, + decode.HookTranslateKeys, + structs.ParseDurationFunc(), + ), Result: &config, WeaklyTypedInput: true, } diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index bb95074d0fce..08c2142b85e1 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/lib/decode" vaultapi "github.com/hashicorp/vault/api" "github.com/mitchellh/mapstructure" ) @@ -412,7 +413,11 @@ func ParseVaultCAConfig(raw map[string]interface{}) (*structs.VaultCAProviderCon } decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: structs.ParseDurationFunc(), + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookWeakDecodeFromSlice, + decode.HookTranslateKeys, + structs.ParseDurationFunc(), + ), Result: &config, WeaklyTypedInput: true, } diff --git a/agent/consul/authmethod/authmethods.go b/agent/consul/authmethod/authmethods.go index 41f791fe43a9..fd89c1fe31a2 100644 --- a/agent/consul/authmethod/authmethods.go +++ b/agent/consul/authmethod/authmethods.go @@ -7,6 +7,7 @@ import ( "sync" "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/lib/decode" "github.com/hashicorp/go-hclog" "github.com/mitchellh/mapstructure" ) @@ -185,6 +186,7 @@ func Types() []string { // ParseConfig parses the config block for a auth method. func ParseConfig(rawConfig map[string]interface{}, out interface{}) error { decodeConf := &mapstructure.DecoderConfig{ + DecodeHook: decode.HookWeakDecodeFromSlice, Result: out, WeaklyTypedInput: true, ErrorUnused: true, diff --git a/agent/discovery_chain_endpoint.go b/agent/discovery_chain_endpoint.go index 01bdf81eb9e0..66fcf1612613 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,10 @@ 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 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 0cacafef381d..922076a82d5b 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/structs/connect_ca.go b/agent/structs/connect_ca.go index 37facbf783e3..380c9d4e13ba 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -6,6 +6,7 @@ import ( "time" "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/lib/decode" "github.com/mitchellh/mapstructure" ) @@ -290,7 +291,11 @@ func (c *CAConfiguration) GetCommonConfig() (*CommonCAProviderConfig, error) { config.CSRMaxPerSecond = 50 // See doc comment for rationale here. decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: ParseDurationFunc(), + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookWeakDecodeFromSlice, + decode.HookTranslateKeys, + ParseDurationFunc(), + ), Result: &config, WeaklyTypedInput: true, } diff --git a/agent/xds/config.go b/agent/xds/config.go index 11c1b9ba42b2..1dee7adda8f0 100644 --- a/agent/xds/config.go +++ b/agent/xds/config.go @@ -114,7 +114,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, }) @@ -212,7 +215,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 81deeb89e36c..8691ab8eaf7c 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/command/services/config.go b/command/services/config.go index ee2477d3c4d5..5c75da6569f8 100644 --- a/command/services/config.go +++ b/command/services/config.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/lib/decode" "github.com/mitchellh/mapstructure" ) @@ -52,8 +53,12 @@ func serviceToAgentService(svc *structs.ServiceDefinition) (*api.AgentServiceReg // helper function in case we need to change the logic in the future. var result api.AgentServiceRegistration d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ - Result: &result, - DecodeHook: timeDurationToStringHookFunc(), + Result: &result, + DecodeHook: mapstructure.ComposeDecodeHookFunc( + decode.HookWeakDecodeFromSlice, + decode.HookTranslateKeys, + timeDurationToStringHookFunc(), + ), WeaklyTypedInput: true, }) if err != nil { diff --git a/lib/patch_hcl.go b/lib/patch_hcl.go deleted file mode 100644 index bd4431421ee8..000000000000 --- 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 f9ce78b2d138..000000000000 --- 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) - } - }) - } -} From e11c27a70b51aaac15d5949d795ec381cd696067 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 22 Apr 2020 13:07:56 -0400 Subject: [PATCH 4/4] config: Do not modify the structure of opaque config HCL and JSON config may have different structures. With this change the opaque configuration will not be modified. When the config is deserialized the same hook is used to handle this difference in structure. --- agent/config/runtime_test.go | 117 ---------------------- agent/connect/ca/provider_consul.go | 12 +++ agent/structs/config_entry_test.go | 4 +- command/config/write/config_write_test.go | 4 +- lib/decode/decode.go | 8 ++ lib/decode/decode_test.go | 38 +++++++ 6 files changed, 62 insertions(+), 121 deletions(-) diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 5081e1913624..9e3509009860 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -3257,120 +3257,6 @@ func TestConfigFlagsAndEdgecases(t *testing.T) { }`}, err: "config_entries.bootstrap[0]: invalid name (\"invalid-name\"), only \"global\" is supported", }, - { - desc: "ConfigEntry bootstrap proxy-defaults (snake-case)", - args: []string{`-data-dir=` + dataDir}, - json: []string{`{ - "config_entries": { - "bootstrap": [ - { - "kind": "proxy-defaults", - "name": "global", - "config": { - "bar": "abc", - "moreconfig": { - "moar": "config" - } - }, - "mesh_gateway": { - "mode": "remote" - } - } - ] - } - }`}, - hcl: []string{` - config_entries { - bootstrap { - kind = "proxy-defaults" - name = "global" - config { - "bar" = "abc" - "moreconfig" { - "moar" = "config" - } - } - mesh_gateway { - mode = "remote" - } - } - }`}, - patch: func(rt *RuntimeConfig) { - rt.DataDir = dataDir - rt.ConfigEntryBootstrap = []structs.ConfigEntry{ - &structs.ProxyConfigEntry{ - Kind: structs.ProxyDefaults, - Name: structs.ProxyConfigGlobal, - Config: map[string]interface{}{ - "bar": "abc", - "moreconfig": map[string]interface{}{ - "moar": "config", - }, - }, - MeshGateway: structs.MeshGatewayConfig{ - Mode: structs.MeshGatewayModeRemote, - }, - }, - } - }, - }, - { - desc: "ConfigEntry bootstrap proxy-defaults (camel-case)", - args: []string{`-data-dir=` + dataDir}, - json: []string{`{ - "config_entries": { - "bootstrap": [ - { - "Kind": "proxy-defaults", - "Name": "global", - "Config": { - "bar": "abc", - "moreconfig": { - "moar": "config" - } - }, - "MeshGateway": { - "Mode": "remote" - } - } - ] - } - }`}, - hcl: []string{` - config_entries { - bootstrap { - Kind = "proxy-defaults" - Name = "global" - Config { - "bar" = "abc" - "moreconfig" { - "moar" = "config" - } - } - MeshGateway { - Mode = "remote" - } - } - }`}, - patch: func(rt *RuntimeConfig) { - rt.DataDir = dataDir - rt.ConfigEntryBootstrap = []structs.ConfigEntry{ - &structs.ProxyConfigEntry{ - Kind: structs.ProxyDefaults, - Name: structs.ProxyConfigGlobal, - Config: map[string]interface{}{ - "bar": "abc", - "moreconfig": map[string]interface{}{ - "moar": "config", - }, - }, - MeshGateway: structs.MeshGatewayConfig{ - Mode: structs.MeshGatewayModeRemote, - }, - }, - } - }, - }, { desc: "ConfigEntry bootstrap service-defaults (snake-case)", args: []string{`-data-dir=` + dataDir}, @@ -3891,9 +3777,6 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) { if tt.patch != nil { tt.patch(&patchedRT) } - // if err := x.Validate(wantRT); err != nil { - // t.Fatalf("validate default failed: %s", err) - // } if got, want := rt, patchedRT; !verify.Values(t, "", got, want) { t.FailNow() } diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index c7482206734d..5a0912618acd 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -687,6 +687,18 @@ func (c *ConsulProvider) parseTestState(rawConfig map[string]interface{}, state return } + // If the state comes from an HCL config it may be wrapped in a slice, + // because that is how HCL decodes blocks. + if ts, ok := rawTestState.([]map[string]interface{}); ok && len(ts) == 1 { + c.testState = make(map[string]string) + for k, v := range ts[0] { + if s, ok := v.(string); ok { + c.testState[k] = s + } + } + return + } + // Secondary's config takes a trip through the state store before Configure // is called and RPC calls that msgpack encode also have the same effect. It // means we end up with map[string]string encoded as map[string]interface{}. diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go index cc1798ee6986..4be59caf5734 100644 --- a/agent/structs/config_entry_test.go +++ b/agent/structs/config_entry_test.go @@ -77,8 +77,8 @@ func TestDecodeConfigEntry(t *testing.T) { Config: map[string]interface{}{ "foo": 19, "bar": "abc", - "moreconfig": map[string]interface{}{ - "moar": "config", + "moreconfig": []map[string]interface{}{ + {"moar": "config"}, }, }, MeshGateway: MeshGatewayConfig{ diff --git a/command/config/write/config_write_test.go b/command/config/write/config_write_test.go index 970709adb7ee..79444fdb7f1c 100644 --- a/command/config/write/config_write_test.go +++ b/command/config/write/config_write_test.go @@ -222,8 +222,8 @@ func TestParseConfigEntry(t *testing.T) { Config: map[string]interface{}{ "foo": 19, "bar": "abc", - "moreconfig": map[string]interface{}{ - "moar": "config", + "moreconfig": []map[string]interface{}{ + {"moar": "config"}, }, }, MeshGateway: api.MeshGatewayConfig{ diff --git a/lib/decode/decode.go b/lib/decode/decode.go index ea4d22395dc9..e70e108c16fd 100644 --- a/lib/decode/decode.go +++ b/lib/decode/decode.go @@ -92,6 +92,8 @@ func canonicalFieldKey(field reflect.StructField) string { return parts[0] } +var typeOfEmptyInterface = reflect.TypeOf((*interface{})(nil)).Elem() + // 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, @@ -112,6 +114,12 @@ func HookWeakDecodeFromSlice(from, to reflect.Type, data interface{}) (interface return data, nil } + // If the target is interface{} then the config is opaque, and it should not + // be modified. + if to == typeOfEmptyInterface { + return data, nil + } + switch d := data.(type) { case []map[string]interface{}: switch { diff --git a/lib/decode/decode_test.go b/lib/decode/decode_test.go index cc2ca1bfb07d..7bd5264a1d84 100644 --- a/lib/decode/decode_test.go +++ b/lib/decode/decode_test.go @@ -272,3 +272,41 @@ item { } require.Equal(t, target, expected) } + +func TestHookNormalizeHCLNestedBlocks_IgnoresOpaqueConfigMaps(t *testing.T) { + source := ` +item { + name = "first" +} +O { + something { + inner = [1, 2, 3, 4] + } + another { + name = "again" + } + another { + name = "last" + } +} +` + target := &nested{} + err := decodeHCLToMapStructure(source, target) + require.NoError(t, err) + + expected := &nested{ + Item: Item{Name: "first"}, + O: raw{ + "something": []raw{ + {"inner": []interface{}{1, 2, 3, 4}}, + }, + "another": []raw{ + {"name": "again"}, + {"name": "last"}, + }, + }, + } + require.Equal(t, expected, target) +} + +type raw = map[string]interface{}