config: Use HookWeakDecodeFromSlice in place of PatchSliceOfMaps #7964
Conversation
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps-forward-compat
branch
2 times, most recently
from
8a073c1
to
9e07deb
Compare
|
I think we should consider getting this merged and included in the 1.8 release. If it does not make it into 1.8 we won't be able to fix the problems in 1.9 and it will push the new envoy config features out even further. |
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps-forward-compat
branch
from
9e07deb
to
4ec6036
Compare
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps-forward-compat
branch
from
4ec6036
to
23e6750
Compare
|
Rebased, and removed the changes to CA provider, since those configs are not structured. |
| case len(d) == 0: | ||
| return nil, nil | ||
| case len(d) == 1: | ||
| return d[0], nil |
There was a problem hiding this comment.
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
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps-forward-compat
branch
from
23e6750
to
072211c
Compare
There was a problem hiding this comment.
Approved after extensive internal discussion on the approach.
@dnephin great job here! There is one TODO inline that seems like it doesn't affect anything but just wanted to flag in case you planned to fix that in this pass. I'm approving because as far as I can see it's passing tests and working as desired now.
Given the other inline comment, it might be good to double check manually the workflow of managing Config Entries defined in HCL via the CLI. I think in that case the HCL -> map parse happens in the CLI command and then the client agent HTTP handler only accepts JSON, but still must account for the JSON being the output of an HCL parse so possibly containing extra slices.
agent/config/config.go
Outdated
| Metadata: &md, | ||
| Result: &c, | ||
| DecodeHook: mapstructure.ComposeDecodeHookFunc( | ||
| decode.HookWeakDecodeFromSlice, // TODO: only apply when format is hcl |
There was a problem hiding this comment.
It doesn't. I'll change the comment here to be more informative.
| case len(d) == 0: | ||
| return nil, nil | ||
| case len(d) == 1: | ||
| return d[0], nil |
Currently opaque config blocks (config entries, and CA provider config) are
modified by PatchSliceOfMaps, making it impossible for these opaque
config sections to contain slices of maps.
In order to fix this problem, any lazy-decoding of these blocks needs to support
weak decoding of []map[string]interface{} to a struct type before
PatchSliceOfMaps is replaces. This is necessary because these config
blobs are persisted, and during an upgrade an older version of Consul
could read one of the new configuration values, which would cause an error.
To support the upgrade path, this commit first introduces the new hooks
for weak decoding of []map[string]interface{} and uses them only in the
lazy-decode paths. That way, in a future release, new style
configuration will be supported by the older version of Consul.
This decode hook has a number of advantages:
1. It no longer panics. It allows mapstructure to report the error
2. It no longer requires the user to declare which fields are slices of
structs. It can deduce that information from the 'to' value.
3. It will make it possible to preserve opaque configuration, allowing
for structured opaque config.
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps-forward-compat
branch
from
072211c
to
75cbbe2
Compare
…-maps-forward-compat config: Use HookWeakDecodeFromSlice in place of PatchSliceOfMaps
Follow up on #7963, finishes the replacement of #7707
Preserves the existing behaviour, but should enable us to change it in a future release. All lazy-decode functions will be using the
HookWeakDecodeFromSlice, so even if they receive new-style config, they will be able to handle it.Currently opaque config blocks (config entries, and CA provider config) are
modified by PatchSliceOfMaps, making it impossible for these opaque
config sections to contain slices of maps.
In order to fix this problem, any lazy-decoding of these blocks needs to support
weak decoding of []map[string]interface{} to a struct type. This is necessary because
these config blocks are persisted, and during an upgrade an older version of Consul
could read one of the new configuration values, which would cause an error.
To support the upgrade path, this commit first introduces the new hooks
for weak decoding of []map[string]interface{} and uses them in the lazy-decode paths.
That way, in a future release, new style configuration will be supported by the older version
of Consul.
This decode hook has a number of advantages:
structs. It can deduce that information from the 'to' value.
for structured opaque config.