Fix: filter out verified remove android certs in host details#37539
Fix: filter out verified remove android certs in host details#37539mostlikelee merged 4 commits intomainfrom
Conversation
mostlikelee
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37539 +/- ##
==========================================
+ Coverage 65.91% 65.92% +0.01%
==========================================
Files 2357 2357
Lines 186781 186880 +99
Branches 7862 7862
==========================================
+ Hits 123124 123208 +84
- Misses 52408 52417 +9
- Partials 11249 11255 +6
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:
|
| WHERE host_uuid = ?` | ||
| WHERE host_uuid = ? | ||
| AND NOT (status = '%s' AND operation_type = '%s')`, | ||
| fleet.CertificateTemplateVerified, fleet.MDMOperationTypeRemove) |
There was a problem hiding this comment.
Why even keep this row around? If remove is verified, can't we just delete the row?
There was a problem hiding this comment.
We indeed can, but wasn't sure if we may need it in the future for auditing purposes. Anything on the roadmap?
There was a problem hiding this comment.
I think we should delete it. We should solve auditing with activities or another table. Keeping old data in this table (and special casing this query) will have some performance impact.
There was a problem hiding this comment.
sounds good i'll update that in this PR
mostlikelee
had a problem deploying
to
Docker Hub
GitHub Actions
Error
This reverts commit 4dee423.
mostlikelee
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
| t.Run("deletes single record successfully", func(t *testing.T) { | ||
| defer TruncateTables(t, ds) | ||
| setup := createCertTemplateTestSetup(t, ctx, ds, "") | ||
|
|
||
| // Insert a host certificate template record | ||
| _, err := ds.writer(ctx).ExecContext(ctx, | ||
| "INSERT INTO host_certificate_templates (host_uuid, certificate_template_id, fleet_challenge, status, operation_type, name) VALUES (?, ?, ?, ?, ?, ?)", | ||
| "host-1", setup.template.ID, "challenge", fleet.CertificateTemplateVerified, fleet.MDMOperationTypeRemove, setup.template.Name, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify record exists | ||
| var count int | ||
| err = ds.writer(ctx).GetContext(ctx, &count, "SELECT COUNT(*) FROM host_certificate_templates WHERE host_uuid = ? AND certificate_template_id = ?", "host-1", setup.template.ID) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 1, count) | ||
|
|
||
| // Delete the record | ||
| err = ds.DeleteHostCertificateTemplate(ctx, "host-1", setup.template.ID) | ||
| require.NoError(t, err) | ||
|
|
||
| // Verify record was deleted | ||
| err = ds.writer(ctx).GetContext(ctx, &count, "SELECT COUNT(*) FROM host_certificate_templates WHERE host_uuid = ? AND certificate_template_id = ?", "host-1", setup.template.ID) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 0, count) | ||
| }) |
There was a problem hiding this comment.
Nit. I don't see much value in this test. It seems to be already covered by only deletes specified record below.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
mostlikelee
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
WalkthroughThis PR adds a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service as UpdateCertificateStatus
participant Datastore
participant Database
Client->>Service: UpdateCertificateStatus(ctx, operation_type="remove", status="verified", ...)
alt operation_type == "remove" AND status == "verified"
Service->>Datastore: DeleteHostCertificateTemplate(hostUUID, certificateTemplateID)
Datastore->>Database: DELETE FROM host_certificate_templates WHERE host_uuid=? AND certificate_template_id=?
Database-->>Datastore: Success/Error
Datastore-->>Service: Error (if any)
Service-->>Client: Return deletion result
else Other conditions
Service->>Datastore: UpsertCertificateStatus(...)
Datastore-->>Service: Result
Service-->>Client: Return result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (1)
server/service/certificates.go (1)
589-592: Delete-on-remove+verifiedpath looks correct and idempotentThe new branch cleanly short-circuits
UpdateCertificateStatusforoperation_type="remove"andstatus="verified", delegating toDeleteHostCertificateTemplate. Given the prior status/operation-type guards and the datastore’s behavior of not erroring on missing rows, this is safe and idempotent, and it aligns with the goal of hiding successfully removed certificates from host details. If you don’t already have one, a small service-level test exercising this exact combination would lock the behavior in.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/datastore/mysql/host_certificate_templates.go(1 hunks)server/datastore/mysql/host_certificate_templates_test.go(2 hunks)server/fleet/datastore.go(1 hunks)server/mock/datastore_mock.go(3 hunks)server/service/certificates.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/datastore.goserver/datastore/mysql/host_certificate_templates.goserver/service/certificates.goserver/mock/datastore_mock.goserver/datastore/mysql/host_certificate_templates_test.go
🧬 Code graph analysis (2)
server/service/certificates.go (1)
server/fleet/mdm.go (2)
MDMOperationTypeRemove(532-532)MDMDeliveryVerified(505-505)
server/datastore/mysql/host_certificate_templates_test.go (1)
server/fleet/host_certificate_template.go (1)
HostCertificateTemplate(6-17)
🔇 Additional comments (7)
server/datastore/mysql/host_certificate_templates.go (1)
179-189: Single-row delete is properly scoped and idempotentThe new
DeleteHostCertificateTemplateuses a parameterizedDELETEwith a precise(host_uuid, certificate_template_id)filter and wraps any DB error, while treating “no matching row” as success. That’s a good fit for the service’s idempotent delete-on-verify semantics and respects the SQL scoping guideline.server/fleet/datastore.go (1)
2576-2578: Interface extension for single-row delete is consistentAdding
DeleteHostCertificateTemplatealongside the existing bulk delete keeps the Datastore contract aligned with the new service behavior and mysql implementation; the comment accurately documents its purpose.server/datastore/mysql/host_certificate_templates_test.go (2)
27-27: New table-driven test entry is correctly wiredRegistering
"DeleteHostCertificateTemplate"in the cases slice ensures the new test runs underTestHostCertificateTemplateslike the rest of this suite.
478-560: Good coverage forDeleteHostCertificateTemplatesemanticsThe new
testDeleteHostCertificateTemplatesubtests exercise successful deletion, idempotent deletion of non-existent rows, and selective deletion when multiple host/template pairs exist. This matches the datastore behavior and guards against regressions in the new delete-on-verify path.server/mock/datastore_mock.go (3)
1688-1689: New DeleteHostCertificateTemplateFunc type matches existing patternsThe function type’s signature and naming are consistent with the surrounding certificate-related datastore function types and the Datastore interface. No issues here.
4200-4202: DataStore fields correctly wired for DeleteHostCertificateTemplateThe added
DeleteHostCertificateTemplateFuncandDeleteHostCertificateTemplateFuncInvokedfields follow the established convention used throughoutDataStorefor other methods, so they will integrate cleanly with existing test patterns.
10047-10052: DeleteHostCertificateTemplate mock implementation follows existing lock/delegate patternThe method acquires the mutex, flips the
DeleteHostCertificateTemplateFuncInvokedflag, releases the lock, and then delegates to the configured function. This matches the behavior of other mock methods and maintains thread-safety in the same way.
2 participants
Related issue: Resolves #37535
Updates and renames existing query used by host details
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.