Support custom host vitals in host name templates - #49586
Conversation
Adds $FLEET_HOST_VITAL_<id> support to host name templates: validation of referenced vital IDs, per-host resolution at delivery, and resend-on-value-change for the device-name enforcement cron.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
WalkthroughAdds Possibly related issues
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: 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 `@server/service/apple_device_names.go`:
- Around line 144-162: Replace the invalid errors.AsType call in the custom host
vital expansion handling with standard errors.As, using a typed
*fleet.MissingCustomHostVitalValueError target. Preserve the existing branching:
wrap non-matching errors and mark the device name status for matching
missing-vital errors.
🪄 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: 9936fad6-502f-4afb-a902-d5bfec3c567e
📒 Files selected for processing (9)
changes/49489-custom-host-vitals-in-name-templatesserver/datastore/mysql/apple_device_names.goserver/datastore/mysql/custom_host_vitals.goserver/datastore/mysql/custom_host_vitals_test.goserver/fleet/name_template.goserver/fleet/name_template_test.goserver/service/apple_device_names.goserver/service/apple_device_names_test.goserver/service/mdm_test.go
| // Expand any custom host vital ($FLEET_HOST_VITAL_<id>) references with this | ||
| // host's stored value. Unlike secrets, vital values are per-host, so this | ||
| // can't be memoized across hosts sharing a template. | ||
| if len(fleet.ContainsCustomHostVitalIDs(expandedTmpl)) > 0 { | ||
| withVitals, vitalErr := ds.ExpandCustomHostVitals(ctx, host.HostID, expandedTmpl) | ||
| if vitalErr != nil { | ||
| if _, ok := errors.AsType[*fleet.MissingCustomHostVitalValueError](vitalErr); !ok { | ||
| return ctxerr.Wrap(ctx, vitalErr, "expand host name template custom host vitals") | ||
| } | ||
| // A referenced vital exists but has no value set for this host. | ||
| if err := ds.SetHostDeviceNameStatus(ctx, host.HostUUID, fleet.MDMDeliveryFailed, nil, "", | ||
| vitalErr.Error()); err != nil { | ||
| logger.ErrorContext(ctx, "mark device name row failed for missing custom host vital value", "host_uuid", host.HostUUID, "err", err) | ||
| } | ||
| continue | ||
| } | ||
| expandedTmpl = withVitals | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for `AsType` in the codebase to see if a custom `errors` package or alias is used.
rg 'func AsType'Repository: fleetdm/fleet
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## apple_device_names.go imports and surrounding lines\n'
sed -n '1,220p' server/service/apple_device_names.go | nl -ba | sed -n '1,80p'
printf '\n## search for errors package aliases and AsType references\n'
rg -n '\bAsType\b|import\s+\(|"errors"|errors\.' server/service server | sed -n '1,200p'
printf '\n## locate any non-std errors packages in repo\n'
fd -a 'errors.go' .Repository: fleetdm/fleet
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## map the file structure around the target file\n'
ast-grep outline server/service/apple_device_names.go --view expanded || trueRepository: fleetdm/fleet
Length of output: 679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## import block and target region\n'
sed -n '1,220p' server/service/apple_device_names.go | sed -n '1,70p'
printf '\n---\n'
sed -n '130,175p' server/service/apple_device_names.go
printf '\n## all AsType / errors usage in repo\n'
rg -n '\bAsType\b|errors\.As\(|errors\.Is\(|errors\.AsType' server/service server . | sed -n '1,200p'Repository: fleetdm/fleet
Length of output: 22409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## one non-target file using AsType\n'
sed -n '1,120p' server/webhooks/failing_policies.go
printf '\n## another file using AsType\n'
sed -n '1,120p' server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy_test.go
printf '\n## search for any AsType definition or custom errors package\n'
rg -n 'func AsType|type .*AsType|package errors\b|module .*errors|AsType\[' . | sed -n '1,200p'Repository: fleetdm/fleet
Length of output: 8954
Replace errors.AsType here. server/service/apple_device_names.go imports the standard library errors, which has no AsType, so this won’t compile. Use errors.As with a typed *fleet.MissingCustomHostVitalValueError instead.
🤖 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/service/apple_device_names.go` around lines 144 - 162, Replace the
invalid errors.AsType call in the custom host vital expansion handling with
standard errors.As, using a typed *fleet.MissingCustomHostVitalValueError
target. Preserve the existing branching: wrap non-matching errors and mark the
device name status for matching missing-vital errors.
There was a problem hiding this comment.
We use errors.AsType in several places in the codebase already, so I think this is a false positive.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49586 +/- ##
=======================================
Coverage 67.89% 67.89%
=======================================
Files 3889 3889
Lines 248314 248367 +53
Branches 13179 13179
=======================================
+ Hits 168586 168624 +38
- Misses 64525 64534 +9
- Partials 15203 15209 +6
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:
|
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 support for referencing custom host vitals ($FLEET_HOST_VITAL_<id>) inside Apple host name templates, including save-time validation, per-host expansion during device-name reconciliation, and automatic re-queueing of device-name enforcement when a host vital value changes.
Changes:
- Validate name templates against referenced custom host vital IDs at save time.
- Expand custom host vital tokens per-host during
ReconcileHostDeviceNames, failing the enforcement row when a referenced vital has no value for that host. - Re-queue device-name enforcement rows when a referenced custom host vital value changes (MySQL datastore).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/mdm_test.go | Adds service-level tests asserting host name template updates validate custom host vital references. |
| server/service/apple_device_names.go | Expands $FLEET_HOST_VITAL_<id> tokens per-host during device-name reconciliation and marks enforcement rows failed when vital values are missing. |
| server/service/apple_device_names_test.go | Adds unit tests covering per-host custom host vital expansion behavior in the reconciliation cron. |
| server/fleet/name_template.go | Updates host name template validation to exclude vital tokens from the fixed-text byte floor and validates referenced vital IDs via datastore. |
| server/fleet/name_template_test.go | Adds syntax-layer tests ensuring vital tokens are permitted and don’t affect the fixed-text byte floor calculation. |
| server/datastore/mysql/custom_host_vitals.go | Ensures host device-name enforcement is re-queued when a referenced custom host vital value changes. |
| server/datastore/mysql/custom_host_vitals_test.go | Adds MySQL test verifying device-name rows are re-queued only when the updated vital is referenced by the template. |
| server/datastore/mysql/apple_device_names.go | Implements transaction-scoped logic to re-queue a host’s device-name enforcement row when its applicable template references the updated vital. |
| changes/49489-custom-host-vitals-in-name-templates | User-visible change note (content excluded from this review). |
Files excluded by content exclusion policy (1)
- changes/49489-custom-host-vitals-in-name-templates
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Vital IDs are dynamic (not a fixed allow-list), so an unknown or malformed | ||
| // $FLEET_HOST_VITAL_<id> reference is only caught here, same as scripts and | ||
| // profiles validate their own embedded vital references. | ||
| if err := ds.ValidateReferencedCustomHostVitals(ctx, []string{validated}); err != nil { | ||
| return "", NewInvalidArgumentError("name_template", err.Error()) | ||
| } |
There was a problem hiding this comment.
Good catch, I'm fixing this here and also for other callers of ValidateReferencedCustomHostVitals.
Adds IsInvalidReferencedCustomHostVitalsError to distinguish genuine validation failures (unknown/malformed vital reference) from infrastructure errors, and updates every caller of ValidateReferencedCustomHostVitals to propagate the latter as-is instead of misreporting it as invalid input.
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 17 out of 18 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/49489-custom-host-vitals-in-name-templates
…host-vitals-in # Conflicts: # server/service/apple_mdm.go
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 17 out of 18 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/49489-custom-host-vitals-in-name-templates
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/service/apple_mdm.go (1)
1060-1063: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate infrastructure errors from custom host vital validation.
Unlike
parseAndValidateAppleConfigProfileandBatchSetMDMAppleProfiles, this validation block maps all errors (including infrastructure failures like database errors) to a 422 Invalid Argument response.Please check
fleet.IsInvalidReferencedCustomHostVitalsError(err)and short-circuit with a wrapped contextual error for infrastructure failures, so they are correctly classified (e.g., as 500 errors).💻 Proposed fix
- // Validate custom host vital references (top-level $FLEET_HOST_VITAL_<id>). - if err := svc.ds.ValidateReferencedCustomHostVitals(ctx, []string{string(data)}); err != nil { - return nil, nil, "", fleet.NewInvalidArgumentError("profile", err.Error()) - } + // Validate custom host vital references (top-level $FLEET_HOST_VITAL_<id>). + if err := svc.ds.ValidateReferencedCustomHostVitals(ctx, []string{string(data)}); err != nil { + if !fleet.IsInvalidReferencedCustomHostVitalsError(err) { + return nil, nil, "", ctxerr.Wrap(ctx, err, "validating referenced custom host vitals") + } + return nil, nil, "", fleet.NewInvalidArgumentError("profile", err.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 `@server/service/apple_mdm.go` around lines 1060 - 1063, Update the custom host vital validation block in the Apple MDM profile flow to check fleet.IsInvalidReferencedCustomHostVitalsError(err). Preserve the 422 fleet.NewInvalidArgumentError response for invalid references, but return infrastructure failures as a wrapped contextual error so they retain 500 classification, matching parseAndValidateAppleConfigProfile and BatchSetMDMAppleProfiles.
🤖 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/service/apple_mdm.go`:
- Around line 1060-1063: Update the custom host vital validation block in the
Apple MDM profile flow to check
fleet.IsInvalidReferencedCustomHostVitalsError(err). Preserve the 422
fleet.NewInvalidArgumentError response for invalid references, but return
infrastructure failures as a wrapped contextual error so they retain 500
classification, matching parseAndValidateAppleConfigProfile and
BatchSetMDMAppleProfiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76c38e58-72f0-45a8-a055-f40f3f35d887
📒 Files selected for processing (5)
ee/server/service/software_installers.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/mdm.goserver/service/mdm_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/service/mdm.go
- server/service/apple_mdm_test.go
- server/service/mdm_test.go
- ee/server/service/software_installers.go
|
Currently, custom host vitals can be deleted even if they're actively used in a host name template, yes?. If so, we should add some guardrails to prevent this. |
Do you also need to add this check for Apple DDM declaration uploads? |
| // by the host's (team) name template re-queues its device-name enforcement | ||
| // row, but setting a vital the template doesn't reference leaves the row | ||
| // untouched, mirroring the precision of the profile-resend case above. | ||
| func testSetHostCustomHostVitalValueResendsDeviceName(t *testing.T, ds *Datastore) { |
There was a problem hiding this comment.
Just to cover all bases, let's extend the tests here to also check for the "Unassigned" case (a.k.a No team).
…templates Custom host vitals could be deleted while still referenced by a team's or "No team"'s host name template, silently breaking name resolution going forward. Extends the existing delete-protection scan to cover host name templates, mirroring the same guard already in place for secret variables. Also fixes a gap in the ValidateReferencedCustomHostVitals error classification: the Apple DDM declaration create/update path was missing the check that distinguishes genuine validation failures from infrastructure errors.
Good catch, I'd missed this 👍
Added 👍 (Had a merge conflict in that area, and I missed keeping that check there.) |
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 17 out of 18 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/49489-custom-host-vitals-in-name-templates
| if err := ds.ValidateReferencedCustomHostVitals(ctx, []string{validated}); err != nil { | ||
| if IsInvalidReferencedCustomHostVitalsError(err) { | ||
| return "", NewInvalidArgumentError("name_template", err.Error()) | ||
| } | ||
| return "", err | ||
| } |
MissingCustomHostVitalsError and InvalidCustomHostVitalRefError are surfaced from every ValidateReferencedCustomHostVitals caller (scripts, profiles, software installers, host name templates, ...), not just upload/create flows, so a fixed "Couldn't add." prefix was wrong for edits and template saves.
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 17 out of 18 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/49489-custom-host-vitals-in-name-templates
| // nameTemplateVitalRegexp matches a $FLEET_HOST_VITAL_<id> / ${FLEET_HOST_VITAL_<id>} | ||
| // custom host vital token. Vital values, like secrets, are only known at | ||
| // resolve time, so this is used to strip vital tokens out of a template when | ||
| // computing its fixed-text byte floor. |
…tion The single-profile-upload (NewMDMAndroidConfigProfile) and Android VPP app config (BatchAssociateVPPApps) call sites for ValidateReferencedCustomHostVitals were reporting any error as a 422 invalid-argument, including a genuine datastore failure. Use fleet.IsInvalidReferencedCustomHostVitalsError to only convert the two known validation error types, matching the pattern already used at every other ValidateReferencedCustomHostVitals call site (introduced in #49586).
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #49421 Custom host vitals (`$FLEET_HOST_VITAL_<id>`) already worked in scripts and Apple/Windows configuration profiles, but Android configuration profiles and managed app configuration explicitly rejected them at upload to keep parity with `$FLEET_SECRET_*`. This left admins unable to inject per-host vitals (e.g. an asset tag) into Android MDM configuration the same way they can for every other platform. For more context, prior PRs: - #49334 - #49586 # Checklist for submitter - [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. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually - Created an "Asset tag" host vital. - Enrolled an Android device. - Initially the test profile showed as "Failed" because no value was set for the vital. - Set a value for the vital, saw that it went from Enforcing to Verified. <img width="1446" height="510" alt="Screenshot 2026-07-24 at 8 57 46 AM" src="https://github.com/user-attachments/assets/c0e2348c-e521-48f3-85cd-6f884689b2cd" /> <img width="1520" height="936" alt="Screenshot 2026-07-24 at 8 56 56 AM" src="https://github.com/user-attachments/assets/169b9545-ec7a-429b-8f45-0e2740f61c77" /> <img width="1607" height="1136" alt="Screenshot 2026-07-24 at 8 57 30 AM" src="https://github.com/user-attachments/assets/a8213745-b224-4a36-a54d-32152a15c377" /> Also tested the rejection cases: - trying to upload a profile with an invalid custom host vital id (either a non-numeric value, a numeric but non-existent ID, and referencing a vital as a JSON key instead of a value) - deleting a vital referenced in a profile https://github.com/user-attachments/assets/e8b4acde-ddf4-41c0-b00a-5ab4945d0bc2 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit * **New Features** * Android app configurations and profiles now support custom host vital placeholders (`$FLEET_HOST_VITAL_<id>`). * Custom host vital values are expanded per device during Android delivery. * Managed Android profiles/configurations are automatically resent when a referenced vital value changes. * **Bug Fixes** * Added validation for malformed, missing, or undefined vital references during Android app association and profile/config uploads. * Prevented deletion of vitals referenced by Android profiles. * Improved error handling and delivery failure details when a device lacks a required vital value. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Related issue: Resolves #49489
Custom host vitals were skipped when host name template enforcement (#38806) shipped, since both features were in development at the same time. This adds
$FLEET_HOST_VITAL_<id>support to host name templates, matching the existing secret-variable pattern (validation, per-host resolution, resend on value change).I also introduced a new
IsInvalidReferencedCustomHostVitalsErrorcall after Copilot's comment below.Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit
Summary by CodeRabbit
New Features
$FLEET_HOST_VITAL_<id>in Apple host name templates.Bug Fixes
Improved Error Handling