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

@@ -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
+// Important caveats:
-// "service-defaults" and "proxy-defaults" (as of Consul 1.5).
 //
-// This is because by parsing HCL into map[string]interface{} and then trying
+// - This will NOT work if the map[string]interface{} was produced using HCL
-// to decode it with mapstructure we run into the problem where hcl generically
+// decoding as that requires more extensive parsing to work around the issues
-// decodes many things into map[string][]interface{} at intermediate nodes in
+// with map[string][]interface{} that arise.
-// the resulting structure (for nested structs not otherwise in an enclosing
-// slice). This breaks decoding.
 //
-// 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
+	entries, err := decodeConfigEntrySlice(raw)
-	for _, rawEntry := range raw {
+	if err != nil {
-		entry, err := DecodeConfigEntry(rawEntry)
+		return nil, nil, err
-		if err != nil {
-			return nil, nil, err
-		}
-		entries = append(entries, entry)
 	}
 
 	return entries, qm, nil

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])
+		})
+	}
+}