Remove use of "partially opaque config"
- Dominant language
- Go
- Stars
- 30.1k
- Forks
- 4.6k
- Avg merge
- 1d 18h
- Merged PRs (30d)
- 39
Description
"Opaque configuration" is a term we use to refer to `Config map[string]interface{}` fields we use in a few places. The few I know of are:
* `agent/structs.CAConfiguration.Config`
* `agent/structs.Upstream.Config`
* `agent/structs.ConnectProxyConfig.Config`
In all cases, Consul expects specific values for these fields, but instead of decoding directly into a specific type, we decode into a raw `map[string]interface{}`. Later on, we do a second pass and decode the `map[string]interface{}` into a proper type (ex: `UpstreamConfig`).
This causes a bunch of problems:
* If we round trip this opaque config through some other encoding (ex: `msgpack`) we lose all the type information, so `msgpack` may change the types (see related issues for bugs this has caused)
* It is a poor UX, because we can't warn the user if they've typed something incorrectly. Since we allow any values in this opaque config, if a user has a typo or the wrong structure, we have no way of warning them about the problem. Consul proceeds as if the config was never set.
* It makes code much more difficult to read. Instead of seeing the whole structure in one place, the reader has to look for every reference to the opaque config field, and see how it is decoded later.
* It makes code more difficult to write and reason about. In a bunch of places we skip the secondary decoding and inspect or modify the `map[string]interface{}` directly.
### Proposal
We already have concrete types for each of these fields. We should use that instead. We'll have to test backwards compat with all of our different encoding methods (json, hcl, msgpack, and mapstructure).
To continue to support opaque config for other proxies we can use a separate field, so that the config is truely opaque, not partially opaque to some of Consul (and not other parts). We should never expect opaque configuration to be round-trippable through any encoding. The opaque config field should be of type `interface{}` or ideally `[]byte`, so that it is clearly fully opaque and should not be modified. We can test how msgpack handles each of those field types, to be sure it won't modify the values.
### Related issues
* #12237 - confirmed
* #11637 - unconfirmed, but likely given that the problem is in the right field
Contributor guide
Research direction
Start with agent/structs.CAConfiguration.Config, agent/structs.Upstream.Config, and agent/structs.ConnectProxyConfig.Config. Trace their secondary decoding and inspect how JSON, HCL, msgpack, and mapstructure encode these fields, including the related cases in issues #12237 and #11637. Done means concrete types are used for known configurations, with a separate fully opaque field for other proxies and compatibility coverage for each encoding method.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, distributed-systems
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100