45178 cpie reconciler query updates#46889
Conversation
…le MDM reconciler
…ter adding combined label UNION arms
… include-all strictness
![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.
There was a problem hiding this comment.
Pull request overview
This PR updates MDM reconciler targeting logic so Windows and Android configuration profile “desired state” queries support combined include + exclude label rules (include-any/include-all plus exclude-any), and adds/extends unit/integration tests to validate the new targeting behavior (including Apple-side applicability logic).
Changes:
- Extend the Windows MDM desired-state SQL to add UNION branches for
include-all + exclude-anyandinclude-any + exclude-any, and update all callers for the additional format placeholders. - Extend the Android applicable-profiles SQL similarly, and update query formatting/
sqlx.Inargument lists accordingly. - Add/extend tests for Apple applicability logic and for Windows/Android combined-label targeting.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/mdm/apple/reconcile_test.go | Adds unit tests for Apple entity applicability under combined include+exclude targeting. |
| server/datastore/mysql/microsoft_mdm.go | Updates Windows profile desired-state query (new combined include+exclude UNION arms) and adjusts query usage to match new placeholder count. |
| server/datastore/mysql/microsoft_mdm_test.go | Adds MySQL integration test coverage for combined include+exclude label targeting on Windows profiles. |
| server/datastore/mysql/android.go | Updates Android applicable-profiles query (new combined include+exclude UNION arms) and adjusts query usage/args to match new placeholder count. |
| server/datastore/mysql/android_test.go | Adds MySQL integration test coverage for combined include+exclude label targeting on Android profiles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DetailUpdatedAt: time.Now(), | ||
| LabelUpdatedAt: time.Now().Add(-5 * time.Second), | ||
| PolicyUpdatedAt: time.Now(), | ||
| SeenTime: time.Now(), |
There was a problem hiding this comment.
Not part of this change, request team review
| // advance label_updated_at so dynamic labels are immediately evaluated | ||
| h.LabelUpdatedAt = time.Now().UTC().Add(time.Second) | ||
| h.PolicyUpdatedAt = time.Now().UTC() | ||
| err = ds.UpdateHost(ctx, h) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Not part of this change, request team review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR extends MDM profile and declaration applicability across Android, Windows, and Apple to support combined include (all/any/none) and exclude-any label targeting. It refactors Android and Windows SQL queries to add NOT EXISTS gating and new UNION branches for combined include+exclude cases, updates callers to match expanded placeholder layouts, and adds tests for Android, Windows, and Apple reconciliation logic to validate combined label behavior. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/microsoft_mdm.go (1)
2467-2687:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the include-mode buckets mutually exclusive.
NewMDMWindowsConfigProfilelater in this file persists bothLabelsIncludeAllandLabelsIncludeAny, but these UNION arms only separate on exclude-label presence. A profile that carries both include modes can therefore match aninclude-anybucket without satisfying its required-all labels, and it can also appear twice in the desired-state set because the bucket predicates overlap. Either reject mixed include modes on write or add an explicit mixed-include branch / extraNOT EXISTSgates so each profile lands in exactly one bucket. As per coding guidelines, SQL queries should use precise scoping so they don't return unintended or non-deterministic results.🤖 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 `@server/datastore/mysql/microsoft_mdm.go` around lines 2467 - 2687, The UNION arms for profile-label matching are not mutually exclusive for profiles that have both LabelsIncludeAll and LabelsIncludeAny, causing overlaps and duplicate matches; update the SQL predicates in each UNION branch (the SELECT blocks that JOIN mdm_configuration_profile_labels and use require_all) to explicitly exclude profiles that contain the opposite include-mode (e.g., add NOT EXISTS checks against mdm_configuration_profile_labels for require_all = 0 when handling include-all branches and NOT EXISTS for require_all = 1 when handling include-any branches), or alternatively enforce the constraint in NewMDMWindowsConfigProfile to reject/normalize mixed include modes so a profile can only have one of LabelsIncludeAll or LabelsIncludeAny and thus land in exactly one bucket.
🧹 Nitpick comments (3)
server/mdm/apple/reconcile_test.go (1)
226-227: ⚡ Quick winMisleading comment: host has no labels.
The comment states "in exclude only, not in include" but the host labels map is empty
{}, meaning the host is not in any label (neither include nor exclude). The test is correct—when the host is not in any include label for aninclude_anyprofile, the profile does not apply—but the comment should reflect the actual scenario.📝 Suggested comment fix
- // in exclude only, not in include -> does not apply + // not in any label -> does not apply require.False(t, EntityAppliesToHost(p, host, map[uint]struct{}{}))🤖 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 `@server/mdm/apple/reconcile_test.go` around lines 226 - 227, Update the misleading test comment to match the actual scenario: the host has an empty labels map, so change the comment near the EntityAppliesToHost(p, host, map[uint]struct{}{}) assertion to state that the host has no labels (not "in exclude only") and therefore, for an include_any profile, the profile does not apply; locate the comment in reconcile_test.go around the EntityAppliesToHost assertion and replace the text accordingly.server/datastore/mysql/microsoft_mdm.go (1)
2750-2802: ⚡ Quick winCentralize the desired-state placeholder expansion.
These call sites now hard-code the desired-state branch count in repeated
fmt.Sprintf(...)slots and matchingsqlx.In(...)argument lists. A small helper that emits the repeated filters/args forwindowsMDMProfilesDesiredStateQuerywould make the next UNION change much less error-prone.Also applies to: 2870-2892, 2975-3040
🤖 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 `@server/datastore/mysql/microsoft_mdm.go` around lines 2750 - 2802, The code repetitively expands the desired-state placeholder slots for windowsProfilesToInstallQuery and duplicates the same args in sqlx.In inside listMDMWindowsProfilesToInstallDB (and similar blocks at the other ranges); create a small helper (e.g., buildWindowsDesiredStatePlaceholders or expandDesiredStateFilters) that given a hostFilter string and the desired repetition count returns the formatted query fragment (or full fmt.Sprintf result) plus a function to build the matching args slice (repeating onlyProfileUUIDs and batchUUIDs the required number of times), then replace the manual fmt.Sprintf/toInstallQuery construction and both sqlx.In calls to use that helper so adding/removing UNION branches requires changing only the helper and not every call site (update references in listMDMWindowsProfilesToInstallDB and the other similar functions).server/datastore/mysql/android_test.go (1)
1756-1838: ⚡ Quick winAdd one combined-label case that exercises dynamic exclude gating.
This test uses only manual exclude labels, so it doesn’t validate the
label_updated_atbehavior in the new combined include+exclude UNION arms. Add a dynamic exclude subcase (before/after host label refresh) to lock that contract down.🤖 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 `@server/datastore/mysql/android_test.go` around lines 1756 - 1838, Summary: add a dynamic exclude-label subcase to testListMDMAndroidProfilesToSendWithCombinedLabels that verifies combined include+exclude logic across label refresh boundaries. Update the test to create a dynamic exclude label (LabelMembershipType: fleet.LabelMembershipTypeDynamic) whose query will match the host only after label re-evaluation, create a profile using that dynamic exclude via ds.NewMDMAndroidConfigProfile, and then assert profile behavior both before and after forcing host label refresh by advancing h.LabelUpdatedAt and calling ds.UpdateHost(ctx, h) (use existing functions AddLabelsToHost/RemoveLabelsFromHost and ListMDMAndroidProfilesToSend to observe results); ensure you exercise the branch where the profile is included before the dynamic exclude takes effect and excluded after the host refresh, mirroring the manual-exclude cases already present.
🤖 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.
Outside diff comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 2467-2687: The UNION arms for profile-label matching are not
mutually exclusive for profiles that have both LabelsIncludeAll and
LabelsIncludeAny, causing overlaps and duplicate matches; update the SQL
predicates in each UNION branch (the SELECT blocks that JOIN
mdm_configuration_profile_labels and use require_all) to explicitly exclude
profiles that contain the opposite include-mode (e.g., add NOT EXISTS checks
against mdm_configuration_profile_labels for require_all = 0 when handling
include-all branches and NOT EXISTS for require_all = 1 when handling
include-any branches), or alternatively enforce the constraint in
NewMDMWindowsConfigProfile to reject/normalize mixed include modes so a profile
can only have one of LabelsIncludeAll or LabelsIncludeAny and thus land in
exactly one bucket.
---
Nitpick comments:
In `@server/datastore/mysql/android_test.go`:
- Around line 1756-1838: Summary: add a dynamic exclude-label subcase to
testListMDMAndroidProfilesToSendWithCombinedLabels that verifies combined
include+exclude logic across label refresh boundaries. Update the test to create
a dynamic exclude label (LabelMembershipType: fleet.LabelMembershipTypeDynamic)
whose query will match the host only after label re-evaluation, create a profile
using that dynamic exclude via ds.NewMDMAndroidConfigProfile, and then assert
profile behavior both before and after forcing host label refresh by advancing
h.LabelUpdatedAt and calling ds.UpdateHost(ctx, h) (use existing functions
AddLabelsToHost/RemoveLabelsFromHost and ListMDMAndroidProfilesToSend to observe
results); ensure you exercise the branch where the profile is included before
the dynamic exclude takes effect and excluded after the host refresh, mirroring
the manual-exclude cases already present.
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 2750-2802: The code repetitively expands the desired-state
placeholder slots for windowsProfilesToInstallQuery and duplicates the same args
in sqlx.In inside listMDMWindowsProfilesToInstallDB (and similar blocks at the
other ranges); create a small helper (e.g., buildWindowsDesiredStatePlaceholders
or expandDesiredStateFilters) that given a hostFilter string and the desired
repetition count returns the formatted query fragment (or full fmt.Sprintf
result) plus a function to build the matching args slice (repeating
onlyProfileUUIDs and batchUUIDs the required number of times), then replace the
manual fmt.Sprintf/toInstallQuery construction and both sqlx.In calls to use
that helper so adding/removing UNION branches requires changing only the helper
and not every call site (update references in listMDMWindowsProfilesToInstallDB
and the other similar functions).
In `@server/mdm/apple/reconcile_test.go`:
- Around line 226-227: Update the misleading test comment to match the actual
scenario: the host has an empty labels map, so change the comment near the
EntityAppliesToHost(p, host, map[uint]struct{}{}) assertion to state that the
host has no labels (not "in exclude only") and therefore, for an include_any
profile, the profile does not apply; locate the comment in reconcile_test.go
around the EntityAppliesToHost assertion and replace the text accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9198a5c-d756-4da6-972c-cea80b301af8
📒 Files selected for processing (5)
server/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/mdm/apple/reconcile_test.go
|
Code rabbit response: Mixed include modes are already rejected at the API layer by ValidateMDMProfileSpecs in server/fleet/mdm.go — a profile with both LabelsIncludeAll and LabelsIncludeAny set returns a validation error before it can be persisted. So a profile with both set can never reach the reconciler queries. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46889 +/- ##
==========================================
- Coverage 67.02% 67.01% -0.01%
==========================================
Files 2859 2843 -16
Lines 224683 224511 -172
Branches 11577 11660 +83
==========================================
- Hits 150592 150455 -137
+ Misses 60445 60402 -43
- Partials 13646 13654 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
I think these changes overall look good. I will test with Android later today and if still good merge
|
Tested on Android and this looks good |
Related issue: Resolves #45178
Checklist for submitter
If some of the following don't apply, delete the relevant line.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.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. Note: Only windows and Mac OS. Android required by someone with a device.
Summary by CodeRabbit
New Features
Tests