-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(router): add support for azure key vault as secret manager #7330
base: main
Are you sure you want to change the base?
Conversation
Implement Azure key vault support for secret management and encryption management in external_services. This can be used to securely storing sensitive configuration settings. Fixes juspay#6181
Changed Files
|
Hi everyone, This is my first pull request to Hyperswitch. The issue is not yet fully resolved and require further guidance on resolving that. I have few doubts which I will raise in the issue comments. I would appreciate any feedback or review you may have. Created Files:
Thank you! |
/// Constructs a new Azure Key Vault client. | ||
pub async fn new(config: &AzureKeyVaultConfig) -> Result<Self, AzureKeyVaultError> { | ||
let credential = DefaultAzureCredential::new() | ||
.map_err(|_| AzureKeyVaultError::AzureKeyVaultClientInitializationFailed)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you preserve the context here? It would good if we propagate the error that actually happened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @NishantJoshi00
decrypt_params | ||
.clone() | ||
.try_into() | ||
.map_err(|_| AzureKeyVaultError::KeyOperationsParameterTypeConversionFailed)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
logger::error!(azure_key_vault_error=?error, "Failed to Azure Key Vault decrypt data"); | ||
metrics::AZURE_KEY_VAULT_ENCRYPTION_FAILURES.add(1, &[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log appropriate error here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dracarys18, Could you please clarify what you mean by “log appropriate error”? Are you suggesting that “Failed to Azure Key Vault decrypt data” is not distinct from the previously logged error?
Would it be clearer if we modified the messages to:
• “Failed while making Azure Key Vault decrypt API call”
• “Failed while parsing Azure Key Vault decrypt API response”
This way, the errors would be more specific. Let me know your thoughts. Thanks!
.inspect_err(|error| { | ||
logger::error!(azure_key_vault_error=?error, "Failed to Azure Key Vault decrypt data"); | ||
metrics::AZURE_KEY_VAULT_DECRYPTION_FAILURES.add(1, &[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log the proper error here
Type of Change
Description
Implement Azure key vault support for secret management and encryption management in external_services similar to the available AWS KMS, HashiCorp Vault, plaintext. This can be used to securely storing sensitive configuration settings.
Additional Changes
Motivation and Context
Fixes #6181
How did you test it?
Checklist
cargo +nightly fmt --all
cargo clippy