From 9753c77a7927366bf3ccb73c4d4603b9de944594 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 5 Aug 2019 17:15:22 -0500 Subject: [PATCH] api: un-deprecate api.DecodeConfigEntry (#6278) Add clarifying commentary about when it is not safe to use it. Also add tests. --- api/config_entry.go | 39 ++-- api/config_entry_test.go | 387 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 410 insertions(+), 16 deletions(-) diff --git a/api/config_entry.go b/api/config_entry.go index f2a4b5b851..63a67b28f9 100644 --- a/api/config_entry.go +++ b/api/config_entry.go @@ -132,18 +132,17 @@ func MakeConfigEntry(kind, name string) (ConfigEntry, error) { return makeConfigEntry(kind, name) } -// DEPRECATED: TODO(rb): remove? +// DecodeConfigEntry will decode the result of using json.Unmarshal of a config +// entry into a map[string]interface{}. // -// DecodeConfigEntry only successfully works on config entry kinds -// "service-defaults" and "proxy-defaults" (as of Consul 1.5). +// Important caveats: // -// This is because by parsing HCL into map[string]interface{} and then trying -// to decode it with mapstructure we run into the problem where hcl generically -// decodes many things into map[string][]interface{} at intermediate nodes in -// the resulting structure (for nested structs not otherwise in an enclosing -// slice). This breaks decoding. +// - This will NOT work if the map[string]interface{} was produced using HCL +// decoding as that requires more extensive parsing to work around the issues +// with map[string][]interface{} that arise. // -// Until a better solution is arrived at don't use this method. +// - This will only decode fields using their camel case json field +// representations. func DecodeConfigEntry(raw map[string]interface{}) (ConfigEntry, error) { var entry ConfigEntry @@ -188,6 +187,18 @@ func DecodeConfigEntryFromJSON(data []byte) (ConfigEntry, error) { return DecodeConfigEntry(raw) } +func decodeConfigEntrySlice(raw []map[string]interface{}) ([]ConfigEntry, error) { + var entries []ConfigEntry + for _, rawEntry := range raw { + entry, err := DecodeConfigEntry(rawEntry) + if err != nil { + return nil, err + } + entries = append(entries, entry) + } + return entries, nil +} + // ConfigEntries can be used to query the Config endpoints type ConfigEntries struct { c *Client @@ -251,13 +262,9 @@ func (conf *ConfigEntries) List(kind string, q *QueryOptions) ([]ConfigEntry, *Q return nil, nil, err } - var entries []ConfigEntry - for _, rawEntry := range raw { - entry, err := DecodeConfigEntry(rawEntry) - if err != nil { - return nil, nil, err - } - entries = append(entries, entry) + entries, err := decodeConfigEntrySlice(raw) + if err != nil { + return nil, nil, err } return entries, qm, nil diff --git a/api/config_entry_test.go b/api/config_entry_test.go index 358c6549d2..21a6bcf3ed 100644 --- a/api/config_entry_test.go +++ b/api/config_entry_test.go @@ -1,7 +1,9 @@ package api import ( + "encoding/json" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -183,3 +185,388 @@ func TestAPI_ConfigEntries(t *testing.T) { require.Error(t, err) }) } + +func TestDecodeConfigEntry(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + body string + expect ConfigEntry + expectErr string + }{ + { + name: "proxy-defaults", + body: ` + { + "Kind": "proxy-defaults", + "Name": "main", + "Config": { + "foo": 19, + "bar": "abc", + "moreconfig": { + "moar": "config" + } + }, + "MeshGateway": { + "Mode": "remote" + } + } + `, + expect: &ProxyConfigEntry{ + Kind: "proxy-defaults", + Name: "main", + Config: map[string]interface{}{ + "foo": float64(19), + "bar": "abc", + "moreconfig": map[string]interface{}{ + "moar": "config", + }, + }, + MeshGateway: MeshGatewayConfig{ + Mode: MeshGatewayModeRemote, + }, + }, + }, + { + name: "service-defaults", + body: ` + { + "Kind": "service-defaults", + "Name": "main", + "Protocol": "http", + "MeshGateway": { + "Mode": "remote" + } + } + `, + expect: &ServiceConfigEntry{ + Kind: "service-defaults", + Name: "main", + Protocol: "http", + MeshGateway: MeshGatewayConfig{ + Mode: MeshGatewayModeRemote, + }, + }, + }, + { + name: "service-router: kitchen sink", + body: ` + { + "Kind": "service-router", + "Name": "main", + "Routes": [ + { + "Match": { + "HTTP": { + "PathExact": "/foo", + "Header": [ + { + "Name": "debug1", + "Present": true + }, + { + "Name": "debug2", + "Present": false, + "Invert": true + }, + { + "Name": "debug3", + "Exact": "1" + }, + { + "Name": "debug4", + "Prefix": "aaa" + }, + { + "Name": "debug5", + "Suffix": "bbb" + }, + { + "Name": "debug6", + "Regex": "a.*z" + } + ] + } + }, + "Destination": { + "Service": "carrot", + "ServiceSubset": "kale", + "Namespace": "leek", + "PrefixRewrite": "/alternate", + "RequestTimeout": "99s", + "NumRetries": 12345, + "RetryOnConnectFailure": true, + "RetryOnStatusCodes": [401, 209] + } + }, + { + "Match": { + "HTTP": { + "PathPrefix": "/foo", + "Methods": [ "GET", "DELETE" ], + "QueryParam": [ + { + "Name": "hack1", + "Present": true + }, + { + "Name": "hack2", + "Exact": "1" + }, + { + "Name": "hack3", + "Regex": "a.*z" + } + ] + } + } + }, + { + "Match": { + "HTTP": { + "PathRegex": "/foo" + } + } + } + ] + } + `, + expect: &ServiceRouterConfigEntry{ + Kind: "service-router", + Name: "main", + Routes: []ServiceRoute{ + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathExact: "/foo", + Header: []ServiceRouteHTTPMatchHeader{ + { + Name: "debug1", + Present: true, + }, + { + Name: "debug2", + Present: false, + Invert: true, + }, + { + Name: "debug3", + Exact: "1", + }, + { + Name: "debug4", + Prefix: "aaa", + }, + { + Name: "debug5", + Suffix: "bbb", + }, + { + Name: "debug6", + Regex: "a.*z", + }, + }, + }, + }, + Destination: &ServiceRouteDestination{ + Service: "carrot", + ServiceSubset: "kale", + Namespace: "leek", + PrefixRewrite: "/alternate", + RequestTimeout: 99 * time.Second, + NumRetries: 12345, + RetryOnConnectFailure: true, + RetryOnStatusCodes: []uint32{401, 209}, + }, + }, + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathPrefix: "/foo", + Methods: []string{"GET", "DELETE"}, + QueryParam: []ServiceRouteHTTPMatchQueryParam{ + { + Name: "hack1", + Present: true, + }, + { + Name: "hack2", + Exact: "1", + }, + { + Name: "hack3", + Regex: "a.*z", + }, + }, + }, + }, + }, + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathRegex: "/foo", + }, + }, + }, + }, + }, + }, + { + name: "service-splitter: kitchen sink", + body: ` + { + "Kind": "service-splitter", + "Name": "main", + "Splits": [ + { + "Weight": 99.1, + "ServiceSubset": "v1" + }, + { + "Weight": 0.9, + "Service": "other", + "Namespace": "alt" + } + ] + } + `, + expect: &ServiceSplitterConfigEntry{ + Kind: ServiceSplitter, + Name: "main", + Splits: []ServiceSplit{ + { + Weight: 99.1, + ServiceSubset: "v1", + }, + { + Weight: 0.9, + Service: "other", + Namespace: "alt", + }, + }, + }, + }, + { + name: "service-resolver: subsets with failover", + body: ` + { + "Kind": "service-resolver", + "Name": "main", + "DefaultSubset": "v1", + "ConnectTimeout": "15s", + "Subsets": { + "v1": { + "Filter": "Service.Meta.version == v1" + }, + "v2": { + "Filter": "Service.Meta.version == v2", + "OnlyPassing": true + } + }, + "Failover": { + "v2": { + "Service": "failcopy", + "ServiceSubset": "sure", + "Namespace": "neighbor", + "Datacenters": ["dc5", "dc14"] + }, + "*": { + "Datacenters": ["dc7"] + } + } + }`, + expect: &ServiceResolverConfigEntry{ + Kind: "service-resolver", + Name: "main", + DefaultSubset: "v1", + ConnectTimeout: 15 * time.Second, + Subsets: map[string]ServiceResolverSubset{ + "v1": { + Filter: "Service.Meta.version == v1", + }, + "v2": { + Filter: "Service.Meta.version == v2", + OnlyPassing: true, + }, + }, + Failover: map[string]ServiceResolverFailover{ + "v2": { + Service: "failcopy", + ServiceSubset: "sure", + Namespace: "neighbor", + Datacenters: []string{"dc5", "dc14"}, + }, + "*": { + Datacenters: []string{"dc7"}, + }, + }, + }, + }, + { + name: "service-resolver: redirect", + body: ` + { + "Kind": "service-resolver", + "Name": "main", + "Redirect": { + "Service": "other", + "ServiceSubset": "backup", + "Namespace": "alt", + "Datacenter": "dc9" + } + } + `, + expect: &ServiceResolverConfigEntry{ + Kind: "service-resolver", + Name: "main", + Redirect: &ServiceResolverRedirect{ + Service: "other", + ServiceSubset: "backup", + Namespace: "alt", + Datacenter: "dc9", + }, + }, + }, + { + name: "service-resolver: default", + body: ` + { + "Kind": "service-resolver", + "Name": "main" + } + `, + expect: &ServiceResolverConfigEntry{ + Kind: "service-resolver", + Name: "main", + }, + }, + } { + tc := tc + + t.Run(tc.name+": DecodeConfigEntry", func(t *testing.T) { + var raw map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(tc.body), &raw)) + + got, err := DecodeConfigEntry(raw) + require.NoError(t, err) + require.Equal(t, tc.expect, got) + }) + + t.Run(tc.name+": DecodeConfigEntryFromJSON", func(t *testing.T) { + got, err := DecodeConfigEntryFromJSON([]byte(tc.body)) + require.NoError(t, err) + require.Equal(t, tc.expect, got) + }) + + t.Run(tc.name+": DecodeConfigEntrySlice", func(t *testing.T) { + var raw []map[string]interface{} + require.NoError(t, json.Unmarshal([]byte("["+tc.body+"]"), &raw)) + + got, err := decodeConfigEntrySlice(raw) + require.NoError(t, err) + require.Len(t, got, 1) + require.Equal(t, tc.expect, got[0]) + }) + } +}