Add host name enforcement persistence layer for Apple hosts#48693
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThis PR adds the persistence layer for Apple host device name enforcement: a new Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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/apple_device_names.go`:
- Around line 91-126: The UpdateHostDeviceNameStatusFromCommand flow can return
stale hostUUID/expectedName if the row is removed or superseded after the SELECT
but before the UPDATE. In Datastore.UpdateHostDeviceNameStatusFromCommand, check
the affected row count from the UPDATE on host_mdm_apple_device_names; if it is
0, return the same notFound-style result used for a missing command_uuid so
callers can ignore the stale command. Keep the existing SELECT/UPDATE logic and
field extraction, but make the update step authoritative before returning
hostUUID and expectedName.
In `@server/fleet/datastore.go`:
- Around line 1781-1839: The host cleanup path is missing removal of Apple
device-name enforcement rows, so stale `host_mdm_apple_device_names` records can
remain after a host is deleted. Update the cleanup logic in `deleteHosts` to
also delete enforcement rows keyed by `host_uuid`, using the host-device-name
datastore methods or the same deletion flow used when clearing a team. Make sure
the new cleanup covers `host_mdm_apple_device_names` alongside the existing
`additionalHostRefsByUUID` removals.
🪄 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: 917ec94c-fc33-4e13-a50d-8b84a77ca877
📒 Files selected for processing (7)
server/datastore/mysql/apple_device_names.goserver/datastore/mysql/apple_device_names_test.goserver/datastore/mysql/migrations/tables/20260703135230_CreateHostMDMAppleDeviceNames.goserver/datastore/mysql/schema.sqlserver/fleet/datastore.goserver/fleet/hosts.goserver/mock/datastore_mock.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 38806-macos-iosipados-rename-hosts #48693 +/- ##
=====================================================================
Coverage ? 68.02%
=====================================================================
Files ? 3681
Lines ? 233963
Branches ? 12268
=====================================================================
Hits ? 159157
Misses ? 60477
Partials ? 14329
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:
|
juan-fdz-hawa
force-pushed
the
48620-rename-hosts-enforcement-state-migrations-and-datastore-methods
branch
from
11ea7f8 to
6e56363
Compare
![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.
Resolves #48620 Ships the datastore layer for host name templates (macOS, iOS, iPadOS) with no user-visible behavior. Adds the host_mdm_apple_device_names enforcement-state table and every Datastore method the rest of the feature consumes.
juan-fdz-hawa
force-pushed
the
48620-rename-hosts-enforcement-state-migrations-and-datastore-methods
branch
from
6e56363 to
57939b5
Compare
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
Did a first pass and overall LGTM 👍
I have a few clarifying questions due to my unfamiliarity with MDM commands and also to get the full picture of the upcoming changes (e.g. the cron mentioned in some comments).
The new table makes sense to me.
| // created before the cron resolves the template into a concrete name. | ||
| _, err := tx.Exec(` | ||
| CREATE TABLE host_mdm_apple_device_names ( | ||
| host_uuid varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL, |
There was a problem hiding this comment.
If a host is deleted, would we still have a record in this new table for that host_uuid?
There was a problem hiding this comment.
nope - this table was added to additionalHostRefsByUUID, so it will be cleared out if the host is removed.
| // template for Apple hosts (macOS, iOS, iPadOS). A NULL status means the row | ||
| // is queued for the cron to pick up and send a Settings/DeviceName command. | ||
| // expected_device_name is nullable because rows are | ||
| // created before the cron resolves the template into a concrete name. |
There was a problem hiding this comment.
I see the cron is mentioned in many places but there's no cron (yet?).
I believe I get the idea of why we would need a cron to enqueue the sending of commands, but maybe we can avoid mentioning "the cron" in the datastore layer?
There was a problem hiding this comment.
The cron will be implemented in another PR at a later time, after this and the other two PRs currently open are merged in.
| // host (the row keeps only the latest). Callers must treat it as a stale | ||
| // result to ignore. | ||
| staleErr := func() error { | ||
| return ctxerr.Wrap(ctx, notFound("HostDeviceNameEnforcement").WithMessage("unable to match command to host")) |
There was a problem hiding this comment.
should we surface the commandUUID in all the errors we return here for potential debugging purposes?
| const selectStmt = ` | ||
| SELECT host_uuid, expected_device_name | ||
| FROM host_mdm_apple_device_names | ||
| WHERE command_uuid = ?` | ||
| if err := sqlx.GetContext(ctx, tx, &row, selectStmt, commandUUID); err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return staleErr() | ||
| } | ||
| return ctxerr.Wrap(ctx, err, "get host device name enforcement by command") | ||
| } | ||
|
|
||
| const updateStmt = ` | ||
| UPDATE host_mdm_apple_device_names | ||
| SET status = ?, detail = ? | ||
| WHERE command_uuid = ?` | ||
| res, err := tx.ExecContext(ctx, updateStmt, status, detail, commandUUID) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "update host device name status from command") | ||
| } | ||
| // The UPDATE is authoritative: if the row's command_uuid changed between | ||
| // the SELECT and here (a concurrent supersede), no row matches and the | ||
| // SELECTed host/expected values are stale — treat as not-found. |
There was a problem hiding this comment.
Do we actually need to perform both queries?
I think that if we just kept the update statement and return staleErr when affected == 0 it'd be equivalent, correct?
| return nil | ||
| } | ||
|
|
||
| func (ds *Datastore) UpdateHostDeviceNameStatusFromCommand(ctx context.Context, commandUUID string, status fleet.MDMDeliveryStatus, detail string) (hostUUID string, expectedName string, err error) { |
There was a problem hiding this comment.
nit: maybe we could return a struct instead of hostUUID, expectedName, err? Like (hostDeviceNameEnforcement HostDeviceNameEnforcement, err error). And in the caller we just pull out whatever values we need from that struct
Perhaps we could even reuse that for the arguments this function receives, so it could be something like (ctx context.Context, hostDeviceNameArgs HostDeviceNameEnforcement)
Maybe this could also be applied to other functions in this file.
There was a problem hiding this comment.
I'm inclined to keep this as it is, and see if the refactoring make sense when implementing the service layer.
| return hostUUID, expectedName, err | ||
| } | ||
|
|
||
| func (ds *Datastore) VerifyHostDeviceName(ctx context.Context, hostUUID, reportedName string) error { |
There was a problem hiding this comment.
where would the reportedName come from?
There was a problem hiding this comment.
From the osquery ingestion logic, added a comment clarifying where it comes from.
| type HostDeviceNamePending struct { | ||
| HostID uint `db:"host_id"` | ||
| HostUUID string `db:"host_uuid"` | ||
| HardwareSerial string `db:"hardware_serial"` | ||
| Platform string `db:"platform"` | ||
| // ComputerName is the host's current name in Fleet; the cron uses it to skip | ||
| // sending a command when the device already matches the resolved name. | ||
| ComputerName string `db:"computer_name"` | ||
| TeamID *uint `db:"team_id"` | ||
| } |
There was a problem hiding this comment.
is there a chance we just reuse the Host struct?
There was a problem hiding this comment.
I'm not a big fan of having sparsely populated struct unless there's a good reason for it (like method reuse), the Host struct is huge, using it here will mean having a bunch of nil properties. I'll keep this for now and see we can refactor later once the service layer is implemented.
| return &enforcement, nil | ||
| } | ||
|
|
||
| func (ds *Datastore) ResendHostDeviceName(ctx context.Context, hostUUID string) error { |
There was a problem hiding this comment.
nit: should this be renamed to reflect the fact that we're just resetting the status to null? The fact that this is used to "resend" something (I guess the command to apply a new name to a device) could be surfaced maybe in the service layer / cron workers layer explicitly
| // ResendHostDeviceName resets a host's enforcement row to a NULL status so the | ||
| // cron re-enqueues the Settings/DeviceName command on its next run. | ||
| ResendHostDeviceName(ctx context.Context, hostUUID string) error |
There was a problem hiding this comment.
Asking because I'm not fully familiar: this method sets status=NULL and leaves the commandUUID untouched. If I follow correctly "resending" would mean to send the DeviceName command a second time... is the commandUUID the same in that case, or would we have to also modify commandUUID when we switch to status=NULL?
There was a problem hiding this comment.
Good catch, this should also set the commandUUID to null
juan-fdz-hawa
merged commit 815c4a3
into
38806-macos-iosipados-rename-hosts
juan-fdz-hawa
deleted the
48620-rename-hosts-enforcement-state-migrations-and-datastore-methods
branch
juan-fdz-hawa
restored the
48620-rename-hosts-enforcement-state-migrations-and-datastore-methods
branch
juan-fdz-hawa
deleted the
48620-rename-hosts-enforcement-state-migrations-and-datastore-methods
branch
2 participants
Resolves #48620
Ships the datastore layer for host name templates (macOS, iOS, iPadOS) with no user-visible behavior. Adds the host_mdm_apple_device_names enforcement-state table and every Datastore method the rest of the feature consumes.
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
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Bug Fixes