diff --git a/changes/42637-clear-stale-broken-label-associations b/changes/42637-clear-stale-broken-label-associations new file mode 100644 index 0000000000..d977c03644 --- /dev/null +++ b/changes/42637-clear-stale-broken-label-associations @@ -0,0 +1 @@ +* Fixed `fleetctl gitops apply` not clearing stale broken `mdm_configuration_profile_labels` rows after a referenced label was deleted, which caused profiles to remain enforced on hosts regardless of updated label targeting. diff --git a/server/datastore/mysql/apple_mdm_test.go b/server/datastore/mysql/apple_mdm_test.go index 760c5afde6..e43927463c 100644 --- a/server/datastore/mysql/apple_mdm_test.go +++ b/server/datastore/mysql/apple_mdm_test.go @@ -55,6 +55,7 @@ func TestMDMApple(t *testing.T) { {"TestHostDetailsMDMProfiles", testHostDetailsMDMProfiles}, {"TestHostDetailsMDMProfilesIOSIPadOS", testHostDetailsMDMProfilesIOSIPadOS}, {"TestBatchSetMDMAppleProfiles", testBatchSetMDMAppleProfiles}, + {"TestBatchSetMDMAppleProfilesClearsStaleBrokenLabels", testBatchSetMDMAppleProfilesClearsStaleBrokenLabels}, {"TestMDMAppleProfileManagement", testMDMAppleProfileManagement}, {"TestMDMAppleProfileManagementBatch2", testMDMAppleProfileManagementBatch2}, {"TestMDMAppleProfileManagementBatch3", testMDMAppleProfileManagementBatch3}, @@ -1489,6 +1490,71 @@ func testBatchSetMDMAppleProfiles(t *testing.T, ds *Datastore) { applyAndExpect(nil, ptr.Uint(1), expectFleetProfiles) } +// Regression test for https://github.com/fleetdm/fleet/issues/42637. +func testBatchSetMDMAppleProfilesClearsStaleBrokenLabels(t *testing.T, ds *Datastore) { + ctx := t.Context() + + labelX, err := ds.NewLabel(ctx, &fleet.Label{Name: "exclude-x-42637", Query: "select 1 from osquery_info;"}) + require.NoError(t, err) + labelY, err := ds.NewLabel(ctx, &fleet.Label{Name: "include-any-y-42637", Query: "select 1 from osquery_info;"}) + require.NoError(t, err) + + // First gitops apply: profile with labels_exclude_any: [labelX]. + prof := configProfileForTest(t, "Repro42637", "com.fleetdm.repro.42637", "u1", labelX) + require.NoError(t, ds.BatchSetMDMAppleProfiles(ctx, nil, []*fleet.MDMAppleConfigProfile{prof})) + + var profileUUID string + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(ctx, q, &profileUUID, + `SELECT profile_uuid FROM mdm_apple_configuration_profiles WHERE identifier = ?`, + prof.Identifier) + }) + require.NotEmpty(t, profileUUID) + + type row struct { + LabelID *uint `db:"label_id"` + LabelName string `db:"label_name"` + Exclude bool `db:"exclude"` + } + loadRows := func() []row { + var rows []row + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.SelectContext(ctx, q, &rows, + `SELECT label_id, label_name, exclude FROM mdm_configuration_profile_labels WHERE apple_profile_uuid = ? ORDER BY label_name`, + profileUUID) + }) + return rows + } + + rows := loadRows() + require.Len(t, rows, 1) + require.NotNil(t, rows[0].LabelID) + require.Equal(t, labelX.ID, *rows[0].LabelID) + require.Equal(t, labelX.Name, rows[0].LabelName) + require.True(t, rows[0].Exclude) + + // Simulate labelX being deleted. + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, + `UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE apple_profile_uuid = ? AND label_name = ?`, + profileUUID, labelX.Name) + return err + }) + rows = loadRows() + require.Len(t, rows, 1) + require.Nil(t, rows[0].LabelID, "row should be in broken (NULL label_id) state") + + prof = configProfileForTest(t, "Repro42637", "com.fleetdm.repro.42637", "u1", labelY) + require.NoError(t, ds.BatchSetMDMAppleProfiles(ctx, nil, []*fleet.MDMAppleConfigProfile{prof})) + + rows = loadRows() + require.Len(t, rows, 1, "stale broken row was not cleared") + require.NotNil(t, rows[0].LabelID) + require.Equal(t, labelY.ID, *rows[0].LabelID) + require.Equal(t, labelY.Name, rows[0].LabelName) + require.False(t, rows[0].Exclude) +} + func configProfileBytesForTest(name, identifier, uuid string) []byte { return []byte(fmt.Sprintf(` diff --git a/server/datastore/mysql/mdm.go b/server/datastore/mysql/mdm.go index f39341c041..499658e78c 100644 --- a/server/datastore/mysql/mdm.go +++ b/server/datastore/mysql/mdm.go @@ -1710,12 +1710,15 @@ func batchSetProfileLabelAssociationsDB( return false, fmt.Errorf("unsupported platform %s", platform) } - // delete any profile+label tuple that is NOT in the list of provided tuples + // Delete any profile+label tuple that is NOT in the list of provided tuples // but are associated with the provided profiles (so we don't delete - // unrelated profile+label tuples) + // unrelated profile+label tuples). + // Also clear "broken" rows where label_id IS NULL. those wouldn't be matched by + // `(profile_uuid, label_id) NOT IN (...)` because three-valued logic makes the comparison NULL, + // leaving the row in place and blocking host removal in generateEntitiesToRemoveQuery. deleteStmt := ` DELETE FROM mdm_configuration_profile_labels - WHERE (%s_profile_uuid, label_id) NOT IN (%s) AND + WHERE ((%s_profile_uuid, label_id) NOT IN (%s) OR label_id IS NULL) AND %s_profile_uuid IN (?) ` diff --git a/server/datastore/mysql/mdm_test.go b/server/datastore/mysql/mdm_test.go index 7e0cdcad01..3d2ca3edb3 100644 --- a/server/datastore/mysql/mdm_test.go +++ b/server/datastore/mysql/mdm_test.go @@ -7686,6 +7686,81 @@ func testBatchSetProfileLabelAssociations(t *testing.T, ds *Datastore) { require.NoError(t, err) expectLabels(t, uuid, platform, nil) }) + + t.Run("broken label association is cleared on upsert "+platform, func(t *testing.T) { + // Regression test for https://github.com/fleetdm/fleet/issues/42637. + startingLabel := &fleet.Label{ + Name: "broken-label-" + platform, + Query: "select 1 from osquery_info;", + } + startingLabel, err := ds.NewLabel(ctx, startingLabel) + require.NoError(t, err) + + profileLabels := []fleet.ConfigurationProfileLabel{ + {ProfileUUID: uuid, LabelName: startingLabel.Name, LabelID: startingLabel.ID, Exclude: true}, + } + err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { + _, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, nil, platform) + return err + }) + require.NoError(t, err) + expectLabels(t, uuid, platform, profileLabels) + + // Simulate the label being deleted: NULL the label_id directly. This is + // what FK ON DELETE SET NULL does when the underlying label row is gone. + p := platform + if p == "darwin" { + p = "apple" + } + ExecAdhocSQL(t, ds, func(tx sqlx.ExtContext) error { + _, err := tx.ExecContext(ctx, + fmt.Sprintf(`UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE %s_profile_uuid = ? AND label_name = ?`, p), + uuid, startingLabel.Name) + return err + }) + + // Sanity: broken row exists. + var brokenCount int + ExecAdhocSQL(t, ds, func(tx sqlx.ExtContext) error { + return sqlx.GetContext(ctx, tx, &brokenCount, + fmt.Sprintf(`SELECT COUNT(*) FROM mdm_configuration_profile_labels WHERE %s_profile_uuid = ? AND label_id IS NULL`, p), + uuid) + }) + require.Equal(t, 1, brokenCount, "expected broken row to exist before re-upsert") + + // Re-apply with a different label, mimicking gitops switching from + // labels_exclude_any: [startingLabel] to labels_include_any: [switchTarget]. + switchTarget := &fleet.Label{ + Name: "switch-target-" + platform, + Query: "select 1 from osquery_info;", + } + switchTarget, err = ds.NewLabel(ctx, switchTarget) + require.NoError(t, err) + + profileLabels = []fleet.ConfigurationProfileLabel{ + {ProfileUUID: uuid, LabelName: switchTarget.Name, LabelID: switchTarget.ID, Exclude: false}, + } + err = ds.withTx(ctx, func(tx sqlx.ExtContext) error { + _, err := batchSetProfileLabelAssociationsDB(ctx, tx, profileLabels, nil, platform) + return err + }) + require.NoError(t, err) + + // The broken row must be gone + ExecAdhocSQL(t, ds, func(tx sqlx.ExtContext) error { + return sqlx.GetContext(ctx, tx, &brokenCount, + fmt.Sprintf(`SELECT COUNT(*) FROM mdm_configuration_profile_labels WHERE %s_profile_uuid = ? AND label_id IS NULL`, p), + uuid) + }) + require.Equal(t, 0, brokenCount, "broken (NULL label_id) row should have been cleared") + + // Only switchTarget should remain. + expectLabels(t, uuid, platform, profileLabels) + + // Other profiles must remain untouched. + expectLabels(t, otherWinProfile.ProfileUUID, "windows", wantOtherWin) + expectLabels(t, otherMacProfile.ProfileUUID, "darwin", wantOtherMac) + }) } t.Run("unsupported platform", func(t *testing.T) {