From 6147a12eceb025b98bb4a5a54385110bb41ea8ed Mon Sep 17 00:00:00 2001 From: Magnus Jensen Date: Tue, 22 Jul 2025 11:22:04 +0200 Subject: [PATCH] Fix stale pending remove apple declarations if host was offline for add and remove declaration (#30981) Fixes: #29824 This PR fixes a situtation where Apple Declarations could be lingering around for hosts, if they were offline when the decl. was added and removed, and no further declaration config is pushed to force a status update. It tackles it by deleting the pending and failed installs from the table, before setting the remaining (verified and verifying) to be remove operation, as those have hit the host. I couldn't come up with a way that would auto-fix the hosts we see in dogfood, as those have the same declaration identifier and token for both an install row and pending remove row. Those needs to manually be adjusted and then it should be good. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated automated tests - [x] Manual QA for all new/changed functionality --- ...delete-installs-that-has-not-reached-hosts | 1 + server/datastore/mysql/apple_mdm.go | 6 ++- server/datastore/mysql/apple_mdm_test.go | 40 +++++++++++++++++++ server/service/integration_mdm_ddm_test.go | 4 +- 4 files changed, 47 insertions(+), 4 deletions(-) create mode 100644 changes/29824-delete-installs-that-has-not-reached-hosts diff --git a/changes/29824-delete-installs-that-has-not-reached-hosts b/changes/29824-delete-installs-that-has-not-reached-hosts new file mode 100644 index 0000000000..a88ae5c93c --- /dev/null +++ b/changes/29824-delete-installs-that-has-not-reached-hosts @@ -0,0 +1 @@ +* Fix stale pending remove apple declarations, if the host was offline while adding and removing the same declaration. \ No newline at end of file diff --git a/server/datastore/mysql/apple_mdm.go b/server/datastore/mysql/apple_mdm.go index 0ca826d790..dbeffbce6a 100644 --- a/server/datastore/mysql/apple_mdm.go +++ b/server/datastore/mysql/apple_mdm.go @@ -643,10 +643,10 @@ func cancelAppleHostInstallsForDeletedMDMDeclarations(ctx context.Context, tx sq host_mdm_apple_declarations WHERE declaration_uuid IN (?) AND - status IS NULL AND + ( status IS NULL OR status IN (?) ) AND operation_type = ?` - stmt, args, err := sqlx.In(delStmt, declUUIDs, fleet.MDMOperationTypeInstall) + stmt, args, err := sqlx.In(delStmt, declUUIDs, []fleet.MDMDeliveryStatus{fleet.MDMDeliveryPending, fleet.MDMDeliveryFailed}, fleet.MDMOperationTypeInstall) if err != nil { return ctxerr.Wrap(ctx, err, "building in statement") } @@ -5888,6 +5888,8 @@ ON DUPLICATE KEY UPDATE } } + // TODO: Do we want to take action if an update coming from the host is not found in the current "expected" declarations? + err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { if len(args) != 0 { stmt := fmt.Sprintf(updateHostDeclarationsStmt, strings.TrimSuffix(insertVals.String(), ",")) diff --git a/server/datastore/mysql/apple_mdm_test.go b/server/datastore/mysql/apple_mdm_test.go index ce5d97fccb..3206e3f927 100644 --- a/server/datastore/mysql/apple_mdm_test.go +++ b/server/datastore/mysql/apple_mdm_test.go @@ -101,6 +101,7 @@ func TestMDMApple(t *testing.T) { {"SetMDMAppleProfilesWithVariables", testSetMDMAppleProfilesWithVariables}, {"GetNanoMDMEnrollmentTimes", testGetNanoMDMEnrollmentTimes}, {"GetNanoMDMUserEnrollment", testGetNanoMDMUserEnrollment}, + {"TestDeleteMDMAppleDeclarationWithPendingInstalls", testDeleteMDMAppleDeclarationWithPendingInstalls}, } for _, c := range cases { @@ -5526,6 +5527,45 @@ func testSetOrUpdateMDMAppleDDMDeclaration(t *testing.T, ds *Datastore) { require.Equal(t, d1tm1B.DeclarationUUID, d1tm1.DeclarationUUID) } +func testDeleteMDMAppleDeclarationWithPendingInstalls(t *testing.T, ds *Datastore) { + ctx := t.Context() + + decl, err := ds.NewMDMAppleDeclaration(ctx, &fleet.MDMAppleDeclaration{ + Identifier: "decl-1", + Name: "decl-1", + }) + require.NoError(t, err) + + host, err := ds.NewHost(ctx, &fleet.Host{ + Hostname: "test-host1-name", + OsqueryHostID: ptr.String("1337"), + NodeKey: ptr.String("1337"), + UUID: "test-uuid-1", + TeamID: nil, + Platform: "darwin", + }) + require.NoError(t, err) + nanoEnroll(t, ds, host, true) + + _, err = ds.BulkSetPendingMDMHostProfiles(ctx, nil, nil, []string{decl.DeclarationUUID}, nil) + require.NoError(t, err) + + // verify the correct state of the declaration for the host + profs, err := ds.GetHostMDMAppleProfiles(ctx, host.UUID) + require.NoError(t, err) + require.Len(t, profs, 1) + require.Equal(t, decl.DeclarationUUID, profs[0].ProfileUUID) + require.Equal(t, fleet.MDMDeliveryPending, *profs[0].Status) + require.Equal(t, fleet.MDMOperationTypeInstall, profs[0].OperationType) + + err = ds.DeleteMDMAppleDeclaration(ctx, decl.DeclarationUUID) + require.NoError(t, err) + + profs, err = ds.GetHostMDMAppleProfiles(ctx, host.UUID) + require.NoError(t, err) + require.Len(t, profs, 0) +} + func TestMDMAppleProfileVerification(t *testing.T) { ds := CreateMySQLDS(t) ctx := t.Context() diff --git a/server/service/integration_mdm_ddm_test.go b/server/service/integration_mdm_ddm_test.go index 44626ede13..a35f87624c 100644 --- a/server/service/integration_mdm_ddm_test.go +++ b/server/service/integration_mdm_ddm_test.go @@ -470,7 +470,8 @@ func (s *integrationMDMTestSuite) TestAppleDDMSecretVariables() { _, mdmDevice := createHostThenEnrollMDM(s.ds, s.server.URL, t) checkDeclarationItemsResp := func(t *testing.T, r fleet.MDMAppleDDMDeclarationItemsResponse, expectedDeclTok string, - expectedDeclsByToken map[string]fleet.MDMAppleDeclaration) { + expectedDeclsByToken map[string]fleet.MDMAppleDeclaration, + ) { require.Equal(t, expectedDeclTok, r.DeclarationsToken) require.NotEmpty(t, r.Declarations.Activations) require.Empty(t, r.Declarations.Assets) @@ -1065,7 +1066,6 @@ func (s *integrationMDMTestSuite) TestAppleDDMStatusReport() { require.NoError(t, err) assertHostDeclarations(mdmHost.UUID, []*fleet.MDMAppleHostDeclaration{ {Identifier: "I1", Status: &fleet.MDMDeliveryVerified, OperationType: fleet.MDMOperationTypeInstall}, - {Identifier: "I2", Status: &fleet.MDMDeliveryPending, OperationType: fleet.MDMOperationTypeRemove}, }) // host sends a report, declaration I2 is removed from the hosts_* table