-
Notifications
You must be signed in to change notification settings - Fork 931
Allow multiple packages - migration #48596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jkatz01
wants to merge
10
commits into
feat/28108-multiple-custom-packages
Choose a base branch
from
48396-multi-package-migration
base: feat/28108-multiple-custom-packages
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
e2ab423
Add regime-aware uniqueness for multiple packages per title (#48396)
jkatz01 2b2d476
Hash-based duplicate guard and 10-package limit for custom software (…
jkatz01 529cf0f
First-added reads, list-all packages method, and multi-active regress…
jkatz01 ec75765
Use defer for rows.Close in migration test and reword list-all doc (#…
jkatz01 ee04167
Test multiple active packages collapse to one title with correct coun…
jkatz01 8265e24
Restore team-wide script dedup and apply review cleanups (#48396)
jkatz01 e835606
check timestamps
jkatz01 db0b67c
Keep active installer when collapsing duplicates in migration (#48396)
jkatz01 d94721f
Re-point setup experience and pending installs in migration, guard ma…
jkatz01 f104d5d
Bump migration timestamp past #48664 (#48396)
jkatz01 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
103 changes: 103 additions & 0 deletions
103
server/datastore/mysql/migrations/tables/20260702232839_MultipleCustomPackagesPerTitle.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| package tables | ||
|
|
||
| import ( | ||
| "database/sql" | ||
| "fmt" | ||
| ) | ||
|
|
||
| func init() { | ||
| MigrationClient.AddMigration(Up_20260702232839, Down_20260702232839) | ||
| } | ||
|
|
||
| func Up_20260702232839(tx *sql.Tx) error { | ||
| // A title can now hold several packages. dedup_token drives the new unique key. Custom | ||
| // rows resolve it to storage_id so they dedupe by content hash, letting different builds of | ||
| // one version coexist. FMA rows resolve it to version, leaving the per-version rows that | ||
| // back version pinning unchanged. VIRTUAL keeps the add in-place. The collation is pinned | ||
| // to match storage_id and version so the migration matches what fresh installs get. | ||
| if _, err := tx.Exec(` | ||
| ALTER TABLE software_installers | ||
| ADD COLUMN dedup_token VARCHAR(255) COLLATE utf8mb4_unicode_ci | ||
| GENERATED ALWAYS AS (IF(fleet_maintained_app_id IS NULL, storage_id, version)) VIRTUAL | ||
| `); err != nil { | ||
| return fmt.Errorf("adding dedup_token column: %w", err) | ||
| } | ||
|
|
||
| // Collapse rows that would violate the new key: keep the first-added active row per group | ||
| // (the row the reads return), or the lowest id if none is active, and delete the rest. | ||
| // Re-point policies off the deleted rows first, since policies.software_installer_id is | ||
| // RESTRICT. Keep policies.updated_at so this content-identical swap doesn't read as a | ||
| // policy edit. | ||
| const dupGroups = ` | ||
| SELECT global_or_team_id, title_id, dedup_token, | ||
| COALESCE(MIN(CASE WHEN is_active = 1 THEN id END), MIN(id)) AS keep_id | ||
| FROM software_installers | ||
| WHERE title_id IS NOT NULL | ||
| GROUP BY global_or_team_id, title_id, dedup_token | ||
| HAVING COUNT(*) > 1` | ||
|
|
||
| if _, err := tx.Exec(fmt.Sprintf(` | ||
| UPDATE policies p | ||
| JOIN software_installers si ON si.id = p.software_installer_id | ||
| JOIN (%s) dup | ||
| ON si.global_or_team_id = dup.global_or_team_id | ||
| AND si.title_id = dup.title_id | ||
| AND si.dedup_token = dup.dedup_token | ||
| SET p.software_installer_id = dup.keep_id, p.updated_at = p.updated_at | ||
| WHERE si.id != dup.keep_id`, dupGroups)); err != nil { | ||
| return fmt.Errorf("re-pointing policies off duplicate installers: %w", err) | ||
| } | ||
|
|
||
| // setup_experience_software_installers has an ON DELETE CASCADE FK, so a selection that | ||
| // lived only on a deleted duplicate would be silently dropped. Re-point those rows onto the | ||
| // survivor first. UPDATE IGNORE skips a row when the survivor already has that platform. | ||
| if _, err := tx.Exec(fmt.Sprintf(` | ||
| UPDATE IGNORE setup_experience_software_installers sesi | ||
| JOIN software_installers si ON si.id = sesi.software_installer_id | ||
| JOIN (%s) dup | ||
| ON si.global_or_team_id = dup.global_or_team_id | ||
| AND si.title_id = dup.title_id | ||
| AND si.dedup_token = dup.dedup_token | ||
| SET sesi.software_installer_id = dup.keep_id | ||
| WHERE si.id != dup.keep_id`, dupGroups)); err != nil { | ||
| return fmt.Errorf("re-pointing setup experience installers off duplicate installers: %w", err) | ||
| } | ||
|
|
||
| // software_install_upcoming_activities has an ON DELETE SET NULL FK, so a queued install on | ||
| // a deleted duplicate would be silently orphaned. Re-point pending installs onto the survivor. | ||
| if _, err := tx.Exec(fmt.Sprintf(` | ||
| UPDATE software_install_upcoming_activities siua | ||
| JOIN software_installers si ON si.id = siua.software_installer_id | ||
| JOIN (%s) dup | ||
| ON si.global_or_team_id = dup.global_or_team_id | ||
| AND si.title_id = dup.title_id | ||
| AND si.dedup_token = dup.dedup_token | ||
| SET siua.software_installer_id = dup.keep_id, siua.updated_at = siua.updated_at | ||
| WHERE si.id != dup.keep_id`, dupGroups)); err != nil { | ||
| return fmt.Errorf("re-pointing upcoming install activities off duplicate installers: %w", err) | ||
| } | ||
|
|
||
| if _, err := tx.Exec(fmt.Sprintf(` | ||
| DELETE si FROM software_installers si | ||
| JOIN (%s) dup | ||
| ON si.global_or_team_id = dup.global_or_team_id | ||
| AND si.title_id = dup.title_id | ||
| AND si.dedup_token = dup.dedup_token | ||
| WHERE si.id != dup.keep_id`, dupGroups)); err != nil { | ||
| return fmt.Errorf("deleting duplicate installers: %w", err) | ||
| } | ||
|
|
||
| if _, err := tx.Exec(` | ||
| ALTER TABLE software_installers | ||
| DROP INDEX idx_software_installers_team_title_version, | ||
| ADD UNIQUE KEY idx_software_installers_dedup (global_or_team_id, title_id, dedup_token) | ||
| `); err != nil { | ||
| return fmt.Errorf("swapping software_installers unique key: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func Down_20260702232839(tx *sql.Tx) error { | ||
| return nil | ||
| } | ||
194 changes: 194 additions & 0 deletions
194
...r/datastore/mysql/migrations/tables/20260702232839_MultipleCustomPackagesPerTitle_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,194 @@ | ||
| package tables | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestUp_20260702232839(t *testing.T) { | ||
| db := applyUpToPrev(t) | ||
|
|
||
| insertTitle := func(name string, source string) int64 { | ||
| return execNoErrLastID(t, db, `INSERT INTO software_titles (name, source) VALUES (?, ?)`, name, source) | ||
| } | ||
|
|
||
| const installerInsert = ` | ||
| INSERT INTO software_installers | ||
| (team_id, global_or_team_id, title_id, filename, extension, version, platform, | ||
| install_script_content_id, uninstall_script_content_id, storage_id, package_ids, patch_query, | ||
| fleet_maintained_app_id, is_active) | ||
| VALUES (?, ?, ?, ?, 'pkg', ?, ?, ?, ?, ?, '', '', ?, ?)` | ||
|
|
||
| insertScript := func(seed string) int64 { | ||
| return execNoErrLastID(t, db, `INSERT INTO script_contents (contents, md5_checksum) VALUES ('#!/bin/sh', UNHEX(MD5(?)))`, seed) | ||
| } | ||
|
|
||
| // teamID nil means no-team with global_or_team_id 0, fmaID nil means a custom package. | ||
| args := func(titleID int64, teamID *int64, platform string, version string, storage string, fmaID *int64, active int) []any { | ||
| script := insertScript(storage + version) | ||
| var globalOrTeamID int64 | ||
| if teamID != nil { | ||
| globalOrTeamID = *teamID | ||
| } | ||
| return []any{teamID, globalOrTeamID, titleID, storage + "-" + version + ".pkg", version, platform, script, script, storage, fmaID, active} | ||
| } | ||
| insertInstaller := func(titleID int64, teamID *int64, platform string, version string, storage string, fmaID *int64, active int) int64 { | ||
| return execNoErrLastID(t, db, installerInsert, args(titleID, teamID, platform, version, storage, fmaID, active)...) | ||
| } | ||
| tryInsertInstaller := func(titleID int64, teamID *int64, platform string, version string, storage string, fmaID *int64, active int) error { | ||
| _, err := db.Exec(installerInsert, args(titleID, teamID, platform, version, storage, fmaID, active)...) | ||
| return err | ||
| } | ||
|
|
||
| countRows := func(query string, qargs ...any) int { | ||
| var n int | ||
| require.NoError(t, db.QueryRow(query, qargs...).Scan(&n)) | ||
| return n | ||
| } | ||
| remainingIDs := func(titleID int64) []int64 { | ||
| var ids []int64 | ||
| r, err := db.Query(`SELECT id FROM software_installers WHERE title_id = ? ORDER BY id`, titleID) | ||
| require.NoError(t, err) | ||
| defer r.Close() | ||
| for r.Next() { | ||
| var id int64 | ||
| require.NoError(t, r.Scan(&id)) | ||
| ids = append(ids, id) | ||
| } | ||
| require.NoError(t, r.Err()) | ||
| return ids | ||
| } | ||
|
|
||
| team := execNoErrLastID(t, db, `INSERT INTO teams (name) VALUES ('Team 1')`) | ||
| label := execNoErrLastID(t, db, `INSERT INTO labels (name, query) VALUES ('L1', '')`) | ||
| category := execNoErrLastID(t, db, `INSERT INTO software_categories (name) VALUES ('C1')`) | ||
| fma := execNoErrLastID(t, db, `INSERT INTO fleet_maintained_apps (name, slug, platform, unique_identifier) VALUES ('AppD', 'appd/darwin', 'darwin', 'com.appd')`) | ||
|
|
||
| // Single custom package (no-team). Untouched. | ||
| titleA := insertTitle("AppA", "apps") | ||
| soloID := insertInstaller(titleA, nil, "darwin", "1.0", "hash-a", nil, 1) | ||
|
|
||
| // Custom hash-duplicate (no-team): two active rows with the same content but | ||
| // different versions, so the still-present version key allows seeding both. Each | ||
| // carries a label and a category, and a policy points at the row to be deleted. | ||
| titleB := insertTitle("AppB", "programs") | ||
| keepB := insertInstaller(titleB, nil, "windows", "1.0", "hash-b", nil, 1) | ||
| dupB := insertInstaller(titleB, nil, "windows", "2.0", "hash-b", nil, 1) | ||
| for _, id := range []int64{keepB, dupB} { | ||
| execNoErr(t, db, `INSERT INTO software_installer_labels (software_installer_id, label_id) VALUES (?, ?)`, id, label) | ||
| execNoErr(t, db, `INSERT INTO software_installer_software_categories (software_installer_id, software_category_id) VALUES (?, ?)`, id, category) | ||
| } | ||
| policyID := execNoErrLastID(t, db, ` | ||
| INSERT INTO policies (name, query, description, checksum, software_installer_id) | ||
| VALUES ('p1', 'SELECT 1', '', UNHEX(MD5('p1')), ?)`, dupB) | ||
| // Freeze updated_at so the re-point can be checked for not bumping it. | ||
| execNoErr(t, db, `UPDATE policies SET updated_at = '2020-01-01 00:00:00' WHERE id = ?`, policyID) | ||
| // A setup experience selection living only on the deleted row (FK is ON DELETE CASCADE). | ||
| execNoErr(t, db, `INSERT INTO setup_experience_software_installers (software_installer_id, platform, global_or_team_id) VALUES (?, 'windows', 0)`, dupB) | ||
| // A pending install queued on the deleted row (FK is ON DELETE SET NULL). | ||
| upcomingID := execNoErrLastID(t, db, `INSERT INTO upcoming_activities (host_id, activity_type, execution_id, payload) VALUES (1, 'software_install', 'dup-install-exec', '{}')`) | ||
| execNoErr(t, db, `INSERT INTO software_install_upcoming_activities (upcoming_activity_id, software_installer_id, software_title_id) VALUES (?, ?, ?)`, upcomingID, dupB, titleB) | ||
|
|
||
| // Custom hash-duplicate scoped to a team, different source. | ||
| titleC := insertTitle("AppC", "rpm_packages") | ||
| keepC := insertInstaller(titleC, &team, "linux", "1.0", "hash-c", nil, 1) | ||
| dupC := insertInstaller(titleC, &team, "linux", "1.1", "hash-c", nil, 1) | ||
|
|
||
| // FMA with the same bytes backing two versions. Tokens resolve to version, so both | ||
| // must survive. | ||
| titleD := insertTitle("AppD", "apps") | ||
| fmaOld := insertInstaller(titleD, nil, "darwin", "1.0", "hash-d", &fma, 0) | ||
| fmaActive := insertInstaller(titleD, nil, "darwin", "2.0", "hash-d", &fma, 1) | ||
|
|
||
| // Custom hash-duplicate where the first-added row is inactive and a later row is active. | ||
| // The active row must be the survivor to match the is_active reads, even though it is not | ||
| // the lowest id. A policy points at the inactive row that gets deleted. | ||
| titleF := insertTitle("AppF", "apps") | ||
| inactiveF := insertInstaller(titleF, nil, "darwin", "1.0", "hash-f", nil, 0) | ||
| activeF := insertInstaller(titleF, nil, "darwin", "2.0", "hash-f", nil, 1) | ||
| policyF := execNoErrLastID(t, db, ` | ||
| INSERT INTO policies (name, query, description, checksum, software_installer_id) | ||
| VALUES ('pf', 'SELECT 1', '', UNHEX(MD5('pf')), ?)`, inactiveF) | ||
|
|
||
| applyNext(t, db) | ||
|
|
||
| // The version key is gone, replaced by the dedup_token key. | ||
| require.Zero(t, countRows(` | ||
| SELECT COUNT(*) FROM information_schema.statistics | ||
| WHERE table_schema = DATABASE() AND table_name = 'software_installers' | ||
| AND index_name = 'idx_software_installers_team_title_version'`)) | ||
| var dedupCols []string | ||
| rows, err := db.Query(` | ||
| SELECT column_name FROM information_schema.statistics | ||
| WHERE table_schema = DATABASE() AND table_name = 'software_installers' | ||
| AND index_name = 'idx_software_installers_dedup' | ||
| ORDER BY seq_in_index`) | ||
| require.NoError(t, err) | ||
| defer rows.Close() | ||
| for rows.Next() { | ||
| var col string | ||
| require.NoError(t, rows.Scan(&col)) | ||
| dedupCols = append(dedupCols, col) | ||
| } | ||
| require.NoError(t, rows.Err()) | ||
| require.Equal(t, []string{"global_or_team_id", "title_id", "dedup_token"}, dedupCols) | ||
|
|
||
| // Single-package title untouched. | ||
| require.Equal(t, []int64{soloID}, remainingIDs(titleA)) | ||
|
|
||
| // Custom hash-duplicates collapse to the first-added row, which stays active. | ||
| require.Equal(t, []int64{keepB}, remainingIDs(titleB)) | ||
| require.Equal(t, []int64{keepC}, remainingIDs(titleC)) | ||
| require.NotContains(t, remainingIDs(titleC), dupC) | ||
| require.Equal(t, 1, countRows(`SELECT is_active FROM software_installers WHERE id = ?`, keepB)) | ||
| require.Equal(t, 1, countRows(`SELECT is_active FROM software_installers WHERE id = ?`, keepC)) | ||
|
|
||
| // The survivor keeps its label and category. The deleted row's cascade away. | ||
| require.Equal(t, 1, countRows(`SELECT COUNT(*) FROM software_installer_labels WHERE software_installer_id = ?`, keepB)) | ||
| require.Equal(t, 1, countRows(`SELECT COUNT(*) FROM software_installer_software_categories WHERE software_installer_id = ?`, keepB)) | ||
| require.Zero(t, countRows(`SELECT COUNT(*) FROM software_installer_labels WHERE software_installer_id = ?`, dupB)) | ||
| require.Zero(t, countRows(`SELECT COUNT(*) FROM software_installer_software_categories WHERE software_installer_id = ?`, dupB)) | ||
|
|
||
| // The policy was re-pointed off the deleted row onto the survivor, without bumping updated_at. | ||
| var repointed int64 | ||
| require.NoError(t, db.QueryRow(`SELECT software_installer_id FROM policies WHERE id = ?`, policyID).Scan(&repointed)) | ||
| require.Equal(t, keepB, repointed) | ||
| var updatedAtUnchanged bool | ||
| require.NoError(t, db.QueryRow(`SELECT updated_at = '2020-01-01 00:00:00' FROM policies WHERE id = ?`, policyID).Scan(&updatedAtUnchanged)) | ||
| require.True(t, updatedAtUnchanged) | ||
|
|
||
| // The setup experience selection on the deleted row was re-pointed to the survivor, not | ||
| // dropped by the ON DELETE CASCADE. | ||
| var setupExperienceInstaller int64 | ||
| require.NoError(t, db.QueryRow(`SELECT software_installer_id FROM setup_experience_software_installers WHERE platform = 'windows'`).Scan(&setupExperienceInstaller)) | ||
| require.Equal(t, keepB, setupExperienceInstaller) | ||
|
|
||
| // The pending install on the deleted row was re-pointed to the survivor, not orphaned by | ||
| // the ON DELETE SET NULL. | ||
| var upcomingInstaller int64 | ||
| require.NoError(t, db.QueryRow(`SELECT software_installer_id FROM software_install_upcoming_activities WHERE upcoming_activity_id = ?`, upcomingID).Scan(&upcomingInstaller)) | ||
| require.Equal(t, keepB, upcomingInstaller) | ||
|
|
||
| // FMA same-hash-different-version rows both survive. | ||
| require.Equal(t, []int64{fmaOld, fmaActive}, remainingIDs(titleD)) | ||
|
|
||
| // The active row is retained over the lower-id inactive one, and the policy re-points to it. | ||
| require.Equal(t, []int64{activeF}, remainingIDs(titleF)) | ||
| var repointedF int64 | ||
| require.NoError(t, db.QueryRow(`SELECT software_installer_id FROM policies WHERE id = ?`, policyF).Scan(&repointedF)) | ||
| require.Equal(t, activeF, repointedF) | ||
|
|
||
| // New key behavior. Custom same-version-different-hash is accepted, and these could not | ||
| // be seeded before the migration because the old version key blocked two rows sharing a | ||
| // version. | ||
| titleE := insertTitle("AppE", "apps") | ||
| require.NoError(t, tryInsertInstaller(titleE, nil, "darwin", "9.0", "hash-e1", nil, 1)) | ||
| require.NoError(t, tryInsertInstaller(titleE, nil, "darwin", "9.0", "hash-e2", nil, 1)) | ||
|
|
||
| // A second package with identical bytes on the same title is rejected by the key. | ||
| require.Error(t, tryInsertInstaller(titleE, nil, "darwin", "8.0", "hash-e1", nil, 1)) | ||
|
|
||
| // FMA can still cache another version backed by the same bytes. | ||
| require.NoError(t, tryInsertInstaller(titleD, nil, "darwin", "3.0", "hash-d", &fma, 0)) | ||
| } |
Large diffs are not rendered by default.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this re-point
setup_experience_software_installersrows onto the surviving installer before deleting duplicates, since that FK is ON DELETE CASCADE (unlike policies, which you already addrewss here) and would otherwise silently drop a setup experience selection that lived only on a deleted row?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also for
software_install_upcoming_activities?