config: Do not modify the structure of opaque config

[dnephin]

Branched from #7964
Fixes #4971

HCL and JSON config may have different structures. With this change the opaque configuration will not be modified. When the opaque config is mapped to a type the same hook is used to handle the difference in raw structure.

[dnephin]

This test case is TestConfigFlagsAndEdgecases. The tests fail because the hcl and json produce different opaque config in the Config section. I think with this change these are no longer special edgecases. The standard behaviour is handled by the TestFullConfig test case.

I couldn't think of any test to replace these, but I am happy to write some more if anyone has ideas.

[mkeeler]

This doesn't look quite right. Given the JSON and HCL above what we want is for this to result in a map not a list of maps.

[rboyer]

Same with the config_entry_test.go line.

[rboyer]

The HCL library always decodes interface{} within a map[string]interface{} as a []map[string]interface{} because of how HCL works it doesn't know the difference between something showing up once or multiple times. This was part of the whole issue with needing PatchSliceOfMaps.

[dnephin]

Yes, I understand what HCL is doing, and how this was solved before. I believe we were doing it wrong. I guess I did not explain the rational very well in the PR description.

Attempting to guess at what the user is expecting from an opaque config is impossible. If a user specifies some HCL, then they should get out the raw config that HCL produces, which is a slice of maps. Sometimes they are going to want a slice of maps, sometimes they are going to want a map. It is up to them to either:

1. Use JSON, which is predictable and does what most people expect
2. Use HCL, and handle the ambiguity it produces themselves.

In the case where the config is only partially opaque (opaque to config loading, but not to other parts of the code), we can decode properly using the same hooks.

[banks]

I was really unsure about this change to begin with for the same reason Matt and RB bring it up - it "feels" wrong that we push this foot gun onto the user (at least the integrator who is storing config in these places and then parsing it back out from our API). We should probably at least figure out how we'd document that clearly wherever we write to or expose the config object in the API docs.

That said, I don't think there is a generally correct alternative since we by definition don't have the schema to know which way the ambiguous HCL presented should be interpreted so we will always be wrong in some cases and hence limit what can be expressed "opaquely" with HCL.

Until this PR that means it's just impossible to nest arbitrarily since we've made it wrong to ever be ambiguous and that is the whole issue we're trying to fix. The only other option I see would be to only allow exactly one off nested maps or nested array/list types in opaque config. That way we can always assume it's one or the other but I think this limits the usefulness of the syntax of HCL and would still cause confusing parse errors if you express things incorrectly.

There could be alternative UX options to consider. Although likely way more work, it's worth exploring them before we decide on the ultimate path:

1. Disallow HCL repeated blocks in our files. To do this well we'd probably need changes to the HCL parser to be able to detect their presence unambiguously (i.e. vs explicit lists of HCL objects).
   • Pros
     1. Fixes the issue at source and makes it unambiguous on input and output
     2. Although it's possible to use repeated blocks in a few places in Consul today, it's already compromised by the plural naming convention we adopt to make the JSON format in the API sensible. e.g. you can specify multiple services as repeated blocks but you have to label each one services rather than service since the JSON equivalent array field is naturally pluralized.
   • Cons
     1. Lots of work to make the error handling right including HCL changes - might not even be feasible. Would likely be easiest to implement an optional "JSON Compatible" parsing mode.
     2. Breaks HCL expectations from other projects. Doesn't seem like a big issue given the poor ergonomics of these above.
     3. It's a breaking change. Although the ergonomics are poor with the naming thing, there will be existing HCL config files that rely on repeated blocks (I've written plenty myself although not important ones).
2. Change "opaque" config to be a string where the actual config is either HCL or JSON depending on what the integration supports but is passed through without being parsed at all.
   • Pros
     1. Simple and unambiguous.
   • Cons
     1. Breaking change
     2. The UX of editing HCL with nested HCL or JSON strings is not great although HEREDOC syntax makes it OK. We already have examples of this in ACL policies and Envoy escape hatch though and it's workable but feels gross especially as that blob of config grows.
     3. THE UX of editing JSON with nested JSON or consuming it from the API is pretty awful though.
