Refactor/make certificate provisioning configurable#217
Conversation
|
@Hermann-Core pls resolve this conflict |
There was a problem hiding this comment.
Hello @Hermann-Core Please take a look at these comments
| manager.persist_signing_key(&signing_key_pem).await?; | ||
| info!("Store certificate material refreshed"); | ||
| Ok(certificate_data) | ||
| } |
There was a problem hiding this comment.
Issue: Cache re-population missing after store provisioning
After persisting the certificate data and signing key (lines 217-218), the certificate chain cache is not being updated. The code only persists to storage but doesn't update the in-memory cache that other parts of the system rely on.
Suggestion: Add cache update call after line 218:
manager.cache_provisioned_chain(&certificate_data.certificate).await?;This ensures the cache is invalidated and re-populated with the new certificate data.
|
|
||
| /// Source for directly provisioned certificate material. | ||
| #[derive(Debug, Clone)] | ||
| pub enum StoreProvisioningSource { |
There was a problem hiding this comment.
Issue: Missing HashiCorp Vault Support
The ticket deliverables mention implementing
Storebackend loading from "HashiCorp Vault", but this variant is missing from theStoreProvisioningSourceenum. OnlyFilesystem,Storage, andSecretsStorageare implemented.Suggestion: Add a
HashiCorpVaultvariant or create a follow-up issue to track this missing deliverable from the ticket requirements.
| @@ -0,0 +1,256 @@ | |||
| use std::{sync::Arc, time::Duration}; | |||
|
|
|||
| use instant_acme::{Account, HttpClient}; | |||
There was a problem hiding this comment.
Issue: Unused Import
The
Accountimport frominstant_acmeis not used in this file.Suggestion: Remove the unused import:
use instant_acme::HttpClient;
|
|
||
| let cert_storage = self.cert_storage.ok_or_else(|| { | ||
| CertError::Validation("certificate storage backend must be configured".to_string()) | ||
| })?; |
There was a problem hiding this comment.
Issue: String-based Strategy Name Comparison
Using string comparison (
strategy.name() == "acme") for determining strategy type is fragile. If someone changes the name returned byAcmeProvisioningStrategy, this validation will break.Suggestion: Consider using a type-based approach or const for the strategy name:
const ACME_STRATEGY_NAME: &str = "acme"; let strategy_uses_acme = strategy.name() == ACME_STRATEGY_NAME;Or better yet, add a method to the trait:
fn is_acme(&self) -> bool { false } // default impl
| #[derive(Debug, Clone, Deserialize)] | ||
| pub struct CertConfig { | ||
| pub provisioning_strategy: String, | ||
| pub email: String, |
There was a problem hiding this comment.
Issue: No Validation for provisioning_strategy
The
provisioning_strategyfield usesStringtype without validation. This allows invalid values to be passed in.Suggestion: Consider using an enum for compile-time safety:
pub enum ProvisioningStrategy { #[serde(rename = "acme")] Acme, #[serde(rename = "store")] Store, }Or add runtime validation in
Config::load()to ensure only valid values are accepted.
| &self, | ||
| _manager: &CertManager, | ||
| _certificate: &CertificateData, | ||
| ) -> bool { |
There was a problem hiding this comment.
Issue: Store Strategy Always Returns
trueforshould_provision_existingThe
StoreProvisioningStrategy::should_provision_existingalways returnstrue, which means the store will be checked on every renewal cycle regardless of whether the certificate is still valid.Suggestion: Consider if this is the intended behavior. For store provisioning, you might want to:
- Check if the file/storage content has changed (hash comparison)
- Or add a TTL-based check to avoid unnecessary reloads
- Or document why unconditional reloading is the desired behavior
Currently, this could cause unnecessary storage reads.
Make certificate provisioning configurable to support multiple provisioning strategies and refactor the certificate manager to use the builder pattern for construction, removing the inline startup construction.
Acceptance Criteria