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

@@ -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)

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)

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.

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)
 	}()

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
+	var ws memdb.WatchSet
-	for i := 0; i < 3; i++ {
+	ws.Add(gotP3.WatchCh)
-		watchWg.Add(1)
-		go func() {
-			<-gotP3.WatchCh
-			watchWg.Done()
-		}()
-	}
 	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
+	assert.False(ws.Watch(time.After(500*time.Millisecond)),
-	// All watchers should have fired so this should not hang the test!
+		"watch should have fired so ws.Watch should not timeout")
-	watchWg.Wait()
 
 	// 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")

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.

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

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)