Remove unnecessary `agent/config` struct copies
- Dominant language
- Go
- Stars
- 30.1k
- Forks
- 4.6k
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 43
Description
[agent/config.config.go](https://github.com/hashicorp/consul/blob/master/agent/config/config.go) defines many struct types. These structs generally have fields with pointer types. The structs are used for two purposes:
1. unmarshalling config from config files (unmarshalling hcl or json into a `map[string]interface{}`, and then `mapstructure` to decode into the types)
2. merging of multiple `Config` structs (config from multiple sources) into a single one
These structs, in many cases, are mirrored in 3 other places (`api`, `agent/structs`, and `proto/*`). This means that adding a field to these structs requires adding it in 4 places!
I discovered recently that in some cases, the copy in `agent/config` is not necessary. If the field on `Config` is a slice or a pointer to a struct, then the merge logic always appends the values, it doesn't merge the fields. This makes the pointer fields unnecessary! In these cases we can use the types defined in the `api` package in the `Config` struct. This change would have a secondary benefit as well. In the past we've encountered a few bugs where the `api` and `agent/config` types decoded differently, resulting in some values being dropped. By using the same type we reduce the likelihood of those bugs. The config is effectively part of the public API of Consul, so using the `api` types for the config seems reasonable. The `agent/config` package already has a direct dependency on `api`, so this change doesn't impact our dependency tree.
The structs that are used in slices, that we should be able to remove, are:
* `CheckDefinition`
* `ServiceDefinition`
* `EnterpriseMeta`
* `ServiceAddress`
* `ServiceWeights`
* `ServiceProxy`
* `Upstream`
* `MeshGatewayConfig`
* `ExposeConfig`
* `ExposePath`
* `ServiceConnect`
The first two `Definition` structs do technically exists as non-slice fields, however [as it says in this comment](https://github.com/hashicorp/consul/blob/v1.10.0-alpha/agent/config/config.go#L151) these definitions can not be spread across multiple sources. The same merging rules apply to pointers and slices, so we don't need pointer fields within the struct.
To make this change we would need to copy the `mapstructure` tags onto the `api` struct fields. In general they should match the `json` tags that already exist on those fields.
Some issues that would be fixed (and future regressions prevented) by this change:
* https://github.com/hashicorp/consul/issues/6923
* https://github.com/hashicorp/consul/issues/7566
Contributor guide
Assessment
This issue has not been assessed yet.