Update to use ugorji/go/codec instead of hashicorp/go-msgpack/codec #5677
Conversation
Remove before merging. This replaces raft, raft-boltdb, memberlist and serf with corresponding versions that remove go-msgpack usage.
|
The following code demonstrates the underlying problem: Basically when using our fork of the codec library this fails because there is some reflection code that will try to modify the internals of a string when decoding into an existing map that already contains a value for the key. |
|
It turns out there are a number of other issues with updating to ugorji/go/codec • It now looks as the Closing this in favor of #5683 |
|
For reference I had the following test file added in at agent/structs/structs_msgpack_test.go which found the incompatibilities. package structs
import (
"fmt"
"testing"
"time"
"github.com/stretchr/testify/require"
)
func TestEncodeDecodeCompatibility(t *testing.T) {
t.Parallel()
type testCase struct {
mtype MessageType
data interface{}
// returns a zeroed struct to decode into
newFn func() interface{}
}
cases := map[string]testCase{
"RegisterRequest": testCase{
RegisterRequestType,
&RegisterRequest{
Datacenter: "foo",
Node: "bar",
Address: "baz",
Service: &NodeService{
Service: "test",
Address: "127.0.0.2",
},
},
func() interface{} { return new(RegisterRequest) },
},
"ACLToken": testCase{
ACLTokenSetRequestType,
&ACLTokenSetRequest{
Datacenter: "foo",
ACLToken: ACLToken{
AccessorID: "c46e9f3f-88c8-48dd-b30d-e771889dbd47",
SecretID: "8369561e-490c-43b0-a12f-9660244ba32d",
Description: "foo",
Policies: []ACLTokenPolicyLink{
ACLTokenPolicyLink{
ID: "a0563168-8971-453f-b747-2674c5db6813",
},
ACLTokenPolicyLink{
Name: "my-policy",
},
},
Type: "client",
Rules: "some legacy rules",
Local: true,
CreateTime: time.Now(),
Hash: []byte("abcdef"),
RaftIndex: RaftIndex{
CreateIndex: 5,
ModifyIndex: 123456,
},
},
},
func() interface{} { return new(ACLTokenSetRequest) },
},
}
for name, tcase := range cases {
// remember these
name := name
tcase := tcase
t.Run(name, func(t *testing.T) {
// encode with the new msgpack encoder
buf, err := Encode(tcase.mtype, tcase.data)
require.NoError(t, err)
// encoded with the old msgpack encoder
bufold, err := EncodeOld(tcase.mtype, tcase.data)
require.NoError(t, err)
// ensure they encoded to the same thing
// cannot do this as they are not byte equivalent
// require.Equal(t, bufold, buf)
// decode old buf with new decoder
out := tcase.newFn()
err = Decode(bufold[1:], out)
require.NoError(t, err)
// decode new buf with old decoder
outold := tcase.newFn()
err = DecodeOld(buf[1:], outold)
require.NoError(t, err)
// decode old buf with old decoder
good := tcase.newFn()
err = DecodeOld(bufold[1:], good)
require.NoError(t, err)
// ensure that the decode structs are identical
require.Equal(t, good, outold)
require.Equal(t, good, out)
})
}
}And with the following differences in agent/structs/structs.go diff --git a/agent/structs/structs.go b/agent/structs/structs.go
index b37ceffae..710815c4d 100644
--- a/agent/structs/structs.go
+++ b/agent/structs/structs.go
@@ -15,6 +15,7 @@ import (
"github.com/hashicorp/consul/agent/cache"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/consul/types"
+ oldcodec "github.com/hashicorp/go-msgpack/codec"
multierror "github.com/hashicorp/go-multierror"
"github.com/hashicorp/serf/coordinate"
"github.com/mitchellh/hashstructure"
@@ -1375,12 +1376,17 @@ func (r *TombstoneRequest) RequestDatacenter() string {
// msgpackHandle is a shared handle for encoding/decoding of structs
var msgpackHandle = &codec.MsgpackHandle{}
+var msgpackHandleOld = &oldcodec.MsgpackHandle{}
// Decode is used to decode a MsgPack encoded object
func Decode(buf []byte, out interface{}) error {
return codec.NewDecoder(bytes.NewReader(buf), msgpackHandle).Decode(out)
}
+func DecodeOld(buf []byte, out interface{}) error {
+ return oldcodec.NewDecoder(bytes.NewReader(buf), msgpackHandleOld).Decode(out)
+}
+
// Encode is used to encode a MsgPack object with type prefix
func Encode(t MessageType, msg interface{}) ([]byte, error) {
var buf bytes.Buffer
@@ -1389,6 +1395,13 @@ func Encode(t MessageType, msg interface{}) ([]byte, error) {
return buf.Bytes(), err
}
+func EncodeOld(t MessageType, msg interface{}) ([]byte, error) {
+ var buf bytes.Buffer
+ buf.WriteByte(uint8(t))
+ err := oldcodec.NewEncoder(&buf, msgpackHandleOld).Encode(msg)
+ return buf.Bytes(), err
+}
+
// CompoundResponse is an interface for gathering multiple responses. It is
// used in cross-datacenter RPC calls where more than 1 datacenter is
// expected to reply.Basically this enabled using either the old or new library and the tests were encoding the same structures with both and trying to ensure that they could both decode each others data and that the resulting decoded data was identical. If we ever go down this path again then we should start by creating a testing framework to check different versions of Consul with each other. In particular we should generate RPC requests and responses from both versions and have each version decode the others and ensure the resulting objects are equivalent. |
This was mostly just a drop in replacement. Going ahead and opening the PR to get some CI builds to execute.
There are a number of other things that would need to happen prior to merging this:
go mod vendor- there wasn't much point doing it yet with the replace directives pointing at unreleased versions.Fixes #4673