From 9e3f945eda1b7e3152699fe35a77a53a6bbd7d9d Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 23 Jan 2025 14:58:42 +0100 Subject: [PATCH] fix postgres migration issue with 0.24 (#2367) * fix postgres migration issue with 0.24 Fixes #2351 Signed-off-by: Kristoffer Dalby * add postgres migration test for 2351 Signed-off-by: Kristoffer Dalby * update changelog Signed-off-by: Kristoffer Dalby --------- Signed-off-by: Kristoffer Dalby --- CHANGELOG.md | 2 + hscontrol/db/db.go | 32 +++++++++ hscontrol/db/db_test.go | 61 +++++++++++++++++- hscontrol/db/suite_test.go | 18 ++++-- .../db/testdata/pre-24-postgresdb.pssql.dump | Bin 0 -> 19869 bytes 5 files changed, 105 insertions(+), 8 deletions(-) create mode 100644 hscontrol/db/testdata/pre-24-postgresdb.pssql.dump diff --git a/CHANGELOG.md b/CHANGELOG.md index 4122dc2c..a06a2ad1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ ### Changes +- Fix migration issue with user table for PostgreSQL + [#2367](https://github.com/juanfont/headscale/pull/2367) - Relax username validation to allow emails [#2364](https://github.com/juanfont/headscale/pull/2364) - Remove invalid routes and add stronger constraints for routes to avoid API panic diff --git a/hscontrol/db/db.go b/hscontrol/db/db.go index 553d7f0e..36955e22 100644 --- a/hscontrol/db/db.go +++ b/hscontrol/db/db.go @@ -478,6 +478,38 @@ func NewHeadscaleDatabase( // populate the user with more interesting information. ID: "202407191627", Migrate: func(tx *gorm.DB) error { + // Fix an issue where the automigration in GORM expected a constraint to + // exists that didnt, and add the one it wanted. + // Fixes https://github.com/juanfont/headscale/issues/2351 + if cfg.Type == types.DatabasePostgres { + err := tx.Exec(` +BEGIN; +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'uni_users_name' + ) THEN + ALTER TABLE users ADD CONSTRAINT uni_users_name UNIQUE (name); + END IF; +END $$; + +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'users_name_key' + ) THEN + ALTER TABLE users DROP CONSTRAINT users_name_key; + END IF; +END $$; +COMMIT; +`).Error + if err != nil { + return fmt.Errorf("failed to rename constraint: %w", err) + } + } + err := tx.AutoMigrate(&types.User{}) if err != nil { return err diff --git a/hscontrol/db/db_test.go b/hscontrol/db/db_test.go index c3d9a835..0672c252 100644 --- a/hscontrol/db/db_test.go +++ b/hscontrol/db/db_test.go @@ -6,6 +6,7 @@ import ( "io" "net/netip" "os" + "os/exec" "path/filepath" "slices" "sort" @@ -23,7 +24,10 @@ import ( "zgo.at/zcache/v2" ) -func TestMigrations(t *testing.T) { +// TestMigrationsSQLite is the main function for testing migrations, +// we focus on SQLite correctness as it is the main database used in headscale. +// All migrations that are worth testing should be added here. +func TestMigrationsSQLite(t *testing.T) { ipp := func(p string) netip.Prefix { return netip.MustParsePrefix(p) } @@ -375,3 +379,58 @@ func TestConstraints(t *testing.T) { }) } } + +func TestMigrationsPostgres(t *testing.T) { + tests := []struct { + name string + dbPath string + wantFunc func(*testing.T, *HSDatabase) + }{ + { + name: "user-idx-breaking", + dbPath: "testdata/pre-24-postgresdb.pssql.dump", + wantFunc: func(t *testing.T, h *HSDatabase) { + users, err := Read(h.DB, func(rx *gorm.DB) ([]types.User, error) { + return ListUsers(rx) + }) + require.NoError(t, err) + + for _, user := range users { + assert.NotEmpty(t, user.Name) + assert.Empty(t, user.ProfilePicURL) + assert.Empty(t, user.Email) + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u := newPostgresDBForTest(t) + + pgRestorePath, err := exec.LookPath("pg_restore") + if err != nil { + t.Fatal("pg_restore not found in PATH. Please install it and ensure it is accessible.") + } + + // Construct the pg_restore command + cmd := exec.Command(pgRestorePath, "--verbose", "--if-exists", "--clean", "--no-owner", "--dbname", u.String(), tt.dbPath) + + // Set the output streams + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + // Execute the command + err = cmd.Run() + if err != nil { + t.Fatalf("failed to restore postgres database: %s", err) + } + + db = newHeadscaleDBFromPostgresURL(t, u) + + if tt.wantFunc != nil { + tt.wantFunc(t, db) + } + }) + } +} diff --git a/hscontrol/db/suite_test.go b/hscontrol/db/suite_test.go index fb7ce1df..e9c71823 100644 --- a/hscontrol/db/suite_test.go +++ b/hscontrol/db/suite_test.go @@ -78,13 +78,11 @@ func newSQLiteTestDB() (*HSDatabase, error) { func newPostgresTestDB(t *testing.T) *HSDatabase { t.Helper() - var err error - tmpDir, err = os.MkdirTemp("", "headscale-db-test-*") - if err != nil { - t.Fatal(err) - } + return newHeadscaleDBFromPostgresURL(t, newPostgresDBForTest(t)) +} - log.Printf("database path: %s", tmpDir+"/headscale_test.db") +func newPostgresDBForTest(t *testing.T) *url.URL { + t.Helper() ctx := context.Background() srv, err := postgrestest.Start(ctx) @@ -100,10 +98,16 @@ func newPostgresTestDB(t *testing.T) *HSDatabase { t.Logf("created local postgres: %s", u) pu, _ := url.Parse(u) + return pu +} + +func newHeadscaleDBFromPostgresURL(t *testing.T, pu *url.URL) *HSDatabase { + t.Helper() + pass, _ := pu.User.Password() port, _ := strconv.Atoi(pu.Port()) - db, err = NewHeadscaleDatabase( + db, err := NewHeadscaleDatabase( types.DatabaseConfig{ Type: types.DatabasePostgres, Postgres: types.PostgresConfig{ diff --git a/hscontrol/db/testdata/pre-24-postgresdb.pssql.dump b/hscontrol/db/testdata/pre-24-postgresdb.pssql.dump new file mode 100644 index 0000000000000000000000000000000000000000..7f8df28b30279ff139058f09fcc6de8c5e0648e1 GIT binary patch literal 19869 zcmd5^3vipo5&mtGHUtM!T2i3UUI<`2sKJsg*$xDv$Vy^jOU9NH45b=b`fdM-ED8N` zVwlpI(v)dohEiUo5ZXx#1rlB@+ zOCd|Mp=BV^HIV2Bo(2@=D`Be%cLBEaQYx286*I|nJ~xstjst5+y;Ow%bj)HtH%$g_ zmopd*_eWOtg%c|&gZ5X#U;jdWha(F-)`|5DF0R{h`*5HnFzgHSY>1!pp(~QL1gd zXvuZz#gqad)MEhjEDSLKB2Q6aVmPa%7n%K0(`*DUErW)&Q7u)Npq0kRS9D1}r_zq7i(ode>tGBfpbsR%{RyhGe)6C@2BUkU zu>@K+gy^GK9QAg`R)>2AqofP|9zM_h9tlTQMv)(YIu#}-K$V%Ahs^erM0^Qc3igo! ze>>wZ@3gGj>B2>LeF4T>yv5hVqFo>+HSY*Cg?kcUg;obfe+}G_IgZ73k(O~9pF<(4 zlCFc1?(Y;4c;}SDPb@Q+&lu+HFvfY{Z$-vPI+wA;@l<+D%c5^JR>r=&xVMA&PwKh|$ozT@HKx{Y%`mCsqYC7~z zj;p1ye8xc&wzV|)f>H{$G}~pQCQ4(NAC^veLw9%%5ZcqdUPd?oJeI8Y_toe{<04U;P06nxo>a}gD__C#dz1WvAL^xkYw4gSQEYQM;_L5v9 z+DWuHu8q=dn+q*g!BCMmvq#u2jdwADV6fQ=Hi!w}_?T#HPy*!RQdSZxh9>;92k<)2qq3={7_IAa@5Gfc`mDc5k5UtT4s*i-OVyXZ)GXTr@kzn`3h zgbc_{>U%)RfQ}8Uy(k?5`)UPFgr^fPK6qrSGPX8G9q$qv8+MUiOn6z|g)mmU*LzXq z%}`_pHL}Sd1Hx`#nZ7s`3y`FqlXET>m;;Bra<-_Ku|fCAMtt&^&{M-%6%FU}Sv8e2 z&Ig4tbzCi`vgTvd6`Blpm35GTQw$v@YOi=_t{{&=qM7*srnjTec{R2dpvol0 z9%JnU4x&@}5;VG2Pwp6Sy0!wxV4e>SfkQYAI4J87%=vi)t8xS(#s=;dIsz$t9>IPu zNKxJam|}PuzFs4PXA?B90*my;8hoIIq)CeTi4q?HpCGYx+PjR2S8QdqKYf!-3A3|< zQMj|4smyw{SkiRW@)d@Q$ziv{Jk<1Lp{R{R%EWXm#PE*?Pni z-r@s6;;-q9a#LRq1SF>J8hQw9{A3d%Hu!K&%!5^xakDAAdxXY~ud7_MdTr}xLuav7 zyGU^6Oj~PY$82(9A23BHL3e#by%IBWXrN;FZV5s7gzF zjgEwO7T{Jtx%ea#$_;J8hMLuP=g31~XA59v;a=*Tmq)4^GizlNO7{xQ4BrnsC-t(j zCD2!FWljp5k&(vQ*jTHd2@GB#x7xrLQGr0HrOmj0YikWCN5EtA4hSyNl+VL=BLbzj z7;E7H6^Pzd#bZ4~>^%jd9U}3b!QL3Yj>A{RW*nP?>$ml(Y*VB7@}qI_V#vv-A)l_} zo3X{rbj%=bCZZw6RMS%ie~08UXqOznR+4WcDNZ4Hql`38@aU*&kYiG<407$vTn2LD zX=(=>#9N2K%mF{^+IP!gYa?oB=4ph;d8wT>up02pL58biz>}z*Ip`3oU7(fX$7Rv> z$ngs*#{%TeQ&G~)+X%tj$f$->4aHQdLRNIp7sI@QVZ_kU%6O=u&IS$4*<{MAa?C@d zs>8{tF2uUTh$zEa=XA=;X}abWV{6BEuA&0hNN&y=V%lzC%AQMKlVjSh91dvF08L49e4QkH7 zg#k^n=ZNR19cB=^l0Aou->$>)8=UdY(mPxahfA`UG9z5+MpqfvT@hN2uyGBvM&f;Z zl+&bTd?lYl(cw&I$OCom}%Ft303?E$@%_Lj4?6^FerHm-IL$g!yFUJXB7j zKBfLrd~1ncH;U!#;? z?{RA)pb;(AA2bELSt6jcZ&>lU$? ze$g$5w|H5g=CplxoIu1PR^%`ZdVc}BZQfZB!vS%fOK*F@+8{u~iTVx`fuD&HaHoB} zMZbg*-Pq$R)?(sVjCCh_ft`89hEKAsV9ZvAwKhP|_@I8!1Y)xU2xs2R8;4#72fA%n z4g;jKY9gAkjo0rsMY|7+MqYR?H!J8x<(Cc6ZsnyQfF}E;fY7?YQv|xXS4C#ucSgT=X#QsC>%amxsHMo?L4y*sa%9W9DlHe@~}Cz2Zi$BH6gy++t_f9 ztE?(I>3}pf6o$)7zs??T>CWX3wY9Y<^MF|^FKvEE79b=cg`JUj41Ts0?v5o$Qd>zt zcjxKddEzxklRJuVR~NDeBeRWAMt%L=z2W{Lv?@A84nS~VTUr5OL{7Fcs(e-&NSZ&` zA|J)SjKId3zm!I zG{Iu+<6>#B%2r)>7Tg!1$dG>+@Gy1gq$ zFhq>h+?|p6L5`l6k(#4L*qLookJPkH`zw>%?bg~%VACD z5V_EEDxJa;TS|YLn6kP259OE!l%s)f<0x$eMN$-TCP}vU(6J);98^WP;c(5HC z;xv(BPkW!pk!m9~jr6ch!!7(nHk_#xhBk2zvB+={RBpOxFxCw}=|oLP-4cNu7KwB5aE8L`!6{n9psNVU>2-h-p;nOsPx;Y*l-BOIUq4x zkqZ)g3Jl9>QdrE)Y!HYh_~Y+$%i4p?ZL7bw;* zP&i%T=uQ(7)**i+g(TEM95RN4dKft(Jt9QdK@LEu-||ord;=%89hU0!;NV>Dr!F|4 zD9k#1hy28N=wWSSU6OpTLL*Y0yL^@73{73j^&p7MQ%X;h9Cfrj-XHB=5u+z?$8}pX z>W?mu_T#@wFkt#6SHuQc>t+mm7o>gRuVmm_3jBv4zz)OZIOC!Mjx8Shs%~#P2}G^I z_BMK^53pgCVx@I(k*0Go+Wn3*y~P*S4cS6?)%uiQ2u#_%2>1!)l|&0bRFnZe5*~0>pq9 z>nemrc=+Siu!gqQTPb7Z*9mhqVmKnLWFA*DLjp zD|f+!;J0vyR+XK+a+U)?eCq>aweD?)~eARU=EzdZ5sA#U1lncI|%iw=Z6>>c_E$ zvxolp)8o$g@QL?7&AhkwtOMbN+t;mq?1{Ut(GS)=-EjIA0|0VCyp=F>=~KP4A6?pT z`K}55inr9uPrmlUKg~b$J2%fe=a@5Q-*(v_Hq86^>Md`4uxI|}%YSss{J@(R9DU%9 zm7xWr+ajNQ?Wm7OUccm(w+H*KdhViE9}eG@KINm-S05=}xczsxZ;tk-uFssZcdj~f z{JDjH{zdak+Uxgjxcar5=Ipzn`^1HfSMI-T<3k6wu72YiPd>VG*1-qvKB{HwdsjTO lZ_~G~iEEFn);f=j+_~nZeOCsb+p(j5`(aOx^u9w_|1V5I(Ki49 literal 0 HcmV?d00001