Reconcile MDM profiles by updating installed profiles in place by gillespi314 · Pull Request #10998 · fleetdm/fleet

gillespi314 · 2023-04-05T15:30:52Z

Revise ReconcileProfiles cron job so that whenever a profile to be installed shares the same profile identifier as an already installed profile, the profile is updated in place (i.e. do not send a RemoveProfile command first).

Checklist for submitter

If some of the following don't apply, delete the relevant line.

Changes file added for user-visible changes in changes/ or orbit/changes/.
See Changes files for more information.
Documented any API changes (docs/Using-Fleet/REST-API.md or docs/Contributing/API-for-contributors.md)
Documented any permissions changes
Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
Added support on fleet's osquery simulator cmd/osquery-perf for new osquery data ingestion features.
Added/updated tests
Manual QA for all new/changed functionality
- For Orbit and Fleet Desktop changes:
  - Manual QA must be performed in the three main OSs, macOS, Windows and Linux.
  - Auto-update manual QA, from released version of component to new version (see tools/tuf/test).

mna

I think this is an awesome approach that could very well work (while being simpler than what I had in mind)! It assumes the queue is reliable and runs commands in sequence for a given host - if we can rely on that, great. AIUI, it uses the fact that profiles are unique per team and profile identifier to detect when a Remove+Install is needed vs a Replace, without having to do that logic when saving the profiles for the team/no team (i.e. we still do a delete+insert at that place, meaning that an update to the contents of the profile results in a new internal profile ID at the DB level).

The other points I mention in the review are, I think, all solvable. I think the way to identify if the operation should be a replace needs to be done per "host UUID+profile identifier" tuple, and not just profile identifier.

mna · 2023-04-05T17:48:36Z

 	//
 	// We'll do another pass at the end to revert any changes for failed
 	// delivieries.
