recreate certificate templates when CA or subject name change - #36772
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #36772 +/- ##
==========================================
+ Coverage 65.89% 65.91% +0.01%
==========================================
Files 2248 2248
Lines 183935 184047 +112
Branches 7647 7647
==========================================
+ Hits 121200 121308 +108
+ Misses 51660 51657 -3
- Partials 11075 11082 +7
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 PR fixes certificate template management during GitOps flows: when a certificate's subjectName or certificate authority changes, the old certificate is marked for deletion and a new one is created rather than updating the existing record. The datastore now skips updating these fields on upsert conflicts, and the service layer compares full certificate specifications to detect when deletion and recreation are required. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–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)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/fleetctl/fleetctl/gitops_test.go(5 hunks)server/datastore/mysql/certificate_templates.go(1 hunks)server/datastore/mysql/certificate_templates_test.go(1 hunks)server/service/client.go(2 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/datastore/mysql/certificate_templates.goserver/service/client.gocmd/fleetctl/fleetctl/gitops_test.goserver/datastore/mysql/certificate_templates_test.go
🧠 Learnings (1)
📚 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/service/client.go
🧬 Code graph analysis (1)
server/service/client.go (1)
server/fleet/certificate_templates.go (1)
CertificateRequestSpec(3-8)
🔇 Additional comments (8)
server/datastore/mysql/certificate_templates.go (1)
163-166: LGTM - Upsert now preserves immutable fields as intended.The
ON DUPLICATE KEY UPDATEclause now correctly excludescertificate_authority_idandsubject_name, which aligns with the PR objective to treat these as immutable fields. When either changes, the service layer will delete and recreate the certificate template rather than updating in place.server/service/client.go (1)
2920-2960: LGTM - Data structure change enables proper certificate comparison.Changing
certsToBeAddedfrommap[string]struct{}tomap[string]*fleet.CertificateRequestSpeccorrectly enables comparing the full spec (SubjectName, CertificateAuthorityId) against existing certificates to determine when recreation is needed.server/datastore/mysql/certificate_templates_test.go (1)
720-731: LGTM - Test correctly validates new upsert semantics.The test now properly verifies that
BatchUpsertCertificateTemplatespreserves the originalsubject_nameon duplicate key conflicts, which aligns with the updated upsert behavior incertificate_templates.go.cmd/fleetctl/fleetctl/gitops_test.go (5)
7-7: LGTM!The
errorsimport is correctly added to support theerrors.Newusage in the new mock functions.
3892-3950: LGTM!The test scaffolding properly tracks deleted certificate IDs and adds the
GetCertificateTemplateByIdFuncmock to return full certificate details (includingSubjectName) for comparing existing vs. new certificate specifications. The mock correctly handles the expected IDs and returns an appropriate error for unknown IDs.
4014-4076: Verify mock state consistency for the CA swap scenario.In the second test scenario (swapped CAs), the
GetCertificateTemplateByIdFuncmock still returns the original subject names ("CN=Original Subject 1" and "CN=Original Subject 2"). The test config at lines 4044-4050 uses the same original subject names but with swapped CAs. This is correct for testing CA-only changes.However, after the first test run (lines 3998-4012) deletes and recreates certificates with "CN=Updated Subject 1/2", the mock doesn't reflect this state change. The test relies on the mock returning stale data. While this works because the mock is static, consider whether this accurately simulates production behavior where the database would reflect the updated state.
If the intent is to test CA changes in isolation (without prior SubjectName changes), consider either:
- Documenting this is intentional (mock returns original state for each scenario), or
- Restructuring as separate sub-tests with isolated mock state.
4077-4130: LGTM!The no-change scenario correctly validates that certificates with unchanged SubjectName and CA are not deleted and recreated. State is properly reset before this test run, and assertions verify the expected behavior (0 deletions, 2 certificates remain).
4171-4195: LGTM!The
GetCertificateTemplateByIdFuncmock is correctly added to return full certificate details. The subject names ("CN=Device Certificate 1" and "CN=Device Certificate 2") are consistent with the test configuration, enabling proper comparison during the deletion scenario.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Related issue: Resolves #36717
When gitops runs and the yml for a certificate template includes a change to a certificate authority or a subject name for an existing certificate template. Do not update the certificate template, delete the old one and create a new one. This will aid in ensuring the new certificate is sent to the android device.
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)Testing
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.