Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

config: Do not modify the structure of opaque config #7935

Closed
wants to merge 4 commits into from
Closed

config: Do not modify the structure of opaque config #7935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8844d8d
config: add HookWeakDecodeFromSlice
dnephin May 27, 2020
7e7a67b
Add decode hooks to all lazy-decode parse functions
dnephin May 8, 2020
09f5c48
config: Use HookWeakDecodeFromSlice in place of PatchSliceOfMaps
dnephin May 27, 2020
e11c27a
config: Do not modify the structure of opaque config
dnephin Apr 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 7 additions & 65 deletions agent/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"strings"

"github.com/hashicorp/consul/lib"
"github.com/hashicorp/consul/lib/decode"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/hcl"
Expand Down Expand Up @@ -49,76 +48,19 @@ func Parse(data string, format string) (c Config, keys []string, err error) {
return Config{}, nil, err
}

// We want to be able to report fields which we cannot map as an
// error so that users find typos in their configuration quickly. To
// achieve this we use the mapstructure library which maps a a raw
// map[string]interface{} to a nested structure and reports unused
// fields. The input for a mapstructure.Decode expects a
// map[string]interface{} as produced by encoding/json.
//
// The HCL language allows to repeat map keys which forces it to
// store nested structs as []map[string]interface{} instead of
// map[string]interface{}. This is an ambiguity which makes the
// generated structures incompatible with a corresponding JSON
// struct. It also does not work well with the mapstructure library.
//
// In order to still use the mapstructure library to find unused
// fields we patch instances of []map[string]interface{} to a
// map[string]interface{} before we decode that into a Config
// struct.
//
// However, Config has some fields which are either
// []map[string]interface{} or are arrays of structs which
// encoding/json will decode to []map[string]interface{}. Therefore,
// we need to be able to specify exceptions for this mapping. The
// PatchSliceOfMaps() implements that mapping. All fields of type
// []map[string]interface{} are mapped to map[string]interface{} if
// it contains at most one value. If there is more than one value it
// panics. To define exceptions one can specify the nested field
// names in dot notation.
//
// todo(fs): There might be an easier way to achieve the same thing
// todo(fs): but this approach works for now.
m := lib.PatchSliceOfMaps(raw, []string{
"checks",
"segments",
"service.checks",
"services",
"services.checks",
"watches",
"service.connect.proxy.config.upstreams", // Deprecated
"services.connect.proxy.config.upstreams", // Deprecated
"service.connect.proxy.upstreams",
"services.connect.proxy.upstreams",
"service.connect.proxy.expose.paths",
"services.connect.proxy.expose.paths",
"service.proxy.upstreams",
"services.proxy.upstreams",
"service.proxy.expose.paths",
"services.proxy.expose.paths",
"acl.tokens.managed_service_provider",

// Need all the service(s) exceptions also for nested sidecar service.
"service.connect.sidecar_service.checks",
"services.connect.sidecar_service.checks",
"service.connect.sidecar_service.proxy.upstreams",
"services.connect.sidecar_service.proxy.upstreams",
"service.connect.sidecar_service.proxy.expose.paths",
"services.connect.sidecar_service.proxy.expose.paths",
}, []string{
"config_entries.bootstrap", // completely ignore this tree (fixed elsewhere)
})

