Added entry parameter for preserving certificate Tags #64
Added entry parameter for preserving certificate Tags #64spbsoluble merged 8 commits intorelease-3.1from
Conversation
joevanwanzeeleKF
changed the title
Removed duplicate entry for bug fix in version 3.1.10.
Bug fix for government cloud host name resolution
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to preserve existing certificate tags in Azure Key Vault when certificates are replaced. The primary purpose is to allow users to maintain existing tags on certificates during re-enrollment or replacement operations while optionally merging them with new tags provided during the operation.
Key changes include:
- Added
PreserveExistingTagsentry parameter to control tag preservation behavior - Updated certificate import logic to merge existing and new tags when preservation is enabled
- Refactored tag handling to use Dictionary objects instead of JSON strings internally
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-manifest.json | Added new PreserveExistingTags boolean entry parameter definition |
| README.md | Updated documentation to include the new entry parameter and reorganized content structure |
| CHANGELOG.md | Added version 3.1.9 entry documenting the new feature and bug fix |
| AzureKeyVault/Jobs/Management.cs | Implemented tag preservation logic in certificate addition workflow |
| AzureKeyVault/Jobs/Inventory.cs | Minor logging improvement using Count property instead of Count() method |
| AzureKeyVault/Jobs/AzureKeyVaultJob.cs | Code formatting and logging improvements for Azure cloud configuration |
| AzureKeyVault/Constants.cs | Added constants for entry parameter names |
| AzureKeyVault/AzureClient.cs | Refactored certificate import method to accept Dictionary instead of JSON string for tags |
| AzureKeyVault/AkvProperties.cs | Updated cloud endpoint handling and made VaultURL property public |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| config.JobProperties.TryGetValue(EntryParameters.PRESERVE_TAGS, out preserveTagsObj); | ||
|
|
||
| preserveTags = (bool)preserveTagsObj; |
There was a problem hiding this comment.
Direct casting to bool can throw InvalidCastException if preserveTagsObj is null or not a boolean. Use safe casting with as bool? and provide a default value, or check for null first.
| preserveTags = (bool)preserveTagsObj; | |
| preserveTags = preserveTagsObj as bool? ?? false; |
| { | ||
| var existingTags = new Dictionary<string, string>(); | ||
| logger.LogTrace($"checking for an existing cert with the alias {alias}"); | ||
| var existing = AzClient.GetCertificate(alias).Result; |
There was a problem hiding this comment.
Using .Result on async operations can cause deadlocks and blocks the thread. Consider making this method async and using await, or use GetAwaiter().GetResult() if async conversion is not possible.
| logger.LogTrace($"there is an existing cert.."); | ||
| } | ||
|
|
||
| existingTags = existing?.Properties.Tags as Dictionary<string, string> ?? new Dictionary<string, string>(); |
There was a problem hiding this comment.
The cast as Dictionary<string, string> may fail silently if Properties.Tags is not actually a Dictionary<string, string>. Consider using a safer conversion method or checking the type first.
| existingTags = existing?.Properties.Tags as Dictionary<string, string> ?? new Dictionary<string, string>(); | |
| if (existing?.Properties.Tags is IDictionary<string, string> tagsDict) | |
| { | |
| existingTags = new Dictionary<string, string>(tagsDict); | |
| } | |
| else | |
| { | |
| existingTags = new Dictionary<string, string>(); | |
| } |
| } | ||
|
|
||
| public virtual async Task<KeyVaultCertificateWithPolicy> ImportCertificateAsync(string certName, string contents, string pfxPassword, string tags = null) | ||
| public virtual async Task<KeyVaultCertificateWithPolicy> ImportCertificateAsync(string certName, string contents, string pfxPassword, Dictionary<string,string> tags) |
There was a problem hiding this comment.
The method signature change from string tags = null to Dictionary<string,string> tags is a breaking change. Consider adding method overloads to maintain backward compatibility or make the parameter nullable with Dictionary<string,string>? tags = null.
Added optional entry parameter to indicate that existing Tags in KeyVault should be preserved if certificate is replaced.