diff --git a/management/server/migration/migration_agentnetwork.go b/management/server/migration/migration_agentnetwork.go index 3581d0165..c6cda56c2 100644 --- a/management/server/migration/migration_agentnetwork.go +++ b/management/server/migration/migration_agentnetwork.go @@ -33,6 +33,15 @@ func (agentNetworkSettingsMigration) TableName() string { return "agent_network_ // bootstrap always wrote both, so such a row indicates corruption and the // migration fails loudly rather than leaving an empty domain to collide with // the unique index confusingly. +// +// The transaction is real only on sqlite and postgres, where DDL is +// transactional. MySQL implicitly commits around every ALTER TABLE, so there +// each step stands alone; what makes an interrupted run resumable on MySQL is +// that every step is guarded by the schema state it changes — the entry check +// fires while either legacy column remains, the adds skip existing columns, +// the backfill and its loud-failure check run only while the legacy cluster +// column exists (they provably completed before any drop), and each drop +// skips what is already gone. func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error { model := &agentNetworkSettingsMigration{} migrator := db.Migrator() @@ -40,7 +49,8 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error if !migrator.HasTable(model) { return nil } - if !migrator.HasColumn(model, "cluster") { + hasCluster := migrator.HasColumn(model, "cluster") + if !hasCluster && !migrator.HasColumn(model, "subdomain") { // Fresh schema or already migrated — nothing to reshape. return nil } @@ -55,27 +65,33 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error } } - concat := "subdomain || '.' || cluster" - if tx.Name() == "mysql" { - concat = "CONCAT(subdomain, '.', cluster)" - } - res := tx.Exec(fmt.Sprintf( - "UPDATE agent_network_settings SET domain = %s, proxy_address = cluster WHERE (domain IS NULL OR domain = '') AND cluster <> '' AND subdomain <> ''", - concat, - )) - if res.Error != nil { - return fmt.Errorf("backfill agent_network_settings domain: %w", res.Error) - } + if hasCluster { + concat := "subdomain || '.' || cluster" + if tx.Name() == "mysql" { + concat = "CONCAT(subdomain, '.', cluster)" + } + res := tx.Exec(fmt.Sprintf( + "UPDATE agent_network_settings SET domain = %s, proxy_address = cluster WHERE (domain IS NULL OR domain = '') AND cluster <> '' AND subdomain <> ''", + concat, + )) + if res.Error != nil { + return fmt.Errorf("backfill agent_network_settings domain: %w", res.Error) + } - var unmigratable int64 - if err := tx.Model(model).Where("domain IS NULL OR domain = ''").Count(&unmigratable).Error; err != nil { - return fmt.Errorf("count unmigratable agent_network_settings rows: %w", err) - } - if unmigratable > 0 { - return fmt.Errorf( - "%d agent_network_settings row(s) have no cluster/subdomain to derive an endpoint from; resolve them manually before upgrading", - unmigratable, - ) + var unmigratable int64 + if err := tx.Model(model).Where("domain IS NULL OR domain = ''").Count(&unmigratable).Error; err != nil { + return fmt.Errorf("count unmigratable agent_network_settings rows: %w", err) + } + if unmigratable > 0 { + return fmt.Errorf( + "%d agent_network_settings row(s) have no cluster/subdomain to derive an endpoint from; resolve them manually before upgrading", + unmigratable, + ) + } + + if res.RowsAffected > 0 { + log.WithContext(ctx).Infof("migrated %d agent_network_settings row(s) to domain/proxy_address", res.RowsAffected) + } } if txMigrator.HasIndex(model, "idx_agent_network_settings_cluster_subdomain") { @@ -84,14 +100,13 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error } } for _, field := range []string{"Cluster", "Subdomain"} { - if err := txMigrator.DropColumn(model, field); err != nil { - return fmt.Errorf("drop legacy agent_network_settings column %s: %w", field, err) + if txMigrator.HasColumn(model, field) { + if err := txMigrator.DropColumn(model, field); err != nil { + return fmt.Errorf("drop legacy agent_network_settings column %s: %w", field, err) + } } } - if res.RowsAffected > 0 { - log.WithContext(ctx).Infof("migrated %d agent_network_settings row(s) to domain/proxy_address", res.RowsAffected) - } return nil }) } diff --git a/management/server/migration/migration_test.go b/management/server/migration/migration_test.go index 6298f1614..868332fdf 100644 --- a/management/server/migration/migration_test.go +++ b/management/server/migration/migration_test.go @@ -818,3 +818,43 @@ func TestMigrateAgentNetworkSettingsToDomain_FailsOnUnmigratableRow(t *testing.T require.Error(t, err, "a row with no identity to derive an endpoint from must fail the migration") assert.Contains(t, err.Error(), "resolve them manually", "the error must tell the operator what to do") } + +// partialAgentNetworkSettings models the one non-atomic state a MySQL run can +// be interrupted in: DDL auto-commits there, so a crash between the two legacy +// column drops leaves subdomain behind while cluster (and the completed +// backfill) are already committed. +type partialAgentNetworkSettings struct { + AccountID string `gorm:"primaryKey"` + Subdomain string + Domain string `gorm:"type:varchar(255)"` + ProxyAddress string `gorm:"type:varchar(255)"` +} + +func (partialAgentNetworkSettings) TableName() string { return "agent_network_settings" } + +// TestMigrateAgentNetworkSettingsToDomain_ResumesAfterPartialDrop pins MySQL +// resumability: a rerun over the interrupted state must remove the leftover +// subdomain column without re-running the backfill (the cluster column that +// feeds it is gone) and without touching the migrated values. +func TestMigrateAgentNetworkSettingsToDomain_ResumesAfterPartialDrop(t *testing.T) { + ctx := context.Background() + db := setupDatabase(t) + require.NoError(t, db.Migrator().DropTable(&partialAgentNetworkSettings{})) + require.NoError(t, db.AutoMigrate(&partialAgentNetworkSettings{})) + require.NoError(t, db.Create(&partialAgentNetworkSettings{ + AccountID: "acct-1", Subdomain: "violet", + Domain: "violet.eu.proxy.netbird.io", ProxyAddress: "eu.proxy.netbird.io", + }).Error) + + require.NoError(t, migration.MigrateAgentNetworkSettingsToDomain(ctx, db), + "a rerun over a partially-dropped schema must resume, not error") + + migrator := db.Migrator() + assert.False(t, migrator.HasColumn(&partialAgentNetworkSettings{}, "subdomain"), + "the leftover legacy column must be dropped on resume") + + var row agentNetworkTypes.Settings + require.NoError(t, db.First(&row, "account_id = ?", "acct-1").Error) + assert.Equal(t, "violet.eu.proxy.netbird.io", row.Domain, "migrated values must be untouched") + assert.Equal(t, "eu.proxy.netbird.io", row.ProxyAddress, "migrated values must be untouched") +}