Install Fleet android agent on device enrollment. - #36050
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes implement automated Fleet Agent installation on Android devices during MDM enrollment. A new struct carries host UUID and enroll secret data through the policy application pipeline, which is integrated into the software worker to apply Fleet Agent policies to host configurations. Changes
Sequence DiagramsequenceDiagram
participant Worker as Software Worker
participant AndroidSvc as Android Service
participant MDMApi as MDM API
Worker->>AndroidSvc: Fetch Android Host
AndroidSvc-->>Worker: Host Data
Worker->>AndroidSvc: Get Team Enroll Secrets
AndroidSvc-->>Worker: Secrets List
Note over Worker: Validate ≥1 Secret
Worker->>AndroidSvc: AddFleetAgentToAndroidPolicy<br/>(hostConfigs with UUID, Secret)
loop Per-Host Policy
AndroidSvc->>AndroidSvc: Marshal ManagedConfig to JSON
AndroidSvc->>MDMApi: EnterprisesPoliciesModifyPolicyApplications<br/>(FORCE_INSTALLED, CERT_INSTALL)
MDMApi-->>AndroidSvc: Policy Applied
end
AndroidSvc-->>Worker: Errors (if any, joined)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/worker/software_worker.go (1)
168-168: Verify androidHost.DeviceID is non-nil before constructing device name.The code constructs
deviceNameusingandroidHost.DeviceID, but there's no nil check. IfDeviceorDeviceIDis nil/empty, this will construct an invalid device name.Apply this diff to add defensive checks:
+ if androidHost.Device == nil || androidHost.Device.DeviceID == "" { + return ctxerr.Errorf(ctx, "android host %s has no device ID", hostUUID) + } device := &androidmanagement.Device{ PolicyName: policyName, - // State must be specified when updating a device, otherwise it fails with - // "Illegal state transition from ACTIVE to DEVICE_STATE_UNSPECIFIED" - // - // > Note that when calling enterprises.devices.patch, ACTIVE and - // > DISABLED are the only allowable values. - // TODO(ap): should we send whatever the previous state was? If it was DISABLED, - // we probably don't want to re-enable it by accident. Those are the only - // 2 valid states when patching a device. State: "ACTIVE", } deviceName := fmt.Sprintf("%s/devices/%s", enterpriseName, androidHost.DeviceID)
🧹 Nitpick comments (1)
server/mdm/android/service/service.go (1)
902-902: Consider logging when the agent package is not configured.The method silently returns when
FLEET_DEV_ANDROID_AGENT_PACKAGEis not set. This could make debugging difficult when the agent isn't installed as expected.Apply this diff to add debug logging:
var errs []error - if packageName := os.Getenv("FLEET_DEV_ANDROID_AGENT_PACKAGE"); packageName != "" { + packageName := os.Getenv("FLEET_DEV_ANDROID_AGENT_PACKAGE") + if packageName == "" { + level.Debug(svc.logger).Log("msg", "skipping fleet agent installation: FLEET_DEV_ANDROID_AGENT_PACKAGE not set") + return nil + } + { for uuid, managedConfig := range hostConfigs {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/mdm/android/android.go(1 hunks)server/mdm/android/service.go(1 hunks)server/mdm/android/service/service.go(1 hunks)server/worker/software_worker.go(2 hunks)tools/android/android.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/mdm/android/service.goserver/mdm/android/android.goserver/mdm/android/service/service.gotools/android/android.goserver/worker/software_worker.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
Applied to files:
server/mdm/android/android.goserver/mdm/android/service/service.gotools/android/android.goserver/worker/software_worker.go
🔇 Additional comments (7)
server/mdm/android/android.go (1)
52-55: LGTM!The
AgentManagedConfigurationstruct is well-defined with appropriate JSON tags for serialization.tools/android/android.go (2)
45-48: LGTM!The enterprise ID normalization is a helpful addition that makes the tool more user-friendly.
168-194: LGTM for development tooling.The hardcoded
ParentFrameUrlon line 174 is acceptable since this is development tooling in thetools/directory.server/mdm/android/service.go (1)
26-26: LGTM!The interface method signature correctly matches the implementation in
service/service.go.server/worker/software_worker.go (1)
107-111: LGTM!Good optimization to fetch the Android host once at the start, avoiding redundant database calls.
server/mdm/android/service/service.go (2)
895-928: Assumption is valid and consistently applied throughout the Android MDM flow.The host UUID to policy ID assumption holds true. The code in
profiles.goexplicitly documents: "using the host uuid as policy id, so we don't need to track the id mapping to the host." The caller insoftware_worker.gopasseshostUUIDas the map key when invokingAddFleetAgentToAndroidPolicy, and the implementation directly uses the map key (uuid) as the policyID in the policy name construction. This pattern is consistently applied across the Android MDM service methods.
913-918: No format issues identified—the code correctly marshals the managed configuration.The verification confirms the implementation is correct:
- The
ManagedConfigurationfield is aRawMessagetype, which stores raw JSON bytes- The API expects a JSON object with string keys mapping to typed values
json.Marshal(managedConfig)produces valid JSON:{"hostUUID": "...", "enrollSecret": "..."}, which matches the required format- The
AgentManagedConfigurationstruct is properly defined with JSON tags for serializationThe code correctly marshals and passes the managed configuration to the Google Android Management API in the expected format.
| // Get enroll secrets for the host's team (nil means global/no team) | ||
| enrollSecrets, err := v.Datastore.GetEnrollSecrets(ctx, androidHost.Host.TeamID) | ||
| if err != nil { | ||
| return ctxerr.Wrapf(ctx, err, "get enroll secrets for team %v", androidHost.Host.TeamID) | ||
| } | ||
| if len(enrollSecrets) == 0 { | ||
| return ctxerr.Errorf(ctx, "no enroll secrets found for team %v", androidHost.Host.TeamID) | ||
| } | ||
| // Use the first enroll secret | ||
| enrollSecret := enrollSecrets[0].Secret | ||
| err = v.AndroidModule.AddFleetAgentToAndroidPolicy(ctx, enterpriseName, map[string]android.AgentManagedConfiguration{ | ||
| hostUUID: { | ||
| HostUUID: hostUUID, | ||
| EnrollSecret: enrollSecret, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return ctxerr.Wrapf(ctx, err, "add fleet agent to android policy for host %s", hostUUID) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify enroll secret selection logic and error handling.
Two concerns:
-
Line 141: The code returns an error if no enroll secrets exist. Consider whether this should be a fatal error or if the flow should continue without agent installation (since the agent is still in development based on the env var gate).
-
Line 144: The code always uses the first enroll secret. Verify this is the intended behavior, especially for teams with multiple secrets.
Run the following script to check how enroll secrets are typically selected in similar flows:
🏁 Script executed:
#!/bin/bash
# Description: Find patterns of enroll secret selection in the codebase
# Search for other locations that retrieve and use enroll secrets
rg -nP --type=go -C3 'GetEnrollSecrets.*\[0\]|enrollSecrets\[0\]' -g '!vendor/**'
# Search for any documented preference or ordering logic
rg -nP --type=go -C3 'enroll.*secret.*first|select.*enroll.*secret' -g '!vendor/**'Length of output: 1098
I need to gather more information to verify these concerns thoroughly. Let me search for additional context:
Let me search the codebase directly for more context:
Now let me search the codebase directly for the GetEnrollSecrets implementation and android context:
🏁 Script executed:
#!/bin/bash
# Find GetEnrollSecrets function implementation
# Search for the GetEnrollSecrets method definition
rg -nP --type=go 'func.*GetEnrollSecrets' -g '!vendor/**'
# Also search for any interface definition
rg -nP --type=go 'GetEnrollSecrets.*\(' -g '!vendor/**' -B2 -A10 | head -100Length of output: 7523
🏁 Script executed:
#!/bin/bash
# Search for Android agent environment variables and feature gates
rg -nP --type=go -i 'android|ENABLE_ANDROID' -g '!vendor/**' -C2 | head -150Length of output: 8163
🏁 Script executed:
#!/bin/bash
# Look for how errors are typically handled when lists are empty in this codebase
# and see if line 141 error is consistent with codebase patterns
rg -nP --type=go 'len.*==.*0.*return.*Err' -g '!vendor/**' | head -30Length of output: 0
Let me check the GetEnrollSecrets implementation and search for any feature gating:
🏁 Script executed:
#!/bin/bash
# Check the actual GetEnrollSecrets implementation
cat server/datastore/mysql/app_configs.go | sed -n '231,280p'Length of output: 1657
🏁 Script executed:
#!/bin/bash
# Search for any environment variable or configuration gate for android feature in software_worker.go
rg -nP --type=go -B20 'makeAndroidAppsAvailableForHost|makeAndroidAppAvailable' server/worker/software_worker.go | head -80Length of output: 2918
🏁 Script executed:
#!/bin/bash
# Check if there's any gate before calling makeAndroidAppsAvailableForHostTask
rg -nP --type=go 'enqueue.*android|makeAndroidApps' -B5 -A5 -g '!vendor/**' | head -100Length of output: 4003
Address undefined enroll secret selection and inconsistent error handling.
-
Line 141 (error handling): The code fails hard if no enroll secrets exist, but this pattern is inconsistent across the codebase. In
server/service/microsoft_mdm.go:1358, missing secrets are logged as a warning and execution continues. Decide whether this should halt processing or continue gracefully for the Android agent flow. -
Line 144 (secret selection):
GetEnrollSecrets(inserver/datastore/mysql/app_configs.go:231) has noORDER BYclause, soenrollSecrets[0]returns an arbitrary/undefined secret, not a deterministic "first" one. The separateAggregateEnrollSecretPerTeammethod explicitly usesORDER BY created_at DESCto select the most recent secret. Either:- Order
GetEnrollSecretsresults bycreated_at DESCto match the intended behavior, OR - Select a secret using explicit logic (e.g., call
AggregateEnrollSecretPerTeamor filter results after retrieval)
- Order
🤖 Prompt for AI Agents
In server/worker/software_worker.go around lines 135 to 153, the code currently
returns an error if no enroll secrets are found and naively uses
enrollSecrets[0] which is non-deterministic; change handling to match the
project convention used in server/service/microsoft_mdm.go:1358 (log a warning
and continue rather than hard-failing) or explicitly decide to abort (but be
consistent across codebase); also make secret selection deterministic by either
calling AggregateEnrollSecretPerTeam to obtain the most recent secret or by
ensuring GetEnrollSecrets returns results ordered by created_at DESC (or
sort/filter the returned slice to pick the newest) and then use that chosen
secret when calling AddFleetAgentToAndroidPolicy; update the error paths to
log/wrap consistently with surrounding code.
There was a problem hiding this comment.
Interesting, but non-urgent. Noted in follow up issue: #36052
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #36050 +/- ##
==========================================
+ Coverage 65.98% 66.00% +0.01%
==========================================
Files 2122 2122
Lines 180706 180636 -70
Branches 7456 7491 +35
==========================================
- Hits 119243 119220 -23
+ Misses 50561 50512 -49
- Partials 10902 10904 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| AppliedPolicyVersion *int64 `db:"applied_policy_version"` | ||
| } | ||
|
|
||
| type AgentManagedConfiguration struct { |
There was a problem hiding this comment.
We also need to send the fleet base URL
2 participants
Related issue: Resolves #35434
Feature is largely behind feature flag
FLEET_DEV_ANDROID_AGENT_PACKAGESet it like:
export FLEET_DEV_ANDROID_AGENT_PACKAGE=com.fleetdm.agent.private.victorRough set up:
build.gradle.kts:defaultConfig { applicationId = "com.fleetdm.agent.private.you"go run tools/android/android.go --command enterprises.webTokens.create --enterprise_id 'XXXX'@ksykulev you can use this Android service method for "notification":
AddFleetAgentToAndroidPolicy(ctx context.Context, enterpriseName string, hostConfigs map[string]AgentManagedConfiguration) errorYou'll need to update
AgentManagedConfigurationstruct to define what to send down to the device. It includes the enroll secret, so I think we need to send it down every time just to be safe.Checklist for submitter
Testing