hashicorp / hashicorp/consul

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

Open
#4,971 2 comments 0 reactions 0 assignees View on GitHub
type/bug
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
2d 6h
Merged PRs (30d)
43

Description

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: https://github.com/hashicorp/consul/blob/db936067d2691d76a04a870d0e315b5c9781109e/agent/config/config.go#L50-L100

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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.