From 730da74369f991185ba1aa60fb30a625946ee271 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Thu, 19 Apr 2018 12:06:32 +0100 Subject: [PATCH] Fix various test failures and vet warnings. Intention de-duplication in previously merged PR actualy failed some tests that were not caught be me or CI. I ran the test files for state changes but they happened not to trigger this case so I made sure they did first and then fixed. That fixed some upstream intention endpoint tests that I'd not run as part of testing the previous fix. --- agent/agent_endpoint.go | 1 - agent/agent_test.go | 4 +- agent/consul/state/intention.go | 5 ++- agent/consul/state/intention_test.go | 62 +++++++++++++++------------- agent/local/state_test.go | 36 +++------------- connect/proxy/listener.go | 1 - connect/testing.go | 2 - connect/tls_test.go | 4 +- 8 files changed, 48 insertions(+), 67 deletions(-) diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 24685ee92a..c19b776ac2 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -960,7 +960,6 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http } return contentHash, reply, nil }) - return nil, nil } type agentLocalBlockingFunc func(ws memdb.WatchSet) (string, interface{}, error) diff --git a/agent/agent_test.go b/agent/agent_test.go index bf24425bc0..c22ce56ba1 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -2366,7 +2366,7 @@ func TestAgent_AddProxy(t *testing.T) { // Test the ID was created as we expect. got := a.State.Proxy("web-proxy") - require.Equal(tt.proxy, got) + require.Equal(tt.proxy, got.Proxy) }) } } @@ -2394,7 +2394,7 @@ func TestAgent_RemoveProxy(t *testing.T) { // Test the ID was created as we expect. gotProxy := a.State.Proxy("web-proxy") - require.Equal(pReg, gotProxy) + require.Equal(pReg, gotProxy.Proxy) err := a.RemoveProxy("web-proxy", false) require.NoError(err) diff --git a/agent/consul/state/intention.go b/agent/consul/state/intention.go index 907bdf1abf..91a61ffe1c 100644 --- a/agent/consul/state/intention.go +++ b/agent/consul/state/intention.go @@ -190,7 +190,10 @@ func (s *Store) intentionSetTxn(tx *memdb.Txn, idx uint64, ixn *structs.Intentio } if duplicate != nil { dupIxn := duplicate.(*structs.Intention) - return fmt.Errorf("duplicate intention found: %s", dupIxn.String()) + // Same ID is OK - this is an update + if dupIxn.ID != ixn.ID { + return fmt.Errorf("duplicate intention found: %s", dupIxn.String()) + } } // We always force meta to be non-nil so that we its an empty map. diff --git a/agent/consul/state/intention_test.go b/agent/consul/state/intention_test.go index 743f698afa..fbf43c19bd 100644 --- a/agent/consul/state/intention_test.go +++ b/agent/consul/state/intention_test.go @@ -42,7 +42,7 @@ func TestStore_IntentionSetGet_basic(t *testing.T) { } // Inserting a with empty ID is disallowed. - assert.Nil(s.IntentionSet(1, ixn)) + assert.NoError(s.IntentionSet(1, ixn)) // Make sure the index got updated. assert.Equal(uint64(1), s.maxIndex(intentionsTableName)) @@ -64,13 +64,18 @@ func TestStore_IntentionSetGet_basic(t *testing.T) { ws = memdb.NewWatchSet() idx, actual, err := s.IntentionGet(ws, ixn.ID) - assert.Nil(err) + assert.NoError(err) assert.Equal(expected.CreateIndex, idx) assert.Equal(expected, actual) // Change a value and test updating ixn.SourceNS = "foo" - assert.Nil(s.IntentionSet(2, ixn)) + assert.NoError(s.IntentionSet(2, ixn)) + + // Change a value that isn't in the unique 4 tuple and check we don't + // incorrectly consider this a duplicate when updating. + ixn.Action = structs.IntentionActionDeny + assert.NoError(s.IntentionSet(2, ixn)) // Make sure the index got updated. assert.Equal(uint64(2), s.maxIndex(intentionsTableName)) @@ -78,10 +83,11 @@ func TestStore_IntentionSetGet_basic(t *testing.T) { // Read it back and verify the data was updated expected.SourceNS = ixn.SourceNS + expected.Action = structs.IntentionActionDeny expected.ModifyIndex = 2 ws = memdb.NewWatchSet() idx, actual, err = s.IntentionGet(ws, ixn.ID) - assert.Nil(err) + assert.NoError(err) assert.Equal(expected.ModifyIndex, idx) assert.Equal(expected, actual) @@ -97,7 +103,7 @@ func TestStore_IntentionSetGet_basic(t *testing.T) { // Duplicate 4-tuple should cause an error ws = memdb.NewWatchSet() - assert.NotNil(s.IntentionSet(3, ixn)) + assert.Error(s.IntentionSet(3, ixn)) // Make sure the index did NOT get updated. assert.Equal(uint64(2), s.maxIndex(intentionsTableName)) @@ -110,11 +116,11 @@ func TestStore_IntentionSet_emptyId(t *testing.T) { ws := memdb.NewWatchSet() _, _, err := s.IntentionGet(ws, testUUID()) - assert.Nil(err) + assert.NoError(err) // Inserting a with empty ID is disallowed. err = s.IntentionSet(1, &structs.Intention{}) - assert.NotNil(err) + assert.Error(err) assert.Contains(err.Error(), ErrMissingIntentionID.Error()) // Index is not updated if nothing is saved. @@ -134,16 +140,16 @@ func TestStore_IntentionSet_updateCreatedAt(t *testing.T) { } // Insert - assert.Nil(s.IntentionSet(1, &ixn)) + assert.NoError(s.IntentionSet(1, &ixn)) // Change a value and test updating ixnUpdate := ixn ixnUpdate.CreatedAt = now.Add(10 * time.Second) - assert.Nil(s.IntentionSet(2, &ixnUpdate)) + assert.NoError(s.IntentionSet(2, &ixnUpdate)) // Read it back and verify _, actual, err := s.IntentionGet(nil, ixn.ID) - assert.Nil(err) + assert.NoError(err) assert.Equal(now, actual.CreatedAt) } @@ -157,11 +163,11 @@ func TestStore_IntentionSet_metaNil(t *testing.T) { } // Insert - assert.Nil(s.IntentionSet(1, &ixn)) + assert.NoError(s.IntentionSet(1, &ixn)) // Read it back and verify _, actual, err := s.IntentionGet(nil, ixn.ID) - assert.Nil(err) + assert.NoError(err) assert.NotNil(actual.Meta) } @@ -176,11 +182,11 @@ func TestStore_IntentionSet_metaSet(t *testing.T) { } // Insert - assert.Nil(s.IntentionSet(1, &ixn)) + assert.NoError(s.IntentionSet(1, &ixn)) // Read it back and verify _, actual, err := s.IntentionGet(nil, ixn.ID) - assert.Nil(err) + assert.NoError(err) assert.Equal(ixn.Meta, actual.Meta) } @@ -191,18 +197,18 @@ func TestStore_IntentionDelete(t *testing.T) { // Call Get to populate the watch set ws := memdb.NewWatchSet() _, _, err := s.IntentionGet(ws, testUUID()) - assert.Nil(err) + assert.NoError(err) // Create ixn := &structs.Intention{ID: testUUID()} - assert.Nil(s.IntentionSet(1, ixn)) + assert.NoError(s.IntentionSet(1, ixn)) // Make sure the index got updated. assert.Equal(s.maxIndex(intentionsTableName), uint64(1)) assert.True(watchFired(ws), "watch fired") // Delete - assert.Nil(s.IntentionDelete(2, ixn.ID)) + assert.NoError(s.IntentionDelete(2, ixn.ID)) // Make sure the index got updated. assert.Equal(s.maxIndex(intentionsTableName), uint64(2)) @@ -210,7 +216,7 @@ func TestStore_IntentionDelete(t *testing.T) { // Sanity check to make sure it's not there. idx, actual, err := s.IntentionGet(nil, ixn.ID) - assert.Nil(err) + assert.NoError(err) assert.Equal(idx, uint64(2)) assert.Nil(actual) } @@ -222,7 +228,7 @@ func TestStore_IntentionsList(t *testing.T) { // Querying with no results returns nil. ws := memdb.NewWatchSet() idx, res, err := s.Intentions(ws) - assert.Nil(err) + assert.NoError(err) assert.Nil(res) assert.Equal(idx, uint64(0)) @@ -244,7 +250,7 @@ func TestStore_IntentionsList(t *testing.T) { // Create for i, ixn := range ixns { - assert.Nil(s.IntentionSet(uint64(1+i), ixn)) + assert.NoError(s.IntentionSet(uint64(1+i), ixn)) } assert.True(watchFired(ws), "watch fired") @@ -268,7 +274,7 @@ func TestStore_IntentionsList(t *testing.T) { }, } idx, actual, err := s.Intentions(nil) - assert.Nil(err) + assert.NoError(err) assert.Equal(idx, uint64(2)) assert.Equal(expected, actual) } @@ -386,7 +392,7 @@ func TestStore_IntentionMatch_table(t *testing.T) { } } - assert.Nil(s.IntentionSet(idx, ixn)) + assert.NoError(s.IntentionSet(idx, ixn)) idx++ } @@ -402,7 +408,7 @@ func TestStore_IntentionMatch_table(t *testing.T) { // Match _, matches, err := s.IntentionMatch(nil, args) - assert.Nil(err) + assert.NoError(err) // Should have equal lengths require.Len(t, matches, len(tc.Expected)) @@ -478,7 +484,7 @@ func TestStore_Intention_Snapshot_Restore(t *testing.T) { // Now create for i, ixn := range ixns { - assert.Nil(s.IntentionSet(uint64(4+i), ixn)) + assert.NoError(s.IntentionSet(uint64(4+i), ixn)) } // Snapshot the queries. @@ -486,7 +492,7 @@ func TestStore_Intention_Snapshot_Restore(t *testing.T) { defer snap.Close() // Alter the real state store. - assert.Nil(s.IntentionDelete(7, ixns[0].ID)) + assert.NoError(s.IntentionDelete(7, ixns[0].ID)) // Verify the snapshot. assert.Equal(snap.LastIndex(), uint64(6)) @@ -520,7 +526,7 @@ func TestStore_Intention_Snapshot_Restore(t *testing.T) { }, } dump, err := snap.Intentions() - assert.Nil(err) + assert.NoError(err) assert.Equal(expected, dump) // Restore the values into a new state store. @@ -528,13 +534,13 @@ func TestStore_Intention_Snapshot_Restore(t *testing.T) { s := testStateStore(t) restore := s.Restore() for _, ixn := range dump { - assert.Nil(restore.Intention(ixn)) + assert.NoError(restore.Intention(ixn)) } restore.Commit() // Read the restored values back out and verify that they match. idx, actual, err := s.Intentions(nil) - assert.Nil(err) + assert.NoError(err) assert.Equal(idx, uint64(6)) assert.Equal(expected, actual) }() diff --git a/agent/local/state_test.go b/agent/local/state_test.go index a8890a540a..16975d9633 100644 --- a/agent/local/state_test.go +++ b/agent/local/state_test.go @@ -6,10 +6,11 @@ import ( "log" "os" "reflect" - "sync" "testing" "time" + "github.com/hashicorp/go-memdb" + "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent" @@ -1708,20 +1709,6 @@ func TestStateProxyManagement(t *testing.T) { }, "fake-token-db") require.NoError(err) - // Record initial local modify index - lastModifyIndex := state.LocalModifyIndex() - assertModIndexUpdate := func(id string) { - t.Helper() - nowIndex := state.LocalModifyIndex() - assert.True(lastModifyIndex < nowIndex) - if id != "" { - p := state.Proxy(id) - require.NotNil(p) - assert.True(lastModifyIndex < p.ModifyIndex) - } - lastModifyIndex = nowIndex - } - // Should work now svc, err := state.AddProxy(&p1, "fake-token") require.NoError(err) @@ -1733,7 +1720,6 @@ func TestStateProxyManagement(t *testing.T) { assert.Equal("", svc.Address, "should have empty address by default") // Port is non-deterministic but could be either of 20000 or 20001 assert.Contains([]int{20000, 20001}, svc.Port) - assertModIndexUpdate(svc.ID) // Second proxy should claim other port p2 := p1 @@ -1742,7 +1728,6 @@ func TestStateProxyManagement(t *testing.T) { require.NoError(err) assert.Contains([]int{20000, 20001}, svc2.Port) assert.NotEqual(svc.Port, svc2.Port) - assertModIndexUpdate(svc2.ID) // Store this for later p2token := state.Proxy(svc2.ID).ProxyToken @@ -1762,7 +1747,6 @@ func TestStateProxyManagement(t *testing.T) { require.NoError(err) require.Equal("0.0.0.0", svc3.Address) require.Equal(1234, svc3.Port) - assertModIndexUpdate(svc3.ID) // Update config of an already registered proxy should work p3updated := p3 @@ -1770,14 +1754,8 @@ func TestStateProxyManagement(t *testing.T) { // Setup multiple watchers who should all witness the change gotP3 := state.Proxy(svc3.ID) require.NotNil(gotP3) - var watchWg sync.WaitGroup - for i := 0; i < 3; i++ { - watchWg.Add(1) - go func() { - <-gotP3.WatchCh - watchWg.Done() - }() - } + var ws memdb.WatchSet + ws.Add(gotP3.WatchCh) svc3, err = state.AddProxy(&p3updated, "fake-token") require.NoError(err) require.Equal("0.0.0.0", svc3.Address) @@ -1785,9 +1763,8 @@ func TestStateProxyManagement(t *testing.T) { gotProxy3 := state.Proxy(svc3.ID) require.NotNil(gotProxy3) require.Equal(p3updated.Config, gotProxy3.Proxy.Config) - assertModIndexUpdate(svc3.ID) // update must change mod index - // All watchers should have fired so this should not hang the test! - watchWg.Wait() + assert.False(ws.Watch(time.After(500*time.Millisecond)), + "watch should have fired so ws.Watch should not timeout") // Remove one of the auto-assigned proxies err = state.RemoveProxy(svc2.ID) @@ -1800,7 +1777,6 @@ func TestStateProxyManagement(t *testing.T) { require.NoError(err) assert.Contains([]int{20000, 20001}, svc2.Port) assert.Equal(svc4.Port, svc2.Port, "should get the same port back that we freed") - assertModIndexUpdate(svc4.ID) // Remove a proxy that doesn't exist should error err = state.RemoveProxy("nope") diff --git a/connect/proxy/listener.go b/connect/proxy/listener.go index c003cb19c3..51ab761ca5 100644 --- a/connect/proxy/listener.go +++ b/connect/proxy/listener.go @@ -87,7 +87,6 @@ func (l *Listener) Serve() error { go l.handleConn(conn) } - return nil } // handleConn is the internal connection handler goroutine. diff --git a/connect/testing.go b/connect/testing.go index 235ff60018..9f6e4f781d 100644 --- a/connect/testing.go +++ b/connect/testing.go @@ -161,8 +161,6 @@ func (s *TestServer) Serve() error { c.Close() }(conn) } - - return nil } // ServeHTTPS runs an HTTPS server with the given config. It invokes the passed diff --git a/connect/tls_test.go b/connect/tls_test.go index 64c473c1e4..d13b786615 100644 --- a/connect/tls_test.go +++ b/connect/tls_test.go @@ -16,7 +16,7 @@ func TestReloadableTLSConfig(t *testing.T) { // The dynamic config should be the one we loaded (with some different hooks) got := c.TLSConfig() - expect := *base + expect := base.Clone() // Equal and even cmp.Diff fail on tls.Config due to unexported fields in // each. Compare a few things to prove it's returning the bits we // specifically set. @@ -39,7 +39,7 @@ func TestReloadableTLSConfig(t *testing.T) { // Change the passed config to ensure SetTLSConfig made a copy otherwise this // is racey. - expect = *new + expect = new.Clone() new.Certificates = nil // The dynamic config should be the one we loaded (with some different hooks)