Notify android devices of certificate_template changes - #36258
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #36258 +/- ##
==========================================
+ Coverage 65.99% 66.01% +0.02%
==========================================
Files 2136 2138 +2
Lines 182057 182213 +156
Branches 7594 7489 -105
==========================================
+ Hits 120155 120296 +141
+ Misses 50913 50907 -6
- Partials 10989 11010 +21
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:
|
42308df to
8c86672
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds infrastructure to deliver certificate templates to MDM-enrolled Android devices. Introduces new datastore methods to list Android hosts awaiting certificate templates, bulk-insert host-template associations, and update delivery status. Extends certificate template response types with delivery status fields and integrates certificate template reconciliation into the Android profile synchronization workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant RP as ReconcileProfiles
participant CT as reconcileCertificateTemplates
participant DS as Datastore
participant PCB as processCertificateTemplateBatch
participant PP as AddFleetAgentToAndroidPolicy
participant DB as Database
RP->>CT: reconcileCertificateTemplates(ctx)
Note over CT: Process in batches of 1000
CT->>DS: ListAndroidHostUUIDsWithCertificateTemplates(offset, limit)
DS->>DB: Query Android hosts lacking cert templates
DB-->>DS: Return host UUIDs
DS-->>CT: Return batch of host UUIDs
CT->>DS: ListCertificateTemplatesForHosts(hostUUIDs)
DS->>DB: Query templates for hosts
DB-->>DS: Return templates with status/challenges
DS-->>CT: Return CertificateTemplateForHost records
CT->>PCB: processCertificateTemplateBatch(templates)
Note over PCB: Detect new certs (no FleetChallenge)
PCB->>PCB: Generate FleetChallenge for new certs
PCB->>PCB: Group templates by host
PCB->>PP: AddFleetAgentToAndroidPolicy(policy, config)
Note over PP: Config includes CertificateTemplateIDs
PP-->>PCB: Policy updated
PCB->>DS: BulkInsertHostCertificateTemplates(records)
DS->>DB: Insert host_certificate_templates rows
DB-->>DS: Rows inserted
DS-->>PCB: Success
PCB-->>CT: Batch complete
CT->>CT: Move to next batch if more hosts
CT-->>RP: All batches processed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 4
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/certificate_templates.go (1)
14-36: Query may return non-deterministic results with multiple host associations.The
LEFT JOIN host_certificate_templatescan return multiple rows when a certificate template is associated with multiple hosts. Sincesqlx.GetContextexpects exactly one row, this will either fail or return an arbitrary row.If the intent is to fetch template details without host-specific data, consider removing the join. If host-specific data is needed, the query should filter by
host_uuidor useLIMIT 1with an explicit ordering.LEFT JOIN host_certificate_templates ON host_certificate_templates.certificate_template_id = certificate_templates.id - WHERE certificate_templates.id = ? + WHERE certificate_templates.id = ? + LIMIT 1Or if this endpoint should not include host-specific status:
- LEFT JOIN host_certificate_templates - ON host_certificate_templates.certificate_template_id = certificate_templates.id WHERE certificate_templates.id = ?As per coding guidelines, ensure queries return precise results when intended for a specific entity.
🧹 Nitpick comments (7)
server/mdm/android/service/profiles_test.go (1)
888-890: Consider usingt.Setenvfor cleaner environment variable handling.Go's
testing.T.Setenvautomatically restores the original value when the test completes, eliminating the need for manual defer cleanup.- oldEnvValue := os.Getenv("FLEET_DEV_ANDROID_AGENT_PACKAGE") - os.Setenv("FLEET_DEV_ANDROID_AGENT_PACKAGE", "com.fleetdm.agent") - defer os.Setenv("FLEET_DEV_ANDROID_AGENT_PACKAGE", oldEnvValue) + t.Setenv("FLEET_DEV_ANDROID_AGENT_PACKAGE", "com.fleetdm.agent")This also handles the case where
Setenvfails (it will fail the test automatically).server/mdm/android/service/profiles.go (2)
590-602: N+1 query pattern: Consider batching database calls.For each host in
hostsNeedingUpdate, two separate database calls are made (AndroidHostLiteByHostUUIDandGetEnrollSecrets). With many hosts, this could significantly impact performance.Consider fetching all required host data and enroll secrets in batch queries before the loop.
619-625: Consider extracting Service instantiation.Creating a new
Servicestruct inline to callAddFleetAgentToAndroidPolicyis unusual. IfprofileReconcileralready has access tor.DSandr.Client, consider either:
- Adding the method directly to
profileReconciler- Injecting the service at construction time
server/fleet/host_certificate_template.go (1)
9-10: Consider usingtime.Timefor timestamp fields.
CreatedAtandUpdatedAtare defined asstringbut typically timestamp fields usetime.Timefor type safety and easier manipulation. If the database returns these as strings and you want to avoid parsing, this is acceptable, but consider whethertime.Timewould be more consistent with other types in the codebase.server/datastore/mysql/host_certificate_templates_test.go (2)
518-519: Variable shadowing:uuidshadows the imported package.The variable
uuidshadows the importeduuidpackage, which could cause confusion.- nodeKey := uuid.New().String() - uuid := uuid.New().String() + nodeKey := uuid.New().String() + hostUUID := uuid.New().String()Then update references from
uuidtohostUUIDthroughout the function.
278-278: Closure variable shared across test cases.
templateWithHostRecordIdis declared at the function scope and modified in thebeforeclosure of one test case, then read in thetestFuncclosure. While this works because tests run sequentially withTruncateTablescleanup, it's somewhat fragile. Consider scoping this variable within the specific test case or using a different pattern.server/datastore/mysql/host_certificate_templates.go (1)
73-105: Bulk insert is safe; consider behavior on duplicates/racesThe multi-row
INSERTis parameterized correctly and avoids injection risk, and the early return on empty input is good. If there is (or will be) a unique constraint on(host_uuid, certificate_template_id), be aware this will fail the whole batch on duplicates (e.g., if a reconcile job races with itself); considerINSERT ... ON DUPLICATE KEY UPDATEorINSERT IGNOREif idempotent behavior is desired.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
changes/35465-send-certificate-templates(1 hunks)server/datastore/mysql/certificate_templates.go(2 hunks)server/datastore/mysql/certificate_templates_test.go(1 hunks)server/datastore/mysql/host_certificate_templates.go(1 hunks)server/datastore/mysql/host_certificate_templates_test.go(1 hunks)server/fleet/certificate_templates.go(1 hunks)server/fleet/datastore.go(1 hunks)server/fleet/host_certificate_template.go(1 hunks)server/mdm/android/android.go(1 hunks)server/mdm/android/service/profiles.go(2 hunks)server/mdm/android/service/profiles_test.go(3 hunks)server/mock/datastore_mock.go(3 hunks)server/service/integration_core_test.go(1 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/fleet/host_certificate_template.goserver/datastore/mysql/certificate_templates.goserver/mdm/android/service/profiles.goserver/datastore/mysql/host_certificate_templates.goserver/mdm/android/android.goserver/service/integration_core_test.goserver/datastore/mysql/host_certificate_templates_test.goserver/datastore/mysql/certificate_templates_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/mdm/android/service/profiles_test.goserver/fleet/certificate_templates.go
🧠 Learnings (9)
📚 Learning: 2025-07-08T16:06:54.576Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
Applied to files:
server/datastore/mysql/certificate_templates.goserver/datastore/mysql/certificate_templates_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/datastore/mysql/certificate_templates.goserver/datastore/mysql/host_certificate_templates_test.goserver/datastore/mysql/certificate_templates_test.goserver/mdm/android/service/profiles_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/datastore/mysql/certificate_templates.goserver/datastore/mysql/certificate_templates_test.goserver/mdm/android/service/profiles_test.go
📚 Learning: 2025-08-13T18:20:42.136Z
Learnt from: titanous
Repo: fleetdm/fleet PR: 31075
File: tools/redis-tests/elasticache/iam_auth.go:4-10
Timestamp: 2025-08-13T18:20:42.136Z
Learning: For test harnesses and CLI tools in the Fleet codebase, resource cleanup on error paths (like closing connections before log.Fatalf) may not be necessary since the OS handles cleanup when the process exits. These tools prioritize simplicity over defensive programming patterns used in production code.
Applied to files:
server/datastore/mysql/certificate_templates.goserver/datastore/mysql/certificate_templates_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet, tests may validly use testing.T.Context() when the module/toolchain targets Go 1.24+. Do not flag t.Context() usage in this codebase if go.mod/toolchain indicates Go >= 1.24.
Applied to files:
server/datastore/mysql/certificate_templates.goserver/mdm/android/service/profiles_test.go
📚 Learning: 2025-07-08T16:12:48.797Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Applied to files:
server/datastore/mysql/certificate_templates.go
📚 Learning: 2025-08-22T01:14:05.454Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 32173
File: server/datastore/mysql/policies.go:360-365
Timestamp: 2025-08-22T01:14:05.454Z
Learning: The sqlx library in Go can handle pointer types as query parameters and will automatically dereference them when passing to the underlying database/sql driver. Unlike raw database/sql which doesn't accept pointer types, sqlx intelligently handles both pointer and non-pointer types for query arguments.
Applied to files:
server/datastore/mysql/certificate_templates.go
📚 Learning: 2025-11-25T00:42:41.117Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36233
File: android/app/src/main/java/com/fleetdm/agent/RoleNotificationReceiverService.kt:18-23
Timestamp: 2025-11-25T00:42:41.117Z
Learning: For Android Management API NotificationReceiverService integration, the SERVICE_APP_ROLES meta-data must have an empty string value (android:value="") in the AndroidManifest.xml. The actual app roles (e.g., COMPANION_APP) are assigned via ApplicationPolicy.roles in the MDM policy payload, not in the manifest meta-data.
Applied to files:
changes/35465-send-certificate-templates
📚 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/datastore/mysql/certificate_templates_test.go
🧬 Code graph analysis (10)
server/fleet/host_certificate_template.go (1)
server/fleet/mdm.go (1)
MDMDeliveryStatus(445-445)
server/datastore/mysql/certificate_templates.go (3)
server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)server/fleet/mdm.go (1)
MDMDeliveryPending(486-486)server/ptr/ptr.go (1)
String(10-12)
server/mdm/android/service/profiles.go (5)
server/contexts/ctxerr/ctxerr.go (2)
Wrap(199-202)Wrapf(211-214)server/fleet/host_certificate_template.go (1)
HostCertificateTemplate(3-11)server/fleet/mdm.go (1)
MDMDeliveryPending(486-486)server/mdm/android/android.go (2)
AgentManagedConfiguration(52-57)AgentCertificateTemplate(59-61)server/fleet/hosts.go (1)
Host(276-405)
server/datastore/mysql/host_certificate_templates.go (3)
server/fleet/datastore.go (1)
Datastore(50-2524)server/fleet/host_certificate_template.go (2)
CertificateTemplateForHost(13-18)HostCertificateTemplate(3-11)server/fleet/mdm.go (1)
MDMDeliveryStatus(445-445)
server/mdm/android/android.go (1)
server/fleet/app.go (1)
EnrollSecret(1367-1375)
server/datastore/mysql/host_certificate_templates_test.go (5)
server/datastore/mysql/testing_utils.go (2)
CreateMySQLDS(406-408)ExecAdhocSQL(420-424)server/fleet/datastore.go (1)
Datastore(50-2524)server/fleet/certificate_templates.go (1)
CertificateTemplate(10-15)server/fleet/mdm.go (3)
MDMDeliveryPending(486-486)MDMDeliveryVerified(484-484)MDMDeliveryStatus(445-445)server/fleet/host_certificate_template.go (1)
HostCertificateTemplate(3-11)
server/datastore/mysql/certificate_templates_test.go (3)
server/ptr/ptr.go (2)
String(10-12)T(76-78)server/fleet/certificate_templates.go (1)
CertificateTemplate(10-15)server/fleet/mdm.go (2)
MDMDeliveryPending(486-486)MDMDeliveryVerifying(485-485)
server/fleet/datastore.go (1)
server/fleet/host_certificate_template.go (2)
CertificateTemplateForHost(13-18)HostCertificateTemplate(3-11)
server/mock/datastore_mock.go (1)
server/fleet/host_certificate_template.go (2)
CertificateTemplateForHost(13-18)HostCertificateTemplate(3-11)
server/fleet/certificate_templates.go (1)
server/fleet/mdm.go (1)
MDMDeliveryStatus(445-445)
🔇 Additional comments (20)
server/service/integration_core_test.go (1)
14677-14683: LGTM — formatting-only SQL changePlaceholders and explicit columns keep the insert safe and deterministic for the test. No functional impact.
server/fleet/certificate_templates.go (1)
29-38: LGTM! Struct extensions for certificate template delivery.The new fields properly support the certificate delivery workflow:
Statusas a pointer correctly handles nullable database valuesSCEPChallengeEncryptedis appropriately excluded from JSON serialization withjson:"-"- Proper use of
MDMDeliveryStatustype fromserver/fleet/mdm.gochanges/35465-send-certificate-templates (1)
1-1: LGTM! Clear changelog entry.The message accurately describes the user-visible change.
server/datastore/mysql/certificate_templates.go (1)
38-50: Verify intentional nullification ofFleetChallengefrom query result.The else branch explicitly sets
FleetChallengeto nil, overwriting any value retrieved from the database. This appears intentional for security (hiding challenges when not pending), but verify this is the desired behavior sinceFleetChallengeis populated from the query result at line 28.server/datastore/mysql/certificate_templates_test.go (2)
203-266: Good test coverage for pending status with challenge decryption.The test properly verifies:
- Status is
MDMDeliveryPendingFleetChallengecontains the expected valueSCEPChallengeis decrypted from the CA's encrypted challenge
267-330: Good test coverage for verifying status with cleared challenges.The test correctly verifies that when status is
MDMDeliveryVerifying, bothFleetChallengeandSCEPChallengeare nil, confirming the post-processing logic in the datastore.server/mdm/android/service/profiles_test.go (2)
797-870: Good test setup for certificate template reconciliation.The test properly:
- Creates necessary entities (team, CA, templates, hosts)
- Pre-populates
host_certificate_templatesfor host2 to test exclusion logic- Sets up app config with server URL
892-948: Thorough reconciliation and idempotency testing.The test correctly verifies:
- API calls target the expected host (host1, not host2)
- Managed configuration contains the expected certificate template IDs
- Database records are created with pending status and fleet challenges
- Re-running reconciliation doesn't create duplicates or trigger unnecessary API calls
server/mdm/android/android.go (2)
53-57: Breaking change: JSON field tags renamed from camelCase to snake_case.The JSON tags changed from
serverURL/hostUUID/enrollSecrettoserver_url/host_uuid/enroll_secret. If any Android client or agent already consumes this configuration with the old field names, this will break deserialization on those clients.Please verify that the Android agent code is updated simultaneously to expect snake_case, or that no deployed agents rely on the previous format.
59-61: LGTM!The new
AgentCertificateTemplatetype is appropriately minimal for carrying template identifiers to the Android agent.server/mdm/android/service/profiles.go (2)
78-80: LGTM!Certificate template reconciliation is appropriately placed before profile application, ensuring templates are processed first.
532-538: Verify pagination logic doesn't miss records.The pagination relies on a stable ordering and no concurrent modifications. If hosts are added or removed between batches, some hosts could be missed or processed twice. Given that this runs in a cron job context, verify this is acceptable for the use case.
server/fleet/host_certificate_template.go (1)
13-18: LGTM!Using pointer types for
FleetChallengeandStatusappropriately handles NULL values from LEFT JOIN queries.server/fleet/datastore.go (1)
2513-2518: LGTM!The new datastore methods follow existing interface patterns and are well-documented with concise comments describing their purpose.
server/datastore/mysql/host_certificate_templates_test.go (1)
36-273: Good test coverage for various scenarios.The test cases cover key scenarios: hosts with no templates, hosts with existing templates, non-Android platforms, and hosts not enrolled in MDM. The use of
TruncateTablesensures proper test isolation.server/datastore/mysql/host_certificate_templates.go (2)
13-38: Android host UUID query and pagination look soundThe query correctly scopes to Android, MDM-enrolled hosts that are missing corresponding
host_certificate_templatesrows and orders withLIMIT/OFFSET, so the pagination will be stable and deterministic.
40-71: Host certificate template listing is consistent with data modelEmpty input is handled early, the
INclause is correctly parameterized over host UUIDs, and the selected columns matchfleet.CertificateTemplateForHost(including nullablefleet_challengeandstatuswhen no row exists), so this read path looks correct.server/mock/datastore_mock.go (3)
1630-1634: New mock function types are consistent and correctly typedThe three new function types follow the existing conventions (context first, specific args, typed return values) and align with the HostCertificateTemplate / CertificateTemplateForHost types and pagination needs. No issues from a mock API or typing perspective.
4047-4054: Struct fields integrate cleanly with existing invocation-tracking patternThe added DataStore fields mirror the established pattern: one Func and one
FuncInvokedbool per method, with naming consistent with the type aliases. This keeps the mock configuration and call tracking coherent with the rest of the file.
9679-9698: Mock methods correctly wrap and delegate to underlying funcsEach new method:
- Locks only long enough to set the
*Invokedflag.- Calls the corresponding
*Funcoutside the lock.- Preserves the intended signatures and argument order.
This matches the pattern used throughout the file and should work as expected in tests.
| for _, c := range cases { | ||
| t.Helper() | ||
| t.Run(c.name, func(t *testing.T) { | ||
| c.fn(t, ds) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Misplaced t.Helper() call.
t.Helper() is called inside the loop but outside the subtest closure. It should either be removed (since this isn't a helper function) or moved inside the closure if intended to mark the subtest as a helper.
for _, c := range cases {
- t.Helper()
t.Run(c.name, func(t *testing.T) {
c.fn(t, ds)
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, c := range cases { | |
| t.Helper() | |
| t.Run(c.name, func(t *testing.T) { | |
| c.fn(t, ds) | |
| }) | |
| } | |
| for _, c := range cases { | |
| t.Run(c.name, func(t *testing.T) { | |
| c.fn(t, ds) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In server/datastore/mysql/host_certificate_templates_test.go around lines 28 to
33, t.Helper() is called in the for-loop scope instead of inside the subtest
closure (or should be removed); either delete the t.Helper() call if this test
function is not a helper, or move the t.Helper() call inside the t.Run closure
as the first line (so the subtest is marked as a helper) to correct its
placement.
| func testUpdateHostCertificateTemplateStatus(t *testing.T, ds *Datastore) { | ||
| db := CreateMySQLDS(t) | ||
| nodeKey := uuid.New().String() |
There was a problem hiding this comment.
Inconsistent datastore usage: creates new datastore instead of using passed parameter.
This test function creates a new datastore (db := CreateMySQLDS(t)) instead of using the ds parameter passed from the parent test. This is inconsistent with the other test functions and means ds goes unused.
func testUpdateHostCertificateTemplateStatus(t *testing.T, ds *Datastore) {
- db := CreateMySQLDS(t)
+ db := ds
nodeKey := uuid.New().String()Alternatively, if a separate datastore is intentional for isolation, the function signature should be updated to not accept ds.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func testUpdateHostCertificateTemplateStatus(t *testing.T, ds *Datastore) { | |
| db := CreateMySQLDS(t) | |
| nodeKey := uuid.New().String() | |
| func testUpdateHostCertificateTemplateStatus(t *testing.T, ds *Datastore) { | |
| db := ds | |
| nodeKey := uuid.New().String() |
🤖 Prompt for AI Agents
In server/datastore/mysql/host_certificate_templates_test.go around lines
516-518, the test function declares a new datastore with db := CreateMySQLDS(t)
while it receives a ds parameter (making ds unused) — either remove the ds
parameter if an isolated datastore is intentional, or delete the CreateMySQLDS
call and use the passed-in ds for all datastore operations; update the function
signature and any callers accordingly to keep usage consistent with other tests.
| func (ds *Datastore) UpdateCertificateStatus(ctx context.Context, hostUUID string, certificateTemplateID uint, status fleet.MDMDeliveryStatus) error { | ||
| // Validate the status. | ||
| if !status.IsValid() { | ||
| return ctxerr.Wrap(ctx, fmt.Errorf("Invalid status '%s'", string(status))) | ||
| } | ||
|
|
||
| // Attempt to update the certificate status for the given host and template. | ||
| result, err := ds.writer(ctx).ExecContext(ctx, ` | ||
| UPDATE host_certificate_templates | ||
| SET status = ? | ||
| WHERE host_uuid = ? AND certificate_template_id = ? | ||
| `, status, hostUUID, certificateTemplateID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| rowsAffected, err := result.RowsAffected() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if rowsAffected == 0 { | ||
| return ctxerr.Wrap(ctx, notFound("Label").WithMessage(fmt.Sprintf("No certificate found for host UUID '%s' and template ID '%d'", hostUUID, certificateTemplateID))) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Fix resource name in not-found error and align error wrapping
Two small issues here:
- The not-found case uses
notFound("Label")even though this is updatinghost_certificate_templates, which will surface the wrong resource type to callers/metrics. - Errors from
ExecContext/RowsAffectedare returned bare, whereas the rest of this file wraps datastore errors withctxerr.Wrapfor context.
You can address both with a small tweak like:
func (ds *Datastore) UpdateCertificateStatus(ctx context.Context, hostUUID string, certificateTemplateID uint, status fleet.MDMDeliveryStatus) error {
// Validate the status.
if !status.IsValid() {
- return ctxerr.Wrap(ctx, fmt.Errorf("Invalid status '%s'", string(status)))
+ return ctxerr.Wrap(ctx, fmt.Errorf("invalid status '%s'", string(status)))
}
// Attempt to update the certificate status for the given host and template.
- result, err := ds.writer(ctx).ExecContext(ctx, `
+ result, err := ds.writer(ctx).ExecContext(ctx, `
UPDATE host_certificate_templates
SET status = ?
WHERE host_uuid = ? AND certificate_template_id = ?
-`, status, hostUUID, certificateTemplateID)
+`, status, hostUUID, certificateTemplateID)
if err != nil {
- return err
+ return ctxerr.Wrap(ctx, err, "update certificate status")
}
rowsAffected, err := result.RowsAffected()
if err != nil {
- return err
+ return ctxerr.Wrap(ctx, err, "rows affected updating certificate status")
}
if rowsAffected == 0 {
- return ctxerr.Wrap(ctx, notFound("Label").WithMessage(fmt.Sprintf("No certificate found for host UUID '%s' and template ID '%d'", hostUUID, certificateTemplateID)))
+ return ctxerr.Wrap(ctx, notFound("CertificateTemplate").
+ WithMessage(fmt.Sprintf("No certificate found for host UUID '%s' and template ID '%d'", hostUUID, certificateTemplateID)))
}
return nil
}(Adjust the "CertificateTemplate" resource string to match whatever you use elsewhere for this table.)
🤖 Prompt for AI Agents
In server/datastore/mysql/host_certificate_templates.go around lines 107 to 133,
the not-found error uses notFound("Label") and raw errors from
ExecContext/RowsAffected are returned unwrapped; change the resource string to
notFound("CertificateTemplate") (or the canonical resource name used elsewhere
for this table) and wrap the ExecContext and RowsAffected error returns with
ctxerr.Wrap(ctx, err) so all datastore errors are consistently context-wrapped.
| if err := svc.AddFleetAgentToAndroidPolicy(ctx, r.Enterprise.Name(), hostConfigs); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "add fleet agent to android policy with certificates") | ||
| } | ||
|
|
||
| // For new certificates | ||
| if err := r.DS.BulkInsertHostCertificateTemplates(ctx, newCertificates); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "bulk insert host certificate templates") | ||
| } |
There was a problem hiding this comment.
Potential data inconsistency if bulk insert fails after policy update.
If AddFleetAgentToAndroidPolicy succeeds but BulkInsertHostCertificateTemplates fails, the Android devices will receive certificate template IDs but Fleet won't have the corresponding host_certificate_templates records. This could lead to devices attempting to use templates that Fleet doesn't recognize.
Consider wrapping these operations in a transaction or implementing compensating logic on failure.
🤖 Prompt for AI Agents
In server/mdm/android/service/profiles.go around lines 623 to 630, the code
updates the Android policy first then does a BulkInsertHostCertificateTemplates
which can fail and leave devices referencing templates that don't exist; modify
this flow so both operations are atomic: either (preferred) use a datastore
transaction that wraps AddFleetAgentToAndroidPolicy and
BulkInsertHostCertificateTemplates (begin transaction via r.DS, call both ops
using the transactional handle, commit on success, rollback on error), or if a
transaction is not available, implement compensating logic that on
BulkInsertHostCertificateTemplates failure calls a rollback API to remove the
fleet agent from the Android policy (e.g., RemoveFleetAgentFromAndroidPolicy)
and returns the original error; ensure errors from rollback are logged/combined
and that the function only returns after the system state is consistent.
There was a problem hiding this comment.
If the BulkInsertHostCertificateTemplates fails, the android device will get the managedConfiguration with the certificate_template id. They will go and hit /api/fleetd/certificates/123, this will return the certificate template response without a status, and therefore without a SCEPChallenge or FleetChallenge. The next time the cron job runs, it will insert the host_certificate_template row. The android device hitting /api/fleetd/certificates/123 will get a pending status with a SCEPChallenge or FleetChallenge.
If we swap the order around and insert the host_certificate_template row first, and the AMAPI fails. The logic which only sends new certificate templates (templates with no host_certificate_template row) will ignore this template, and the android device won't get the AMAPI call. We cannot modify the logic that selects which templates to include a pending status, because it will continue sending AMAPI requests until the android device goes through the flow.
We COULD minimize this a bit by adding another status, call it "delivering" or "sending" that happens before "pending". And have the device update to "pending" when it reaches out to /api/fleetd/certificates/123. But even in that case there is a chance we will send AMAPI requests to the device multiple times.
@mostlikelee @dantecatalfamo , I think we will need to handle the failure case on the application, where we make a request to /api/fleetd/certificates/123 and if it doesn't contain a status/secrets we will have to ignore the request, and wait for another AMAPI request to come in to retry.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good overall. Had a couple comments.
| return nil, ctxerr.Wrap(ctx, err, "getting certificate_template by id") | ||
| } | ||
|
|
||
| if template.Status != nil && *template.Status == fleet.MDMDeliveryPending { |
| ) | ||
|
|
||
| // ListAndroidHostUUIDsWithCertificateTemplates returns a batch of host UUIDs that have certificate templates to deliver | ||
| func (ds *Datastore) ListAndroidHostUUIDsWithCertificateTemplates(ctx context.Context, offset int, limit int) ([]string, error) { |
There was a problem hiding this comment.
The name WithCertificateTemplates doesn't seem right, since the query explicitly looking for ones without: host_certificate_templates.id IS NULL
There was a problem hiding this comment.
🤔
ListAndroidHostUUIDsWithPendingCertificateTemplates
or maybe..
ListAndroidHostUUIDsWithDeliverableCertificateTemplates
| } | ||
|
|
||
| func testListAndroidHostUUIDsWithCertificateTemplates(t *testing.T, ds *Datastore) { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Nit. We can use t.Context() now.
| } | ||
|
|
||
| // For new certificates | ||
| if err := r.DS.BulkInsertHostCertificateTemplates(ctx, newCertificates); err != nil { |
There was a problem hiding this comment.
Feels like we should update DB before we send managed configs updates to Android hosts. If we're updating 1K policies, this could take a while. So the first host could try to get its info, but it may not be in DB yet, right?
There was a problem hiding this comment.
I replied on the AI comment why I don't think that's a good idea.
| host_certificate_templates.id IS NULL | ||
| ORDER BY hosts.uuid | ||
| LIMIT ? OFFSET ? | ||
| ` |
There was a problem hiding this comment.
If gitops deletes a certificate template, we need to return the host uuid in the list of hosts with pending manageConfiguration changes.
There was a problem hiding this comment.
If we don't delete the host_certificate_template row associated with the certificate_template, we could modify the query here to surface those hosts.
SELECT uuid
FROM (
(
SELECT DISTINCT
hosts.uuid AS uuid
FROM certificate_templates
INNER JOIN hosts ON hosts.team_id = certificate_templates.team_id
INNER JOIN host_mdm ON host_mdm.host_id = hosts.id
LEFT JOIN host_certificate_templates
ON host_certificate_templates.host_uuid = hosts.uuid
AND host_certificate_templates.certificate_template_id = certificate_templates.id
WHERE
hosts.platform = 'android' AND
host_mdm.enrolled = 1 AND
host_certificate_templates.id IS NULL
)
UNION
(
SELECT DISTINCT
host_certificate_templates.host_uuid AS uuid
FROM host_certificate_templates
LEFT JOIN certificate_templates
ON certificate_templates.id = host_certificate_templates.certificate_template_id
WHERE
certificate_templates.id IS NULL
)
) AS combined
ORDER BY uuid
LIMIT ? OFFSET ?
however, this means that we won't be able to track deletions properly. Profiles have an MDMOperationType:
type MDMOperationType string
const (
MDMOperationTypeInstall MDMOperationType = "install"
MDMOperationTypeRemove MDMOperationType = "remove"
)
We should most likely implement something similar with certificates.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Changes looks good since my last review. I see there are some merge conflicts that need resolving.
| var stmt string | ||
| var args []interface{} | ||
|
|
||
| if teamID != nil && *teamID > 0 { |
There was a problem hiding this comment.
Nit. Recommend for clarity:
| GROUP BY 1` | |
| GROUP BY hct.status` |
| FROM host_certificate_templates hct | ||
| INNER JOIN certificate_templates ct ON hct.certificate_template_id = ct.id | ||
| WHERE ct.team_id = ? | ||
| GROUP BY 1` |
There was a problem hiding this comment.
Nit. Recommend for clarity
| GROUP BY 1` | |
| GROUP BY hct.status` |
|
|
||
| require.NoError(t, err) | ||
| require.NotNil(t, result) | ||
| require.Equal(t, uint(1), result.Pending) |
There was a problem hiding this comment.
Nit. Recommend adding a test case where there is more than 1 of a specific status.
| return ctxerr.Wrapf(ctx, err, "get android host %s", hostUUID) | ||
| } | ||
|
|
||
| enrollSecrets, err := r.DS.GetEnrollSecrets(ctx, androidHost.Host.TeamID) |
There was a problem hiding this comment.
Should we cache the enrollSecret so that we don't have to call it every time for 1000 hosts on the same team?
2 participants
Related issue: Resolves #35465
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.