SAAD: Support DDM assets in sync + reconciliation - #49016
Conversation
MagnusHJensen
changed the title
|
@coderabbitai review once |
|
✅ Action performedReview finished.
|
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
Adds backend support for Apple Declarative Device Management (DDM) assets in the DDM sync/reconciliation flow by tracking declaration→asset references, incorporating assets_updated_at into per-host effective tokens, including assets in the DDM manifest, and serving asset JSON from a new declaration/asset/<identifier> endpoint.
Changes:
- Extend per-host DDM token computation (
EffectiveDDMToken, SQL token calculation, reconciliation delta logic) to incorporateassets_updated_at. - Persist and load declaration→asset reference edges, and include referenced assets in the
declaration-itemsresponse plus a newdeclaration/asset/<id>handler. - Add/adjust unit + integration tests covering reconciliation behavior and token changes when assets are edited.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_ddm_test.go | Adds an integration test asserting asset edits trigger a DDM resync and token change. |
| server/service/integration_core_test.go | Updates call sites for the extended declaration creation API. |
| server/service/apple_mdm.go | Adds asset reference extraction/validation, serves assets via DDM endpoints, and incorporates assets_updated_at into effective tokens and manifests. |
| server/service/apple_mdm_test.go | Updates mocks + adds service-level tests for asset reference validation during declaration creation. |
| server/service/apple_mdm_ddm_test.go | Updates datastore declaration creation signature in DDM tests. |
| server/mock/datastore_mock.go | Extends datastore mock surface for new declaration signature and asset lookup/list helpers. |
| server/mdm/apple/reconcile.go | Treats assets_updated_at as a reason to re-deliver an unchanged declaration and stamps it on host rows. |
| server/mdm/apple/reconcile_test.go | Adds unit test coverage for assets_updated_at delta behavior. |
| server/fleet/datastore.go | Extends datastore interface for asset reference persistence and lookups. |
| server/fleet/apple_mdm.go | Adds AssetsUpdatedAt fields and extends EffectiveDDMToken; adjusts declaration type validation copy and forbidden-type set. |
| server/fleet/apple_mdm_test.go | Updates declaration validation tests and adds EffectiveDDMToken tests for vars/assets ordering. |
| server/datastore/mysql/teams_test.go | Updates declaration creation signature in team tests. |
| server/datastore/mysql/secret_variables_test.go | Updates declaration creation signature in secret-variable tests. |
| server/datastore/mysql/labels_test.go | Updates declaration creation signature in label tests. |
| server/datastore/mysql/hosts_test.go | Updates declaration creation signature in host tests. |
| server/datastore/mysql/apple_mdm.go | Persists asset references on declaration insert; extends token SQL and adds asset lookup/query helpers. |
| server/datastore/mysql/apple_mdm_test.go | Updates declaration creation signature in datastore tests. |
| server/datastore/mysql/apple_mdm_ddm_test.go | Adds datastore tests for asset lookup-by-identifier, referenced-asset queries, and token round-trip with assets_updated_at. |
| server/datastore/mysql/apple_mdm_batched.go | Loads assets_updated_at for reconcile inputs and persists it to host declaration rows. |
| cmd/fleetctl/fleetctl/gitops_test.go | Updates GitOps tests for the extended declaration creation signature. |
| changes/38986-support-ddm-assets | User-visible changes entry (content excluded from review). |
Files excluded by content exclusion policy (1)
- changes/38986-support-ddm-assets
💡 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 #49016 +/- ##
==========================================
- Coverage 68.09% 68.03% -0.07%
==========================================
Files 3732 3743 +11
Lines 235783 237392 +1609
Branches 12371 12371
==========================================
+ Hits 160557 161503 +946
- Misses 60807 61298 +491
- Partials 14419 14591 +172
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:
|
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds Apple DDM asset support across declaration creation, datastore persistence, reconciliation, and delivery. It introduces Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 2
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/apple_mdm.go (1)
4924-4964: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove the
aff == 0exists-check before inserting asset references.mdm_apple_declaration_asset_references.declaration_uuidhas a foreign key tomdm_apple_declarations.declaration_uuid, so the duplicate-name path can raise a raw FK error instead ofexistsError. It also does unnecessary writes on the conflict path.🤖 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/apple_mdm.go` around lines 4924 - 4964, The exists-check in the declaration insert flow should happen before writing asset references, because the current order in the new declaration transaction can hit a foreign key error instead of returning the intended existsError on duplicate names. Update the apple MDM declaration insert logic in the withTx callback to check RowsAffected on the ExecContext result immediately after the main insert/upsert, and only run the mdm_apple_declaration_asset_references insert when aff is non-zero; keep the existing ctxerr.Wrap and reloadStmt handling unchanged.
🤖 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/fleet/apple_mdm.go`:
- Around line 895-916: The doc comment for EffectiveDDMToken is stale because it
only describes variablesUpdatedAt and omits the new assetsUpdatedAt input.
Update the comment above EffectiveDDMToken to mention both timestamps and
explain that the returned token combines the static token with any non-nil
host-specific variables_updated_at and assets_updated_at values.
In `@server/service/apple_mdm.go`:
- Around line 6664-6683: The handleDeclarationAsset flow in MDMAppleDDMService
currently scopes only by team, so a host can fetch another host’s asset if it
knows the identifier. Update this path to verify the requested asset is actually
referenced by the requesting host’s DDM declarations—either by adding a
datastore lookup scoped to hostUUID plus assetIdentifier or by checking the
host’s manifest/reference graph before ExpandEmbeddedSecrets runs. If the asset
is not assigned to that host, return a 404 instead of serving it.
---
Outside diff comments:
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 4924-4964: The exists-check in the declaration insert flow should
happen before writing asset references, because the current order in the new
declaration transaction can hit a foreign key error instead of returning the
intended existsError on duplicate names. Update the apple MDM declaration insert
logic in the withTx callback to check RowsAffected on the ExecContext result
immediately after the main insert/upsert, and only run the
mdm_apple_declaration_asset_references insert when aff is non-zero; keep the
existing ctxerr.Wrap and reloadStmt handling unchanged.
🪄 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: 27d9b299-08bd-4740-8713-b91ae78100cc
📥 Commits
Reviewing files that changed from the base of the PR and between a53207a and c3c53c38ec03b68365041555b7d22b49650f5c73.
📒 Files selected for processing (21)
changes/38986-support-ddm-assetscmd/fleetctl/fleetctl/gitops_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_batched.goserver/datastore/mysql/apple_mdm_ddm_test.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/secret_variables_test.goserver/datastore/mysql/teams_test.goserver/fleet/apple_mdm.goserver/fleet/apple_mdm_test.goserver/fleet/datastore.goserver/mdm/apple/reconcile.goserver/mdm/apple/reconcile_test.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_ddm_test.goserver/service/apple_mdm_test.goserver/service/integration_core_test.goserver/service/integration_mdm_ddm_test.go
2ec5c9a to
05657d0
Compare
| decoded.TeamID = new(uint(0)) | ||
| } else { | ||
| fleetID, err := strconv.ParseUint(val[0], 10, 32) | ||
| fleetID, err := strconv.ParseUint(val[0], 10, 32) // nolint:staticcheck // it's used... |
There was a problem hiding this comment.
This is very odd. I wonder why staticcheck flagged that?
There was a problem hiding this comment.
I honestly have no idea, and I tried multiple times.. :(
Related issue: Resolves #48568 second part
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.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
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
Summary by CodeRabbit
New Features
Bug Fixes
Other