VPP/in-house app managed configuration bug fixes and integration test - #45452
Merged
jkatz01 merged 10 commits intoMay 14, 2026
Conversation
Verify the three-state contract on SaveInHouseAppUpdates (nil = leave unchanged, empty = clear, non-empty = set) and that each case applies to both the iOS row and its iPadOS sibling. Also reflow the VPPAppTeam/VPPAppStoreApp Configuration comments so the JSON-vs-XML note sits on its own line.
The singular PATCH /software/titles/{id}/package endpoint targets
one software title. iOS and iPadOS rows for the same .ipa live
under different title_ids, so silently mirroring the configuration
across siblings on update would mutate a title the user didn't edit.
Drop installerIDsForInHouseAppSibling and write only to the
installer ID in the payload. The insert path keeps the dual-row
write because a single .ipa upload genuinely owns both rows.
Tests updated to match: the CRUD three-state semantics now assert
against the targeted row only, and testInHouseAppConfigSiblingRows
keeps the insert-time propagation check but drops the obsolete
update-time propagation assertion.
InsertHostVPPSoftwareInstall runs activateNextUpcomingActivity in the same transaction as the upcoming-activity insert, so a host that's missing the IDP mapping referenced by the stored config surfaces ErrUnresolvableAppConfigVar synchronously on the install POST. The error wasn't translated, so it bubbled to a 500. Wrap it as a BadRequestError with a user-facing message at the service-layer entry that both the admin and self-service install endpoints fan into. Test exercises the path against an iOS host with no IDP mapping and also asserts the failed install leaves no upcoming_activities row behind.
The Contains check is the load-bearing assertion: if the field type regresses from json.RawMessage to []byte, encoding/json base64-encodes the bytes and the literal substring would no longer be on the wire. NotContains was looking for the base64 of a synthesized payload that doesn't match what would actually get encoded (HTML-escaped wrap bytes on iOS, server-renormalized whitespace on Android), so it would miss the regression anyway.
drainVerifyCmds was used in two places: inside installAndCaptureCmd and in the self-service subtest. The self-service call ran against a freshly enrolled host with nothing pending, so it was dead. With that gone the helper only had one caller; inline the loop into installAndCaptureCmd and remove the closure.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45452 +/- ##
===========================================
- Coverage 66.77% 55.22% -11.55%
===========================================
Files 2729 2731 +2
Lines 218648 218611 -37
Branches 10751 10751
===========================================
- Hits 146004 120733 -25271
- Misses 59454 86631 +27177
+ Partials 13190 11247 -1943
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Mirrors the scheduled-update orchestration (refetch → version comparison → auto-update dispatch) for a VPP app with a managed Configuration, asserts the resulting InstallApplication command carries the Configuration bytes and the activity is recorded with from_auto_update=true. Lives next to the other VPP managed-configuration subtests rather than alongside TestVPPAppScheduledUpdates so the Configuration coverage stays grouped.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
WalkthroughThis PR refactors VPP managed app configuration storage and lifecycle. The Configuration field in VPPAppStoreApp changes from raw bytes to json.RawMessage to support JSON-aware semantics. In-house app configuration sibling propagation is removed, so updates apply only to the specified installer row. VPP configuration rows are now cleaned up when teams are deleted. Service-layer error handling explicitly catches unresolvable Fleet variable substitution and returns a BadRequestError. Android tests are updated to use JSON semantic assertions. Two comprehensive integration tests validate wire-format correctness and end-to-end MDM command injection with variable resolution, clearing, platform isolation, and auto-update scenarios. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/server/service/software_installers.go`:
- Around line 1531-1538: The code currently converts
apple_mdm.ErrUnresolvableAppConfigVar from InsertHostVPPSoftwareInstall into a
400 without undoing a prior AssociateAssets call, leaving the host assigned a
VPP license; fix by either validating the managed app configuration before
calling AssociateAssets or, if keeping the current call order, explicitly
disassociate the assets when InsertHostVPPSoftwareInstall returns errors.Is(err,
apple_mdm.ErrUnresolvableAppConfigVar): call the corresponding disassociation
routine for the same asset IDs you passed to AssociateAssets (undo the external
assignment) and only then return the fleet.BadRequestError; reference
InsertHostVPPSoftwareInstall, AssociateAssets, and
apple_mdm.ErrUnresolvableAppConfigVar when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b61ec77-0e71-48de-8c56-5acd0f7700cd
📒 Files selected for processing (8)
ee/server/service/software_installers.goserver/datastore/mysql/in_house_apps.goserver/datastore/mysql/in_house_apps_test.goserver/datastore/mysql/teams.goserver/datastore/mysql/vpp_test.goserver/fleet/vpp.goserver/service/integration_android_software_test.goserver/service/integration_apple_vpp_config_test.go
💤 Files with no reviewable changes (1)
- server/datastore/mysql/in_house_apps_test.go
Comment on lines
1531
to
+1538
| err = svc.ds.InsertHostVPPSoftwareInstall(ctx, host.ID, vppApp.VPPAppID, cmdUUID, eventID, opts) | ||
| if err != nil { | ||
| if errors.Is(err, apple_mdm.ErrUnresolvableAppConfigVar) { | ||
| return "", &fleet.BadRequestError{ | ||
| Message: "Couldn't install. The managed app configuration references Fleet variables that can't be resolved for this host.", | ||
| InternalErr: err, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid consuming a VPP license on the new 400 path.
By the time InsertHostVPPSoftwareInstall returns apple_mdm.ErrUnresolvableAppConfigVar, AssociateAssets may already have succeeded above. This branch now turns that into a 400 without undoing the external assignment, so the host can consume a VPP license with no queued install or upcoming activity. Validate the managed config before AssociateAssets, or explicitly disassociate on this error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ee/server/service/software_installers.go` around lines 1531 - 1538, The code
currently converts apple_mdm.ErrUnresolvableAppConfigVar from
InsertHostVPPSoftwareInstall into a 400 without undoing a prior AssociateAssets
call, leaving the host assigned a VPP license; fix by either validating the
managed app configuration before calling AssociateAssets or, if keeping the
current call order, explicitly disassociate the assets when
InsertHostVPPSoftwareInstall returns errors.Is(err,
apple_mdm.ErrUnresolvableAppConfigVar): call the corresponding disassociation
routine for the same asset IDs you passed to AssociateAssets (undo the external
assignment) and only then return the fleet.BadRequestError; reference
InsertHostVPPSoftwareInstall, AssociateAssets, and
apple_mdm.ErrUnresolvableAppConfigVar when making the change.
Member
Author
There was a problem hiding this comment.
Looks like this is a real issue, but it's out of scope for this PR:
fleet/ee/server/service/software_installers.go
Lines 1518 to 1526 in 77a2916
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
cdcme
approved these changes
May 14, 2026
3 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #43973
Fixes a few bugs regarding managed app configurations. Includes a few cherry-picks for commits that were supposed to be merged, but got overwritten by subsequent PRs that didn't get pushed properly while working with the github stack.
Also includes some more integration test cases.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Bug Fixes