Refactor some of the TUF metadata storage tests to be more general, s…

…o they can be applied to rethink.

This also fixes the following bugs:

1.  MemStorage's UpdateMany previously did a partial update even if there was a conflict.
    Now we check for conflicts before updating.
2.  MemStorage's UpdateMany allowed you to insert the same version twice.
3.  RethinkDB's UpdateMany does a partial update even if there was a conflict, because the
    deletion criteria value (for timestamp_checksum) was passed as a list not as strings.
4.  RethinkDB's delete did not actually delete metadata, because the deletion criteria value
    (for gun) was passed as a list and not as strings.

Signed-off-by: Ying Li <[email protected]>

buildscripts/integrationtest.sh

@@ -30,7 +30,7 @@ fi
 set -e
 set -x
 
-# cleanup
+cleanup
 
 docker-compose -f $composeFile config
 docker-compose -f $composeFile build ${BUILDOPTS} --pull | tee
@@ -63,4 +63,4 @@ esac
 
 docker-compose -f $composeFile down -v
 
-# docker-compose -f $composeFile up --abort-on-container-exit
+docker-compose -f $composeFile up --abort-on-container-exit

development.rethink.yml

@@ -87,13 +87,14 @@ services:
         - rdb-02
         - rdb-03
     client:
+      build:
+        context: .
+        dockerfile: Dockerfile
       volumes:
         - ./test_output:/test_output
       networks:
         - rdb
-      build:
-        context: .
-        dockerfile: Dockerfile
+      env_file: buildscripts/env.list
       links:
         - server:notary-server
       command: buildscripts/testclient.sh

server/storage/memory.go

