config: opaque maps like service.proxy.config are mangled by patchSliceOfMaps hack

[banks]

While adding sidecar service changes to service definition in 1.3.0. The curse of our config rube-goldberg machine caused a lot of pain.

In the process I realised that places where we use "opaque" map[string]interface{} in config (including service definitions) for "pass through" config for plugins or proxies are actually subtly broken by the several layers of hacks we currently put config through for historical reasons.

The main one is the patchSliceOfMaps hack:

consul/agent/config/config.go

Lines 50 to 100 in db93606

	// 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 := 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.proxy.upstreams",
	"services.proxy.upstreams",
	// Need all the service(s) exceptions also for nested sidecar service except
	// managed proxy which is explicitly not supported there.
	"service.connect.sidecar_service.checks",
	"services.connect.sidecar_service.checks",
	"service.connect.sidecar_service.proxy.upstreams",
	"services.connect.sidecar_service.proxy.upstreams",
	})

The patch hack is the egregious one here: If a user needs to configure something that contains an array of objects, we will panic!

The issue is that their config path will not be whitelisted as an "allowed" list of objects.

At a minimum it seems we need a second whitelist of places we use map[string]interface{} as an opaque bag of config which is checked in the patch recursion but behaves differently on a match: it stops recursion into those keys.

The problem with that is that then the opaque config will have the same mangling this hack was meant to fix: all structs/objects will become maps of structs since HCL supports repeated key names.

The alternative and probably better option to this whole patch hack for general config would be to update mapstructure so that it when it's given the output of HCL parse into a map[string]interface{} (which is why HCL parses this into an ambiguous form) it uses the same logic that HCL does to marshall that into a target struct. For our general config where the actual config struct describes whether it's meant to be a list of object or a single object, this breaks the ambiguity and would allow us to get rid of the whitelist hack entirely and not have to maintain it for future changes.

But when it comes to "opaque" map[string]interface{} values in config, we still have the same problem - we don't actually know what the original config intent was since we don't know the structure this is meant to represent.

I think the right solution to this would be the above change to mapstructure as well as something like an custom OpaqueConfigMap type as an alias on which we customize the HCL parsing behaviour to just leave it alone or similar. Not 100% sure how that would work though.

[mkeeler]

We are also running into the same issue with centralized configurations.

Example:

mkeeler-corp:~ mkeeler$ cat proxy.hcl
Kind = "proxy-defaults"
Name = "global"
Config {
   nested {
      foo = "bar"
   }
}
mkeeler-corp:~ mkeeler$ consul config write proxy.hcl
mkeeler-corp:~ mkeeler$ consul config read -kind proxy-defaults -name global
{
    "Kind": "proxy-defaults",
    "Name": "global",
    "Config": {
        "nested": [
            {
                "foo": "bar"
            }
        ]
    },
    "CreateIndex": 12,
    "ModifyIndex": 19
}

Basically we are giving the API a nested map and it somehow gets translated to an object with a slice of maps.

[dnephin]

Re: from #7935 (comment)

I think #7964 may have solved this issue, and it can be closed. We can support nested structure and slices of structures in our partially-opaque configuration, and there is no longer a panic.