+	if err := ds.BulkDeleteMDMAppleHostProfiles(ctx, replacedHostProfiles); err != nil {


I think this should be done only once the Install of the replaced profile is successfully done, otherwise we remove a profile from our table for a host, but it may still have the old one "to-be-replaced".

My thinking here was that if the install fails, there would be an entry for the install failure, which would tie back to the profile identifier of the to-be-replaced profile. I think that is consistent with what the UI wants to display, but do you think we need more than that entry for our own table hygiene?

Ah yeah, you're right, the profile to be removed would be deleted from the table, but the one to be inserted as replacement would also be inserted and with a status NULL so that enqueuing the Install command is retried on the next run. It would be reported in stats/UI/etc. as "pending install" which is exactly what we want.

mna · 2023-04-05T17:55:34Z

 	installTargets, removeTargets := make(map[uint]*cmdTarget), make(map[uint]*cmdTarget)
 	for _, p := range toInstall {
 		toGetContents[p.ProfileID] = true
+		toInstallIdentifiers[p.ProfileIdentifier] = true


This assumes that when a ProfileIdentifier is seen in toInstall, then it is necessarily being replaced for all hosts if it is also seen in toRemove. I'm not sure this is right, e.g.

Add profile with Identifier Custom1 to team 1

Add profile with Identifier Custom1 to team 2

ReconcileProfiles deploys the profile to hosts in team 1 and 2

Update profile Custom1 for team 1 (must be replaced for hosts in team 1)

Remove profile Custom1 for team 2 (must be deleted for hosts in team 2)

ReconcileProfiles would see that Custom1 needs to be installed (because it needs to, but just for hosts in team 1)

ReconcileProfiles sees that all hosts in teams 1 and 2 have Custom1 to be removed

Thus it does not send a Remove for all hosts in teams 1 and 2, expecting them to have the profile replaced (which is true only for hosts in team 1)

Result would be (I think) that hosts in team 2 still have the profile installed, but it is deleted from our host profiles table

Thanks! I've modified the map to use the combined key (HostUUID,ProfileIdentifier) and I'll add some tests to cover these scenarios.

mna · 2023-04-05T17:58:29Z

+WHERE host_uuid IN (?) 
+	AND ((operation_type = ? 
+		AND profile_identifier IN (?)) 
+	OR profile_id IN (?))`


I don't think the query is quite right - it will delete rows where the host uuid is in the list of uuids whenever it is part of the profile identifiers or ids, but maybe it shouldn't be deleted for some particular combinations.

E.g. if I call this with [{hostA, profID1}, {hostB, profID2}], it would delete the row if hostA happens to have profile profID2.

Thanks again! I've modified query and I'll add some tests that cover this too.

codecov · 2023-04-05T18:27:17Z

Codecov Report

Patch coverage: 40.00% and project coverage change: -0.02 ⚠️

Comparison is base (337d61c) 60.41% compared to head (005d501) 60.40%.

❗ Current head 005d501 differs from pull request most recent head c0d5b29. Consider uploading reports for the commit c0d5b29 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10998      +/-   ##
==========================================
- Coverage   60.41%   60.40%   -0.02%     
==========================================
  Files         522      524       +2     
  Lines       54200    54392     +192     
==========================================
+ Hits        32747    32855     +108     
- Misses      18468    18542      +74     
- Partials     2985     2995      +10

Impacted Files	Coverage Δ
server/fleet/apple_mdm.go	`40.38% <ø> (+1.49%)`	⬆️
server/fleet/datastore.go	`0.00% <ø> (ø)`
server/datastore/mysql/apple_mdm.go	`70.62% <3.57%> (-0.26%)`	⬇️
server/service/apple_mdm.go	`58.50% <100.00%> (+1.71%)`	⬆️

... and 13 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mna

### Related tickets #10775 #10678 #11024 #11026 ### What's happening - Implemented the hashing mechanism defined by @mna in #10678, however this mechanism is mainly relevant for batch profile updates via the CLI, we can't leverage it when a host switches teams. - Modified `BulkSetPendingMDMAppleHostProfiles` so when two profiles with the same identifier are sheduled both for removal and update, the function will now mark only the `install` as `pending` so it's picked by the cron, and will `DELETE` the `remove` entry from the database so it's not picked by the cron and never sent to the user. - `GetHostMDMProfiles` and consequently the profiles returned in `GET /api/_version_/fleet/hosts` return `host_mdm_apple_profiles.state = NULL` as "Enforcing (pending", the distinction between `status = 'pending'` and `status IS NULL` is only useful for the cron, for users both mean the same thing, and all our profile aggregations already behave this way. - Using the solution implemented by @gillespi314 in #10998 we're now deleting the host row from `host_disk_encryption_keys` if a host is moved from a team that enforces disk encryption to a team that doesn't. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Added/updated tests - [x] Manual QA for all new/changed functionality

gillespi314 · 2023-04-10T01:21:54Z

Closed in favor of #11084

@mna

### Related tickets fleetdm#10775 fleetdm#10678 fleetdm#11024 fleetdm#11026 ### What's happening - Implemented the hashing mechanism defined by @mna in fleetdm#10678, however this mechanism is mainly relevant for batch profile updates via the CLI, we can't leverage it when a host switches teams. - Modified `BulkSetPendingMDMAppleHostProfiles` so when two profiles with the same identifier are sheduled both for removal and update, the function will now mark only the `install` as `pending` so it's picked by the cron, and will `DELETE` the `remove` entry from the database so it's not picked by the cron and never sent to the user. - `GetHostMDMProfiles` and consequently the profiles returned in `GET /api/_version_/fleet/hosts` return `host_mdm_apple_profiles.state = NULL` as "Enforcing (pending", the distinction between `status = 'pending'` and `status IS NULL` is only useful for the cron, for users both mean the same thing, and all our profile aggregations already behave this way. - Using the solution implemented by @gillespi314 in fleetdm#10998 we're now deleting the host row from `host_disk_encryption_keys` if a host is moved from a team that enforces disk encryption to a team that doesn't. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Added/updated tests - [x] Manual QA for all new/changed functionality

gillespi314 added 2 commits April 5, 2023 10:23

Skip removal for updated mdm profiles

76941f0

Update integration test

005d501

gillespi314 temporarily deployed to Docker Hub April 5, 2023 17:58 — with GitHub Actions Inactive

mna reviewed Apr 5, 2023

View reviewed changes

Address comments

e253a69

gillespi314 temporarily deployed to Docker Hub April 5, 2023 22:48 — with GitHub Actions Inactive

gillespi314 added 3 commits April 7, 2023 14:55

Commit WIP

d60d953

Use trxn to reconcile profiles on team change

4236512

WIP

5d299f7

gillespi314 temporarily deployed to Docker Hub April 7, 2023 21:56 — with GitHub Actions Inactive

WIP

d55a012

gillespi314 had a problem deploying to Docker Hub April 7, 2023 23:05 — with GitHub Actions Failure

Fix compile errors

5ee3a2b

gillespi314 temporarily deployed to Docker Hub April 7, 2023 23:10 — with GitHub Actions Inactive

Fix tests

afcd58b

gillespi314 temporarily deployed to Docker Hub April 8, 2023 01:19 — with GitHub Actions Inactive

Add todo

c0d5b29

gillespi314 temporarily deployed to Docker Hub April 8, 2023 02:26 — with GitHub Actions Inactive

gillespi314 mentioned this pull request Apr 8, 2023

various profile fixes #11084

Merged

2 tasks

gillespi314 closed this Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile MDM profiles by updating installed profiles in place#10998

Reconcile MDM profiles by updating installed profiles in place#10998
gillespi314 wants to merge 10 commits intomainfrom
10775-update-mdm-profile

gillespi314 commented Apr 5, 2023

Uh oh!

mna left a comment

Uh oh!

mna Apr 5, 2023

Uh oh!

gillespi314 Apr 5, 2023

Uh oh!

mna Apr 5, 2023

Uh oh!

mna Apr 5, 2023

Uh oh!

gillespi314 Apr 6, 2023

Uh oh!

mna Apr 5, 2023

Uh oh!

gillespi314 Apr 6, 2023

Uh oh!

codecov Bot commented Apr 5, 2023 •

edited

Loading

Uh oh!

gillespi314 commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

gillespi314 commented Apr 5, 2023

Checklist for submitter

Uh oh!

mna left a comment

Choose a reason for hiding this comment

Uh oh!

mna Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

gillespi314 Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

mna Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

mna Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

gillespi314 Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

mna Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

gillespi314 Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gillespi314 commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Apr 5, 2023 •

edited

Loading