Renew android certificates backend #37959
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #37959 +/- ##
==========================================
+ Coverage 65.81% 65.84% +0.02%
==========================================
Files 2386 2389 +3
Lines 190086 190315 +229
Branches 8323 8323
==========================================
+ Hits 125108 125305 +197
- Misses 53597 53620 +23
- Partials 11381 11390 +9
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request adds Android certificate template renewal functionality to Fleet's server. A new cron job initiates renewal for Android certificate templates nearing expiration. Certificate validity metadata (not_valid_before, not_valid_after, serial) is now tracked and updated through the status flow. Database schema expands with validity columns and renewal queries, while service and datastore layers support passing validity data during certificate updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Cron Scheduler
participant AndroidSvc as Android Service
participant DS as Datastore
participant DB as Database
participant Device as Device/App
rect rgb(240, 248, 255)
Note over Cron,Device: Certificate Renewal Initiation
Cron->>AndroidSvc: RenewCertificateTemplates()
AndroidSvc->>DS: GetAndroidCertificateTemplatesForRenewal(limit: 1000)
DS->>DB: SELECT templates WHERE not_valid_after < NOW() + 30 days
DB-->>DS: []HostCertificateTemplateForRenewal
DS-->>AndroidSvc: templates list
end
alt Templates found
rect rgb(240, 255, 240)
Note over AndroidSvc,DB: Mark for Renewal
AndroidSvc->>DS: SetAndroidCertificateTemplatesForRenewal(templates)
DS->>DB: UPDATE status='pending',<br/>CLEAR validity fields,<br/>GENERATE new UUID
DB-->>DS: success
DS-->>AndroidSvc: success
AndroidSvc->>AndroidSvc: log count processed
end
else No templates
AndroidSvc->>AndroidSvc: return
end
rect rgb(255, 250, 240)
Note over AndroidSvc,Device: Device Certificate Update
Device->>Device: Poll for config/certificate
Device->>Device: Detect new UUID
Device->>Device: Request new certificate
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 0
🧹 Nitpick comments (4)
server/datastore/mysql/host_certificate_templates_test.go (1)
639-639: Consider adding a test case for the new validity parameter.The
nilvalue correctly tests backwards compatibility, but it would be valuable to add a test case that validates the new validity parameter with actual data to ensure it's correctly stored and retrieved.server/datastore/mysql/migrations/tables/20260106000000_AddValidityColumnsToHostCertificateTemplates_test.go (1)
10-163: Migration test thoroughly exercises new validity columns and renewal query shapeThis test does a nice job of:
- Verifying NULL defaults on existing rows post‑migration.
- Inserting and updating rows with
not_valid_before,not_valid_after, andserial, with tolerant time assertions.- Exercising the “expiring within 30 days” query using
not_valid_after,status IN ('delivered','verified'), andoperation_type = 'install', which keeps the result set well‑scoped as expected for renewal.No issues detected; this is solid coverage for the migration behavior.
As per coding guidelines, your SQL filtering here is already appropriately constrained; if the production renewal query evolves, mirroring it here will keep this test aligned.
server/datastore/mysql/migrations/tables/20260106000000_AddValidityColumnsToHostCertificateTemplates.go (1)
13-47: Migration structure is correct.The columns and index are appropriate for the renewal feature. Based on learnings, the empty
Downmigration is intentional in the Fleet codebase.Consider combining ALTER statements (optional).
For large tables, multiple
ALTER TABLEstatements can be slower than a single combined statement. This is a minor optimization opportunity.♻️ Optional: Combine ALTER statements
func Up_20260106000000(tx *sql.Tx) error { - // Add not_valid_before column for certificate validity tracking - if _, err := tx.Exec(` - ALTER TABLE host_certificate_templates - ADD COLUMN not_valid_before DATETIME(6) NULL - `); err != nil { - return errors.Wrap(err, "adding not_valid_before column to host_certificate_templates") - } - - // Add not_valid_after column for certificate expiration tracking (used for renewal) - if _, err := tx.Exec(` - ALTER TABLE host_certificate_templates - ADD COLUMN not_valid_after DATETIME(6) NULL - `); err != nil { - return errors.Wrap(err, "adding not_valid_after column to host_certificate_templates") - } - - // Add serial column to store certificate serial number - if _, err := tx.Exec(` - ALTER TABLE host_certificate_templates - ADD COLUMN serial VARCHAR(40) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL - `); err != nil { - return errors.Wrap(err, "adding serial column to host_certificate_templates") - } - - // Add index on not_valid_after for efficient renewal queries - if _, err := tx.Exec(` - ALTER TABLE host_certificate_templates - ADD INDEX idx_host_certificate_templates_not_valid_after (not_valid_after) - `); err != nil { - return errors.Wrap(err, "adding index on not_valid_after to host_certificate_templates") - } + if _, err := tx.Exec(` + ALTER TABLE host_certificate_templates + ADD COLUMN not_valid_before DATETIME(6) NULL, + ADD COLUMN not_valid_after DATETIME(6) NULL, + ADD COLUMN serial VARCHAR(40) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL, + ADD INDEX idx_host_certificate_templates_not_valid_after (not_valid_after) + `); err != nil { + return errors.Wrap(err, "adding validity columns and index to host_certificate_templates") + } return nil }server/datastore/mysql/host_certificate_templates.go (1)
645-683: LGTM - Renewal update implementation is correct.The batch update correctly:
- Generates a new UUID per row (MySQL evaluates
UUID()for each affected row)- Clears validity fields to prepare for fresh certificate data
- Sets status to 'pending' to trigger re-delivery
The tuple-based WHERE clause ensures precise targeting of specific host/template combinations.
Optional consideration: Adding a status filter (
AND status IN ('delivered', 'verified')) in the WHERE clause would add defense-in-depth against race conditions, though the risk is minimal given the cron execution pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmd/fleet/cron.goserver/datastore/mysql/host_certificate_templates.goserver/datastore/mysql/host_certificate_templates_test.goserver/datastore/mysql/migrations/tables/20260106000000_AddValidityColumnsToHostCertificateTemplates.goserver/datastore/mysql/migrations/tables/20260106000000_AddValidityColumnsToHostCertificateTemplates_test.goserver/datastore/mysql/schema.sqlserver/fleet/datastore.goserver/fleet/host_certificate_template.goserver/fleet/service.goserver/mdm/android/service/certificate_renewal.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/certificates.goserver/service/integration_android_certificate_templates_test.goserver/service/integration_mdm_test.go
🧰 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/service/integration_mdm_test.goserver/mock/service/service_mock.goserver/mdm/android/service/certificate_renewal.goserver/service/certificates.goserver/datastore/mysql/migrations/tables/20260106000000_AddValidityColumnsToHostCertificateTemplates_test.goserver/fleet/host_certificate_template.gocmd/fleet/cron.goserver/fleet/service.goserver/datastore/mysql/host_certificate_templates_test.goserver/fleet/datastore.goserver/service/integration_android_certificate_templates_test.goserver/datastore/mysql/migrations/tables/20260106000000_AddValidityColumnsToHostCertificateTemplates.goserver/datastore/mysql/host_certificate_templates.goserver/mock/datastore_mock.go
🧠 Learnings (12)
📓 Common learnings
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36139
File: android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt:75-76
Timestamp: 2025-11-26T18:58:18.865Z
Learning: In Fleet's Android MDM agent SCEP implementation (android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt), OptimisticCertificateVerifier is intentionally used because: (1) SCEP URL is provided by authenticated MDM server, (2) challenge password authenticates enrollment, (3) enterprise SCEP servers use internal CAs not in system trust stores, (4) enrolled certificate is validated when used.
📚 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/service/integration_mdm_test.goserver/service/integration_android_certificate_templates_test.go
📚 Learning: 2025-12-11T23:02:52.191Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36978
File: server/mdm/android/service/profiles.go:441-449
Timestamp: 2025-12-11T23:02:52.191Z
Learning: In server/mdm/android/service, Go allows assigning an interface value of type fleet.Datastore to a field ds of type fleet.Datastore because the underlying dynamic type implements both the embedded AndroidDatastore (via fleet.Datastore embedding AndroidDatastore). When reviewing code in profiles.go and related files, verify assignments between interfaces respect embedding and ensure the concrete type actually implements all required interfaces. This pattern is valid wherever fleet.Datastore embeds AndroidDatastore and the receiver expects fleetDS or similar fields; prefer keeping interface boundaries clear and rely on the dynamic type to satisfy both interfaces.
Applied to files:
server/mdm/android/service/certificate_renewal.go
📚 Learning: 2026-01-02T22:48:09.865Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 37640
File: android/app/src/main/java/com/fleetdm/agent/ApiClient.kt:518-522
Timestamp: 2026-01-02T22:48:09.865Z
Learning: In the Fleet Android app's SCEP proxy URL format (android/app/src/main/java/com/fleetdm/agent/ApiClient.kt), the `g` prefix before the certificate ID (e.g., `g$id`) is intentional and stands for "Google Android" to differentiate from other platforms like Apple and Windows.
Applied to files:
server/fleet/host_certificate_template.go
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/fleet/host_certificate_template.go
📚 Learning: 2025-12-11T23:02:59.284Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36978
File: server/mdm/android/service/profiles.go:441-449
Timestamp: 2025-12-11T23:02:59.284Z
Learning: Fleet’s server/mdm/android/service.Service has fields ds (type fleet.AndroidDatastore) and fleetDS (type fleet.Datastore). The fleet.Datastore interface embeds AndroidDatastore (server/fleet/datastore.go), so assigning r.DS (a fleet.Datastore) to Service.ds is valid in Go due to interface-to-interface assignment using the dynamic type.
Applied to files:
server/fleet/datastore.goserver/mock/datastore_mock.go
📚 Learning: 2025-07-07T22:21:15.748Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: server/datastore/mysql/schema.sql:501-517
Timestamp: 2025-07-07T22:21:15.748Z
Learning: In the host_identity_scep_certificates table schema, the VARBINARY(100) size for public_key_raw, the nullable host_id without a foreign key constraint, and the use of plain DATETIME instead of DATETIME(6) are intentional design decisions, not issues to be addressed.
Applied to files:
server/datastore/mysql/schema.sql
📚 Learning: 2025-08-01T15:08:16.858Z
Learnt from: sgress454
Repo: fleetdm/fleet PR: 31508
File: server/datastore/mysql/schema.sql:102-116
Timestamp: 2025-08-01T15:08:16.858Z
Learning: The schema.sql file in server/datastore/mysql/ is auto-generated from migrations for use with tests, so it cannot be manually edited. Any changes must be made through migrations.
Applied to files:
server/datastore/mysql/schema.sql
📚 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/service/integration_android_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/service/integration_android_certificate_templates_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/service/integration_android_certificate_templates_test.go
📚 Learning: 2025-07-08T16:13:39.114Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go:53-55
Timestamp: 2025-07-08T16:13:39.114Z
Learning: In the Fleet codebase, Down migration functions are intentionally left empty/no-op. The team does not implement rollback functionality for database migrations, so empty Down_* functions in migration files are correct and should not be flagged as issues.
Applied to files:
server/datastore/mysql/migrations/tables/20260106000000_AddValidityColumnsToHostCertificateTemplates.go
🧬 Code graph analysis (11)
server/service/integration_mdm_test.go (2)
server/service/schedule/schedule.go (1)
WithJob(124-131)server/mdm/android/service/certificate_renewal.go (1)
RenewCertificateTemplates(14-28)
server/mock/service/service_mock.go (2)
server/fleet/mdm.go (1)
MDMDeliveryStatus(466-466)server/fleet/host_certificate_template.go (1)
HostCertificateValidity(59-63)
server/mdm/android/service/certificate_renewal.go (1)
server/contexts/ctxerr/ctxerr.go (1)
Wrap(186-189)
server/service/certificates.go (3)
server/ptr/ptr.go (1)
Time(51-53)server/fleet/host_certificate_template.go (1)
HostCertificateValidity(59-63)server/fleet/mdm.go (1)
MDMDeliveryStatus(466-466)
server/datastore/mysql/migrations/tables/20260106000000_AddValidityColumnsToHostCertificateTemplates_test.go (1)
server/ptr/ptr.go (2)
T(86-88)Time(51-53)
server/fleet/host_certificate_template.go (1)
server/ptr/ptr.go (1)
Time(51-53)
cmd/fleet/cron.go (2)
server/service/schedule/schedule.go (1)
WithJob(124-131)server/mdm/android/service/certificate_renewal.go (1)
RenewCertificateTemplates(14-28)
server/fleet/service.go (3)
cmd/fleetctl/fleetctl/config.go (1)
Context(24-32)server/fleet/mdm.go (1)
MDMDeliveryStatus(466-466)server/fleet/host_certificate_template.go (1)
HostCertificateValidity(59-63)
server/fleet/datastore.go (2)
server/fleet/mdm.go (2)
MDMDeliveryStatus(466-466)MDMOperationType(528-528)server/fleet/host_certificate_template.go (2)
HostCertificateValidity(59-63)HostCertificateTemplateForRenewal(66-70)
server/datastore/mysql/host_certificate_templates.go (2)
server/fleet/host_certificate_template.go (2)
HostCertificateValidity(59-63)HostCertificateTemplateForRenewal(66-70)server/fleet/certificate_templates.go (3)
CertificateTemplateDelivered(53-53)CertificateTemplateVerified(55-55)CertificateTemplatePending(51-51)
server/mock/datastore_mock.go (2)
server/fleet/mdm.go (2)
MDMDeliveryStatus(466-466)MDMOperationType(528-528)server/fleet/host_certificate_template.go (2)
HostCertificateValidity(59-63)HostCertificateTemplateForRenewal(66-70)
🔇 Additional comments (25)
server/datastore/mysql/host_certificate_templates_test.go (3)
40-41: LGTM!The new test cases are properly registered and follow the existing test suite conventions.
1516-1644: LGTM!Comprehensive test coverage for the renewal query with well-chosen scenarios:
- Different validity periods (1 year vs. 14 days)
- Different expiration timeframes
- Status filtering (pending vs. delivered/verified)
- Limit/pagination
- Ordering by urgency (earliest expiration first)
The assertions are thorough and validate the renewal logic correctly.
1646-1777: LGTM!Excellent test coverage for the renewal operation. The test validates all critical aspects:
- Status transition to pending
- UUID regeneration (essential for triggering device re-enrollment)
- Clearing of validity fields (not_valid_before, not_valid_after, serial)
- Empty slice handling for robustness
The helper function
insertHostCertTemplateis clean and reusable for other tests.server/service/integration_mdm_test.go (1)
386-394: LGTM!The new
renew_android_certificate_templatesscheduled job follows the establishedWithJobpattern and correctly integrates with the cleanup schedule. The placement ofonCleanupScheduleDonein the renewal job ensures both Android cleanup jobs complete before signaling done, which is appropriate for test synchronization.cmd/fleet/cron.go (1)
992-994: Android renewal cron job is correctly wired into the cleanup scheduleThe new
renew_android_certificate_templatesjob follows the existing renewal pattern (RenewSCEPCertificates,RenewMDMManagedCertificates), passes the right dependencies, and is placed appropriately innewCleanupsAndAggregationSchedule. No issues spotted.server/mock/service/service_mock.go (1)
410-410: Mock UpdateCertificateStatus correctly extended to carry validityThe mock’s function type and method now include
validity *fleet.HostCertificateValidityand correctly forward all parameters while preserving the invoked flag behavior. This keeps the mock aligned with the updated service/datastore API and ensures any stale test stubs will be caught at compile time.Also applies to: 3519-3523
server/mdm/android/service/certificate_renewal.go (1)
12-27: RenewCertificateTemplates is a clean, datastore‑driven renewal helperThe function cleanly delegates selection and update logic to the datastore, uses a reasonable batch size, handles empty/results and errors correctly, and logs only when work is performed. This is a good fit for being called from cron.
server/service/integration_android_certificate_templates_test.go (1)
1033-1238: LGTM! Comprehensive renewal test coverage.The test covers the key renewal scenarios well:
- Long-lived certificates with 30-day threshold
- Short-lived certificates with half-validity threshold
- Boundary conditions
The table-driven approach with clear descriptions makes the test cases easy to understand and maintain.
server/fleet/service.go (1)
653-653: Interface signature update is appropriate.The new
validity *HostCertificateValidityparameter is correctly placed as the last parameter and follows the nullable pointer pattern for optional data.server/fleet/host_certificate_template.go (2)
20-22: Field additions are consistent with migration schema.The new fields correctly map to the database columns added by the migration.
56-70: New types are well-structured.
HostCertificateValiditycorrectly uses pointer types for optional/nullable fieldsHostCertificateTemplateForRenewalcorrectly uses non-pointertime.TimeforNotValidAftersince the renewal query only returns rows where this value existsserver/service/certificates.go (3)
590-595: Validity object construction is correct.The validity object is properly constructed from the request fields and passed to the service method. Downstream code will correctly handle the case where all fields are nil.
603-659: Service method implementation is correct.The implementation properly:
- Validates status and operation type
- Handles the remove+verified case by deleting the record
- Only accepts updates for "delivered" certificates
- Propagates validity to the datastore layer
240-244: Correctly passes nil validity for failed certificates.When a certificate fails due to variable replacement issues, there's no validity data to store, so passing
nilis appropriate.server/datastore/mysql/schema.sql (3)
521-523: LGTM: Certificate validity columns follow existing schema patterns.The new
not_valid_before,not_valid_after, andserialcolumns are consistent with similar certificate management tables (host_mdm_managed_certificatesuses the same data types). Thedatetime(6)precision provides microsecond accuracy for certificate lifecycle tracking, and nullable defaults are appropriate for backward compatibility with existing records.
527-527: LGTM: Index supports efficient certificate renewal queries.The index on
not_valid_afteris well-positioned to optimize the renewal cron job's queries for certificates approaching expiration. This follows the pattern used inhost_certificates(line 558) which has a similar index for certificate expiration tracking.
1752-1754: LGTM: Migration metadata correctly updated.The AUTO_INCREMENT value and migration status entry properly reflect the new migration
20260106000000_AddValidityColumnsToHostCertificateTemplates.Note: Per learnings, this schema.sql file is auto-generated from migrations, so these changes are the expected result of running the migration.
server/fleet/datastore.go (1)
2560-2562: LGTM - Interface changes are well-defined.The new
validity *HostCertificateValidityparameter is appropriately optional (pointer type), maintaining backward compatibility. The new Android certificate renewal methods have clear documentation explaining the renewal threshold logic, which mirrors the existing Apple/Windows behavior.Also applies to: 2625-2635
server/datastore/mysql/host_certificate_templates.go (2)
251-329: LGTM - UpsertCertificateStatus handles validity correctly.The conditional logic properly handles both the update and insert paths:
- When validity is provided with at least one field, the validity columns are included in the update/insert
- When validity is nil or empty, only the core status fields are updated
The parameterization is correct and the column/placeholder counts match.
607-643: LGTM - Renewal query logic is sound.The query correctly implements the documented renewal threshold logic:
- Certificates with >30 day validity period renew within 30 days of expiration
- Certificates with ≤30 day validity period renew within half the period
The filtering is appropriately scoped (status, operation_type, NOT NULL checks) and results are ordered by expiration urgency. The DATEDIFF integer division is acceptable here as it provides conservative renewal timing.
server/mock/datastore_mock.go (5)
1670-1670: UpsertCertificateStatus mock signature correctly extended with validityThe additional
validity *fleet.HostCertificateValidityparameter keeps the mock type aligned with the datastore interface and downstream usage. No issues.
1714-1716: New Android renewal func types are consistent with model types
GetAndroidCertificateTemplatesForRenewalFunc/SetAndroidCertificateTemplatesForRenewalFuncusefleet.HostCertificateTemplateForRenewalas expected and follow existing naming and typing conventions for this mock.
4259-4263: DataStore fields for Android renewal funcs wired like existing mocksThe new func fields and
...Invokedbooleans for Android certificate template renewal mirror the established pattern across the struct and will integrate cleanly with tests that assert invocation.
10034-10038: UpsertCertificateStatus method correctly propagates validity argumentThe method signature and the delegated call both include the new
validityparameter, matchingUpsertCertificateStatusFuncand ensuring the mock can exercise validity-aware behavior in tests.
10188-10200: Android certificate renewal methods follow standard mock patternBoth
GetAndroidCertificateTemplatesForRenewalandSetAndroidCertificateTemplatesForRenewal:
- Take the correct parameters (
limit/templates),- Mark their
...Invokedflags under the shared mutex,- Delegate to the corresponding func fields.
This is consistent with the rest of the generated mock and should behave as expected in tests.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Still reviewing, but see comments so far.
| // Build update statement - include validity fields if provided | ||
| var updateStmt string | ||
| var updateParams []any | ||
| if validity != nil && (validity.NotValidBefore != nil || validity.NotValidAfter != nil || validity.Serial != nil) { |
There was a problem hiding this comment.
This condition seems somewhat difficult to understand what exactly is it trying to check. Would doing just validity != nil make sense? Or validity.isPresent() method?
There was a problem hiding this comment.
this was overkill, i refactored this out
| } | ||
|
|
||
| func testGetAndroidCertificateTemplatesForRenewal(t *testing.T, ds *Datastore) { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Nit. We can use t.Context() now instead.
| ) | ||
| ORDER BY not_valid_after ASC | ||
| LIMIT ? | ||
| `, fleet.CertificateTemplateDelivered, fleet.CertificateTemplateVerified, fleet.MDMOperationTypeInstall) |
There was a problem hiding this comment.
Why do we need to check CertificateTemplateDelivered here?
There was a problem hiding this comment.
great catch, i removed that filter
| DetailUpdatedAt: time.Now(), | ||
| LabelUpdatedAt: time.Now(), | ||
| PolicyUpdatedAt: time.Now(), | ||
| SeenTime: time.Now(), |
There was a problem hiding this comment.
Nit. Do we need to set all these fields? Other tests in this file don't set all of these when creating new hosts.
There was a problem hiding this comment.
no, i replaced with a test helper
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good overall. No major issues. We should fix the DB types before merging.
| // Add not_valid_before column for certificate validity tracking | ||
| if _, err := tx.Exec(` | ||
| ALTER TABLE host_certificate_templates | ||
| ADD COLUMN not_valid_before DATETIME(6) NULL |
There was a problem hiding this comment.
I like DATETIME(6), but in this case, the cert valid times have a max precision of one second, per: https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.5.1
There was a problem hiding this comment.
good catch, is this also an issue in host_mdm_managed_certificates?
There was a problem hiding this comment.
Yes, it is also an issue there, an may have been my fault :(
| // Add index on not_valid_after for efficient renewal queries | ||
| if _, err := tx.Exec(` | ||
| ALTER TABLE host_certificate_templates | ||
| ADD INDEX idx_host_certificate_templates_not_valid_after (not_valid_after) |
There was a problem hiding this comment.
We're using DATEDIFF, so I'm not sure if this index will be helpful.
There was a problem hiding this comment.
good point, tho i think it's still partially useful for ORDER BY not_valid_after ASC
Co-authored-by: Victor Lyuboslavsky <[email protected]>
Related issue: Resolves #37566
Large PR, so I think the easiest way to review is by commit history.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.