3. This approach.
   • Pros
     1. Implemented and relatively simple in code.
   • Cons

     1. User is potentially left to figure out why the "same" config in HCL or JSON produces different results. Integrators have to be made aware of and handle both cases correctly or the UX of configuring via Consul is broken. This could lead to integrators documenting things like "don't use HCL as it messes stuff up, we only support JSON" which kind of undermines our slight preference towards HCL for more readable (e.g. commentable) config. An example here would be if a plugin (lets say a proxy implementation) choose to nest config like this in proxy.config:

        config {
            foo_proxy { defaults { connection_timeout_ms = 1000 } } 
        }

        {
          "config" : {
              "foo_proxy": {"defaults": {"connection_timeout_ms": 1000}}
          }
        }

        A user might reasonably expect both of those to produce the same output in the API for the proxy to decode but in fact the JSON one would look the same, while the HCL one would come back as:

        {
          "config": {
            "foo_proxy": [
              {
                "defaults": [
                  {
                    "connection_timeout_ms": 1000
                  }
                ]
              }
            ]
          }
        }
        

[dnephin]

My take on these approaches is that all 3 of them are breaking changes, but option 3 (this PR) is the least sever breakage as it is isolated to only config-entries for external proxy integrations.

• Option 1 can potentially break any user, even those that don't use connect.
• Option 2 could break anyone using config entries, including users of the envoy proxy.
• Option 3 can only break users of external proxy integrations which use nested structures (config entries for envoy proxies are safe and won't break). This should be the smallest possible group of users. We can further reduce the risk here by looking at any known integrations and confirming they do not use nested config. I've confirmed https://github.com/haproxytech/haproxy-consul-connect will not break. A very loud notice in the changelog would also be appropriate.

Backwards compatibility aside, I also think that option 3 is the best UX. HCL is unfortunately not a good option for opaque configuration that needs to be output as JSON. However, the one negative point for option 3 that you outline is real and something we will need to mitigate.

I think we would do the following:

• add support for heredoc strings for the opaque configurations. This should be relatively easy to do with mapstructure.
• continue to support inline opaque configuration so that we do not break any existing users
• change all of our documentation to show either HCL+heredoc, or JSON+inline (ie: remove any inline HCL opaque config examples)
• add a deprecation warning to our documentation about HCL+inline, and explain why it is not recommended. It will continue to work (with the caveat that nested structures will not come out as most people would expect), but we should encourage everyone to move away from it.
• we could potentially even go as far as printing a warning to stderr when an HCL config uses inline opaque configuration.

[dnephin]

If the changes in #7964 make it into 1.8, it will ensure forwards compatibility with this change. Consul 1.8 would be able to correctly read any configs saved by Consul 1.9 without errors.

I believe that only leaves 1 backwards compatibility concern, which is integrations using structured config. I've verified that https://github.com/haproxytech/haproxy-consul-connect only depends on scalar values, so will not be impacted by this change. I'll look into other integrations as well.

[banks]

which is integrations using structured config

🤔 isn't it the case that no integrations can have used structured config until now since it would cause a panic? I guess that means it's safe to assume we aren't breaking any one? Or maybe I missed something.

[dnephin]

isn't it the case that no integrations can have used structured config until now since it would cause a panic?

Integrations can use structured config, as long as it doesn't contain lists of structures. A list of structures would cause a panic. But the top level of the config can contain other structures as long as all the fields are scalar values or other maps.

[dnephin]

After more reflecting, I think this breaking change is not necessary after all.

With #7964 we no longer have a panic. Single item lists are still modified and replaced with their single value, but mapstructure.WeakDecode handles turning single values into a single-item list when necessary.

This means that we can start using slices of types in our own partially opaque config. Anyone writing an integration will need to know that their decode may need to turn values into single-item lists, but that is a lot better than a panic, and roughly comparable to the workaround this PR was going to require (handling slices for every structure).