fix(dockerhub-migration): prevent duplicate migrated dockerhub entries EE-2042 (#6084)

* add missing changes to make updateDockerhubToDB32 idempotent

* fix(migration) make dockerhub registry migration idempotent EE-2042

* add tests for bad migrations

api/bolt/migrate_data.go

@@ -18,13 +18,16 @@ const beforePortainerVersionUpgradeBackup = "portainer.db.bak"
 var migrateLog = plog.NewScopedLog("bolt, migrate")
 
 // FailSafeMigrate backup and restore DB if migration fail
-func (store *Store) FailSafeMigrate(migrator *migrator.Migrator) error {
+func (store *Store) FailSafeMigrate(migrator *migrator.Migrator) (err error) {
 	defer func() {
-		if err := recover(); err != nil {
-			migrateLog.Info(fmt.Sprintf("Error during migration, recovering [%v]", err))
+		if e := recover(); e != nil {
 			store.Rollback(true)
+			err = fmt.Errorf("%v", e)
 		}
 	}()
+
+	// !Important: we must use a named return value in the function definition and not a local
+	// !variable referenced from the closure or else the return value will be incorrectly set
 	return migrator.Migrate()
 }
 

api/bolt/migrator/migrate_ce.go

@@ -301,7 +301,7 @@ func (m *Migrator) Migrate() error {
 		}
 	}
 
-	// Portainer 2.9.1
+	// Portainer 2.9.1, 2.9.2
 	if m.currentDBVersion < 33 {
 		err := m.migrateDBVersionToDB33()
 		if err != nil {
@@ -316,6 +316,13 @@ func (m *Migrator) Migrate() error {
 		}
 	}
 
+	// Portainer 2.9.3 (yep out of order, but 2.10 is EE only)
+	if m.currentDBVersion < 35 {
+		if err := m.migrateDBVersionToDB35(); err != nil {
+			return migrationError(err, "migrateDBVersionToDB35")
+		}
+	}
+
 	err = m.versionService.StoreDBVersion(portainer.DBVersion)
 	if err != nil {
 		return migrationError(err, "StoreDBVersion")

api/bolt/migrator/migrate_dbversion31.go

@@ -100,6 +100,32 @@ func (m *Migrator) updateDockerhubToDB32() error {
 		RegistryAccesses: portainer.RegistryAccesses{},
 	}
 
+	// The following code will make this function idempotent.
+	// i.e. if run again, it will not change the data.  It will ensure that
+	// we only have one migrated registry entry. Duplicates will be removed
+	// if they exist and which has been happening due to earlier migration bugs
+	migrated := false
+	registries, _ := m.registryService.Registries()
+	for _, r := range registries {
+		if r.Type == registry.Type &&
+			r.Name == registry.Name &&
+			r.URL == registry.URL &&
+			r.Authentication == registry.Authentication {
+
+			if !migrated {
+				// keep this one entry
+				migrated = true
+			} else {
+				// delete subsequent duplicates
+				m.registryService.DeleteRegistry(portainer.RegistryID(r.ID))
+			}
+		}
+	}
+
+	if migrated {
+		return nil
+	}
+
 	endpoints, err := m.endpointService.Endpoints()
 	if err != nil {
 		return err
@@ -218,8 +244,12 @@ func findResourcesToUpdateForDB32(dockerID string, volumesData map[string]interf
 		if !nameExist {
 			continue
 		}
+		createTime, createTimeExist := volume["CreatedAt"].(string)
+		if !createTimeExist {
+			continue
+		}
 
-		oldResourceID := fmt.Sprintf("%s%s", volumeName, volume["CreatedAt"].(string))
+		oldResourceID := fmt.Sprintf("%s%s", volumeName, createTime)
 		resourceControl, ok := volumeResourceControls[oldResourceID]
 
 		if ok {

api/bolt/migrator/migrate_dbversion34.go

@@ -0,0 +1,11 @@
+package migrator
+
+func (m *Migrator) migrateDBVersionToDB35() error {
+	// These should have been migrated already, but due to an earlier bug and a bunch of duplicates,
+	// calling it again will now fix the issue as the function has been repaired.
+	err := m.updateDockerhubToDB32()
+	if err != nil {
+		return err
+	}
+	return nil
+}

api/bolt/migrator/migrate_dbversion34_test.go

@@ -0,0 +1,108 @@
+package migrator
+
+import (
+	"os"
+	"path"
+	"testing"
+	"time"
+
+	"github.com/boltdb/bolt"
+	portainer "github.com/portainer/portainer/api"
+	"github.com/portainer/portainer/api/bolt/dockerhub"
+	"github.com/portainer/portainer/api/bolt/endpoint"
+	"github.com/portainer/portainer/api/bolt/internal"
+	"github.com/portainer/portainer/api/bolt/registry"
+	"github.com/stretchr/testify/assert"
+)
+
+const (
+	db35TestFile = "portainer-mig-35.db"
+	username     = "portainer"
+	password     = "password"
+)
+
+func setupDB35Test(t *testing.T) *Migrator {
+	is := assert.New(t)
+	dbConn, err := bolt.Open(path.Join(t.TempDir(), db35TestFile), 0600, &bolt.Options{Timeout: 1 * time.Second})
+	is.NoError(err, "failed to init testing DB connection")
+
+	// Create an old style dockerhub authenticated account
+	dockerhubService, err := dockerhub.NewService(&internal.DbConnection{DB: dbConn})
+	is.NoError(err, "failed to init testing registry service")
+	err = dockerhubService.UpdateDockerHub(&portainer.DockerHub{true, username, password})
+	is.NoError(err, "failed to create dockerhub account")
+
+	registryService, err := registry.NewService(&internal.DbConnection{DB: dbConn})
+	is.NoError(err, "failed to init testing registry service")
+
+	endpointService, err := endpoint.NewService(&internal.DbConnection{DB: dbConn})
+	is.NoError(err, "failed to init endpoint service")
+
+	m := &Migrator{
+		db:               dbConn,
+		dockerhubService: dockerhubService,
+		registryService:  registryService,
+		endpointService:  endpointService,
+	}
+
+	return m
+}
+
+// TestUpdateDockerhubToDB32 tests a normal upgrade
+func TestUpdateDockerhubToDB32(t *testing.T) {
+	is := assert.New(t)
+	m := setupDB35Test(t)
+	defer m.db.Close()
+	defer os.Remove(db35TestFile)
+
+	if err := m.updateDockerhubToDB32(); err != nil {
+		t.Errorf("failed to update settings: %v", err)
+	}
+
+	// Verify we have a single registry were created
+	registries, err := m.registryService.Registries()
+	is.NoError(err, "failed to read registries from the RegistryService")
+	is.Equal(len(registries), 1, "only one migrated registry expected")
+}
+
+// TestUpdateDockerhubToDB32_with_duplicate_migrations tests an upgrade where in earlier versions a broken migration
+// created a large number of duplicate "dockerhub migrated" registry entries.
+func TestUpdateDockerhubToDB32_with_duplicate_migrations(t *testing.T) {
+	is := assert.New(t)
+	m := setupDB35Test(t)
+	defer m.db.Close()
+	defer os.Remove(db35TestFile)
+
+	// Create lots of duplicate entries...
+	registry := &portainer.Registry{
+		Type:             portainer.DockerHubRegistry,
+		Name:             "Dockerhub (authenticated - migrated)",
+		URL:              "docker.io",
+		Authentication:   true,
+		Username:         "portainer",
+		Password:         "password",
+		RegistryAccesses: portainer.RegistryAccesses{},
+	}
+
+	for i := 1; i < 150; i++ {
+		err := m.registryService.CreateRegistry(registry)
+		assert.NoError(t, err, "create registry failed")
+	}
+
+	// Verify they were created
+	registries, err := m.registryService.Registries()
+	is.NoError(err, "failed to read registries from the RegistryService")
+	is.Condition(func() bool {
+		return len(registries) > 1
+	}, "expected multiple duplicate registry entries")
+
+	// Now run the migrator
+	if err := m.updateDockerhubToDB32(); err != nil {
+		t.Errorf("failed to update settings: %v", err)
+	}
+
+	// Verify we have a single registry were created
+	registries, err = m.registryService.Registries()
+	is.NoError(err, "failed to read registries from the RegistryService")
+	is.Equal(len(registries), 1, "only one migrated registry expected")
+}

api/http/handler/handler.go

@@ -74,7 +74,7 @@ type Handler struct {
 }
 
 // @title PortainerCE API
-// @version 2.9.2
+// @version 2.9.3
 // @description.markdown api-description.md
 // @termsOfService
 

api/portainer.go

@@ -1470,9 +1470,9 @@ type (
 
 const (
 	// APIVersion is the version number of the Portainer API
-	APIVersion = "2.9.2"
+	APIVersion = "2.9.3"
 	// DBVersion is the version number of the Portainer database
-	DBVersion = 33
+	DBVersion = 35
 	// ComposeSyntaxMaxVersion is a maximum supported version of the docker compose syntax
 	ComposeSyntaxMaxVersion = "3.9"
 	// AssetsServerURL represents the URL of the Portainer asset server

app/docker/views/images/edit/image.html

@@ -82,7 +82,7 @@
           <!-- !tag-note -->
           <div class="form-group">
             <div class="col-sm-12">
-              <button type="button" class="btn btn-primary btn-sm" ng-disabled="!state.pullImageValidity || !formValues.RegistryModel.Image" ng-click="tagImage()">Tag</button>
+              <button type="button" class="btn btn-primary btn-sm" ng-disabled="!formValues.RegistryModel.Image" ng-click="tagImage()">Tag</button>
             </div>
           </div>
         </form>

package.json

@@ -2,7 +2,7 @@
   "author": "Portainer.io",
   "name": "portainer",
   "homepage": "http://portainer.io",
-  "version": "2.9.2",
+  "version": "2.9.3",
   "repository": {
     "type": "git",
     "url": "git@github.com:portainer/portainer.git"