From b876b05a150b0b1c2a90c05a240c21078f22b6d5 Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Mon, 11 May 2020 10:23:09 -0400 Subject: [PATCH 1/2] Fixes #5606: Tokens converted from legacy ACLs get their Hash computed This allows new style token replication to work for legacy tokens as well when they change. --- agent/consul/fsm/snapshot_oss.go | 3 + agent/consul/fsm/snapshot_oss_test.go | 439 ++++++++++++-------------- agent/consul/state/acl.go | 3 + agent/consul/state/acl_test.go | 5 + agent/structs/acl_legacy.go | 5 +- agent/structs/acl_legacy_test.go | 1 + 6 files changed, 217 insertions(+), 239 deletions(-) diff --git a/agent/consul/fsm/snapshot_oss.go b/agent/consul/fsm/snapshot_oss.go index f9cad072f55e..16d146f3f2b1 100644 --- a/agent/consul/fsm/snapshot_oss.go +++ b/agent/consul/fsm/snapshot_oss.go @@ -658,6 +658,9 @@ func restoreToken(header *snapshotHeader, restore *state.Restore, decoder *codec structs.SanitizeLegacyACLToken(&req) } + // only set if unset - mitigates a bug where converted legacy tokens could end up without a hash + req.SetHash(false) + return restore.ACLToken(&req) } diff --git a/agent/consul/fsm/snapshot_oss_test.go b/agent/consul/fsm/snapshot_oss_test.go index c0ae90b6ec8e..227aaf6c41ad 100644 --- a/agent/consul/fsm/snapshot_oss_test.go +++ b/agent/consul/fsm/snapshot_oss_test.go @@ -2,7 +2,6 @@ package fsm import ( "bytes" - "reflect" "testing" "time" @@ -14,21 +13,17 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib/stringslice" "github.com/hashicorp/consul/sdk/testutil" + "github.com/hashicorp/go-msgpack/codec" "github.com/hashicorp/go-raftchunking" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestFSM_SnapshotRestore_OSS(t *testing.T) { t.Parallel() - assert := assert.New(t) - require := require.New(t) logger := testutil.Logger(t) fsm, err := New(nil, logger) - if err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, err) // Add some state node1 := &structs.Node{ @@ -49,8 +44,8 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { "testMeta": "testing123", }, } - require.NoError(fsm.state.EnsureNode(1, node1)) - require.NoError(fsm.state.EnsureNode(2, node2)) + require.NoError(t, fsm.state.EnsureNode(1, node1)) + require.NoError(t, fsm.state.EnsureNode(2, node2)) // Add a service instance with Connect config. connectConf := structs.ServiceConnect{ @@ -89,7 +84,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { Syntax: acl.SyntaxCurrent, } policy.SetHash(true) - require.NoError(fsm.state.ACLPolicySet(1, policy)) + require.NoError(t, fsm.state.ACLPolicySet(1, policy)) role := &structs.ACLRole{ ID: "86dedd19-8fae-4594-8294-4e6948a81f9a", @@ -102,7 +97,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { }, } role.SetHash(true) - require.NoError(fsm.state.ACLRoleSet(1, role)) + require.NoError(t, fsm.state.ACLRoleSet(1, role)) token := &structs.ACLToken{ AccessorID: "30fca056-9fbb-4455-b94a-bf0e2bc575d6", @@ -118,7 +113,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { // DEPRECATED (ACL-Legacy-Compat) - This is used so that the bootstrap token is still visible via the v1 acl APIs Type: structs.ACLTokenTypeManagement, } - require.NoError(fsm.state.ACLBootstrap(10, 0, token, false)) + require.NoError(t, fsm.state.ACLBootstrap(10, 0, token, false)) method := &structs.ACLAuthMethod{ Name: "some-method", @@ -128,7 +123,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { "SessionID": "952ebfa8-2a42-46f0-bcd3-fd98a842000e", }, } - require.NoError(fsm.state.ACLAuthMethodSet(1, method)) + require.NoError(t, fsm.state.ACLAuthMethodSet(1, method)) bindingRule := &structs.ACLBindingRule{ ID: "85184c52-5997-4a84-9817-5945f2632a17", @@ -138,7 +133,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { BindType: structs.BindingRuleBindTypeService, BindName: "${serviceaccount.name}", } - require.NoError(fsm.state.ACLBindingRuleSet(1, bindingRule)) + require.NoError(t, fsm.state.ACLBindingRuleSet(1, bindingRule)) fsm.state.KVSSet(11, &structs.DirEntry{ Key: "/remove", @@ -146,12 +141,8 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { }) fsm.state.KVSDelete(12, "/remove", nil) idx, _, err := fsm.state.KVSList(nil, "/remove", nil) - if err != nil { - t.Fatalf("err: %s", err) - } - if idx != 12 { - t.Fatalf("bad index: %d", idx) - } + require.NoError(t, err) + require.EqualValues(t, 12, idx, "bad index") updates := structs.Coordinates{ &structs.Coordinate{ @@ -163,9 +154,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { Coord: generateRandomCoordinate(), }, } - if err := fsm.state.CoordinateBatchUpdate(13, updates); err != nil { - t.Fatalf("err: %s", err) - } + require.NoError(t, fsm.state.CoordinateBatchUpdate(13, updates)) query := structs.PreparedQuery{ ID: generateUUID(), @@ -177,18 +166,14 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { ModifyIndex: 14, }, } - if err := fsm.state.PreparedQuerySet(14, &query); err != nil { - t.Fatalf("err: %s", err) - } + require.NoError(t, fsm.state.PreparedQuerySet(14, &query)) autopilotConf := &autopilot.Config{ CleanupDeadServers: true, LastContactThreshold: 100 * time.Millisecond, MaxTrailingLogs: 222, } - if err := fsm.state.AutopilotSetConfig(15, autopilotConf); err != nil { - t.Fatalf("err: %s", err) - } + require.NoError(t, fsm.state.AutopilotSetConfig(15, autopilotConf)) // Intentions ixn := structs.TestIntention(t) @@ -197,7 +182,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { CreateIndex: 14, ModifyIndex: 14, } - require.NoError(fsm.state.IntentionSet(14, ixn)) + require.NoError(t, fsm.state.IntentionSet(14, ixn)) // CA Roots roots := []*structs.CARoot{ @@ -208,16 +193,16 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { r.Active = false } ok, err := fsm.state.CARootSetCAS(15, 0, roots) - require.NoError(err) - assert.True(ok) + require.NoError(t, err) + require.True(t, ok) ok, err = fsm.state.CASetProviderState(16, &structs.CAConsulProviderState{ ID: "asdf", PrivateKey: "foo", RootCert: "bar", }) - require.NoError(err) - assert.True(ok) + require.NoError(t, err) + require.True(t, ok) // CA Config caConfig := &structs.CAConfiguration{ @@ -229,7 +214,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { }, } err = fsm.state.CASetConfig(17, caConfig) - require.NoError(err) + require.NoError(t, err) // Config entries serviceConfig := &structs.ServiceConfigEntry{ @@ -241,8 +226,8 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { Kind: structs.ProxyDefaults, Name: "global", } - require.NoError(fsm.state.EnsureConfigEntry(18, serviceConfig, structs.DefaultEnterpriseMeta())) - require.NoError(fsm.state.EnsureConfigEntry(19, proxyConfig, structs.DefaultEnterpriseMeta())) + require.NoError(t, fsm.state.EnsureConfigEntry(18, serviceConfig, structs.DefaultEnterpriseMeta())) + require.NoError(t, fsm.state.EnsureConfigEntry(19, proxyConfig, structs.DefaultEnterpriseMeta())) ingress := &structs.IngressGatewayConfigEntry{ Kind: structs.IngressGateway, @@ -259,9 +244,9 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { }, }, } - require.NoError(fsm.state.EnsureConfigEntry(20, ingress, structs.DefaultEnterpriseMeta())) + require.NoError(t, fsm.state.EnsureConfigEntry(20, ingress, structs.DefaultEnterpriseMeta())) _, gatewayServices, err := fsm.state.GatewayServices(nil, "ingress", structs.DefaultEnterpriseMeta()) - require.NoError(err) + require.NoError(t, err) // Raft Chunking chunkState := &raftchunking.State{ @@ -292,7 +277,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { }, } err = fsm.chunker.RestoreState(chunkState) - require.NoError(err) + require.NoError(t, err) // Federation states fedState1 := &structs.FederationState{ @@ -395,267 +380,269 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { }, UpdatedAt: time.Now().UTC(), } - require.NoError(fsm.state.FederationStateSet(21, fedState1)) - require.NoError(fsm.state.FederationStateSet(22, fedState2)) + require.NoError(t, fsm.state.FederationStateSet(21, fedState1)) + require.NoError(t, fsm.state.FederationStateSet(22, fedState2)) // Snapshot snap, err := fsm.Snapshot() - if err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, err) defer snap.Release() // Persist buf := bytes.NewBuffer(nil) sink := &MockSink{buf, false} - if err := snap.Persist(sink); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, snap.Persist(sink)) + + // create an encoder to handle some custom persisted data + // this is mainly to inject data that would no longer ever + // be persisted but that we still need to be able to restore + encoder := codec.NewEncoder(sink, structs.MsgpackHandle) + + // Persist a legacy ACL token - this is not done in newer code + // but we want to ensure that restoring legacy tokens works as + // expected so we must inject one here manually + _, err = sink.Write([]byte{byte(structs.ACLRequestType)}) + require.NoError(t, err) + + acl := structs.ACL{ + ID: "1057354f-69ef-4487-94ab-aead3c755445", + Name: "test-legacy", + Type: "client", + Rules: `operator = "read"`, + RaftIndex: structs.RaftIndex{CreateIndex: 1, ModifyIndex: 2}, + } + require.NoError(t, encoder.Encode(&acl)) + + // Persist a ACLToken without a Hash - the state store will + // now tack these on but we want to ensure we can restore + // tokens without a hash and have the hash be set. + token2 := &structs.ACLToken{ + AccessorID: "4464e4c2-1c55-4c37-978a-66cb3abe6587", + SecretID: "fc8708dc-c5ae-4bb2-a9af-a1ca456548fb", + Description: "Test No Hash", + CreateTime: time.Now(), + Local: false, + Rules: `operator = "read"`, + RaftIndex: structs.RaftIndex{CreateIndex: 1, ModifyIndex: 2}, + } + + _, err = sink.Write([]byte{byte(structs.ACLTokenSetRequestType)}) + require.NoError(t, err) + require.NoError(t, encoder.Encode(&token2)) // Try to restore on a new FSM fsm2, err := New(nil, logger) - if err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, err) // Do a restore - if err := fsm2.Restore(sink); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, fsm2.Restore(sink)) // Verify the contents _, nodes, err := fsm2.state.Nodes(nil) - if err != nil { - t.Fatalf("err: %s", err) - } - if len(nodes) != 2 { - t.Fatalf("bad: %v", nodes) - } - if nodes[0].ID != node2.ID || - nodes[0].Node != "baz" || - nodes[0].Datacenter != "dc1" || - nodes[0].Address != "127.0.0.2" || - len(nodes[0].Meta) != 1 || - nodes[0].Meta["testMeta"] != "testing123" || - len(nodes[0].TaggedAddresses) != 1 || - nodes[0].TaggedAddresses["hello"] != "1.2.3.4" { - t.Fatalf("bad: %v", nodes[0]) - } - if nodes[1].ID != node1.ID || - nodes[1].Node != "foo" || - nodes[1].Datacenter != "dc1" || - nodes[1].Address != "127.0.0.1" || - len(nodes[1].TaggedAddresses) != 0 { - t.Fatalf("bad: %v", nodes[1]) - } + require.NoError(t, err) + require.Len(t, nodes, 2, "incorect number of nodes: %v", nodes) + + // validate the first node. Note that this test relies on stable + // iteration through the memdb index and the fact that node2 has + // a name of "baz" so it should be indexed before node1 with a + // name of "foo". If memdb our our indexing changes this is likely + // to break. + require.Equal(t, node2.ID, nodes[0].ID) + require.Equal(t, "baz", nodes[0].Node) + require.Equal(t, "dc1", nodes[0].Datacenter) + require.Equal(t, "127.0.0.2", nodes[0].Address) + require.Len(t, nodes[0].Meta, 1) + require.Equal(t, "testing123", nodes[0].Meta["testMeta"]) + require.Len(t, nodes[0].TaggedAddresses, 1) + require.Equal(t, "1.2.3.4", nodes[0].TaggedAddresses["hello"]) + + require.Equal(t, node1.ID, nodes[1].ID) + require.Equal(t, "foo", nodes[1].Node) + require.Equal(t, "dc1", nodes[1].Datacenter) + require.Equal(t, "127.0.0.1", nodes[1].Address) + require.Empty(t, nodes[1].TaggedAddresses) _, fooSrv, err := fsm2.state.NodeServices(nil, "foo", nil) - if err != nil { - t.Fatalf("err: %s", err) - } - if len(fooSrv.Services) != 2 { - t.Fatalf("Bad: %v", fooSrv) - } - if !stringslice.Contains(fooSrv.Services["db"].Tags, "primary") { - t.Fatalf("Bad: %v", fooSrv) - } - if fooSrv.Services["db"].Port != 5000 { - t.Fatalf("Bad: %v", fooSrv) - } + require.NoError(t, err) + require.Len(t, fooSrv.Services, 2) + require.Contains(t, fooSrv.Services["db"].Tags, "primary") + require.True(t, stringslice.Contains(fooSrv.Services["db"].Tags, "primary")) + require.Equal(t, 5000, fooSrv.Services["db"].Port) connectSrv := fooSrv.Services["web"] - if !reflect.DeepEqual(connectSrv.Connect, connectConf) { - t.Fatalf("got: %v, want: %v", connectSrv.Connect, connectConf) - } + require.Equal(t, connectConf, connectSrv.Connect) _, checks, err := fsm2.state.NodeChecks(nil, "foo", nil) - if err != nil { - t.Fatalf("err: %s", err) - } - if len(checks) != 1 { - t.Fatalf("Bad: %v", checks) - } + require.NoError(t, err) + require.Len(t, checks, 1) // Verify key is set _, d, err := fsm2.state.KVSGet(nil, "/test", nil) - if err != nil { - t.Fatalf("err: %v", err) - } - if string(d.Value) != "foo" { - t.Fatalf("bad: %v", d) - } + require.NoError(t, err) + require.EqualValues(t, "foo", d.Value) // Verify session is restored idx, s, err := fsm2.state.SessionGet(nil, session.ID, nil) - if err != nil { - t.Fatalf("err: %v", err) - } - if s.Node != "foo" { - t.Fatalf("bad: %v", s) - } - if idx <= 1 { - t.Fatalf("bad index: %d", idx) - } + require.NoError(t, err) + require.Equal(t, "foo", s.Node) + require.EqualValues(t, 9, idx) // Verify ACL Binding Rule is restored _, bindingRule2, err := fsm2.state.ACLBindingRuleGetByID(nil, bindingRule.ID, nil) - require.NoError(err) - require.Equal(bindingRule, bindingRule2) + require.NoError(t, err) + require.Equal(t, bindingRule, bindingRule2) // Verify ACL Auth Method is restored _, method2, err := fsm2.state.ACLAuthMethodGetByName(nil, method.Name, nil) - require.NoError(err) - require.Equal(method, method2) + require.NoError(t, err) + require.Equal(t, method, method2) // Verify ACL Token is restored - _, token2, err := fsm2.state.ACLTokenGetByAccessor(nil, token.AccessorID, nil) - require.NoError(err) - { - // time.Time is tricky to compare generically when it takes a ser/deserialization round trip. - require.True(token.CreateTime.Equal(token2.CreateTime)) - token2.CreateTime = token.CreateTime - } - require.Equal(token, token2) + _, rtoken, err := fsm2.state.ACLTokenGetByAccessor(nil, token.AccessorID, nil) + require.NoError(t, err) + require.NotNil(t, rtoken) + // the state store function will add on the Hash if its empty + require.NotEmpty(t, rtoken.Hash) + token.CreateTime.Round(0) + rtoken.CreateTime.Round(0) + + // note that this can work because the state store will add the Hash to the token before + // storing. That token just happens to be a pointer to the one in this function so it + // adds the Hash to our local var. + require.Equal(t, token, rtoken) + + // Verify legacy ACL is restored + _, rtoken, err = fsm2.state.ACLTokenGetBySecret(nil, acl.ID, nil) + require.NoError(t, err) + require.NotNil(t, rtoken) + require.NotEmpty(t, rtoken.Hash) + + restoredACL, err := rtoken.Convert() + require.NoError(t, err) + require.Equal(t, &acl, restoredACL) + + // Verify ACLToken without hash computes the Hash during restoration + _, rtoken, err = fsm2.state.ACLTokenGetByAccessor(nil, token2.AccessorID, nil) + require.NoError(t, err) + require.NotNil(t, rtoken) + require.NotEmpty(t, rtoken.Hash) + // nil the Hash so we can compare them + rtoken.Hash = nil + token2.CreateTime.Round(0) + rtoken.CreateTime.Round(0) + require.Equal(t, token2, rtoken) // Verify the acl-token-bootstrap index was restored canBootstrap, index, err := fsm2.state.CanBootstrapACLToken() - require.False(canBootstrap) - require.True(index > 0) + require.False(t, canBootstrap) + require.True(t, index > 0) // Verify ACL Role is restored _, role2, err := fsm2.state.ACLRoleGetByID(nil, role.ID, nil) - require.NoError(err) - require.Equal(role, role2) + require.NoError(t, err) + require.Equal(t, role, role2) // Verify ACL Policy is restored _, policy2, err := fsm2.state.ACLPolicyGetByID(nil, structs.ACLPolicyGlobalManagementID, nil) - require.NoError(err) - require.Equal(policy, policy2) + require.NoError(t, err) + require.Equal(t, policy, policy2) // Verify tombstones are restored func() { snap := fsm2.state.Snapshot() defer snap.Close() stones, err := snap.Tombstones() - if err != nil { - t.Fatalf("err: %s", err) - } + require.NoError(t, err) stone := stones.Next().(*state.Tombstone) - if stone == nil { - t.Fatalf("missing tombstone") - } - if stone.Key != "/remove" || stone.Index != 12 { - t.Fatalf("bad: %v", stone) - } - if stones.Next() != nil { - t.Fatalf("unexpected extra tombstones") - } + require.NotNil(t, stone) + require.Equal(t, "/remove", stone.Key) + require.Nil(t, stones.Next()) }() // Verify coordinates are restored _, coords, err := fsm2.state.Coordinates(nil) - if err != nil { - t.Fatalf("err: %s", err) - } - if !reflect.DeepEqual(coords, updates) { - t.Fatalf("bad: %#v", coords) - } + require.NoError(t, err) + require.Equal(t, updates, coords) // Verify queries are restored. _, queries, err := fsm2.state.PreparedQueryList(nil) - if err != nil { - t.Fatalf("err: %s", err) - } - if len(queries) != 1 { - t.Fatalf("bad: %#v", queries) - } - if !reflect.DeepEqual(queries[0], &query) { - t.Fatalf("bad: %#v", queries[0]) - } + require.NoError(t, err) + require.Len(t, queries, 1) + require.Equal(t, &query, queries[0]) // Verify autopilot config is restored. _, restoredConf, err := fsm2.state.AutopilotConfig() - if err != nil { - t.Fatalf("err: %s", err) - } - if !reflect.DeepEqual(restoredConf, autopilotConf) { - t.Fatalf("bad: %#v, %#v", restoredConf, autopilotConf) - } + require.NoError(t, err) + require.Equal(t, autopilotConf, restoredConf) // Verify intentions are restored. _, ixns, err := fsm2.state.Intentions(nil) - require.NoError(err) - assert.Len(ixns, 1) - assert.Equal(ixn, ixns[0]) + require.NoError(t, err) + require.Len(t, ixns, 1) + require.Equal(t, ixn, ixns[0]) // Verify CA roots are restored. _, roots, err = fsm2.state.CARoots(nil) - require.NoError(err) - assert.Len(roots, 2) + require.NoError(t, err) + require.Len(t, roots, 2) // Verify provider state is restored. _, state, err := fsm2.state.CAProviderState("asdf") - require.NoError(err) - assert.Equal("foo", state.PrivateKey) - assert.Equal("bar", state.RootCert) + require.NoError(t, err) + require.Equal(t, "foo", state.PrivateKey) + require.Equal(t, "bar", state.RootCert) // Verify CA configuration is restored. _, caConf, err := fsm2.state.CAConfig(nil) - require.NoError(err) - assert.Equal(caConfig, caConf) + require.NoError(t, err) + require.Equal(t, caConfig, caConf) // Verify config entries are restored _, serviceConfEntry, err := fsm2.state.ConfigEntry(nil, structs.ServiceDefaults, "foo", structs.DefaultEnterpriseMeta()) - require.NoError(err) - assert.Equal(serviceConfig, serviceConfEntry) + require.NoError(t, err) + require.Equal(t, serviceConfig, serviceConfEntry) _, proxyConfEntry, err := fsm2.state.ConfigEntry(nil, structs.ProxyDefaults, "global", structs.DefaultEnterpriseMeta()) - require.NoError(err) - assert.Equal(proxyConfig, proxyConfEntry) + require.NoError(t, err) + require.Equal(t, proxyConfig, proxyConfEntry) _, ingressRestored, err := fsm2.state.ConfigEntry(nil, structs.IngressGateway, "ingress", structs.DefaultEnterpriseMeta()) - require.NoError(err) - assert.Equal(ingress, ingressRestored) + require.NoError(t, err) + require.Equal(t, ingress, ingressRestored) _, restoredGatewayServices, err := fsm2.state.GatewayServices(nil, "ingress", structs.DefaultEnterpriseMeta()) - require.NoError(err) - require.Equal(gatewayServices, restoredGatewayServices) + require.NoError(t, err) + require.Equal(t, gatewayServices, restoredGatewayServices) newChunkState, err := fsm2.chunker.CurrentState() - require.NoError(err) - assert.Equal(newChunkState, chunkState) + require.NoError(t, err) + require.Equal(t, newChunkState, chunkState) // Verify federation states are restored. _, fedStateLoaded1, err := fsm2.state.FederationStateGet(nil, "dc1") - require.NoError(err) - assert.Equal(fedState1, fedStateLoaded1) + require.NoError(t, err) + require.Equal(t, fedState1, fedStateLoaded1) _, fedStateLoaded2, err := fsm2.state.FederationStateGet(nil, "dc2") - require.NoError(err) - assert.Equal(fedState2, fedStateLoaded2) + require.NoError(t, err) + require.Equal(t, fedState2, fedStateLoaded2) // Snapshot snap, err = fsm2.Snapshot() - if err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, err) defer snap.Release() // Persist buf = bytes.NewBuffer(nil) sink = &MockSink{buf, false} - if err := snap.Persist(sink); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, snap.Persist(sink)) // Try to restore on the old FSM and make sure it abandons the old state // store. abandonCh := fsm.state.AbandonCh() - if err := fsm.Restore(sink); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, fsm.Restore(sink)) select { case <-abandonCh: default: - t.Fatalf("bad") + require.Fail(t, "Old state not abandoned") } } @@ -664,37 +651,27 @@ func TestFSM_BadRestore_OSS(t *testing.T) { // Create an FSM with some state. logger := testutil.Logger(t) fsm, err := New(nil, logger) - if err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, err) fsm.state.EnsureNode(1, &structs.Node{Node: "foo", Address: "127.0.0.1"}) abandonCh := fsm.state.AbandonCh() // Do a bad restore. buf := bytes.NewBuffer([]byte("bad snapshot")) sink := &MockSink{buf, false} - if err := fsm.Restore(sink); err == nil { - t.Fatalf("err: %v", err) - } + require.Error(t, fsm.Restore(sink)) // Verify the contents didn't get corrupted. _, nodes, err := fsm.state.Nodes(nil) - if err != nil { - t.Fatalf("err: %s", err) - } - if len(nodes) != 1 { - t.Fatalf("bad: %v", nodes) - } - if nodes[0].Node != "foo" || - nodes[0].Address != "127.0.0.1" || - len(nodes[0].TaggedAddresses) != 0 { - t.Fatalf("bad: %v", nodes[0]) - } + require.NoError(t, err) + require.Len(t, nodes, 1) + require.Equal(t, "foo", nodes[0].Node) + require.Equal(t, "127.0.0.1", nodes[0].Address) + require.Empty(t, nodes[0].TaggedAddresses) // Verify the old state store didn't get abandoned. select { case <-abandonCh: - t.Fatalf("bad") + require.FailNow(t, "FSM state was abandoned when it should not have been") default: } } @@ -702,46 +679,32 @@ func TestFSM_BadRestore_OSS(t *testing.T) { func TestFSM_BadSnapshot_NilCAConfig(t *testing.T) { t.Parallel() - require := require.New(t) - // Create an FSM with no config entry. logger := testutil.Logger(t) fsm, err := New(nil, logger) - if err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, err) // Snapshot snap, err := fsm.Snapshot() - if err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, err) defer snap.Release() // Persist buf := bytes.NewBuffer(nil) sink := &MockSink{buf, false} - if err := snap.Persist(sink); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, snap.Persist(sink)) // Try to restore on a new FSM fsm2, err := New(nil, logger) - if err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, err) // Do a restore - if err := fsm2.Restore(sink); err != nil { - t.Fatalf("err: %v", err) - } + require.NoError(t, fsm2.Restore(sink)) // Make sure there's no entry in the CA config table. state := fsm2.State() idx, config, err := state.CAConfig(nil) - require.NoError(err) - require.Equal(uint64(0), idx) - if config != nil { - t.Fatalf("config should be nil") - } + require.NoError(t, err) + require.EqualValues(t, 0, idx) + require.Nil(t, config) } diff --git a/agent/consul/state/acl.go b/agent/consul/state/acl.go index c39f67b51043..8b2a218f2007 100644 --- a/agent/consul/state/acl.go +++ b/agent/consul/state/acl.go @@ -765,6 +765,9 @@ func (s *Store) aclTokenSetTxn(tx *memdb.Txn, idx uint64, token *structs.ACLToke token.ModifyIndex = idx } + // ensure that a hash is set + token.SetHash(false) + return s.aclTokenInsert(tx, token) } diff --git a/agent/consul/state/acl_test.go b/agent/consul/state/acl_test.go index 73d8c8c3e5f8..e5b665fb87ed 100644 --- a/agent/consul/state/acl_test.go +++ b/agent/consul/state/acl_test.go @@ -501,6 +501,7 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) { idx, rtoken, err := s.ACLTokenGetByAccessor(nil, "daf37c07-d04d-4fd5-9678-a8206a57d61a", nil) require.NoError(t, err) require.Equal(t, uint64(2), idx) + require.NotEmpty(t, rtoken.Hash) compareTokens(t, token, rtoken) require.Equal(t, uint64(2), rtoken.CreateIndex) require.Equal(t, uint64(2), rtoken.ModifyIndex) @@ -3843,6 +3844,10 @@ func stripIrrelevantTokenFields(token *structs.ACLToken) *structs.ACLToken { // The raft indexes won't match either because the requester will not // have access to that. tokenCopy.RaftIndex = structs.RaftIndex{} + + // nil out the hash - this is a computed field and we should assert + // elsewhere that its not empty when expected + tokenCopy.Hash = nil return tokenCopy } diff --git a/agent/structs/acl_legacy.go b/agent/structs/acl_legacy.go index fa182d888423..6e3ac351a1d7 100644 --- a/agent/structs/acl_legacy.go +++ b/agent/structs/acl_legacy.go @@ -72,7 +72,7 @@ func (a *ACL) Convert() *ACLToken { a.Rules = correctedRules } - return &ACLToken{ + token := &ACLToken{ AccessorID: "", SecretID: a.ID, Description: a.Name, @@ -83,6 +83,9 @@ func (a *ACL) Convert() *ACLToken { Local: false, RaftIndex: a.RaftIndex, } + + token.SetHash(true) + return token } // Convert attempts to convert an ACLToken into an ACLCompat. diff --git a/agent/structs/acl_legacy_test.go b/agent/structs/acl_legacy_test.go index 343a38050ae9..161e904b3e01 100644 --- a/agent/structs/acl_legacy_test.go +++ b/agent/structs/acl_legacy_test.go @@ -72,6 +72,7 @@ func TestStructs_ACL_Convert(t *testing.T) { require.Equal(t, acl.Rules, token.Rules) require.Equal(t, acl.CreateIndex, token.CreateIndex) require.Equal(t, acl.ModifyIndex, token.ModifyIndex) + require.NotEmpty(t, token.Hash) } func TestStructs_ACLToken_Convert(t *testing.T) { From b95638c5b28accfacb32fca0ad30577fc734b9bd Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Mon, 8 Jun 2020 11:14:41 +0200 Subject: [PATCH 2/2] tests: fix timestamp comparison --- agent/consul/fsm/snapshot_oss_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/agent/consul/fsm/snapshot_oss_test.go b/agent/consul/fsm/snapshot_oss_test.go index 227aaf6c41ad..70fdae164b41 100644 --- a/agent/consul/fsm/snapshot_oss_test.go +++ b/agent/consul/fsm/snapshot_oss_test.go @@ -502,8 +502,8 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { require.NotNil(t, rtoken) // the state store function will add on the Hash if its empty require.NotEmpty(t, rtoken.Hash) - token.CreateTime.Round(0) - rtoken.CreateTime.Round(0) + token.CreateTime = token.CreateTime.Round(0) + rtoken.CreateTime = rtoken.CreateTime.Round(0) // note that this can work because the state store will add the Hash to the token before // storing. That token just happens to be a pointer to the one in this function so it @@ -527,8 +527,8 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { require.NotEmpty(t, rtoken.Hash) // nil the Hash so we can compare them rtoken.Hash = nil - token2.CreateTime.Round(0) - rtoken.CreateTime.Round(0) + token2.CreateTime = token2.CreateTime.Round(0) + rtoken.CreateTime = rtoken.CreateTime.Round(0) require.Equal(t, token2, rtoken) // Verify the acl-token-bootstrap index was restored