var md mapstructure.Metadata
d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
DecodeHook: decode.HookTranslateKeys,
Metadata: &md,
Result: &c,
DecodeHook: mapstructure.ComposeDecodeHookFunc(
decode.HookWeakDecodeFromSlice, // TODO: only apply when format is hcl
decode.HookTranslateKeys,
),
Metadata: &md,
Result: &c,
})
if err != nil {
return Config{}, nil, err
}
if err := d.Decode(m); err != nil {
if err := d.Decode(raw); err != nil {
return Config{}, nil, err
}

Expand Down
117 changes: 0 additions & 117 deletions agent/config/runtime_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3257,120 +3257,6 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
}`},
err: "config_entries.bootstrap[0]: invalid name (\"invalid-name\"), only \"global\" is supported",
},
{
desc: "ConfigEntry bootstrap proxy-defaults (snake-case)",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

args: []string{`-data-dir=` + dataDir},
json: []string{`{
"config_entries": {
"bootstrap": [
{
"kind": "proxy-defaults",
"name": "global",
"config": {
"bar": "abc",
"moreconfig": {
"moar": "config"
}
},
"mesh_gateway": {
"mode": "remote"
}
}
]
}
}`},
hcl: []string{`
config_entries {
bootstrap {
kind = "proxy-defaults"
name = "global"
config {
"bar" = "abc"
"moreconfig" {
"moar" = "config"
}
}
mesh_gateway {
mode = "remote"
}
}
}`},
patch: func(rt *RuntimeConfig) {
rt.DataDir = dataDir
rt.ConfigEntryBootstrap = []structs.ConfigEntry{
&structs.ProxyConfigEntry{
Kind: structs.ProxyDefaults,
Name: structs.ProxyConfigGlobal,
Config: map[string]interface{}{
"bar": "abc",
"moreconfig": map[string]interface{}{
"moar": "config",
},
},
MeshGateway: structs.MeshGatewayConfig{
Mode: structs.MeshGatewayModeRemote,
},
},
}
},
},
{
desc: "ConfigEntry bootstrap proxy-defaults (camel-case)",
args: []string{`-data-dir=` + dataDir},
json: []string{`{
"config_entries": {
"bootstrap": [
{
"Kind": "proxy-defaults",
"Name": "global",
"Config": {
"bar": "abc",
"moreconfig": {
"moar": "config"
}
},
"MeshGateway": {
"Mode": "remote"
}
}
]
}
}`},
hcl: []string{`
config_entries {
bootstrap {
Kind = "proxy-defaults"
Name = "global"
Config {
"bar" = "abc"
"moreconfig" {
"moar" = "config"
}
}
MeshGateway {
Mode = "remote"
}
}
}`},
patch: func(rt *RuntimeConfig) {
rt.DataDir = dataDir
rt.ConfigEntryBootstrap = []structs.ConfigEntry{
&structs.ProxyConfigEntry{
Kind: structs.ProxyDefaults,
Name: structs.ProxyConfigGlobal,
Config: map[string]interface{}{
"bar": "abc",
"moreconfig": map[string]interface{}{
"moar": "config",
},
},
MeshGateway: structs.MeshGatewayConfig{
Mode: structs.MeshGatewayModeRemote,
},
},
}
},
},
{
desc: "ConfigEntry bootstrap service-defaults (snake-case)",
args: []string{`-data-dir=` + dataDir},
Expand Down Expand Up @@ -3891,9 +3777,6 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) {
if tt.patch != nil {
tt.patch(&patchedRT)
}
// if err := x.Validate(wantRT); err != nil {
// t.Fatalf("validate default failed: %s", err)
// }
if got, want := rt, patchedRT; !verify.Values(t, "", got, want) {
t.FailNow()
}
Expand Down
7 changes: 6 additions & 1 deletion agent/connect/ca/provider_aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/acmpca"
"github.com/hashicorp/consul/lib/decode"
"github.com/mitchellh/mapstructure"

"github.com/hashicorp/consul/agent/connect"
Expand Down Expand Up @@ -690,7 +691,11 @@ func ParseAWSCAConfig(raw map[string]interface{}) (*structs.AWSCAProviderConfig,
}

decodeConf := &mapstructure.DecoderConfig{
DecodeHook: structs.ParseDurationFunc(),
DecodeHook: mapstructure.ComposeDecodeHookFunc(
decode.HookWeakDecodeFromSlice,
decode.HookTranslateKeys,
structs.ParseDurationFunc(),
),
Result: &config,
WeaklyTypedInput: true,
}
Expand Down
12 changes: 12 additions & 0 deletions agent/connect/ca/provider_consul.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,18 @@ func (c *ConsulProvider) parseTestState(rawConfig map[string]interface{}, state
return
}

// If the state comes from an HCL config it may be wrapped in a slice,
// because that is how HCL decodes blocks.
if ts, ok := rawTestState.([]map[string]interface{}); ok && len(ts) == 1 {
c.testState = make(map[string]string)
for k, v := range ts[0] {
if s, ok := v.(string); ok {
c.testState[k] = s
}
}
return
}

// Secondary's config takes a trip through the state store before Configure
// is called and RPC calls that msgpack encode also have the same effect. It
// means we end up with map[string]string encoded as map[string]interface{}.
Expand Down
7 changes: 6 additions & 1 deletion agent/connect/ca/provider_consul_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,18 @@ import (

"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/lib/decode"
"github.com/mitchellh/mapstructure"
)

func ParseConsulCAConfig(raw map[string]interface{}) (*structs.ConsulCAProviderConfig, error) {
config := defaultConsulCAProviderConfig()
decodeConf := &mapstructure.DecoderConfig{
DecodeHook: structs.ParseDurationFunc(),
DecodeHook: mapstructure.ComposeDecodeHookFunc(
decode.HookWeakDecodeFromSlice,
decode.HookTranslateKeys,
structs.ParseDurationFunc(),
),
Result: &config,
WeaklyTypedInput: true,
}
Expand Down
7 changes: 6 additions & 1 deletion agent/connect/ca/provider_vault.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/lib/decode"
vaultapi "github.com/hashicorp/vault/api"
"github.com/mitchellh/mapstructure"
)
Expand Down Expand Up @@ -412,7 +413,11 @@ func ParseVaultCAConfig(raw map[string]interface{}) (*structs.VaultCAProviderCon
}

decodeConf := &mapstructure.DecoderConfig{
DecodeHook: structs.ParseDurationFunc(),
DecodeHook: mapstructure.ComposeDecodeHookFunc(
decode.HookWeakDecodeFromSlice,
decode.HookTranslateKeys,
structs.ParseDurationFunc(),
),
Result: &config,
WeaklyTypedInput: true,
}
Expand Down
2 changes: 2 additions & 0 deletions agent/consul/authmethod/authmethods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"sync"

"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/lib/decode"
"github.com/hashicorp/go-hclog"
"github.com/mitchellh/mapstructure"
)
Expand Down Expand Up @@ -185,6 +186,7 @@ func Types() []string {
// ParseConfig parses the config block for a auth method.
func ParseConfig(rawConfig map[string]interface{}, out interface{}) error {
decodeConf := &mapstructure.DecoderConfig{
DecodeHook: decode.HookWeakDecodeFromSlice,
Result: out,
WeaklyTypedInput: true,
ErrorUnused: true,
Expand Down
6 changes: 1 addition & 5 deletions agent/discovery_chain_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

cachetype "github.com/hashicorp/consul/agent/cache-types"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/lib"
"github.com/hashicorp/consul/lib/decode"
"github.com/mitchellh/mapstructure"
)
Expand Down Expand Up @@ -102,13 +101,10 @@ type discoveryChainReadResponse struct {
}

func decodeDiscoveryChainReadRequest(raw map[string]interface{}) (*discoveryChainReadRequest, error) {
// lib.TranslateKeys doesn't understand []map[string]interface{} so we have
// to do this part first.
raw = lib.PatchSliceOfMaps(raw, nil, nil)

var apiReq discoveryChainReadRequest
decodeConf := &mapstructure.DecoderConfig{
DecodeHook: mapstructure.ComposeDecodeHookFunc(
decode.HookWeakDecodeFromSlice,
decode.HookTranslateKeys,
mapstructure.StringToTimeDurationHookFunc(),
),
Expand Down