revise after discussing

Signed-off-by: Andrew Moore <[email protected]>
esweiss · Sep 13, 2019 · 26f7343 · 26f7343
1 parent 2e383f5
commit 26f7343
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 132 deletions.
diff --git a/go.mod b/go.mod
@@ -33,7 +33,7 @@ require (
  github.com/gogo/googleapis v1.2.0
  github.com/gogo/protobuf v1.2.1
  github.com/golang/mock v1.3.1
- github.com/golang/protobuf v1.3.1
+ github.com/golang/protobuf v1.3.2
  github.com/google/gofuzz v1.0.0 // indirect
  github.com/googleapis/gnostic v0.3.0 // indirect
  github.com/gorilla/mux v1.7.2 // indirect
@@ -69,7 +69,7 @@ require (
  golang.org/x/time v0.0.0-20190308202827-9d24e82272b4
  golang.org/x/tools v0.0.0-20190618163018-fdf1049a943a
  google.golang.org/api v0.6.0
- google.golang.org/grpc v1.22.0
+ google.golang.org/grpc v1.23.1
  gopkg.in/inf.v0 v0.9.1 // indirect
  gopkg.in/square/go-jose.v2 v2.3.1
  gopkg.in/tomb.v2 v2.0.0-20161208151619-d5d1b5820637

diff --git a/go.sum b/go.sum
@@ -126,6 +126,8 @@ github.com/golang/mock v1.3.1/go.mod h1:sBzyDLLjw3U8JLTeZvSv8jJB+tU5PVekmnlKIyFU
 github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
 github.com/golang/protobuf v1.3.1 h1:YF8+flBXS5eO826T4nzqPrxfhQThhXl0YzfuUPu4SBg=
 github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
+github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs=
+github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
 github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
 github.com/google/btree v0.0.0-20180813153112-4030bb1f1f0c h1:964Od4U6p2jUkFxvCydnIczKteheJEzHRToSGK3Bnlw=
 github.com/google/btree v0.0.0-20180813153112-4030bb1f1f0c/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ=
@@ -392,6 +394,8 @@ google.golang.org/grpc v1.21.1 h1:j6XxA85m/6txkUCHvzlV5f+HBNl/1r5cZ2A/3IEFOO8=
 google.golang.org/grpc v1.21.1/go.mod h1:oYelfM1adQP15Ek0mdvEgi9Df8B9CZIaU1084ijfRaM=
 google.golang.org/grpc v1.22.0 h1:J0UbZOIrCAl+fpTOf8YLs4dJo8L/owV4LYVtAXQoPkw=
 google.golang.org/grpc v1.22.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg=
+google.golang.org/grpc v1.23.1 h1:q4XQuHFC6I28BKZpo6IYyb3mNO+l7lSOxRuYTCiDfXk=
+google.golang.org/grpc v1.23.1/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg=
 gopkg.in/alecthomas/kingpin.v2 v2.2.6 h1:jMFz6MfLP0/4fUyZle81rXUoxOBFi19VUFKVDOQfozc=
 gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=

diff --git a/pkg/server/plugin/datastore/sql/migration.go b/pkg/server/plugin/datastore/sql/migration.go
@@ -2,7 +2,6 @@ package sql
 
 import (
  "fmt"
- "math"
  "time"
 
  "github.com/blang/semver"
@@ -19,32 +18,13 @@ const (
  // the latest schema version of the database in the code
  latestSchemaVersion = 12
 
- // the version in which the current migration design was
- // introduced
- // TODO related epic https://github.com/spiffe/spire/issues/1083
- // This is not a part of the final design for migration safety discussed,
- // but helps in this in-between phase to make sure a version is available
- // for comparison. When SPIRE Code version gets bumped to 0.10, this
- // should be removed.
- preMigrationDesignVersion = "0.8.2"
+ // version in which new DB migration / compatibility design
+ // was introduced; it is the minimum SPIRE Code version in the DB
+ // to be able to disable auto-migration
+ minimumCodeVersionToDisableMigrate = "0.9.0"
 )
 
 var (
- // Versions of the database that the current code version
- // could run against without degradation / failure in the event
- // that auto-migration is disabled
- // This is not a part of the final design for migration safety discussed
- // in https://github.com/spiffe/spire/issues/1083, but acknowledges versions
- // safe to use pre-tracking of SPIRE Code version in migration table.
- // When SPIRE Code version gets bumped to 0.9, this `safeVersions` map should be removed.
- // A map is cleaner and faster to look for a particular version
- // in than looping on a slice
- safeVersions = map[int]struct{}{
- 9: {},
- 10: {},
- 11: {},
- }
-
  // the current code version
  codeVersion = semver.MustParse(version.Version())
 )
@@ -62,7 +42,9 @@ func migrateDB(db *gorm.DB, dbType string, disableMigration bool, log hclog.Logg
  // TODO related epic https://github.com/spiffe/spire/issues/1083
  // continue doing this automigration for now
  // eventually SPIRE Code will be bumped to version 0.10, where we can
- // expect that the `migrations` table exists and this can be removed
+ // expect that the `migrations` table exists and has the CodeVersion column,
+ // and then this can be removed. The below `Assign` check may be a new guard
+ // against pre-0.9 versions due to mismatching model
  if err := db.AutoMigrate(&Migration{}).Error; err != nil {
  return sqlError.Wrap(err)
  }
@@ -74,13 +56,19 @@ func migrateDB(db *gorm.DB, dbType string, disableMigration bool, log hclog.Logg
 
  schemaVersion := migration.Version
 
- dbCodeVersionStr := migration.CodeVersion
- if dbCodeVersionStr == "" {
- dbCodeVersionStr = preMigrationDesignVersion
- }
- dbCodeVersion, err := semver.Parse(dbCodeVersionStr)
- if err != nil {
- return err
+ var dbCodeVersion semver.Version
+ if migration.CodeVersion == "" {
+ if disableMigration {
+ err := sqlError.New("You must be upgrading from version %s or higher to disable auto-migration", minimumCodeVersionToDisableMigrate)
+ log.Error(err.Error())
+ return err
+ }
+ dbCodeVersion = semver.Version{}
+ } else {
+ dbCodeVersion, err = semver.Parse(migration.CodeVersion)
+ if err != nil {
+ return err
+ }
  }
 
  if schemaVersion == latestSchemaVersion {
@@ -105,30 +93,18 @@ func migrateDB(db *gorm.DB, dbType string, disableMigration bool, log hclog.Logg
  }
 
  // TODO related epic https://github.com/spiffe/spire/issues/1083
- // currently we are comparing minor version changes since SPIRE is pre-1.0.
- // Once SPIRE is 1.0 we should compare major versions (+- 1 allowed),
- // either allowing the last pre-1.0 minor release or disallowing all 0.X
- // releases. Once SPIRE is 2.0 we can get rid of the special logic and
- // only compare major versions.
- if dbCodeVersion.Major != codeVersion.Major || math.Abs(float64(dbCodeVersion.Minor-codeVersion.Minor)) > 1 {
+ // We do not allow breaking changes in 0.X beyond version 0.9.0, but
+ // 1.0+ may have such changes. Thus iff Major version is 0, we are
+ // compatible. This logic will change in 1.0+.
+ if dbCodeVersion.Major != 0 {
  // DB schema versions do not match, and the Code versions are incompatible
  err = sqlError.New("Code version %s is NOT compatible with DB code version %s", codeVersion.String(), dbCodeVersion.String())
  log.Error(err.Error())
  return err
  }
 
  if disableMigration {
- if _, ok := safeVersions[schemaVersion]; ok {
- // safe schemaVersion to run against
- log.Info(fmt.Sprintf("auto-migration disabled and DB schema versions do not match."+
- "Code schema version %d is compatible with DB schema version %d", latestSchemaVersion, schemaVersion))
- return nil
- }
- // unsafe schemaVersion to run against
- err = sqlError.New("auto-migration disabled and DB schema versions do not match."+
- " Code schema version %d is NOT compatible with DB schema version %d", latestSchemaVersion, schemaVersion)
- log.Error(err.Error())
- return err
+ return nil
  }
 
  // at this point:

diff --git a/pkg/server/plugin/datastore/sql/migration_test.go b/pkg/server/plugin/datastore/sql/migration_test.go
@@ -409,6 +409,7 @@ COMMIT;
  CREATE INDEX idx_federated_registration_entries_registered_entry_id ON "federated_registration_entries"(registered_entry_id) ;
  COMMIT;
  `,
+ // below this point is SPIRE Code version 0.9.X
  // future v12 database entry, in which code_version string was added to migrations table
  }
 )

diff --git a/pkg/server/plugin/datastore/sql/sql_test.go b/pkg/server/plugin/datastore/sql/sql_test.go
@@ -1741,87 +1741,8 @@ func (s *PluginSuite) TestDisabledMigrationBreakingChanges() {
  disable_migration = true
  `, dbPath),
  })
- s.Require().EqualError(err, fmt.Sprintf("rpc error: code = Unknown desc = datastore-sql: auto-migration disabled and DB schema versions do not match."+
- " Code schema version %d is NOT compatible with DB schema version %d", latestSchemaVersion, dbVersion))
-}
-
-func (s *PluginSuite) TestDisabledMigrationNonBreakingChanges() {
- tests := []struct {
- dbVersion int
- noMigrateValidation func(dbPath string)
- }{
- {
- dbVersion: 9,
- noMigrateValidation: func(dbPath string) {
- // between 9 and 12 only a new index was added; this should be non-breaking
- s.createRegistrationEntry(&common.RegistrationEntry{
- SpiffeId: "spiffe://example.org/foo",
- Selectors: []*common.Selector{{Type: "TYPE", Value: "VALUE"}},
- })
- db, err := sqlite{}.connect(&configuration{
- DatabaseType: "sqlite3",
- ConnectionString: fmt.Sprintf("file://%s", dbPath),
- })
- s.Require().NoError(err)
- s.Require().False(db.Dialect().HasIndex("registered_entries", "idx_registered_entries_expiry"))
- },
- },
- {
- dbVersion: 10,
- noMigrateValidation: func(dbPath string) {
- // between 10 and 12 only a new index was added; this should be non-breaking
- s.createRegistrationEntry(&common.RegistrationEntry{
- SpiffeId: "spiffe://example.org/foo",
- Selectors: []*common.Selector{{Type: "TYPE", Value: "VALUE"}},
- })
- db, err := sqlite{}.connect(&configuration{
- DatabaseType: "sqlite3",
- ConnectionString: fmt.Sprintf("file://%s", dbPath),
- })
- s.Require().NoError(err)
- s.Require().False(db.Dialect().HasIndex("federated_registration_entries", "idx_federated_registration_entries_registered_entry_id"))
- },
- },
- {
- dbVersion: 11,
- noMigrateValidation: func(dbPath string) {
- // between 11 and 12 a new column was added to the migrations table, and we
- // *always* migrate that table; this should be non-breaking
- s.createRegistrationEntry(&common.RegistrationEntry{
- SpiffeId: "spiffe://example.org/foo",
- Selectors: []*common.Selector{{Type: "TYPE", Value: "VALUE"}},
- })
- db, err := sqlite{}.connect(&configuration{
- DatabaseType: "sqlite3",
- ConnectionString: fmt.Sprintf("file://%s", dbPath),
- })
- s.Require().NoError(err)
- s.Require().True(db.Dialect().HasColumn("migrations", "code_version"))
- },
- },
- }
-
- for _, tt := range tests {
- s.Run(fmt.Sprintf("safe with version %d", tt.dbVersion), func() {
- dbName := fmt.Sprintf("v%d.sqlite3", tt.dbVersion)
- dbPath := filepath.Join(s.dir, "safe-disabled-migration-"+dbName)
- dump := migrationDump(tt.dbVersion)
- s.Require().NotEmpty(dump, "no migration dump set up for version %d", tt.dbVersion)
- s.Require().NoError(dumpDB(dbPath, dump), "error with DB dump for version %d", tt.dbVersion)
-
- // configure the datastore to use the new database
- _, err := s.ds.Configure(context.Background(), &spi.ConfigureRequest{
- Configuration: fmt.Sprintf(`
- database_type = "sqlite3"
- connection_string = "file://%s"
- disable_migration = true
- `, dbPath),
- })
- s.Require().NoError(err)
-
- tt.noMigrateValidation(dbPath)
- })
- }
+ s.Require().EqualError(err, fmt.Sprintf("rpc error: code = Unknown desc = datastore-sql:"+
+ " You must be upgrading from version %s or higher to disable auto-migration", minimumCodeVersionToDisableMigrate))
 }
 
 func (s *PluginSuite) TestMigration() {

diff --git a/proto/spire/go.mod b/proto/spire/go.mod
@@ -1,3 +1,8 @@
 module github.com/spiffe/spire/proto/spire
 
 go 1.13
+
+require (
+ github.com/golang/protobuf v1.3.2
+ google.golang.org/grpc v1.23.1
+)
diff --git a/proto/spire/go.sum b/proto/spire/go.sum
@@ -0,0 +1,26 @@
+cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
+github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
+github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
+github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=
+github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A=
+github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
+github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs=
+github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
+github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
+golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
+golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc=
+golang.org/x/net v0.0.0-20190311183353-d8887717615a h1:oWX7TPOiFAMXLq8o0ikBYfCJVlRHBcsciT5bXOrH628=
+golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
+golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
+golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
+golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
+golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
+golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
+golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q=
+google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9YwlJXL52JkM=
+google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8 h1:Nw54tB0rB7hY/N0NQvRW8DG4Yk3Q6T9cu9RcFQDu1tc=
+google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc=
+google.golang.org/grpc v1.23.1 h1:q4XQuHFC6I28BKZpo6IYyb3mNO+l7lSOxRuYTCiDfXk=
+google.golang.org/grpc v1.23.1/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg=
+honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=