diff --git a/management/server/migration/migration_agentnetwork.go b/management/server/migration/migration_agentnetwork.go index d81bcc91a..2daaff2db 100644 --- a/management/server/migration/migration_agentnetwork.go +++ b/management/server/migration/migration_agentnetwork.go @@ -3,6 +3,7 @@ package migration import ( "context" "fmt" + "strings" log "github.com/sirupsen/logrus" "gorm.io/gorm" @@ -67,11 +68,12 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error if hasCluster { // The legacy bootstrap stored the cluster as the caller spelled - // it (trimmed, never folded), while every path that reads these - // columns now compares against canonical lowercase: proxy - // addresses are canonicalised at connect and the proxy folds the - // SNI host it routes on. Fold here so the reshaped row is - // addressable, rather than copying a spelling nothing will match. + // it, trimmed but never folded, while every reader of these + // columns matches exactly against canonical lowercase: proxy + // addresses are canonicalised at connect, and the proxy's host + // map is keyed by the domain verbatim. Fold here so the reshaped + // row is addressable, rather than copying a spelling nothing + // will match. concat := "LOWER(subdomain || '.' || cluster)" if tx.Name() == "mysql" { concat = "LOWER(CONCAT(subdomain, '.', cluster))" @@ -94,6 +96,9 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error unmigratable, ) } + if err := failOnDuplicateAgentNetworkDomains(tx); err != nil { + return err + } if res.RowsAffected > 0 { log.WithContext(ctx).Infof("migrated %d agent_network_settings row(s) to domain/proxy_address", res.RowsAffected) @@ -128,28 +133,23 @@ type agentNetworkSettingsIdentity struct { func (agentNetworkSettingsIdentity) TableName() string { return "agent_network_settings" } -// NormalizeAgentNetworkSettingsIdentity lowercases domain and proxy_address -// on rows already reshaped by a release whose backfill copied the legacy -// cluster spelling verbatim. +// NormalizeAgentNetworkSettingsIdentity lowercases domain and proxy_address on +// rows already reshaped by a release whose backfill copied the legacy cluster +// spelling verbatim. // -// Both columns are compared exactly against canonical lowercase values: a -// proxy registering at a host asks whether another account's gateway is -// pinned there by proxy_address, cluster-scoped mapping synthesis finds the -// accounts a proxy serves the same way, and the proxy itself folds the SNI -// host before matching a mapping's domain. A row that kept capitals from the -// legacy schema is invisible to all three — its pin does not protect the -// host, and its endpoint is never matched — so the value is repaired where it -// is stored rather than folded on every read. +// Every reader of these columns matches exactly against canonical lowercase: +// the gateway-pin check a proxy registration runs and cluster-scoped mapping +// synthesis look proxy_address up by the canonical address, the domain lookup +// is followed by an exact Go compare, and the proxy's host map is keyed by the +// domain verbatim. A row that kept capitals is invisible to all of them, so +// the value is repaired where it is stored rather than folded on every read. // -// Idempotent: the WHERE clause selects only rows that would change, so a -// normalised table costs one pass over a table holding one row per account. -// On MySQL the default collation already compares case-insensitively, so the -// predicate never matches there and the statement is a no-op, which is the -// right answer: nothing on MySQL was invisible to begin with. Runs after the -// reshape, so the columns exist whenever the table does. Two rows that differ -// only by case would collapse onto one domain, which the unique index -// refuses; that state is unreachable through the API and the migration fails -// loudly rather than guessing which endpoint to keep. +// MySQL needs the predicate spelled byte-wise: under its default +// case-insensitive collation `domain <> LOWER(domain)` is false for every row, +// which would leave the rows unrepaired while the Go-side compares still miss +// them. Idempotent: the predicate selects only rows that would change, one +// pass over a table holding one row per account. Runs after the reshape, so +// the columns exist whenever the table does. func NormalizeAgentNetworkSettingsIdentity(ctx context.Context, db *gorm.DB) error { model := &agentNetworkSettingsIdentity{} migrator := db.Migrator() @@ -158,8 +158,15 @@ func NormalizeAgentNetworkSettingsIdentity(ctx context.Context, db *gorm.DB) err return nil } - res := db.Exec("UPDATE agent_network_settings SET domain = LOWER(domain), proxy_address = LOWER(proxy_address) " + - "WHERE domain <> LOWER(domain) OR proxy_address <> LOWER(proxy_address)") + if err := failOnDuplicateAgentNetworkDomains(db); err != nil { + return err + } + + predicate := "domain <> LOWER(domain) OR proxy_address <> LOWER(proxy_address)" + if db.Name() == "mysql" { + predicate = "BINARY domain <> BINARY LOWER(domain) OR BINARY proxy_address <> BINARY LOWER(proxy_address)" + } + res := db.Exec("UPDATE agent_network_settings SET domain = LOWER(domain), proxy_address = LOWER(proxy_address) WHERE " + predicate) if res.Error != nil { return fmt.Errorf("normalize agent_network_settings identity casing: %w", res.Error) } @@ -169,3 +176,28 @@ func NormalizeAgentNetworkSettingsIdentity(ctx context.Context, db *gorm.DB) err return nil } + +// failOnDuplicateAgentNetworkDomains refuses to continue when two settings +// rows would fold onto one endpoint hostname. Two accounts cannot share an +// endpoint, the unique index would refuse the fold with a driver message that +// names no row, and there is no right answer as to which account keeps the +// name, so the migration stops and says which hostname needs a human. +func failOnDuplicateAgentNetworkDomains(db *gorm.DB) error { + var rows []struct{ Domain string } + err := db.Raw("SELECT LOWER(domain) AS domain FROM agent_network_settings GROUP BY LOWER(domain) HAVING COUNT(*) > 1"). + Scan(&rows).Error + if err != nil { + return fmt.Errorf("check agent_network_settings for endpoints differing only by case: %w", err) + } + if len(rows) == 0 { + return nil + } + duplicates := make([]string, 0, len(rows)) + for _, row := range rows { + duplicates = append(duplicates, row.Domain) + } + return fmt.Errorf( + "agent_network_settings holds endpoints that differ only by case (%s); resolve them manually before upgrading", + strings.Join(duplicates, ", "), + ) +} diff --git a/management/server/migration/migration_test.go b/management/server/migration/migration_test.go index 65cf3a99e..662780770 100644 --- a/management/server/migration/migration_test.go +++ b/management/server/migration/migration_test.go @@ -757,9 +757,11 @@ func TestMigrateAgentNetworkSettingsToDomain_BackfillsAndDropsLegacyColumns(t *t db := setupDatabase(t) require.NoError(t, db.Migrator().DropTable(&legacyAgentNetworkSettings{})) require.NoError(t, db.AutoMigrate(&legacyAgentNetworkSettings{})) - // Spelled the way the legacy bootstrap kept it: trimmed, never folded. + // The cluster is spelled the way the legacy bootstrap kept it: as the + // caller typed it, trimmed but never folded. The subdomain was always + // server-assigned lowercase. require.NoError(t, db.Create(&legacyAgentNetworkSettings{ - AccountID: "acct-1", Cluster: "EU.Proxy.NetBird.io", Subdomain: "Violet", EnableLogCollection: true, + AccountID: "acct-1", Cluster: "EU.Proxy.NetBird.io", Subdomain: "violet", EnableLogCollection: true, }).Error) require.NoError(t, db.Create(&legacyAgentNetworkSettings{ AccountID: "acct-2", Cluster: "us.proxy.netbird.io", Subdomain: "violet", @@ -904,3 +906,52 @@ func TestNormalizeAgentNetworkSettingsIdentity_SkipsMissingTable(t *testing.T) { "no table must be a no-op, not an error") assert.False(t, db.Migrator().HasTable(&agentNetworkTypes.Settings{}), "the normaliser must not create the table") } + +// TestNormalizeAgentNetworkSettingsIdentity_RefusesCaseOnlyCollision pins the +// loud failure: two rows that would fold onto one endpoint stop the migration +// with the hostname named, and neither row is touched, rather than letting the +// unique index refuse the fold with a driver message that names no row. +func TestNormalizeAgentNetworkSettingsIdentity_RefusesCaseOnlyCollision(t *testing.T) { + ctx := context.Background() + db := setupDatabase(t) + if db.Name() == "mysql" { + t.Skip("MySQL's default collation refuses two rows differing only by case at insert; the collision cannot exist there") + } + require.NoError(t, db.Migrator().DropTable(&agentNetworkTypes.Settings{})) + require.NoError(t, db.AutoMigrate(&agentNetworkTypes.Settings{})) + require.NoError(t, db.Create(&agentNetworkTypes.Settings{ + AccountID: "acct-1", Domain: "Violet.eu.proxy.netbird.io", ProxyAddress: "eu.proxy.netbird.io", + }).Error) + require.NoError(t, db.Create(&agentNetworkTypes.Settings{ + AccountID: "acct-2", Domain: "violet.eu.proxy.netbird.io", ProxyAddress: "eu.proxy.netbird.io", + }).Error) + + err := migration.NormalizeAgentNetworkSettingsIdentity(ctx, db) + require.Error(t, err, "two rows folding onto one endpoint must stop the migration") + assert.Contains(t, err.Error(), "violet.eu.proxy.netbird.io", "the failure must name the colliding hostname") + + var one agentNetworkTypes.Settings + require.NoError(t, db.First(&one, "account_id = ?", "acct-1").Error) + assert.Equal(t, "Violet.eu.proxy.netbird.io", one.Domain, "a refused normalisation must leave every row as it was") +} + +// TestMigrateAgentNetworkSettingsToDomain_RefusesCaseOnlyCollision pins the +// same loud failure on the reshape: legacy rows whose identities differ only +// by case would fold onto one endpoint, and the reshape must say so rather +// than leave AutoMigrate to fail on the unique index. +func TestMigrateAgentNetworkSettingsToDomain_RefusesCaseOnlyCollision(t *testing.T) { + ctx := context.Background() + db := setupDatabase(t) + require.NoError(t, db.Migrator().DropTable(&legacyAgentNetworkSettings{})) + require.NoError(t, db.AutoMigrate(&legacyAgentNetworkSettings{})) + require.NoError(t, db.Create(&legacyAgentNetworkSettings{ + AccountID: "acct-1", Cluster: "EU.proxy.netbird.io", Subdomain: "violet", + }).Error) + require.NoError(t, db.Create(&legacyAgentNetworkSettings{ + AccountID: "acct-2", Cluster: "eu.proxy.netbird.io", Subdomain: "violet", + }).Error) + + err := migration.MigrateAgentNetworkSettingsToDomain(ctx, db) + require.Error(t, err, "legacy rows folding onto one endpoint must stop the reshape") + assert.Contains(t, err.Error(), "violet.eu.proxy.netbird.io", "the failure must name the colliding hostname") +}