Fix gitops wiping all VPP and ABM token assignments when config references a fleet created in the same run - #52565
Fix gitops wiping all VPP and ABM token assignments when config references a fleet created in the same run#52565raju249 wants to merge 2 commits into
Conversation
…ences a fleet created in the same run When volume_purchasing_program or apple_business referenced a not-yet-created fleet, the whole section was held back from the first config apply, which the server received as an empty list and answered by clearing every token's fleet assignments. A run failing before the end-of-run re-apply left them permanently removed. Apply a filtered interim config instead (missing fleets removed from VPP entries, missing *_fleet references blanked on ABM entries) so existing assignments always survive. Also count byod_fleet when detecting missing fleets; it was previously unchecked, so a new BYOD fleet failed the run. Fixes #51687
| for _, item := range settingMap { | ||
| if cfg, ok := item.(map[string]any); ok { | ||
| for _, teamConfigKey := range []string{"macos_fleet", "ios_fleet", "ipados_fleet"} { | ||
| for _, teamConfigKey := range []string{"macos_fleet", "ios_fleet", "ipados_fleet", "byod_fleet"} { |
There was a problem hiding this comment.
byod was discovered during testing and it was missing, so added it to handle it here.
Not asked in the main issue, but good idea to not wait for another bug around it.
Reviewer can let me know if we need to remove it for any reasons.
There was a problem hiding this comment.
🟡 Changes recommended
The ABM missing-fleet detection should normalize and exclude reserved fleet names to avoid incorrect “missing” classification, and the new ABM regression test assertions should be strengthened to cover iOS/iPadOS/BYOD defaults (not only macOS).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR fixes a GitOps edge case where interim application of org_settings.mdm.volume_purchasing_program and/or org_settings.mdm.apple_business could unintentionally clear existing VPP token fleet assignments and ABM token default fleets when the config references fleets created later in the same GitOps run (issue #51687). The approach keeps an interim “safe” config (filtering/blanking only the not-yet-created fleet references) and then reapplies the full original config after fleets are created, and adds regression tests.
Changes:
- Apply an interim MDM config that preserves existing VPP/ABM assignments by filtering/blanking only missing-fleet references, then re-apply the original config at end-of-run.
- Include
byod_fleetwhen detecting missing fleet references for ABM. - Add integration regression tests to ensure assignments are not dropped mid-run.
File summaries
| File | Description |
|---|---|
| cmd/fleetctl/fleetctl/gitops.go | Adds interim config filtering/blanking logic for VPP/ABM when referenced fleets are missing; includes BYOD fleet detection. |
| cmd/fleetctl/integrationtest/gitops/software_test.go | Adds regression tests ensuring interim GitOps applies don’t drop existing VPP/ABM assignments. |
| changes/51687-gitops-vpp-abm-assignment-wipe | Release note/change entry (diff not available due to content exclusion policy). |
Review details
Files excluded by content exclusion policy (1)
- changes/51687-gitops-vpp-abm-assignment-wipe
Suppressed comments (5)
cmd/fleetctl/fleetctl/gitops.go:1207
- teamNames is populated with raw tm.Name but later comparisons normalize names with norm.NFC.String(...). This can incorrectly flag an existing fleet as missing for names that aren’t already NFC-normalized. Normalize tm.Name when building the map to keep the Unicode-support intent consistent.
teamNames := map[string]struct{}{}
for _, tm := range teams {
teamNames[tm.Name] = struct{}{}
}
cmd/fleetctl/integrationtest/gitops/software_test.go:1133
- saveCalls currently records only the macOS default, so later assertions can’t verify that other platform defaults survived each SaveABMToken call. Capture iOS/iPadOS/BYOD defaults too.
type abmSave struct {
org string
macos *uint
}
cmd/fleetctl/integrationtest/gitops/software_test.go:1137
- SaveABMTokenFunc should snapshot all default-team IDs so the test can assert none were cleared/changed during interim apply.
ds.SaveABMTokenFunc = func(ctx context.Context, tok *fleet.ABMToken) error {
saveCalls = append(saveCalls, abmSave{org: tok.OrganizationName, macos: tok.MacOSDefaultTeamID})
return nil
cmd/fleetctl/integrationtest/gitops/software_test.go:1201
- The assertions only verify macOS defaults survived; iOS/iPadOS/BYOD defaults can still be wiped by an interim apply. Extend assertions to check all platform defaults remain set to the existing team for every SaveABMToken call.
for i, call := range saveCalls {
if assert.NotNil(t, call.macos, "SaveABMToken call %d (org %s) dropped the existing macOS default fleet", i, call.org) {
assert.Equal(t, existingTeam.ID, *call.macos, "SaveABMToken call %d (org %s) changed the existing macOS default fleet", i, call.org)
}
}
cmd/fleetctl/fleetctl/gitops.go:1232
- Reserved fleet names (e.g. "No team"/"Unassigned") aren’t returned by ListTeams, so they should not be considered “missing” here. Otherwise a config that intentionally sets a default to Unassigned can be misclassified as missing and take the deferred-assignment path.
for _, teamConfigKey := range []string{"macos_fleet", "ios_fleet", "ipados_fleet", "byod_fleet"} {
if team, ok := cfg[teamConfigKey].(string); ok && team != "" {
// normalize for Unicode support
team = norm.NFC.String(team)
abmTeams = append(abmTeams, team)
if _, ok := teamNames[team]; !ok {
missingTeams = append(missingTeams, team)
}
- Files reviewed: 2/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| abmTeams = append(abmTeams, appleBMDefaultTeam) | ||
| usesLegacyConfig = true | ||
| if _, ok = teamNames[appleBMDefaultTeam]; !ok { | ||
| missingTeam = true | ||
| missingTeams = append(missingTeams, appleBMDefaultTeam) | ||
| } |
| return []*fleet.ABMToken{{ | ||
| ID: 1, | ||
| OrganizationName: "Fleet ABM", | ||
| MacOSDefaultTeamID: new(existingTeam.ID), | ||
| }}, nil | ||
| } |
Related issue: Resolves #51687
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Details
volume_purchasing_programorapple_businessconfig referenced a fleet created later in the same run, the section was held back from the first config apply. The server received it as an empty list and cleared every token's fleet assignments — and a run failing before the end-of-run re-apply left them permanently removed.*_fleetreferences are blanked on ABM entries, so assignments to existing fleets always survive. The original config is still applied in full at the end of the run.byod_fleetis now included when detecting missing fleets (previously unchecked, so a config referencing a new BYOD fleet failed the run).Testing
Regression tests assert no assignment write ever drops an existing fleet (verified failing before the fix). Manually verified against a live server: a run interrupted mid-apply now leaves VPP/ABM assignments intact, and a completed run converges to the declared state.