From 8417f6670b24308b3003084639e22422fdc0fa4c Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 4 Dec 2015 09:55:52 -0800 Subject: [PATCH] KeyDBStore refactor so that it just directly takes the DB arguments. Rather than create an SQL DB, then create a gorm BD using the SQL DB. Also split the Create/Get test into two tests. Signed-off-by: Ying Li --- cmd/notary-signer/main.go | 11 +- signer/keydbstore/keydbstore.go | 15 +-- signer/keydbstore/keydbstore_test.go | 155 +++++++++++++++------------ 3 files changed, 99 insertions(+), 82 deletions(-) diff --git a/cmd/notary-signer/main.go b/cmd/notary-signer/main.go index 91b66f72c1..f609ce9a0b 100644 --- a/cmd/notary-signer/main.go +++ b/cmd/notary-signer/main.go @@ -4,7 +4,6 @@ package main import ( "crypto/tls" - "database/sql" "errors" _ "expvar" "flag" @@ -93,18 +92,12 @@ func setUpCryptoservices(configuration *viper.Viper, allowedBackends []string) ( } logrus.Debug("Default Alias: ", defaultAlias) - dbSQL, err := sql.Open(storeConfig.Backend, storeConfig.Source) - if err != nil { - return nil, fmt.Errorf("failed to open the %s database: %s, %v", - storeConfig.Backend, storeConfig.Source, err) - } - logrus.Debugf("Using %s DB: %s", storeConfig.Backend, storeConfig.Source) - dbStore, err := keydbstore.NewKeyDBStore( - passphraseRetriever, defaultAlias, storeConfig.Backend, dbSQL) + passphraseRetriever, defaultAlias, storeConfig.Backend, storeConfig.Source) if err != nil { return nil, fmt.Errorf("failed to create a new keydbstore: %v", err) } + logrus.Debugf("Using %s DB: %s", storeConfig.Backend, storeConfig.Source) health.RegisterPeriodicFunc( "DB operational", dbStore.HealthCheck, time.Second*60) diff --git a/signer/keydbstore/keydbstore.go b/signer/keydbstore/keydbstore.go index 2f9813932f..a0a48163e7 100644 --- a/signer/keydbstore/keydbstore.go +++ b/signer/keydbstore/keydbstore.go @@ -1,7 +1,6 @@ package keydbstore import ( - "database/sql" "errors" "fmt" "sync" @@ -46,13 +45,17 @@ func (g GormPrivateKey) TableName() string { } // NewKeyDBStore returns a new KeyDBStore backed by a SQL database -func NewKeyDBStore(passphraseRetriever passphrase.Retriever, defaultPassAlias, dbType string, dbSQL *sql.DB) (*KeyDBStore, error) { +func NewKeyDBStore(passphraseRetriever passphrase.Retriever, defaultPassAlias string, + dbDialect string, dbArgs ...interface{}) (*KeyDBStore, error) { cachedKeys := make(map[string]data.PrivateKey) - // Open a connection to our database - db, _ := gorm.Open(dbType, dbSQL) + db, err := gorm.Open(dbDialect, dbArgs...) + if err != nil { + return nil, err + } - return &KeyDBStore{db: db, + return &KeyDBStore{ + db: db, defaultPassAlias: defaultPassAlias, retriever: passphraseRetriever, cachedKeys: cachedKeys}, nil @@ -88,7 +91,7 @@ func (s *KeyDBStore) AddKey(name, alias string, privKey data.PrivateKey) error { // Add encrypted private key to the database s.db.Create(&gormPrivKey) - // Value will be false if Create suceeds + // Value will be false if Create succeeds failure := s.db.NewRecord(gormPrivKey) if failure { return fmt.Errorf("failed to add private key to database: %s", privKey.ID()) diff --git a/signer/keydbstore/keydbstore_test.go b/signer/keydbstore/keydbstore_test.go index d22e5a590f..b0c9dbaef2 100644 --- a/signer/keydbstore/keydbstore_test.go +++ b/signer/keydbstore/keydbstore_test.go @@ -2,13 +2,15 @@ package keydbstore import ( "crypto/rand" - "database/sql" "errors" "io/ioutil" "os" + "path/filepath" "testing" "github.com/docker/notary/trustmanager" + "github.com/docker/notary/tuf/data" + "github.com/jinzhu/gorm" _ "github.com/mattn/go-sqlite3" "github.com/stretchr/testify/assert" ) @@ -27,97 +29,126 @@ var anotherRetriever = func(keyName, alias string, createNew bool, attempts int) return "", false, errors.New("password alias no found") } -func TestCreateRead(t *testing.T) { - tempBaseDir, err := ioutil.TempDir("", "notary-test-") - defer os.RemoveAll(tempBaseDir) - - testKey, err := trustmanager.GenerateECDSAKey(rand.Reader) +// Create a temporary file, open a database connection to it, and create the +// necessary table. Return the file name to use and clean up. +func initializeDB(t *testing.T) (tmpfilename string) { + tmpFile, err := ioutil.TempFile("/tmp", "notary-test-sqlite-db-") assert.NoError(t, err) + tmpFile.Close() // We are using SQLite for the tests - db, err := sql.Open("sqlite3", tempBaseDir+"test_db") - assert.NoError(t, err) - - // Create a new KeyDB store - dbStore, err := NewKeyDBStore(retriever, "", "sqlite3", db) + gormDB, err := gorm.Open("sqlite3", tmpFile.Name()) assert.NoError(t, err) // Ensure that the private_key table exists - dbStore.db.CreateTable(&GormPrivateKey{}) + gormDB.CreateTable(&GormPrivateKey{}) - // Test writing new key in database/cache - err = dbStore.AddKey("", "", testKey) + return tmpFile.Name() +} + +// gets a key from the DB store, and asserts that the key is the expected key +func testGetSuccess(t *testing.T, dbStore *KeyDBStore, expectedKey data.PrivateKey) { + retrKey, _, err := dbStore.GetKey(expectedKey.ID()) + assert.NoError(t, err) + assert.Equal(t, retrKey, expectedKey) +} + +// closes the DB connection first so we can test that the successful get was +// from the cache +func testGetSuccessFromCache(t *testing.T, dbStore *KeyDBStore, + expectedKey data.PrivateKey) { + + err := dbStore.db.Close() assert.NoError(t, err) - // Test retrieval of key from DB - delete(dbStore.cachedKeys, testKey.ID()) + testGetSuccess(t, dbStore, expectedKey) +} - retrKey, _, err := dbStore.GetKey(testKey.ID()) - assert.NoError(t, err) - assert.Equal(t, retrKey, testKey) +// Creating a new KeyDBStore propogates any db opening error +func TestNewKeyDBStorePropogatesDBError(t *testing.T) { + dbStore, err := NewKeyDBStore(retriever, "ignoredalias", "nodb", "somestring") + assert.Error(t, err) + assert.Nil(t, dbStore) +} - // Tests retrieval of key from Cache - // Close database connection - err = dbStore.db.Close() +// Creating a key, on succcess, populates the cache. +func TestCreateSuccessPopulatesCache(t *testing.T) { + testKey, err := trustmanager.GenerateECDSAKey(rand.Reader) assert.NoError(t, err) - retrKey, _, err = dbStore.GetKey(testKey.ID()) + tmpFilename := initializeDB(t) + defer os.Remove(tmpFilename) + + // Create a new KeyDB store + dbStore, err := NewKeyDBStore(retriever, "ignoredalias", "sqlite3", tmpFilename) assert.NoError(t, err) - assert.Equal(t, retrKey, testKey) + + // Test writing new key in database + err = dbStore.AddKey("gun/ignored", data.CanonicalTimestampRole, testKey) + assert.NoError(t, err) + + testGetSuccessFromCache(t, dbStore, testKey) +} + +// Getting a key, on succcess, populates the cache. +func TestGetSuccessPopulatesCache(t *testing.T) { + testKey, err := trustmanager.GenerateECDSAKey(rand.Reader) + assert.NoError(t, err) + + tmpFilename := initializeDB(t) + defer os.Remove(tmpFilename) + + // Create a new KeyDB store and add a key + dbStore, err := NewKeyDBStore(retriever, "ignoredalias", "sqlite3", tmpFilename) + assert.NoError(t, err) + err = dbStore.AddKey("gun/ignored", data.CanonicalTimestampRole, testKey) + assert.NoError(t, err) + + // delete the cache + dbStore.cachedKeys = make(map[string]data.PrivateKey) + + testGetSuccess(t, dbStore, testKey) + testGetSuccessFromCache(t, dbStore, testKey) } func TestDoubleCreate(t *testing.T) { - tempBaseDir, err := ioutil.TempDir("", "notary-test-") - defer os.RemoveAll(tempBaseDir) - testKey, err := trustmanager.GenerateECDSAKey(rand.Reader) assert.NoError(t, err) anotherTestKey, err := trustmanager.GenerateECDSAKey(rand.Reader) assert.NoError(t, err) - // We are using SQLite for the tests - db, err := sql.Open("sqlite3", tempBaseDir+"test_db") - assert.NoError(t, err) + tmpFilename := initializeDB(t) + defer os.Remove(tmpFilename) - // Create a new KeyDB store - dbStore, err := NewKeyDBStore(retriever, "", "sqlite3", db) + // Create a new KeyDB store and add a key + dbStore, err := NewKeyDBStore(retriever, "ignoredalias", "sqlite3", tmpFilename) assert.NoError(t, err) - // Ensure that the private_key table exists - dbStore.db.CreateTable(&GormPrivateKey{}) - // Test writing new key in database/cache - err = dbStore.AddKey("", "", testKey) + err = dbStore.AddKey("gun/ignored", data.CanonicalTimestampRole, testKey) assert.NoError(t, err) // Test writing the same key in the database. Should fail. - err = dbStore.AddKey("", "", testKey) + err = dbStore.AddKey("gun/ignored", data.CanonicalTimestampRole, testKey) assert.Error(t, err, "failed to add private key to database:") // Test writing new key succeeds - err = dbStore.AddKey("", "", anotherTestKey) + err = dbStore.AddKey("gun/ignored", data.CanonicalTimestampRole, anotherTestKey) assert.NoError(t, err) } func TestCreateDelete(t *testing.T) { - tempBaseDir, err := ioutil.TempDir("", "notary-test-") - defer os.RemoveAll(tempBaseDir) - testKey, err := trustmanager.GenerateECDSAKey(rand.Reader) assert.NoError(t, err) - // We are using SQLite for the tests - db, err := sql.Open("sqlite3", tempBaseDir+"test_db") - assert.NoError(t, err) + tmpFilename := initializeDB(t) + defer os.Remove(tmpFilename) // Create a new KeyDB store - dbStore, err := NewKeyDBStore(retriever, "", "sqlite3", db) + dbStore, err := NewKeyDBStore(retriever, "ignoredalias", "sqlite3", tmpFilename) assert.NoError(t, err) - // Ensure that the private_key table exists - dbStore.db.CreateTable(&GormPrivateKey{}) - // Test writing new key in database/cache err = dbStore.AddKey("", "", testKey) assert.NoError(t, err) @@ -126,31 +157,24 @@ func TestCreateDelete(t *testing.T) { err = dbStore.RemoveKey(testKey.ID()) assert.NoError(t, err) - // This should fail + // This should fail, since it is neither in the cache nor the DB _, _, err = dbStore.GetKey(testKey.ID()) assert.Error(t, err, "signing key not found:") } func TestKeyRotation(t *testing.T) { - tempBaseDir, err := ioutil.TempDir("", "notary-test-") - defer os.RemoveAll(tempBaseDir) - testKey, err := trustmanager.GenerateECDSAKey(rand.Reader) assert.NoError(t, err) - // We are using SQLite for the tests - db, err := sql.Open("sqlite3", tempBaseDir+"test_db") - assert.NoError(t, err) + tmpFilename := initializeDB(t) + defer os.Remove(tmpFilename) // Create a new KeyDB store - dbStore, err := NewKeyDBStore(anotherRetriever, "alias_1", "sqlite3", db) + dbStore, err := NewKeyDBStore(anotherRetriever, "alias_1", "sqlite3", tmpFilename) assert.NoError(t, err) - // Ensure that the private_key table exists - dbStore.db.CreateTable(&GormPrivateKey{}) - // Test writing new key in database/cache - err = dbStore.AddKey("", "", testKey) + err = dbStore.AddKey("gun/ignore", data.CanonicalTimestampRole, testKey) assert.NoError(t, err) // Try rotating the key to alias-2 @@ -159,19 +183,16 @@ func TestKeyRotation(t *testing.T) { // Try rotating the key to alias-3 err = dbStore.RotateKeyPassphrase(testKey.ID(), "alias_3") - assert.Error(t, err, "password alias no found") + assert.Error(t, err, "there should be no password for alias_3") } func TestDBHealthCheck(t *testing.T) { - tempBaseDir, err := ioutil.TempDir("", "notary-test-") + tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-") defer os.RemoveAll(tempBaseDir) - // We are using SQLite for the tests - db, err := sql.Open("sqlite3", tempBaseDir+"test_db") - assert.NoError(t, err) - // Create a new KeyDB store - dbStore, err := NewKeyDBStore(retriever, "", "sqlite3", db) + dbStore, err := NewKeyDBStore(retriever, "ignoredalias", + "sqlite3", filepath.Join(tempBaseDir, "test_db")) assert.NoError(t, err) // No key table, health check fails