@@ -65,10 +65,47 @@ func (st *MemStorage) UpdateCurrent(gun string, update MetaUpdate) error {
 
 // UpdateMany updates multiple TUF records
 func (st *MemStorage) UpdateMany(gun string, updates []MetaUpdate) error {
+	st.lock.Lock()
+	defer st.lock.Unlock()
+
+	versioner := make(map[string]map[int]struct{})
+	constant := struct{}{}
+
+	// ensure that we only update in one transaction
 	for _, u := range updates {
-		if err := st.UpdateCurrent(gun, u); err != nil {
-			return err
+		id := entryKey(gun, u.Role)
+
+		// prevent duplicate versions of the same role
+		if _, ok := versioner[u.Role][u.Version]; ok {
+			return &ErrOldVersion{}
+		}
+		if _, ok := versioner[u.Role]; !ok {
+			versioner[u.Role] = make(map[int]struct{})
+		}
+		versioner[u.Role][u.Version] = constant
+
+		if space, ok := st.tufMeta[id]; ok {
+			for _, v := range space {
+				if v.version >= u.Version {
+					return &ErrOldVersion{}
+				}
+			}
+		}
+	}
+
+	for _, u := range updates {
+		id := entryKey(gun, u.Role)
+
+		version := ver{version: u.Version, data: u.Data, createupdate: time.Now()}
+		st.tufMeta[id] = append(st.tufMeta[id], &version)
+		checksumBytes := sha256.Sum256(u.Data)
+		checksum := hex.EncodeToString(checksumBytes[:])
+
+		_, ok := st.checksums[gun]
+		if !ok {
+			st.checksums[gun] = make(map[string]ver)
 		}
+		st.checksums[gun][checksum] = version
 	}
 	return nil
 }

server/storage/memory_test.go

@@ -9,38 +9,58 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestUpdateCurrent(t *testing.T) {
-	s := NewMemStorage()
-	s.UpdateCurrent("gun", MetaUpdate{"role", 1, []byte("test")})
+func assertExpectedMemoryTUFMeta(t *testing.T, expected []tufMeta, s *MemStorage) {
+	counter := make(map[string]int)
+	for _, tufObj := range expected {
+		k := entryKey(tufObj.Gun, tufObj.Role)
+		gun, ok := s.tufMeta[k]
+		require.True(t, len(gun) >= counter[k])
+		v := gun[counter[k]]
+		require.True(t, ok, "Did not find gun in store")
+		require.Equal(t, tufObj.Version, v.version, "Version mismatch. Expected %d, found %d",
+			tufObj.Version, v.version)
+		require.Equal(t, tufObj.Data, v.data, "Data was incorrect")
+		counter[k]++
+	}
+}
 
-	k := entryKey("gun", "role")
-	gun, ok := s.tufMeta[k]
-	v := gun[0]
-	require.True(t, ok, "Did not find gun in store")
-	require.Equal(t, 1, v.version, "Version mismatch. Expected 1, found %d", v.version)
-	require.Equal(t, []byte("test"), v.data, "Data was incorrect")
+// UpdateCurrent should succeed if there was no previous metadata of the same
+// gun and role.  They should be gettable.
+func TestMemoryUpdateCurrentEmpty(t *testing.T) {
+	s := NewMemStorage()
+	expected := testUpdateCurrentEmptyStore(t, s)
+	assertExpectedMemoryTUFMeta(t, expected, s)
 }
 
-func TestUpdateMany(t *testing.T) {
+// UpdateCurrent will successfully add a new (higher) version of an existing TUF file,
+// but will return an error if there is an older version of a TUF file.
+func TestMemoryUpdateCurrentVersionCheck(t *testing.T) {
 	s := NewMemStorage()
-	require.NoError(t, s.UpdateMany("gun", []MetaUpdate{
-		{"role1", 1, []byte("test1")},
-		{"role2", 1, []byte("test2")},
-	}))
+	expected := testUpdateCurrentVersionCheck(t, s)
+	assertExpectedMemoryTUFMeta(t, expected, s)
+}
 
-	_, d, err := s.GetCurrent("gun", "role1")
-	require.Nil(t, err, "Expected error to be nil")
-	require.Equal(t, []byte("test1"), d, "Data was incorrect")
+// UpdateMany succeeds if the updates do not conflict with each other or with what's
+// already in the DB
+func TestMemoryUpdateManyNoConflicts(t *testing.T) {
+	s := NewMemStorage()
+	expected := testUpdateManyNoConflicts(t, s)
+	assertExpectedMemoryTUFMeta(t, expected, s)
+}
 
-	_, d, err = s.GetCurrent("gun", "role2")
-	require.Nil(t, err, "Expected error to be nil")
-	require.Equal(t, []byte("test2"), d, "Data was incorrect")
+// UpdateMany does not insert any rows (or at least rolls them back) if there
+// are any conflicts.
+func TestMemoryUpdateManyConflictRollback(t *testing.T) {
+	s := NewMemStorage()
+	expected := testUpdateManyConflictRollback(t, s)
+	assertExpectedMemoryTUFMeta(t, expected, s)
+}
 
-	// updating even one with an equal version fails
-	require.IsType(t, &ErrOldVersion{}, s.UpdateMany("gun", []MetaUpdate{
-		{"role1", 1, []byte("test1")},
-		{"role2", 2, []byte("test2")},
-	}))
+// Delete will remove all TUF metadata, all versions, associated with a gun
+func TestMemoryDeleteSuccess(t *testing.T) {
+	s := NewMemStorage()
+	testDeleteSuccess(t, s)
+	assertExpectedMemoryTUFMeta(t, nil, s)
 }
 
 func TestGetCurrent(t *testing.T) {
@@ -55,16 +75,6 @@ func TestGetCurrent(t *testing.T) {
 	require.Equal(t, []byte("test"), d, "Data was incorrect")
 }
 
-func TestDelete(t *testing.T) {
-	s := NewMemStorage()
-	s.UpdateCurrent("gun", MetaUpdate{"role", 1, []byte("test")})
-	s.Delete("gun")
-
-	k := entryKey("gun", "role")
-	_, ok := s.tufMeta[k]
-	require.False(t, ok, "Found gun in store, should have been deleted")
-}
-
 func TestGetTimestampKey(t *testing.T) {
 	s := NewMemStorage()
 

server/storage/rethink_realdb_test.go

@@ -28,6 +28,19 @@ func rethinkSessionSetup(t *testing.T) (*gorethink.Session, string) {
 	return sess, rethinkSource
 }
 
+func rethinkDBSetup(t *testing.T) (RethinkDB, func()) {
+	session, _ := rethinkSessionSetup(t)
+	dbName := "testdb"
+	var cleanup = func() { gorethink.DBDrop(dbName).Exec(session) }
+
+	cleanup()
+	require.NoError(t, rethinkdb.SetupDB(session, dbName, []rethinkdb.Table{
+		TUFFilesRethinkTable,
+		PubKeysRethinkTable,
+	}))
+	return NewRethinkDBStorage(dbName, "", "", session), cleanup
+}
+
 func TestBootstrapSetsUsernamePassword(t *testing.T) {
 	adminSession, source := rethinkSessionSetup(t)
 	dbname, username, password := "testdb", "testuser", "testpassword"
@@ -59,3 +72,48 @@ func TestBootstrapSetsUsernamePassword(t *testing.T) {
 	require.Error(t, err)
 	require.IsType(t, ErrNotFound{}, err)
 }
+
+// UpdateCurrent will add a new TUF file if no previous version of that gun and role existed.
+func TestRethinkUpdateCurrentEmpty(t *testing.T) {
+	dbStore, cleanup := rethinkDBSetup(t)
+	defer cleanup()
+
+	testUpdateCurrentEmptyStore(t, dbStore)
+}
+
+// UpdateCurrent will add a new TUF file if the version is higher than previous, but fail
+// if the version is equal to or less than the current, whether or not that previous
+// version exists
+func TestRethinkUpdateCurrentVersionCheck(t *testing.T) {
+	t.Skip("Currently rethink only errors if the previous version exists - it doesn't check for strictly increasing")
+	dbStore, cleanup := rethinkDBSetup(t)
+	defer cleanup()
+
+	testUpdateCurrentVersionCheck(t, dbStore)
+}
+
+// UpdateMany succeeds if the updates do not conflict with each other or with what's
+// already in the DB
+func TestRethinkUpdateManyNoConflicts(t *testing.T) {
+	dbStore, cleanup := rethinkDBSetup(t)
+	defer cleanup()
+
+	testUpdateManyNoConflicts(t, dbStore)
+}
+
+// UpdateMany does not insert any rows (or at least rolls them back) if there
+// are any conflicts.
+func TestRethinkUpdateManyConflictRollback(t *testing.T) {
+	dbStore, cleanup := rethinkDBSetup(t)
+	defer cleanup()
+
+	testUpdateManyConflictRollback(t, dbStore)
+}
+
+// Delete will remove all TUF metadata, all versions, associated with a gun
+func TestRethinkDeleteSuccess(t *testing.T) {
+	dbStore, cleanup := rethinkDBSetup(t)
+	defer cleanup()
+
+	testDeleteSuccess(t, dbStore)
+}

server/storage/rethinkdb.go

@@ -8,6 +8,7 @@ import (
 	"sort"
 	"time"
 
+	"github.com/Sirupsen/logrus"
 	"github.com/docker/notary/storage/rethinkdb"
 	"github.com/docker/notary/tuf/data"
 	"gopkg.in/dancannon/gorethink.v2"
@@ -233,7 +234,11 @@ func (rdb RethinkDB) UpdateMany(gun string, updates []MetaUpdate) error {
 	for _, up := range updates {
 		if err := rdb.UpdateCurrentWithTSChecksum(gun, tsChecksum, up); err != nil {
 			// roll back with best-effort deletion, and then error out
-			rdb.deleteByTSChecksum(tsChecksum)
+			rollbackErr := rdb.deleteByTSChecksum(tsChecksum)
+			if rollbackErr != nil {
+				logrus.Errorf("Unable to rollback DB conflict - items with timestamp_checksum %s: %v",
+					tsChecksum, rollbackErr)
+			}
 			return err
 		}
 	}
@@ -288,7 +293,7 @@ func (rdb RethinkDB) GetChecksum(gun, role, checksum string) (created *time.Time
 // error if no metadata exists for the given GUN.
 func (rdb RethinkDB) Delete(gun string) error {
 	_, err := gorethink.DB(rdb.dbName).Table(RDBTUFFile{}.TableName()).GetAllByIndex(
-		"gun", []string{gun},
+		"gun", gun,
 	).Delete().RunWrite(rdb.sess)
 	if err != nil {
 		return fmt.Errorf("unable to delete %s from database: %s", gun, err.Error())
@@ -300,10 +305,10 @@ func (rdb RethinkDB) Delete(gun string) error {
 // from a call to rethinkdb's UpdateMany
 func (rdb RethinkDB) deleteByTSChecksum(tsChecksum string) error {
 	_, err := gorethink.DB(rdb.dbName).Table(RDBTUFFile{}.TableName()).GetAllByIndex(
-		"timestamp_checksum", []string{tsChecksum},
+		"timestamp_checksum", tsChecksum,
 	).Delete().RunWrite(rdb.sess)
 	if err != nil {
-		return fmt.Errorf("unable to delete timestamp checksum data: %s from database: %s", tsChecksum, err.Error())
+		return fmt.Errorf("unable to delete timestamp checksum data: %s from database: %v", tsChecksum, err.Error())
 	}
 	return nil
 }