diff --git a/server/datastore/mysql/common_mysql/batch.go b/server/datastore/mysql/common_mysql/batch.go new file mode 100644 index 000000000000..ea24476005f9 --- /dev/null +++ b/server/datastore/mysql/common_mysql/batch.go @@ -0,0 +1,26 @@ +package common_mysql + +// BatchProcessSimple is a simple utility function to batch process a slice of payloads. +// Provide a slice of payloads, a batch size, and a function to execute on each batch. +func BatchProcessSimple[T any]( + payloads []T, + batchSize int, + executeBatch func(payloadsInThisBatch []T) error, +) error { + if len(payloads) == 0 || batchSize <= 0 || executeBatch == nil { + return nil + } + + for i := 0; i < len(payloads); i += batchSize { + start := i + end := i + batchSize + if end > len(payloads) { + end = len(payloads) + } + if err := executeBatch(payloads[start:end]); err != nil { + return err + } + } + + return nil +} diff --git a/server/datastore/mysql/common_mysql/batch_test.go b/server/datastore/mysql/common_mysql/batch_test.go new file mode 100644 index 000000000000..5f561e44de8f --- /dev/null +++ b/server/datastore/mysql/common_mysql/batch_test.go @@ -0,0 +1,52 @@ +package common_mysql + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestBatchProcessSimple(t *testing.T) { + payloads := []int{1, 2, 3, 4, 5} + executeBatch := func(payloadsInThisBatch []int) error { + t.Fatal("executeBatch should not be called") + return nil + } + + // No payloads + err := BatchProcessSimple(nil, 10, executeBatch) + assert.NoError(t, err) + + // No batch size + err = BatchProcessSimple(payloads, 0, executeBatch) + assert.NoError(t, err) + + // No executeBatch + err = BatchProcessSimple(payloads, 10, nil) + assert.NoError(t, err) + + // Large batch size -- all payloads executed in one batch + executeBatch = func(payloadsInThisBatch []int) error { + assert.Equal(t, payloads, payloadsInThisBatch) + return nil + } + err = BatchProcessSimple(payloads, 10, executeBatch) + assert.NoError(t, err) + + // Small batch size + numCalls := 0 + executeBatch = func(payloadsInThisBatch []int) error { + numCalls++ + switch numCalls { + case 1: + assert.Equal(t, []int{1, 2, 3}, payloadsInThisBatch) + case 2: + assert.Equal(t, []int{4, 5}, payloadsInThisBatch) + default: + t.Errorf("Unexpected number of calls to executeBatch: %d", numCalls) + } + return nil + } + err = BatchProcessSimple(payloads, 3, executeBatch) + assert.NoError(t, err) +} diff --git a/server/fleet/apple_mdm.go b/server/fleet/apple_mdm.go index 3a23b4cbe18c..827b373b00c6 100644 --- a/server/fleet/apple_mdm.go +++ b/server/fleet/apple_mdm.go @@ -330,10 +330,22 @@ type MDMAppleProfilePayload struct { // DidNotInstallOnHost indicates whether this profile was not installed on the host (and // therefore is not, as far as Fleet knows, currently on the host). +// The profile in Pending status could be on the host, but Fleet has not received an Acknowledged status yet. func (p *MDMAppleProfilePayload) DidNotInstallOnHost() bool { return p.Status != nil && (*p.Status == MDMDeliveryFailed || *p.Status == MDMDeliveryPending) && p.OperationType == MDMOperationTypeInstall } +// FailedInstallOnHost indicates whether this profile failed to install on the host. +func (p *MDMAppleProfilePayload) FailedInstallOnHost() bool { + return p.Status != nil && *p.Status == MDMDeliveryFailed && p.OperationType == MDMOperationTypeInstall +} + +// PendingInstallOnHost indicates whether this profile is pending to install on the host. +// The profile in Pending status could be on the host, but Fleet has not received an Acknowledged status yet. +func (p *MDMAppleProfilePayload) PendingInstallOnHost() bool { + return p.Status != nil && *p.Status == MDMDeliveryPending && p.OperationType == MDMOperationTypeInstall +} + type MDMAppleBulkUpsertHostProfilePayload struct { ProfileUUID string ProfileIdentifier string diff --git a/server/mdm/apple/commander.go b/server/mdm/apple/commander.go index 4f1a279d7e49..15914ac393b7 100644 --- a/server/mdm/apple/commander.go +++ b/server/mdm/apple/commander.go @@ -425,6 +425,11 @@ func (svc *MDMAppleCommander) SendNotifications(ctx context.Context, hostUUIDs [ return nil } +// BulkDeleteHostUserCommandsWithoutResults calls the storage method with the same name. +func (svc *MDMAppleCommander) BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToIDs map[string][]string) error { + return svc.storage.BulkDeleteHostUserCommandsWithoutResults(ctx, commandToIDs) +} + // APNSDeliveryError records an error and the associated host UUIDs in which it // occurred. type APNSDeliveryError struct { diff --git a/server/mdm/nanomdm/storage/allmulti/allmulti.go b/server/mdm/nanomdm/storage/allmulti/allmulti.go index 4ca7d6300d1c..9bc017202761 100644 --- a/server/mdm/nanomdm/storage/allmulti/allmulti.go +++ b/server/mdm/nanomdm/storage/allmulti/allmulti.go @@ -104,3 +104,10 @@ func (ms *MultiAllStorage) ExpandEmbeddedSecrets(ctx context.Context, document s }) return doc.(string), err } + +func (ms *MultiAllStorage) BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToIDs map[string][]string) error { + _, err := ms.execStores(ctx, func(s storage.AllStorage) (interface{}, error) { + return nil, s.BulkDeleteHostUserCommandsWithoutResults(ctx, commandToIDs) + }) + return err +} diff --git a/server/mdm/nanomdm/storage/file/file.go b/server/mdm/nanomdm/storage/file/file.go index c816a34e730f..a0846b248fb6 100644 --- a/server/mdm/nanomdm/storage/file/file.go +++ b/server/mdm/nanomdm/storage/file/file.go @@ -245,3 +245,8 @@ func (s *FileStorage) ExpandEmbeddedSecrets(_ context.Context, document string) // NOT IMPLEMENTED return document, nil } + +func (m *FileStorage) BulkDeleteHostUserCommandsWithoutResults(_ context.Context, _ map[string][]string) error { + // NOT IMPLEMENTED + return nil +} diff --git a/server/mdm/nanomdm/storage/mysql/queue.go b/server/mdm/nanomdm/storage/mysql/queue.go index 5e84b30bb940..ead83efb6c79 100644 --- a/server/mdm/nanomdm/storage/mysql/queue.go +++ b/server/mdm/nanomdm/storage/mysql/queue.go @@ -7,6 +7,7 @@ import ( "fmt" "strings" + "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/datastore/mysql/common_mysql" "github.com/fleetdm/fleet/v4/server/mdm/nanomdm/mdm" "github.com/google/uuid" @@ -260,3 +261,54 @@ WHERE ) return err } + +// BulkDeleteHostUserCommandsWithoutResults deletes all commands without results for the given host/user IDs. +// This is used to clean up the queue when a profile is deleted from Fleet. +func (m *MySQLStorage) BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToIDs map[string][]string) error { + if len(commandToIDs) == 0 { + return nil + } + return common_mysql.WithRetryTxx(ctx, sqlx.NewDb(m.db, ""), func(tx sqlx.ExtContext) error { + return m.bulkDeleteHostUserCommandsWithoutResults(ctx, tx, commandToIDs) + }, loggerWrapper{m.logger}) +} + +func (m *MySQLStorage) bulkDeleteHostUserCommandsWithoutResults(ctx context.Context, tx sqlx.ExtContext, + commandToIDs map[string][]string) error { + stmt := ` +DELETE + eq +FROM + nano_enrollment_queue AS eq + LEFT JOIN nano_command_results AS cr + ON cr.command_uuid = eq.command_uuid AND cr.id = eq.id +WHERE + cr.command_uuid IS NULL AND eq.command_uuid = ? AND eq.id IN (?);` + + // We process each commandUUID one at a time, in batches of hostUserIDs. + // This is because the number of hostUserIDs can be large, and number of unique commands is normally small. + // If we have a use case where each host has a unique command, we can create a separate method for that use case. + for commandUUID, hostUserIDs := range commandToIDs { + if len(hostUserIDs) == 0 { + continue + } + + batchSize := 10000 + err := common_mysql.BatchProcessSimple(hostUserIDs, batchSize, func(hostUserIDsToProcess []string) error { + expanded, args, err := sqlx.In(stmt, commandUUID, hostUserIDsToProcess) + if err != nil { + return ctxerr.Wrap(ctx, err, "expanding bulk delete nano commands") + } + _, err = tx.ExecContext(ctx, expanded, args...) + if err != nil { + return ctxerr.Wrap(ctx, err, "bulk delete nano commands") + } + return nil + }) + if err != nil { + return err + } + } + + return nil +} diff --git a/server/mdm/nanomdm/storage/storage.go b/server/mdm/nanomdm/storage/storage.go index 89efc1fb4b11..759c50b72827 100644 --- a/server/mdm/nanomdm/storage/storage.go +++ b/server/mdm/nanomdm/storage/storage.go @@ -27,6 +27,8 @@ type CommandAndReportResultsStore interface { StoreCommandReport(r *mdm.Request, report *mdm.CommandResults) error RetrieveNextCommand(r *mdm.Request, skipNotNow bool) (*mdm.CommandWithSubtype, error) ClearQueue(r *mdm.Request) error + // BulkDeleteHostUserCommandsWithoutResults deletes all commands without results for the given host/user IDs. + BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToId map[string][]string) error } type BootstrapTokenStore interface { diff --git a/server/mock/mdm/datastore_mdm_mock.go b/server/mock/mdm/datastore_mdm_mock.go index cbed04c82540..cd094f0f9b0e 100644 --- a/server/mock/mdm/datastore_mdm_mock.go +++ b/server/mock/mdm/datastore_mdm_mock.go @@ -29,6 +29,8 @@ type RetrieveNextCommandFunc func(r *mdm.Request, skipNotNow bool) (*mdm.Command type ClearQueueFunc func(r *mdm.Request) error +type BulkDeleteHostUserCommandsWithoutResultsFunc func(ctx context.Context, commandToId map[string][]string) error + type StoreBootstrapTokenFunc func(r *mdm.Request, msg *mdm.SetBootstrapToken) error type RetrieveBootstrapTokenFunc func(r *mdm.Request, msg *mdm.GetBootstrapToken) (*mdm.BootstrapToken, error) @@ -89,6 +91,9 @@ type MDMAppleStore struct { ClearQueueFunc ClearQueueFunc ClearQueueFuncInvoked bool + BulkDeleteHostUserCommandsWithoutResultsFunc BulkDeleteHostUserCommandsWithoutResultsFunc + BulkDeleteHostUserCommandsWithoutResultsFuncInvoked bool + StoreBootstrapTokenFunc StoreBootstrapTokenFunc StoreBootstrapTokenFuncInvoked bool @@ -198,6 +203,13 @@ func (fs *MDMAppleStore) ClearQueue(r *mdm.Request) error { return fs.ClearQueueFunc(r) } +func (fs *MDMAppleStore) BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToId map[string][]string) error { + fs.mu.Lock() + fs.BulkDeleteHostUserCommandsWithoutResultsFuncInvoked = true + fs.mu.Unlock() + return fs.BulkDeleteHostUserCommandsWithoutResultsFunc(ctx, commandToId) +} + func (fs *MDMAppleStore) StoreBootstrapToken(r *mdm.Request, msg *mdm.SetBootstrapToken) error { fs.mu.Lock() fs.StoreBootstrapTokenFuncInvoked = true diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index 7394a82aeb93..7e159b0c91ec 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -3464,17 +3464,23 @@ func ReconcileAppleProfiles( } for _, p := range toRemove { + // Exclude profiles that are also marked for installation. if _, ok := profileIntersection.GetMatchingProfileInDesiredState(p); ok { hostProfilesToCleanup = append(hostProfilesToCleanup, p) continue } - if p.DidNotInstallOnHost() { - // then we shouldn't send an additional remove command since it wasn't installed on the - // host. + if p.FailedInstallOnHost() { + // then we shouldn't send an additional remove command since it failed to install on the host. hostProfilesToCleanup = append(hostProfilesToCleanup, p) continue } + if p.PendingInstallOnHost() { + // The profile most likely did not install on host. However, it is possible that the profile + // is currently being installed. So, we clean up the profile from the database, but also send + // a remove command to the host. + hostProfilesToCleanup = append(hostProfilesToCleanup, p) + } target := removeTargets[p.ProfileUUID] if target == nil { @@ -3504,6 +3510,16 @@ func ReconcileAppleProfiles( // `InstallProfile` for the same identifier, which can cause race // conditions. It's better to "update" the profile by sending a single // `InstallProfile` command. + // + // Create a map of command UUIDs to host IDs + commandUUIDToHostIDsCleanupMap := make(map[string][]string) + for _, hp := range hostProfilesToCleanup { + commandUUIDToHostIDsCleanupMap[hp.CommandUUID] = append(commandUUIDToHostIDsCleanupMap[hp.CommandUUID], hp.HostUUID) + } + // We need to delete commands from the nano queue so they don't get sent to device. + if err := commander.BulkDeleteHostUserCommandsWithoutResults(ctx, commandUUIDToHostIDsCleanupMap); err != nil { + return ctxerr.Wrap(ctx, err, "deleting nano commands without results") + } if err := ds.BulkDeleteMDMAppleHostsConfigProfiles(ctx, hostProfilesToCleanup); err != nil { return ctxerr.Wrap(ctx, err, "deleting profiles that didn't change") }