From 2e3e6bf4ac0cd38ad49fc685e308c4e47f166c33 Mon Sep 17 00:00:00 2001 From: Christopher Swenson Date: Fri, 22 Dec 2023 10:28:45 -0800 Subject: [PATCH] Address PR comments --- physical/raft/msgpack.go | 13 --- physical/raft/raft.go | 6 - physical/raft/raft_network_test.go | 173 ----------------------------- physical/raft/testing.go | 10 +- 4 files changed, 3 insertions(+), 199 deletions(-) delete mode 100644 physical/raft/msgpack.go delete mode 100644 physical/raft/raft_network_test.go diff --git a/physical/raft/msgpack.go b/physical/raft/msgpack.go deleted file mode 100644 index 4fe8808a656d..000000000000 --- a/physical/raft/msgpack.go +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -package raft - -// If we downgrade msgpack from v1.1.5 to v0.5.5, everything will still -// work, but any pre-existing raft clusters will break on upgrade. -// This file exists so that the Vault project has an explicit dependency -// on the library, which allows us to pin the version in go.mod. - -import ( - _ "github.com/hashicorp/go-msgpack/v2/codec" -) diff --git a/physical/raft/raft.go b/physical/raft/raft.go index e1cb5f9d0a3a..e937697777ad 100644 --- a/physical/raft/raft.go +++ b/physical/raft/raft.go @@ -904,9 +904,6 @@ type SetupOpts struct { // RecoveryModeConfig is the configuration for the raft cluster in recovery // mode. RecoveryModeConfig *raft.Configuration - - // overrideMsgpackUseNewTimeFormat is used for testing backwards compatability - overrideMsgpackUseNewTimeFormat *bool } func (b *RaftBackend) StartRecoveryCluster(ctx context.Context, peer Peer) error { @@ -1008,9 +1005,6 @@ func (b *RaftBackend) SetupCluster(ctx context.Context, opts SetupOpts) error { Logger: b.logger.Named("raft-net"), MsgpackUseNewTimeFormat: true, } - if opts.overrideMsgpackUseNewTimeFormat != nil { - transConfig.MsgpackUseNewTimeFormat = *opts.overrideMsgpackUseNewTimeFormat - } transport := raft.NewNetworkTransportWithConfig(transConfig) b.streamLayer = streamLayer diff --git a/physical/raft/raft_network_test.go b/physical/raft/raft_network_test.go deleted file mode 100644 index caebe1342e6e..000000000000 --- a/physical/raft/raft_network_test.go +++ /dev/null @@ -1,173 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build !race - -// The raft networking layer tends to reset the TLS keyring, which triggers -// the race detector even though it should be a no-op. - -package raft - -import ( - "bytes" - "context" - "crypto/rand" - "crypto/tls" - "fmt" - "net" - "os" - "testing" - "time" - - "github.com/hashicorp/consul/sdk/freeport" - "github.com/hashicorp/go-hclog" - "github.com/hashicorp/vault/sdk/physical" - "github.com/hashicorp/vault/vault/cluster" -) - -func BenchmarkRaftWithNetwork(b *testing.B) { - b.StopTimer() - raft1, _ := createRaftNetworkCluster(b, true, false) - b.StartTimer() - for i := 0; i < b.N; i++ { - err := raft1.Put(context.Background(), &physical.Entry{ - Key: "test", - Value: []byte{byte(i)}, - }) - if err != nil { - b.Fatal(err) - } - snapOut, err := os.CreateTemp(b.TempDir(), "bench") - if err != nil { - b.Fatal(err) - } - b.Cleanup(func() { - _ = snapOut.Close() - _ = os.Remove(snapOut.Name()) - }) - err = raft1.Snapshot(snapOut, nil) - if err != nil { - b.Fatal(err) - } - } -} - -// TestRaftNetworkClusterWithMultipleTimeEncodingsSet tests that Raft nodes -// with different msgpack time.Time encodings set will still cluster together. -// However, with go-msgpack 2.1.0+, the decoder is tolerant of both encodings, -// so this could only fail if the decoder drastically changes in the future. -func TestRaftNetworkClusterWithMultipleTimeEncodingsSet(t *testing.T) { - raft1, raft2 := createRaftNetworkCluster(t, true, false) - for i := 0; i < 10; i++ { - err := raft1.Put(context.Background(), &physical.Entry{ - Key: "test", - Value: []byte{byte(i)}, - }) - if err != nil { - t.Error(err) - } - } - for raft2.AppliedIndex() != raft1.AppliedIndex() { - time.Sleep(1 * time.Millisecond) - } - entry, err := raft2.Get(context.Background(), "test") - if err != nil { - t.Fatal(err) - } - if entry == nil { - t.Fatal("Entry from raft secondary is nil") - } - if !bytes.Equal(entry.Value, []byte{9}) { - t.Errorf("Expected {9} but got %+v", entry.Value) - } -} - -func createRaftNetworkCluster(tb testing.TB, overrideTimeFormat1, overrideTimeFormat2 bool) (*RaftBackend, *RaftBackend) { - cipherSuites := []uint16{ - // 1.3 - tls.TLS_AES_128_GCM_SHA256, - tls.TLS_AES_256_GCM_SHA384, - tls.TLS_CHACHA20_POLY1305_SHA256, - } - - port1 := freeport.GetOne(tb) - port2 := freeport.GetOne(tb) - addr1, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("127.0.0.1:%d", port1)) - if err != nil { - tb.Fatal(err) - } - addr2, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("127.0.0.1:%d", port2)) - if err != nil { - tb.Fatal(err) - } - key1, err := GenerateTLSKey(rand.Reader) - if err != nil { - tb.Fatal(err) - } - key2, err := GenerateTLSKey(rand.Reader) - if err != nil { - tb.Fatal(err) - } - logger1 := hclog.New(&hclog.LoggerOptions{ - Name: "raft1", - }) - logger2 := hclog.New(&hclog.LoggerOptions{ - Name: "raft2", - }) - listener1 := cluster.NewListener( - cluster.NewTCPLayer([]*net.TCPAddr{addr1}, logger1), cipherSuites, logger1, time.Minute) - listener2 := cluster.NewListener( - cluster.NewTCPLayer([]*net.TCPAddr{addr2}, logger2), cipherSuites, logger2, time.Minute) - go listener1.Run(context.Background()) - go listener2.Run(context.Background()) - tb.Cleanup(listener1.Stop) - tb.Cleanup(listener2.Stop) - - raft1, dir1 := GetRaftWithOpts(tb, true, true, SetupOpts{ - TLSKeyring: &TLSKeyring{ - Keys: []*TLSKey{key1, key2}, - ActiveKeyID: key1.ID, - }, - ClusterListener: listener1, - overrideMsgpackUseNewTimeFormat: &overrideTimeFormat1, - }) - - setupOpts2 := SetupOpts{ - TLSKeyring: &TLSKeyring{ - Keys: []*TLSKey{key2, key1}, - ActiveKeyID: key2.ID, - }, - ClusterListener: listener2, - overrideMsgpackUseNewTimeFormat: &overrideTimeFormat2, - } - raft2, dir2 := GetRaftWithOpts(tb, false, true, setupOpts2) - defer os.RemoveAll(dir1) - defer os.RemoveAll(dir2) - - // Add raft2 to the cluster - addNetworkPeer(tb, raft1, raft2, addr2, setupOpts2) - - return raft1, raft2 -} - -func addNetworkPeer(tb testing.TB, leader, follower *RaftBackend, followerAddr *net.TCPAddr, setupOpts SetupOpts) { - tb.Helper() - if err := leader.AddPeer(context.Background(), follower.NodeID(), followerAddr.String()); err != nil { - tb.Fatal(err) - } - - peers, err := leader.Peers(context.Background()) - if err != nil { - tb.Fatal(err) - } - - err = follower.Bootstrap(peers) - if err != nil { - tb.Fatal(err) - } - - err = follower.SetupCluster(context.Background(), setupOpts) - if err != nil { - tb.Fatal(err) - } -} diff --git a/physical/raft/testing.go b/physical/raft/testing.go index 89d09747771a..632924950081 100644 --- a/physical/raft/testing.go +++ b/physical/raft/testing.go @@ -14,20 +14,16 @@ import ( ) func GetRaft(t testing.TB, bootstrap bool, noStoreState bool) (*RaftBackend, string) { - return GetRaftWithOpts(t, bootstrap, noStoreState, SetupOpts{}) -} - -func GetRaftWithOpts(t testing.TB, bootstrap bool, noStoreState bool, setupOpts SetupOpts) (*RaftBackend, string) { raftDir, err := ioutil.TempDir("", "vault-raft-") if err != nil { t.Fatal(err) } t.Logf("raft dir: %s", raftDir) - return getRaftWithDir(t, bootstrap, noStoreState, raftDir, setupOpts) + return getRaftWithDir(t, bootstrap, noStoreState, raftDir) } -func getRaftWithDir(t testing.TB, bootstrap bool, noStoreState bool, raftDir string, setupOpts SetupOpts) (*RaftBackend, string) { +func getRaftWithDir(t testing.TB, bootstrap bool, noStoreState bool, raftDir string) (*RaftBackend, string) { id, err := uuid.GenerateUUID() if err != nil { t.Fatal(err) @@ -66,7 +62,7 @@ func getRaftWithDir(t testing.TB, bootstrap bool, noStoreState bool, raftDir str t.Fatal(err) } - err = backend.SetupCluster(context.Background(), setupOpts) + err = backend.SetupCluster(context.Background(), SetupOpts{}) if err != nil { t.Fatal(err) }