Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: Use HookWeakDecodeFromSlice in place of PatchSliceOfMaps #7964

Merged
merged 1 commit into from
Jun 8, 2020
Merged

config: Use HookWeakDecodeFromSlice in place of PatchSliceOfMaps #7964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 12 additions & 65 deletions agent/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down
10 changes: 5 additions & 5 deletions agent/discovery_chain_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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,
dnephin marked this conversation as resolved.
Show resolved Hide resolved
mapstructure.StringToTimeDurationHookFunc(),
),
Expand Down
58 changes: 1 addition & 57 deletions agent/structs/config_entry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
),
Expand Down Expand Up @@ -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 (
Expand Down
28 changes: 25 additions & 3 deletions agent/xds/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
})
Expand Down Expand Up @@ -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,
}
Expand Down
12 changes: 1 addition & 11 deletions command/config/write/config_write.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(),
),
Expand Down
41 changes: 41 additions & 0 deletions lib/decode/decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@dnephin dnephin Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a second I was worried this un-boxing of a single item from a slice would cause problems when a partially opaque config contained slices. I verified it is not a problem because we always use WeaklyTypedInput: true in the Decodeconfig, which will handle turning it back into a slice of a single item when the target is a slice.

I wonder where I can document that. I guess in all the Parse functions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a comment here?

default:
return data, nil
}
default:
return data, nil
}
}
Loading