From 671a2ed80430d9534cc2785c3d523748730e80e0 Mon Sep 17 00:00:00 2001 From: Andrew Harding Date: Thu, 16 Aug 2018 15:52:39 -0600 Subject: [PATCH 1/3] remove soft-delete support + migration - removes soft-delete support and drops soft-deleted rows - naive forward-only migration support - moves test data into idiomatic "testdata" folder Signed-off-by: Andrew Harding --- pkg/server/plugin/datastore/sql/migration.go | 132 ++++++++++++++++++ pkg/server/plugin/datastore/sql/models.go | 32 +++-- pkg/server/plugin/datastore/sql/sql.go | 16 ++- pkg/server/plugin/datastore/sql/sql_test.go | 75 +++++++++- pkg/server/plugin/datastore/sql/sqlite.go | 6 - .../invalid_registration_entries.json | 0 .../sql/testdata/migration/v0.sqlite3 | Bin 0 -> 98304 bytes .../valid_registration_entries.json | 0 8 files changed, 234 insertions(+), 27 deletions(-) create mode 100644 pkg/server/plugin/datastore/sql/migration.go rename pkg/server/plugin/datastore/sql/{_test_data => testdata}/invalid_registration_entries.json (100%) create mode 100644 pkg/server/plugin/datastore/sql/testdata/migration/v0.sqlite3 rename pkg/server/plugin/datastore/sql/{_test_data => testdata}/valid_registration_entries.json (100%) diff --git a/pkg/server/plugin/datastore/sql/migration.go b/pkg/server/plugin/datastore/sql/migration.go new file mode 100644 index 0000000000..036aa88515 --- /dev/null +++ b/pkg/server/plugin/datastore/sql/migration.go @@ -0,0 +1,132 @@ +package sql + +import ( + "fmt" + + "github.com/jinzhu/gorm" + "github.com/sirupsen/logrus" +) + +const ( + // version of the database in the code + codeVersion = 1 +) + +func migrateDB(db *gorm.DB) (err error) { + isNew := !db.HasTable(&Bundle{}) + if err := db.Error; err != nil { + return err + } + + if isNew { + return initDB(db) + } + + if err := db.AutoMigrate(&Migration{}).Error; err != nil { + return err + } + + migration := new(Migration) + if err := db.Assign(Migration{}).FirstOrCreate(migration).Error; err != nil { + return err + } + version := migration.Version + + if version > codeVersion { + err = fmt.Errorf("backwards migration not supported! (current=%d, code=%d)", version, codeVersion) + logrus.Error(err) + return err + } + + logrus.Infof("running migrations...") + for version < codeVersion { + tx := db.Begin() + version, err = migrateVersion(tx, version) + if err != nil { + tx.Rollback() + return err + } + if err := tx.Commit().Error; err != nil { + return err + } + } + + logrus.Infof("done running migrations.") + return nil +} + +func initDB(db *gorm.DB) (err error) { + logrus.Infof("initializing database.") + tx := db.Begin() + if err := tx.Error; err != nil { + return err + } + + defer func() { + if err != nil { + tx.Rollback() + } else { + err = tx.Commit().Error + } + }() + + if err := tx.AutoMigrate(&Bundle{}, &CACert{}, &AttestedNodeEntry{}, + &NodeResolverMapEntry{}, &RegisteredEntry{}, &JoinToken{}, + &Selector{}, &Migration{}).Error; err != nil { + return err + } + + if err := tx.Assign(Migration{Version: codeVersion}).FirstOrCreate(&Migration{}).Error; err != nil { + return err + } + + return nil +} + +func migrateVersion(tx *gorm.DB, version int) (versionOut int, err error) { + logrus.Infof("migrating from version %d", version) + + // When a new version is added and entry here must be included that knows + // how to bring the previous version up. The migrations are run + // sequentially, each in its own transaction, to move from one version to + // the next. + switch version { + case 0: + err = migrateToV1(tx) + default: + err = fmt.Errorf("no migration support for version %d", version) + } + if err != nil { + return version, err + } + + nextVersion := version + 1 + if err := tx.Model(&Migration{}).Updates(Migration{Version: nextVersion}).Error; err != nil { + return version, err + } + + return nextVersion, nil +} + +func migrateToV1(tx *gorm.DB) error { + v0tables := []string{ + "ca_certs", + "bundles", + "attested_node_entries", + "join_tokens", + "node_resolver_map_entries", + "selectors", + "registered_entries", + } + + // soft-delete support is being removed. drop all of the records that have + // been soft-deleted. unfortunately the "deleted_at" column cannot dropped + // easily because that operation is not supported by all dialects (thanks, + // sqlite3). + for _, table := range v0tables { + if err := tx.Exec(fmt.Sprintf("DELETE FROM %s WHERE deleted_at IS NOT NULL;", table)).Error; err != nil { + return err + } + } + return nil +} diff --git a/pkg/server/plugin/datastore/sql/models.go b/pkg/server/plugin/datastore/sql/models.go index 0873057027..2f2d0c000e 100644 --- a/pkg/server/plugin/datastore/sql/models.go +++ b/pkg/server/plugin/datastore/sql/models.go @@ -2,12 +2,17 @@ package sql import ( "time" - - "github.com/jinzhu/gorm" ) +// Using our own model struct to remove DeletedAt. We don't want soft-delete support. +type Model struct { + ID uint `gorm:"primary_key"` + CreatedAt time.Time + UpdatedAt time.Time +} + type CACert struct { - gorm.Model + Model Cert []byte `gorm:"not null"` Expiry time.Time `gorm:"not null;index"` @@ -16,14 +21,14 @@ type CACert struct { } type Bundle struct { - gorm.Model + Model TrustDomain string `gorm:"not null;unique_index"` CACerts []CACert } type AttestedNodeEntry struct { - gorm.Model + Model SpiffeID string `gorm:"unique_index"` DataType string @@ -32,7 +37,7 @@ type AttestedNodeEntry struct { } type NodeResolverMapEntry struct { - gorm.Model + Model SpiffeID string `gorm:"unique_index:idx_node_resolver_map"` Type string `gorm:"unique_index:idx_node_resolver_map"` @@ -40,7 +45,7 @@ type NodeResolverMapEntry struct { } type RegisteredEntry struct { - gorm.Model + Model EntryID string `gorm:"unique_index"` SpiffeID string @@ -52,24 +57,23 @@ type RegisteredEntry struct { // Keep time simple and easily comparable with UNIX time type JoinToken struct { - gorm.Model + Model Token string `gorm:"unique_index"` Expiry int64 } type Selector struct { - gorm.Model + Model RegisteredEntryID uint `gorm:"unique_index:idx_selector_entry"` Type string `gorm:"unique_index:idx_selector_entry"` Value string `gorm:"unique_index:idx_selector_entry"` } -func migrateDB(db *gorm.DB) { - db.AutoMigrate(&Bundle{}, &CACert{}, &AttestedNodeEntry{}, - &NodeResolverMapEntry{}, &RegisteredEntry{}, &JoinToken{}, - &Selector{}) +type Migration struct { + Model - return + // Database version + Version int } diff --git a/pkg/server/plugin/datastore/sql/sql.go b/pkg/server/plugin/datastore/sql/sql.go index 6152bf9959..1081a9241b 100644 --- a/pkg/server/plugin/datastore/sql/sql.go +++ b/pkg/server/plugin/datastore/sql/sql.go @@ -7,6 +7,7 @@ import ( "fmt" "net/url" "sync" + "sync/atomic" "time" "github.com/hashicorp/hcl" @@ -28,6 +29,9 @@ var ( Author: "", Company: "", } + + // nextDB is used to provide a unique name so there is not false sharing between in-memory sqlite3 databases + nextDB uint64 ) type configuration struct { @@ -187,6 +191,7 @@ func (ds *sqlPlugin) DeleteBundle(ctx context.Context, req *datastore.Bundle) (* return nil, result.Error } + result = tx.Delete(model) resp, err := ds.modelToBundle(model) if err != nil { tx.Rollback() @@ -1133,24 +1138,29 @@ func (ds *sqlPlugin) restart() error { default: return fmt.Errorf("unsupported database_type: %v", ds.DatabaseType) } - if err != nil { return err } + if err := migrateDB(db); err != nil { + db.Close() + return err + } + if ds.db != nil { ds.db.Close() } - migrateDB(db) ds.db = db return nil } func newPlugin() *sqlPlugin { + u := fmt.Sprintf("file:memdb%d?mode=memory&cache=shared", atomic.AddUint64(&nextDB, 1)) + p := &sqlPlugin{ mutex: new(sync.Mutex), - ConnectionString: ":memory:", + ConnectionString: u, DatabaseType: "sqlite3", } diff --git a/pkg/server/plugin/datastore/sql/sql_test.go b/pkg/server/plugin/datastore/sql/sql_test.go index 0928abcb33..2a0b459dcd 100644 --- a/pkg/server/plugin/datastore/sql/sql_test.go +++ b/pkg/server/plugin/datastore/sql/sql_test.go @@ -3,7 +3,11 @@ package sql import ( "context" "encoding/json" + "fmt" + "io" "io/ioutil" + "os" + "path/filepath" "sync" "testing" "time" @@ -370,7 +374,7 @@ func Test_CreateRegistrationEntry(t *testing.T) { ds := createDefault(t) var validRegistrationEntries []*common.RegistrationEntry - err := getTestDataFromJsonFile(t, "_test_data/valid_registration_entries.json", &validRegistrationEntries) + err := getTestDataFromJsonFile(t, filepath.Join("testdata", "valid_registration_entries.json"), &validRegistrationEntries) require.NoError(t, err) for _, validRegistrationEntry := range validRegistrationEntries { @@ -385,7 +389,7 @@ func Test_CreateInvalidRegistrationEntry(t *testing.T) { ds := createDefault(t) var invalidRegistrationEntries []*common.RegistrationEntry - err := getTestDataFromJsonFile(t, "_test_data/invalid_registration_entries.json", &invalidRegistrationEntries) + err := getTestDataFromJsonFile(t, filepath.Join("testdata", "invalid_registration_entries.json"), &invalidRegistrationEntries) require.NoError(t, err) for _, invalidRegisteredEntry := range invalidRegistrationEntries { @@ -622,7 +626,8 @@ func Test_ListSelectorEntries(t *testing.T) { t.Run(test.name, func(t *testing.T) { ds := createDefault(t) for _, entry := range test.registrationEntries { - r, _ := ds.CreateRegistrationEntry(ctx, &datastore.CreateRegistrationEntryRequest{RegisteredEntry: entry}) + r, err := ds.CreateRegistrationEntry(ctx, &datastore.CreateRegistrationEntryRequest{RegisteredEntry: entry}) + require.NoError(t, err) entry.EntryId = r.RegisteredEntryId } result, err := ds.ListSelectorEntries(ctx, &datastore.ListSelectorEntriesRequest{ @@ -668,7 +673,8 @@ func Test_ListMatchingEntries(t *testing.T) { t.Run(test.name, func(t *testing.T) { ds := createDefault(t) for _, entry := range test.registrationEntries { - r, _ := ds.CreateRegistrationEntry(ctx, &datastore.CreateRegistrationEntryRequest{RegisteredEntry: entry}) + r, err := ds.CreateRegistrationEntry(ctx, &datastore.CreateRegistrationEntryRequest{RegisteredEntry: entry}) + require.NoError(t, err) entry.EntryId = r.RegisteredEntryId } result, err := ds.ListMatchingEntries(ctx, &datastore.ListSelectorEntriesRequest{ @@ -787,6 +793,47 @@ func Test_GetPluginInfo(t *testing.T) { require.NotNil(t, resp) } +func Test_Migration(t *testing.T) { + require := require.New(t) + + tmpDir, err := ioutil.TempDir("", "spire-sql-datastore") + require.NoError(err) + defer os.RemoveAll(tmpDir) + + ds := New() + + for i := 0; i < codeVersion; i++ { + dbName := fmt.Sprintf("v%d.sqlite3", i) + dbPath := filepath.Join(tmpDir, dbName) + // copy the database file from the test data + require.NoError(copyFile(filepath.Join("testdata", "migration", dbName), dbPath)) + + // configure the datastore to use the new database + _, err = ds.Configure(context.Background(), &spi.ConfigureRequest{ + Configuration: fmt.Sprintf(` + database_type = "sqlite3" + connection_string = "file://%s" + `, dbPath), + }) + require.NoError(err) + + switch i { + case 0: + // the v0 database has two bundles. the spiffe://otherdomain.org + // bundle has been soft-deleted. after migration, it should not + // exist. if we try and create a bundle with the same id, it should + // fail if the migration did not run, due to uniqueness + // constraints. + _, err := ds.CreateBundle(context.Background(), &datastore.Bundle{ + TrustDomain: "spiffe://otherdomain.org", + }) + require.NoError(err) + default: + t.Fatalf("no migration test added for version %d", i) + } + } +} + func Test_race(t *testing.T) { ds := createDefault(t) @@ -824,3 +871,23 @@ func getTestDataFromJsonFile(t *testing.T, filePath string, jsonValue interface{ return nil } + +func copyFile(src, dst string) error { + in, err := os.Open(src) + if err != nil { + return err + } + defer in.Close() + + out, err := os.Create(dst) + if err != nil { + return err + } + defer out.Close() + + _, err = io.Copy(out, in) + if err != nil { + return err + } + return out.Close() +} diff --git a/pkg/server/plugin/datastore/sql/sqlite.go b/pkg/server/plugin/datastore/sql/sqlite.go index 3a7097f075..11f6d75f9c 100644 --- a/pkg/server/plugin/datastore/sql/sqlite.go +++ b/pkg/server/plugin/datastore/sql/sqlite.go @@ -8,12 +8,6 @@ import ( type sqlite struct{} func (s sqlite) connect(connectionString string) (*gorm.DB, error) { - path := connectionString - if path == ":memory:" { - path = path + "?cache=shared" - } - path = "file:" + path - db, err := gorm.Open("sqlite3", connectionString) if err != nil { return nil, err diff --git a/pkg/server/plugin/datastore/sql/_test_data/invalid_registration_entries.json b/pkg/server/plugin/datastore/sql/testdata/invalid_registration_entries.json similarity index 100% rename from pkg/server/plugin/datastore/sql/_test_data/invalid_registration_entries.json rename to pkg/server/plugin/datastore/sql/testdata/invalid_registration_entries.json diff --git a/pkg/server/plugin/datastore/sql/testdata/migration/v0.sqlite3 b/pkg/server/plugin/datastore/sql/testdata/migration/v0.sqlite3 new file mode 100644 index 0000000000000000000000000000000000000000..b5ff415846bdb48843c715c4a3af6a98e704c937 GIT binary patch literal 98304 zcmeI*d3Y0L9tZH5Buz-K06kKK!jOWtw9rY?q)ltLLM>XTMJR`&Skh)FkscvQu_y{D z_a%y`kAg}C#C^mIQ9O2CL6BEQrE2PDN>vblJbqq$beuHyq1c?5_oCM-&nYW<6eyO;JEQhS<;vDe_e&>A zI_Q7X3rY1$@Nmi03bl3s?JBX>Tev2xrK!@v)pFI&274t}@3hbMy>p}#k1H8IzJ!@j zUOIL{2~%2rZOKiHxIQzcoYDAK(xltD8mq&}**S}+y-KSki_th|+c*t*o@K6WrPXKZ$Bo!LH{nZeCw%#F?lYkjqyt0Qz-8uG&I z>9~fmkXKG?9haqPwDGU`;Ru%o`X*c&0?)tYEDs_x%WSWnZnmfE42Fy>FKEz0CSzuz zLYp#@4d9W$miJbu)6(eXYIk4-O$c5l1P518qPdje^>1 ziE~0vg?bF>mfjy>@d|>di3@v);w`QtXdrB#gwbzKR*~sUOV6L=j=s1-@sggwxgf4^ zNmIfHtm&2&4x4q_G>*u3FR+>IWStNNPG_xut{_>&#FIIoh;~i#hpWE9!d2QiM?>u_ z&R$t(wuu(F^MPcSDpU&bQ@>~pcaTI&eeGz{{lf^c4{&A2Db!O)7srI}qOT=EUWDCR zyePlIRuwe&YaV1^qGVo@(x$cDKCr?7)K- zl4MJZQK&0&X;-O#$}&5h9Qhhx@uUo&<@~JW-&Dxr*idQ5?}$Ry4m9QtVaPgy1SB)q zfM|t!0_kOuKi@*GBuI{s3t3Ij%*8qwlwVm&@}bS8=ilMZuaM1#To@gkS0U^C zK5~kD7cy6dN-PIwx0-7!>l^E;ID7X@bnhcc{s_NAF&Wc)DAcA@+NJQvv)Wu)&Douf z%BseCOD#Wr`j#j~LGzUa>bk?^TdYaL!Q02Z>1jLx8kYTZ4 z0`Xxa--&7)s+jUI}Gf+Byg0{g&mB_m75m6R8kR4|@WM>_vS#@8H^ z;gpeskiN7l)ospCG~RlZPJ5%nS!rpgGh6Gu%c8s}L5D-()^Xc=SJb2nTgzZ-q$F#} zRs&h<+iD<0-X$_ooW)gxHx+0igSk;6S7&C@H@jo(m|07Xddacg%to%hn(Ovf8Wizv zh3WNXavVd}%y5mM73#iy>1I#fcst{Lk_5G(@h-ijNh5>~oXZg5WL?4Dm*hBer0rqX;zD1&$_#$hI1 zu3ndC(&vQhh^UHxD_oPg&Q{CuTZmJ2lH>nRDAgyb?;|M07%BuH009U<00Izz00bZa z0SG|gk0B64OZ(EkixJ3>VrlP{E3p!3s_(T!7EOvIs&-0sf&7C91Rwwb2tWV=5P$## zAOHafKmY=ljlfmX3R-l@KmY;|fB*y_009U< z00Iz*T!DVlLfU_cf43V6Bx-M^DxTlPwYgf8G1t=y+bl7$&Ge(|r4m{q zp=mZ+-ebt3>-QDPq%yX@HilKor4wZG1j&R7t%{A~-$cjCD{d?uIkJT9ua48^v7}S{ z-F-QR$^N%meU8B}Io$dQzV%&e*`9KRdvg+69+l~mQE5`wB1%R-zWKJ7Zr#?hcjcP% z(~8gUQ5@Hv|Lg**aLgUqSa@RjyH9P9=lpcn!V%WenT@CZ_6}znB7gJK-%{()e|9ClApK#_fF08< zV{ZLr`?7{%#b*;U2FaJp&-^g;$eN@pJ4-jX7P78pHl7SQRYlXk$Yg|Q5v$_gC-Cp1 zSSfj8Q~3q)vSeA}*^wP}ds+$(rGI!xl3g+6o%?pK+s_W*i_>H&Z1TLsk?*be`b=xr zT_bbfUTiEr*>yh3678wbi?6UKG1Qk9VML^^6%^}QA{jyN+ulDbC;e$!c|0NY-3zL+ zV{bhB%7DG9vvX8kU%%cm=#J%OTN0Udt!ER;M)xVMTVF9&^J3ydv4dWm@m`J1@1x%1s@F>6j+jo3+gdzipQhqS@hI1z;*HrYGbBwZN9*R4rHvnbcgpDdJJPI` zZRtDC?Rqe&@xthWZHEsR%UUOR&b#P_YFUQW=Ie9v4TemXB{zyn=HDgdbD3JCD^iNi z&gl7@HS_Weg?t0(uB8Yi<9 zdo!C5$VAU4KuRm5BPdyE?RN#y=A_4Jt{i+od-RjFvx*1pKKaYBBtvKGq}BIk|J)G$ zMq131Uv?e+Vw39{?t!Y$AKAM(`tP?cv2g$4S>lfVVq4hg8;!YIqyD1N7giyW?;?NB5Yv=2T=ct5Q{kf@W}5R)Q?o>~ltx-b z$*zyja9sPRA+55{y0V=yg$vg$`gHqoXRM*4AZpT(r;cSUn7XF6YEIETb4LySbl}DX zk94$;UH_zYeSO=`k6cxBHICQe<`J}ve3aX<8%{fPR+xrZ|ssL~#sDO;tR)SUgw zGiQJM1Qartgvqve)9Fc1o-%BC>#Swh_@niQUN^NK`z;fYR63nvIf<4!Gx5j|&%WR9 zv9-#q52=C6PQS3{Yx54rvtM5E&C%6e?dMOtKelYv#sMv>P0w$dcVyg_?@*8AkI!joJ|>VdrY#On^8drt77`fvAz2CZIe((7|_$rPj0 zvj(ls`}S9pG)$6BX771y;ObqAH2Y><=;-^}O;bNvwfCy*mtS_w`A5g@mbTJIPu|LWi4NF^crsibPHtjFZw+_ue*tTU^y*_GjOILQ?jU_LiY*@AFh`kuVN0|F3$00bZa0SG_<0uX=z z1Rwx`-?6|&uKE|A-5|f&Q?!)-$AA65Otq9!EmiGSZB}*sjt7XYK>z{}fB*y_009U< z00Izz00b_7fmEsQ62}l1kI4e1?*EM-_}U`rMN1|9r2JJO7d`*~t?&H*x0gR8MgRc_ zKmY;|fB*y_009U<00Iz*Y=J0pbDo5p|5x(o|CRjtf2A^xC+Qj4E*L!oAOHafKmY;| zfB*y_009U<;EyiA-~W%}|37+^F+K=D00Izz00bZa0SG_<0uYFF0UZBFdIn(p5P$## zAOHafKmY;|fB*y_5D5af{y!3;j4?t00uX=z1Rwwb2tWV=5P(3W3*h{Jq-Oxe4*>{3 z00Izz00bZa0SG_<0+AqqjXg*00bZa0SG_<0uX=z1Rwx`NEg8Q|47dOj2{9J zfB*y_009U<00Izz00bgI0LTB45M_)J0uX=z1Rwwb2tWV=5P$##B3%I2|3`WTVEhn( z00bZa0SG_<0uX=z1RxLz0yzI42~oxvApijgKmY;|fB*y_009Ub^G72W&* literal 0 HcmV?d00001 diff --git a/pkg/server/plugin/datastore/sql/_test_data/valid_registration_entries.json b/pkg/server/plugin/datastore/sql/testdata/valid_registration_entries.json similarity index 100% rename from pkg/server/plugin/datastore/sql/_test_data/valid_registration_entries.json rename to pkg/server/plugin/datastore/sql/testdata/valid_registration_entries.json From 584fe7ff24350bd452e04e4845a7c6f5ef608138 Mon Sep 17 00:00:00 2001 From: Andrew Harding Date: Mon, 20 Aug 2018 16:56:31 -0600 Subject: [PATCH 2/3] address PR comments Signed-off-by: Andrew Harding --- pkg/server/plugin/datastore/sql/migration.go | 9 +++++- pkg/server/plugin/datastore/sql/sql.go | 30 ++------------------ pkg/server/plugin/datastore/sql/sql_test.go | 21 ++++++++++---- 3 files changed, 26 insertions(+), 34 deletions(-) diff --git a/pkg/server/plugin/datastore/sql/migration.go b/pkg/server/plugin/datastore/sql/migration.go index 036aa88515..346f474652 100644 --- a/pkg/server/plugin/datastore/sql/migration.go +++ b/pkg/server/plugin/datastore/sql/migration.go @@ -38,9 +38,16 @@ func migrateDB(db *gorm.DB) (err error) { return err } + if version == codeVersion { + return nil + } + logrus.Infof("running migrations...") for version < codeVersion { tx := db.Begin() + if err := tx.Error; err != nil { + return err + } version, err = migrateVersion(tx, version) if err != nil { tx.Rollback() @@ -86,7 +93,7 @@ func initDB(db *gorm.DB) (err error) { func migrateVersion(tx *gorm.DB, version int) (versionOut int, err error) { logrus.Infof("migrating from version %d", version) - // When a new version is added and entry here must be included that knows + // When a new version is added an entry must be included here that knows // how to bring the previous version up. The migrations are run // sequentially, each in its own transaction, to move from one version to // the next. diff --git a/pkg/server/plugin/datastore/sql/sql.go b/pkg/server/plugin/datastore/sql/sql.go index 1081a9241b..833ebb730f 100644 --- a/pkg/server/plugin/datastore/sql/sql.go +++ b/pkg/server/plugin/datastore/sql/sql.go @@ -7,7 +7,6 @@ import ( "fmt" "net/url" "sync" - "sync/atomic" "time" "github.com/hashicorp/hcl" @@ -29,9 +28,6 @@ var ( Author: "", Company: "", } - - // nextDB is used to provide a unique name so there is not false sharing between in-memory sqlite3 databases - nextDB uint64 ) type configuration struct { @@ -191,7 +187,6 @@ func (ds *sqlPlugin) DeleteBundle(ctx context.Context, req *datastore.Bundle) (* return nil, result.Error } - result = tx.Delete(model) resp, err := ds.modelToBundle(model) if err != nil { tx.Rollback() @@ -1156,15 +1151,9 @@ func (ds *sqlPlugin) restart() error { } func newPlugin() *sqlPlugin { - u := fmt.Sprintf("file:memdb%d?mode=memory&cache=shared", atomic.AddUint64(&nextDB, 1)) - - p := &sqlPlugin{ - mutex: new(sync.Mutex), - ConnectionString: u, - DatabaseType: "sqlite3", + return &sqlPlugin{ + mutex: new(sync.Mutex), } - - return p } // New creates a new sql plugin struct. Configure must be called @@ -1172,18 +1161,3 @@ func newPlugin() *sqlPlugin { func New() datastore.Plugin { return newPlugin() } - -// NewTemp create a new plugin with a temporal database, allowing new -// connections to receive a fresh copy. Primarily meant for testing. -func NewTemp() (datastore.Plugin, error) { - p := newPlugin() - - // Call restart() to start the db - normally triggered by call to Configure - err := p.restart() - if err != nil { - return nil, fmt.Errorf("start database: %v", err) - } - - p.db.LogMode(true) - return p, nil -} diff --git a/pkg/server/plugin/datastore/sql/sql_test.go b/pkg/server/plugin/datastore/sql/sql_test.go index 2a0b459dcd..182cae7bc1 100644 --- a/pkg/server/plugin/datastore/sql/sql_test.go +++ b/pkg/server/plugin/datastore/sql/sql_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "sync" + "sync/atomic" "testing" "time" @@ -22,6 +23,13 @@ import ( var ( ctx = context.Background() + + // nextInMemoryId is atomically incremented and appended to the database + // name for in-memory databases. A unique name is required to prevent + // the in-memory database from being shared. + // + // See https://www.sqlite.org/inmemorydb.html for details. + nextInMemoryId uint64 ) type regEntries []*common.RegistrationEntry @@ -851,11 +859,14 @@ func Test_race(t *testing.T) { } func createDefault(t *testing.T) datastore.Plugin { - ds, err := NewTemp() - if err != nil { - t.Fatal(err) - } - return ds + p := newPlugin() + p.DatabaseType = "sqlite3" + p.ConnectionString = fmt.Sprintf("file:memdb%d?mode=memory&cache=shared", atomic.AddUint64(&nextInMemoryId, 1)) + + require.NoError(t, p.restart()) + + p.db.LogMode(true) + return p } func getTestDataFromJsonFile(t *testing.T, filePath string, jsonValue interface{}) error { From 5a924f56a3708706662641701cd6f4a6a99223f9 Mon Sep 17 00:00:00 2001 From: Andrew Harding Date: Mon, 20 Aug 2018 17:06:50 -0600 Subject: [PATCH 3/3] address more PR comments Signed-off-by: Andrew Harding --- pkg/server/plugin/datastore/sql/migration.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/server/plugin/datastore/sql/migration.go b/pkg/server/plugin/datastore/sql/migration.go index 346f474652..0c969711f9 100644 --- a/pkg/server/plugin/datastore/sql/migration.go +++ b/pkg/server/plugin/datastore/sql/migration.go @@ -69,25 +69,19 @@ func initDB(db *gorm.DB) (err error) { return err } - defer func() { - if err != nil { - tx.Rollback() - } else { - err = tx.Commit().Error - } - }() - if err := tx.AutoMigrate(&Bundle{}, &CACert{}, &AttestedNodeEntry{}, &NodeResolverMapEntry{}, &RegisteredEntry{}, &JoinToken{}, &Selector{}, &Migration{}).Error; err != nil { + tx.Rollback() return err } if err := tx.Assign(Migration{Version: codeVersion}).FirstOrCreate(&Migration{}).Error; err != nil { + tx.Rollback() return err } - return nil + return tx.Commit().Error } func migrateVersion(tx *gorm.DB, version int) (versionOut int, err error) {