On var change resend Android certificate templates and managed app configs#48278
Merged
Conversation
![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
There was a problem hiding this comment.
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 extends Fleet’s SCIM-triggered “resend on variable change” behavior to cover Android certificate templates and Android managed app configurations by tracking which Fleet variables each artifact references and resending/repushing when those variables change.
Changes:
- Track Fleet variable usage for Android certificate templates (service layer + datastore persistence).
- Track Fleet variable usage for Android app configurations and enqueue repush jobs when affected SCIM variables change.
- Extend
mdm_configuration_profile_variablesschema/migration to associate variables with certificate templates and Android app configurations.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/certificates.go | Persists variable associations when creating/applying certificate templates. |
| server/service/certificate_templates.go | Adds helper to extract Fleet vars from subject name + SAN. |
| server/service/certificate_templates_test.go | Adds unit tests for extraction + verifies variable tracking is called on create. |
| server/mock/datastore_mock.go | Adds mock support for SetCertificateTemplateVariables. |
| server/fleet/datastore.go | Extends datastore interface to include SetCertificateTemplateVariables. |
| server/datastore/mysql/scim.go | On SCIM var change: marks cert templates pending and queues managed-config repush jobs. |
| server/datastore/mysql/schema.sql | Updates schema snapshot for new variable-tracking columns/constraints. |
| server/datastore/mysql/migrations/tables/20260624152755_AddAppConfigVariableTracking.go | Migration adding cert-template/app-config variable tracking columns + updated CHECK constraint. |
| server/datastore/mysql/migrations/tables/20260624152755_AddAppConfigVariableTracking_test.go | Migration test covering uniqueness/check constraint + cascade deletes. |
| server/datastore/mysql/certificate_templates.go | Implements variable association persistence for cert templates. |
| server/datastore/mysql/android.go | Persists variable associations when Android app configs are inserted/updated. |
| changes/48042-resend-managed-config-on-var-change | Release-note entry (content excluded from review per policy). |
Files excluded by content exclusion policy (1)
- changes/48042-resend-managed-config-on-var-change
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #48278 +/- ##
========================================
Coverage 67.30% 67.31%
========================================
Files 3661 3662 +1
Lines 231437 231749 +312
Branches 12155 12155
========================================
+ Hits 155774 156007 +233
- Misses 61725 61768 +43
- Partials 13938 13974 +36
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:
|
Contributor
|
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 with no reviewable changes (2)
WalkthroughThis PR extends Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
Contributor
There was a problem hiding this comment.
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
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/48042-resend-managed-config-on-var-change
Comment on lines
+1420
to
+1423
| // Queue make_android_app_available jobs for managed app configs that use affected variables. | ||
| if err := queueManagedConfigResendJobs(ctx, tx, vars); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "queue managed config resend jobs") | ||
| } |
Contributor
Author
There was a problem hiding this comment.
The calls are idempotent (AMAPI returns NotModified) but it wastes rate limit quota unnecessarily. I think this is a reasonable approach for now and can be fixed in a follow up.
![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: 3
🧹 Nitpick comments (2)
server/datastore/mysql/certificate_templates.go (2)
327-383: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the delete+insert "replace" atomic.
setCertTemplateVariableAssociationsdeletes existing rows and then performs a separate bulk insert, and the exportedSetCertificateTemplateVariablesruns it onds.writer(ctx)(no transaction). If the insert fails after the delete commits, the template is left with no variable associations, so a subsequent IdP variable change would silently fail to resend that template (the SCIM resend query joins throughmdm_configuration_profile_variables). Consider wrapping the delete+insert in a transaction.♻️ Run the replace inside a transaction
func (ds *Datastore) SetCertificateTemplateVariables(ctx context.Context, certTemplateID uint, fleetVars []fleet.FleetVarName) error { - return setCertTemplateVariableAssociations(ctx, ds.writer(ctx), certTemplateID, fleetVars) + return ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { + return setCertTemplateVariableAssociations(ctx, tx, certTemplateID, fleetVars) + }) }🤖 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/certificate_templates.go` around lines 327 - 383, `setCertTemplateVariableAssociations` currently performs a delete followed by a separate insert, and `SetCertificateTemplateVariables` calls it through `ds.writer(ctx)` without a transaction, so a failed insert can leave `mdm_configuration_profile_variables` empty; make the replace atomic by running the delete+insert together in one transaction. Update `SetCertificateTemplateVariables` (or the helper it calls) to begin a transaction, pass the transactional executor into `setCertTemplateVariableAssociations`, and only commit after the insert succeeds so the associations are never partially replaced.
369-373: 📐 Maintainability & Code Quality | 🔵 TrivialConsider replacing deprecated
VALUES()with row aliases.The
VALUES()function usage within theON DUPLICATE KEY UPDATEclause is deprecated as of MySQL 8.0.20 and subject to removal in future versions. Replacing it with the aliased-row syntax is recommended for forward compatibility.Update the statement to include a row alias (e.g.,
AS new) after the values list to reference the inserted values directly in the update clause:INSERT INTO mdm_configuration_profile_variables (certificate_template_id, fleet_variable_id) VALUES %s AS new ON DUPLICATE KEY UPDATE fleet_variable_id = new.fleet_variable_id🤖 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/certificate_templates.go` around lines 369 - 373, The SQL built in certificate templates insertion still uses deprecated VALUES() inside the ON DUPLICATE KEY UPDATE clause. Update the statement in the certificate template variable insert logic to use a row alias after the VALUES list and reference that alias in the update assignment, keeping the behavior in sync while avoiding VALUES() in the statement constructed by the code that formats the INSERT for mdm_configuration_profile_variables.
🤖 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
`@server/datastore/mysql/migrations/tables/20260624152755_AddAppConfigVariableTracking_test.go`:
- Around line 57-58: The CHECK constraint assertions in the migration test are
being masked by duplicate unique-key violations. Update the invalid insert
checks in the migration test around the mdm_configuration_profile_variables
inserts so they don’t reuse existing `(certificate_template_id,
fleet_variable_id)` and `(android_app_configuration_id, fleet_variable_id)`
pairs; instead, perform the exclusivity checks before the valid inserts, remove
the existing association first, or use a different fleet variable. Make the same
fix for both the certificate_template/windows_profile_uuid case and the
android_app_configuration case so the test specifically validates
`ck_mdm_configuration_profile_variables_exactly_one`.
- Around line 40-44: Scope the inserted-row lookups in the migration test so
they only resolve the record created by the test. The current `db.QueryRow`
lookups for `certTemplateID` (and the later similar lookup around the second
insert) rely on non-unique fields like `name`, which can bind to the wrong row;
update the test to use `LastInsertId()` from the insert result or add the full
entity scoping criteria such as `team_id`, `global_or_team_id`, and the relevant
CA/team identifiers. Keep the fix localized to the test helpers/queries in this
migration test so the identifiers retrieved always correspond to the inserted
rows.
In `@server/datastore/mysql/scim.go`:
- Around line 1420-1447: queueManagedConfigResendJobs is too broadly scoped
because it only filters affected Android app configs by variable name, which can
enqueue resend jobs for unrelated hosts and teams. Update the SCIM flow to pass
hostIDs into queueManagedConfigResendJobs, and tighten the lookup in
queueManagedConfigResendJobs/findAffectedApps so it only selects app configs
tied to the affected hosts’ team scope before creating jobs. Use the existing
queueManagedConfigResendJobs helper and the affected app/team joins to add the
host-specific filtering before job insertion.
---
Nitpick comments:
In `@server/datastore/mysql/certificate_templates.go`:
- Around line 327-383: `setCertTemplateVariableAssociations` currently performs
a delete followed by a separate insert, and `SetCertificateTemplateVariables`
calls it through `ds.writer(ctx)` without a transaction, so a failed insert can
leave `mdm_configuration_profile_variables` empty; make the replace atomic by
running the delete+insert together in one transaction. Update
`SetCertificateTemplateVariables` (or the helper it calls) to begin a
transaction, pass the transactional executor into
`setCertTemplateVariableAssociations`, and only commit after the insert succeeds
so the associations are never partially replaced.
- Around line 369-373: The SQL built in certificate templates insertion still
uses deprecated VALUES() inside the ON DUPLICATE KEY UPDATE clause. Update the
statement in the certificate template variable insert logic to use a row alias
after the VALUES list and reference that alias in the update assignment, keeping
the behavior in sync while avoiding VALUES() in the statement constructed by the
code that formats the INSERT for mdm_configuration_profile_variables.
🪄 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: c4e0ad84-ea3e-4619-b4f7-b5d000964150
📒 Files selected for processing (13)
changes/48042-resend-managed-config-on-var-changeserver/datastore/mysql/android.goserver/datastore/mysql/certificate_templates.goserver/datastore/mysql/migrations/tables/20260624152755_AddAppConfigVariableTracking.goserver/datastore/mysql/migrations/tables/20260624152755_AddAppConfigVariableTracking_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/scim.goserver/datastore/mysql/scim_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/certificate_templates.goserver/service/certificate_templates_test.goserver/service/certificates.go
JordanMontgomery
left a comment
JordanMontgomery
left a comment
Member
There was a problem hiding this comment.
My only real concern is whether or not it's possible the current resend is too broad for the app - like do we need to check label applicability since this could potentially "widen" the applicability of an app sinec it queues that make available job. It's possible I'm off base there so happy to be convinced I am wrong. Otherwise this looks good to me and matches what we've been doing for this sort of thing elsewhere
| ON mcpv.android_app_configuration_id = aac.id | ||
| JOIN fleet_variables fv | ||
| ON mcpv.fleet_variable_id = fv.id | ||
| JOIN vpp_apps_teams vat |
Member
There was a problem hiding this comment.
Do we need to validate that the app is applicable to the given hosts here by chance(e.g. with label settings)? Or am I missing something(not familiar with this feature so possible)
Contributor
Author
There was a problem hiding this comment.
Valid concern. But the makeAndroidAppAvailable calls GetIncludedHostUUIDMapForAppStoreApp. this resolves the in-scope hosts including label filtering. Adding label filtering to the query would duplicate that logic (joining vpp_app_host_label_conditions etc.) for not much safety gain. If someone decides to call this outside of makeAndroidAppAvailable, worst case we queue a job that resolves to zero in-scope hosts and no-ops.
Member
There was a problem hiding this comment.
That makes sense. The model is a little different for other things that apply or don't apply based on scoping so it wasn't clear to me how this works. In that light, this lgtm
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
JordanMontgomery
approved these changes
Jun 25, 2026
JordanMontgomery
pushed a commit
that referenced
this pull request
Jun 26, 2026
…nfigs (#48278) **Related issue:** Resolves #36681, #48042 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `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 - [x] Added/updated automated tests - [ ] QA'd all new/changed functionality manually ## Database migrations - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Certificate templates and managed Android app configurations now keep track of referenced variables. * Variable changes can now trigger automatic re-sending of affected profiles and app availability updates. * **Bug Fixes** * Resend behavior now refreshes certificate templates when related variable values change. * Android managed app configurations are re-queued when their variables are updated. * **Database** * Added support for variable tracking on certificate templates and Android app configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
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 #36681, #48042
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
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
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit