From 23e6750f2780db7e5bfe448da424797934878809 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 27 May 2020 13:25:35 -0400 Subject: [PATCH] config: Use HookWeakDecodeFromSlice in place of PatchSliceOfMaps --- agent/config/config.go | 72 ++-------------- 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 +-- lib/patch_hcl.go | 91 -------------------- lib/patch_hcl_test.go | 123 --------------------------- 8 files changed, 25 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 6d1da49f2459..8ec9a58bcc87 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,18 @@ 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, // TODO: only apply when format is hcl + 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 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 b576c406a5c9..4dcc6a195c49 100644 --- a/agent/xds/config.go +++ b/agent/xds/config.go @@ -118,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, }) @@ -219,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 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/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) - } - }) - } -}