SAAD: User channel + DDM assets for osquery-perf - #49108
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends cmd/osquery-perf to simulate macOS MDM user-channel enrollment and exercise the DDM tokens → declaration-items → declarations (including assets) → status report flow for load testing, controlled by a new mdm_user_prob flag so it can remain disabled by default.
Changes:
- Document
mdm_proband newmdm_user_probin the osquery-perf README. - Add new user-channel and asset-related counters to osquery-perf stats/log output.
- Refactor DDM handling into a shared helper (
ddm.go) and integrate both device-channel and user-channel DDM flows into the macOS MDM loop.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/osquery-perf/README.md | Adds documentation for mdm_prob / mdm_user_prob usage. |
| cmd/osquery-perf/osquery_perf/stats.go | Adds user-channel and asset-related counters and includes them in periodic stats logging. |
| cmd/osquery-perf/ddm.go | Introduces shared DDM request/status logic abstracted over device vs user channel methods. |
| cmd/osquery-perf/agent.go | Adds mdm_user_prob, performs optional user enrollment, and runs user-channel MDM/DDM handling in the macOS MDM loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughAdds configurable macOS MDM user enrollment with concurrency-safe state and user-scoped DDM token caches. Implements reusable DDM token polling, declaration retrieval, removal detection, and status reporting. Extends device and user check-ins for DeclarativeManagement commands and adds thread-safe MDM-user and DDM-user metrics to logged statistics. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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
🧹 Nitpick comments (1)
cmd/osquery-perf/ddm.go (1)
76-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated declaration-fetch loop into a helper.
The three loops over Activations, Assets, and Configurations are structurally identical: record the current token, compare with the cached token, fetch if different, set
changed = true. Extracting a helper would eliminate ~30 lines of duplication and reduce the risk of them diverging.♻️ Proposed refactor
+// fetchChangedDecls fetches declarations whose server token differs from the +// cached value, records current tokens, and sets changed=true if any fetch occurred. +func (a *agent) fetchChangedDecls( + decls []fleet.MDMAppleDDMDeclarationItem, + kind string, + methods ddmMethods, + currentTokens map[string]string, +) (bool, error) { + changed := false + for _, d := range decls { + currentTokens[d.Identifier] = d.ServerToken + if methods.getDeclTokens()[d.Identifier] != d.ServerToken { + if err := a.ddmFetchDeclaration(kind, d.Identifier, methods); err != nil { + return false, err + } + changed = true + } + } + return changed, nil +}Then in
doDeclarativeManagement:- for _, d := range items.Declarations.Activations { - currentTokens[d.Identifier] = d.ServerToken - if methods.getDeclTokens()[d.Identifier] != d.ServerToken { - if err := a.ddmFetchDeclaration("activation", d.Identifier, methods); err != nil { - return - } - changed = true - } - } - - for _, d := range items.Declarations.Assets { - currentTokens[d.Identifier] = d.ServerToken - if methods.getDeclTokens()[d.Identifier] != d.ServerToken { - if err := a.ddmFetchDeclaration("asset", d.Identifier, methods); err != nil { - return - } - changed = true - } - } - - for _, d := range items.Declarations.Configurations { - currentTokens[d.Identifier] = d.ServerToken - if methods.getDeclTokens()[d.Identifier] != d.ServerToken { - if err := a.ddmFetchDeclaration("configuration", d.Identifier, methods); err != nil { - return - } - changed = true - } - } + var err error + if changed, err = a.fetchChangedDecls(items.Declarations.Activations, "activation", methods, currentTokens); err != nil { + return + } + if c, err := a.fetchChangedDecls(items.Declarations.Assets, "asset", methods, currentTokens); err != nil { + return + } else { + changed = changed || c + } + if c, err := a.fetchChangedDecls(items.Declarations.Configurations, "configuration", methods, currentTokens); err != nil { + return + } else { + changed = changed || c + }Note: The exact
MDMAppleDDMDeclarationItemtype name should be verified against thefleetpackage — the declaration slices may use a different element type.🤖 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 `@cmd/osquery-perf/ddm.go` around lines 76 - 104, Extract the duplicated logic in doDeclarativeManagement into a helper that accepts the declaration type, declaration items, currentTokens, and methods, then records server tokens, compares cached tokens, fetches changed declarations via ddmFetchDeclaration, and reports whether processing changed or failed. Use the actual declaration item type from the fleet package, and replace the Activations, Assets, and Configurations loops with helper calls while preserving early error returns and changed-state behavior.
🤖 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 `@cmd/osquery-perf/agent.go`:
- Around line 862-872: User enrollment failure in the agent startup path
prematurely exits runLoop and records the wrong error metric. In the UserEnroll
error branch within runLoop, replace the return with non-aborting handling so
execution continues to start runMacosMDMLoop and the subsequent goroutines, and
change IncrementMDMErrors to IncrementMDMUserErrors while preserving the failure
log.
In `@cmd/osquery-perf/ddm.go`:
- Line 267: Update ddmSendStatus to invoke
methods.DeclarativeManagement("status", report) instead of calling
a.macMDMClient.DeclarativeManagement directly, preserving the selected device-
or user-channel adapter and its corresponding endpoint.
---
Nitpick comments:
In `@cmd/osquery-perf/ddm.go`:
- Around line 76-104: Extract the duplicated logic in doDeclarativeManagement
into a helper that accepts the declaration type, declaration items,
currentTokens, and methods, then records server tokens, compares cached tokens,
fetches changed declarations via ddmFetchDeclaration, and reports whether
processing changed or failed. Use the actual declaration item type from the
fleet package, and replace the Activations, Assets, and Configurations loops
with helper calls while preserving early error returns and changed-state
behavior.
🪄 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: 439dce0b-9ede-49ea-9d83-00e0f2448873
⛔ Files ignored due to path filters (1)
cmd/osquery-perf/README.mdis excluded by!**/*.md
📒 Files selected for processing (3)
cmd/osquery-perf/agent.gocmd/osquery-perf/ddm.gocmd/osquery-perf/osquery_perf/stats.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49108 +/- ##
==========================================
- Coverage 68.03% 67.98% -0.06%
==========================================
Files 3743 3744 +1
Lines 237279 237477 +198
Branches 12389 12389
==========================================
+ Hits 161444 161447 +3
- Misses 61264 61459 +195
Partials 14571 14571
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:
|
| a.stats.IncrementMDMUserCommandsReceived() | ||
|
|
||
| switch mdmCommandPayload.Command.RequestType { | ||
| case "InstallProfile": |
There was a problem hiding this comment.
Somehow I never really realized that we don't track these for osquery purposes. Might be a worthwhile thing at some point(Maybe good discussion at our next tech debt meeting?)
There was a problem hiding this comment.
I did see that as well, forgot to bring it up a couple of months ago.
Related issue: Resolves #48573
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. In another PR
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
Summary by CodeRabbit
New Features
Bug Fixes