Fix uint8 conversion issues for service config response maps.

agent/consul/config_endpoint_test.go

@@ -6,7 +6,6 @@ import (
 
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/lib"
 	"github.com/hashicorp/consul/testrpc"
 	msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
 	"github.com/stretchr/testify/require"
@@ -681,15 +680,6 @@ func TestConfigEntry_ResolveServiceConfig(t *testing.T) {
 	}
 	var out structs.ServiceConfigResponse
 	require.NoError(msgpackrpc.CallWithCodec(codec, "ConfigEntry.ResolveServiceConfig", &args, &out))
-	// Hack to fix up the string encoding in the map[string]interface{}.
-	// msgpackRPC's codec doesn't use RawToString.
-	var err error
-	out.ProxyConfig, err = lib.MapWalk(out.ProxyConfig)
-	require.NoError(err)
-	for k := range out.UpstreamConfigs {
-		out.UpstreamConfigs[k], err = lib.MapWalk(out.UpstreamConfigs[k])
-		require.NoError(err)
-	}
 
 	expected := structs.ServiceConfigResponse{
 		ProxyConfig: map[string]interface{}{

agent/structs/config_entry.go

@@ -419,6 +419,54 @@ type ServiceConfigResponse struct {
 	QueryMeta
 }
 
+// MarshalBinary writes ServiceConfigResponse as msgpack encoded. It's only here
+// because we need custom decoding of the raw interface{} values.
+func (r *ServiceConfigResponse) MarshalBinary() (data []byte, err error) {
+	// bs will grow if needed but allocate enough to avoid reallocation in common
+	// case.
+	bs := make([]byte, 128)
+	enc := codec.NewEncoderBytes(&bs, msgpackHandle)
+
+	type Alias ServiceConfigResponse
+
+	if err := enc.Encode((*Alias)(r)); err != nil {
+		return nil, err
+	}
+
+	return bs, nil
+}
+
+// UnmarshalBinary decodes msgpack encoded ServiceConfigResponse. It used
+// default msgpack encoding but fixes up the uint8 strings and other problems we
+// have with encoding map[string]interface{}.
+func (r *ServiceConfigResponse) UnmarshalBinary(data []byte) error {
+	dec := codec.NewDecoderBytes(data, msgpackHandle)
+
+	type Alias ServiceConfigResponse
+	var a Alias
+
+	if err := dec.Decode(&a); err != nil {
+		return err
+	}
+
+	*r = ServiceConfigResponse(a)
+
+	var err error
+
+	// Fix strings and maps in the returned maps
+	r.ProxyConfig, err = lib.MapWalk(r.ProxyConfig)
+	if err != nil {
+		return err
+	}
+	for k := range r.UpstreamConfigs {
+		r.UpstreamConfigs[k], err = lib.MapWalk(r.UpstreamConfigs[k])
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // ConfigEntryResponse returns a single ConfigEntry
 type ConfigEntryResponse struct {
 	Entry ConfigEntry

agent/structs/config_entry_test.go

@@ -1,8 +1,10 @@
 package structs
 
 import (
+	"bytes"
 	"testing"
 
+	"github.com/hashicorp/go-msgpack/codec"
 	"github.com/stretchr/testify/require"
 )
 
@@ -100,3 +102,44 @@ func TestDecodeConfigEntry(t *testing.T) {
 		})
 	}
 }
+
+func TestServiceConfigResponse_MsgPack(t *testing.T) {
+	// TODO(banks) lib.MapWalker doesn't actually fix the map[interface{}] issue
+	// it claims to in docs yet. When it does uncomment those cases below.
+	a := ServiceConfigResponse{
+		ProxyConfig: map[string]interface{}{
+			"string": "foo",
+			// "map": map[string]interface{}{
+			// 	"baz": "bar",
+			// },
+		},
+		UpstreamConfigs: map[string]map[string]interface{}{
+			"a": map[string]interface{}{
+				"string": "aaaa",
+				// "map": map[string]interface{}{
+				// 	"baz": "aa",
+				// },
+			},
+			"b": map[string]interface{}{
+				"string": "bbbb",
+				// "map": map[string]interface{}{
+				// 	"baz": "bb",
+				// },
+			},
+		},
+	}
+
+	var buf bytes.Buffer
+
+	// Encode as msgPack using a regular handle i.e. NOT one with RawAsString
+	// since our RPC codec doesn't use that.
+	enc := codec.NewEncoder(&buf, msgpackHandle)
+	require.NoError(t, enc.Encode(&a))
+
+	var b ServiceConfigResponse
+
+	dec := codec.NewDecoder(&buf, msgpackHandle)
+	require.NoError(t, dec.Decode(&b))
+
+	require.Equal(t, a, b)
+}

lib/map_walker_test.go

@@ -38,6 +38,20 @@ func TestMapWalk(t *testing.T) {
 			},
 			unexpected: true,
 		},
+		// TODO(banks): despite the doc comment, MapWalker doesn't actually fix
+		// these cases yet. Do that in a later PR.
+		// "map iface": tcase{
+		//  input: map[string]interface{}{
+		//    "foo": map[interface{}]interface{}{
+		//      "bar": "baz",
+		//    },
+		//  },
+		//  expected: map[string]interface{}{
+		//    "foo": map[string]interface{}{
+		//      "bar": "baz",
+		//    },
+		//  },
+		// },
 	}
 
 	for name, tcase := range cases {