-
-
Notifications
You must be signed in to change notification settings - Fork 117
Add Azure Key Vault support for Atmos Stores #963
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/store/azure_keyvault_store.go (3)
83-89
: Refine secret name normalization for consistency.Replacing
"--"
with"-"
may not handle cases with more than two consecutive dashes. Using a regular expression to replace multiple dashes ensures consistent normalization of thesecretName
.Apply this diff to enhance the handling:
- secretName = strings.ReplaceAll(secretName, "--", "-") + secretName = regexp.MustCompile("-+").ReplaceAllString(secretName, "-")
162-179
: Improve the exponential backoff strategy in retries.The current backoff increases linearly, which might not be optimal for handling transient errors. Implementing an exponential backoff can provide better performance and resource utilization during retries.
Modify the sleep duration to implement exponential backoff:
// Add exponential backoff for retries - time.Sleep(time.Duration(i+1) * time.Second) + sleepDuration := time.Duration(math.Pow(2, float64(i))) * time.Second + time.Sleep(sleepDuration)Don't forget to import the
math
package:+ "math"
125-138
: Consider adding context with timeout to theSet
method.Including a context with a timeout, similar to the
Get
method, can prevent potential hangs and improve responsiveness during secret storage operations.Apply this diff to incorporate context with timeout:
func (s *KeyVaultStore) Set(stack string, component string, key string, value interface{}) error { // Existing code... + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + _, err = s.client.SetSecret(ctx, secretName, params, nil) if err != nil { var respErr *azcore.ResponseError // Error handling...website/docs/core-concepts/projects/configuration/stores.mdx (1)
203-206
: Consider these typographical improvements:
- Use typographic quotes (") instead of straight quotes (") for the parameters.
- Use an en dash (–) instead of a hyphen (-) in the range "1-127".
Also applies to: 216-216
🧰 Tools
🪛 LanguageTool
[typographical] ~203-~203: Consider using a typographic opening quote here.
Context: ...ample, with these parameters: - prefix: "myapp" - stack: "prod-us-west" - compone...(EN_QUOTES)
[typographical] ~203-~203: Consider using a typographic close quote here.
Context: ... with these parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "d...(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic opening quote here.
Context: ... parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "database/pos...(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic close quote here.
Context: ...- prefix: "myapp" - stack: "prod-us-west" - component: "database/postgres" - key:...(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic opening quote here.
Context: ...p" - stack: "prod-us-west" - component: "database/postgres" - key: "password" Th...(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic close quote here.
Context: ...us-west" - component: "database/postgres" - key: "password" The resulting Key Va...(EN_QUOTES)
[typographical] ~206-~206: Consider using typographic quotation marks here.
Context: ...- component: "database/postgres" - key: "password" The resulting Key Vault secret name wo...(EN_QUOTES)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod
(5 hunks)pkg/store/azure_keyvault_store.go
(1 hunks)pkg/store/azure_keyvault_store_test.go
(1 hunks)pkg/store/registry.go
(1 hunks)website/docs/core-concepts/projects/configuration/stores.mdx
(2 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/projects/configuration/stores.mdx
[typographical] ~203-~203: Consider using a typographic opening quote here.
Context: ...ample, with these parameters: - prefix: "myapp" - stack: "prod-us-west" - compone...
(EN_QUOTES)
[typographical] ~203-~203: Consider using a typographic close quote here.
Context: ... with these parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "d...
(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic opening quote here.
Context: ... parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "database/pos...
(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic close quote here.
Context: ...- prefix: "myapp" - stack: "prod-us-west" - component: "database/postgres" - key:...
(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic opening quote here.
Context: ...p" - stack: "prod-us-west" - component: "database/postgres" - key: "password" Th...
(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic close quote here.
Context: ...us-west" - component: "database/postgres" - key: "password" The resulting Key Va...
(EN_QUOTES)
[typographical] ~206-~206: Consider using typographic quotation marks here.
Context: ...- component: "database/postgres" - key: "password" The resulting Key Vault secret name wo...
(EN_QUOTES)
[typographical] ~216-~216: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...dashes - Are case-insensitive - Must be 1-127 characters long The store automaticall...
(HYPHEN_TO_EN)
🪛 GitHub Actions: Dependency Review
go.mod
[error] Incompatible license detected: Package 'github.com/pkg/browser@0.0.0-20240102092130-5ac0b6a4141c' uses BSD-2-Clause license which is not compatible with project requirements
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/store/registry.go (1)
42-53
: Integration looks solid.The addition of the Azure Key Vault case in the registry is clear and follows the existing pattern.
website/docs/core-concepts/projects/configuration/stores.mdx (4)
20-21
: LGTM! Clear and concise introduction of Azure Key Vault support.The configuration example is well-structured and includes all necessary fields with proper formatting.
Also applies to: 119-131
155-170
: Well-documented authentication methods!The authentication documentation clearly outlines all supported methods and provides appropriate recommendations for different environments.
171-193
: Excellent examples that demonstrate practical usage!The examples effectively cover both basic secret retrieval and component output storage scenarios.
195-235
: Comprehensive documentation of key naming and error handling!The documentation clearly explains the key name construction rules and provides helpful error messages.
🧰 Tools
🪛 LanguageTool
[typographical] ~203-~203: Consider using a typographic opening quote here.
Context: ...ample, with these parameters: - prefix: "myapp" - stack: "prod-us-west" - compone...(EN_QUOTES)
[typographical] ~203-~203: Consider using a typographic close quote here.
Context: ... with these parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "d...(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic opening quote here.
Context: ... parameters: - prefix: "myapp" - stack: "prod-us-west" - component: "database/pos...(EN_QUOTES)
[typographical] ~204-~204: Consider using a typographic close quote here.
Context: ...- prefix: "myapp" - stack: "prod-us-west" - component: "database/postgres" - key:...(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic opening quote here.
Context: ...p" - stack: "prod-us-west" - component: "database/postgres" - key: "password" Th...(EN_QUOTES)
[typographical] ~205-~205: Consider using a typographic close quote here.
Context: ...us-west" - component: "database/postgres" - key: "password" The resulting Key Va...(EN_QUOTES)
[typographical] ~206-~206: Consider using typographic quotation marks here.
Context: ...- component: "database/postgres" - key: "password" The resulting Key Vault secret name wo...(EN_QUOTES)
[typographical] ~216-~216: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...dashes - Are case-insensitive - Must be 1-127 characters long The store automaticall...(HYPHEN_TO_EN)
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.
@jamengual thanks for taking a stab at this!
The pluggable store MUST implement the store interface in order to be used as a store. It looks like you changed the interface that it's implementing here. Could you take a shot at implementing that?
Also, please take a look at one of the other store implementations to see how we mock the calls to the underlying cloud provider in tests so we don't need to have each setup.
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/store/azure_keyvault_store_test.go (1)
112-114
:⚠️ Potential issueHandle errors returned by
getKey
functionThe
getKey
function returns a(string, error)
, but the error isn't checked in the test. Capturing and asserting the error enhances test robustness.Consider applying this change:
- result := store.getKey(tt.stack, tt.component, tt.key) - assert.Equal(t, tt.expected, result) + result, err := store.getKey(tt.stack, tt.component, tt.key) + assert.NoError(t, err) + assert.Equal(t, tt.expected, result)
🧹 Nitpick comments (3)
pkg/store/azure_keyvault_store.go (3)
77-101
: Simplify the replacement of multiple dashes in secret namesIn the
getKey
function, replacing multiple consecutive dashes usingstrings.ReplaceAll
might not handle more than two dashes effectively. Consider using a regular expression to replace any sequence of dashes with a single dash.Here's a possible improvement:
- secretName = strings.ReplaceAll(secretName, "--", "-") + re := regexp.MustCompile("-+") + secretName = re.ReplaceAllString(secretName, "-")
149-193
: Usedefer
to ensure the context is canceled properlyIn the
Get
method, callingcancel()
immediately afterGetSecret
might not cover all execution paths, especially if future modifications are made. Usingdefer cancel()
ensures the context is canceled when the function returns.Here's how you might adjust the code:
- ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()
114-118
: Enhance value type handling inSet
methodCurrently, the
Set
method only accepts string values. If there's a need to store other types, consider converting them to strings or handling different types appropriately.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/store/azure_keyvault_store.go
(1 hunks)pkg/store/azure_keyvault_store_test.go
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/store/azure_keyvault_store_test.go (1)
122-125
: Great job initializing the mock clientIn
TestKeyVaultStore_InputValidation
, initializing theclient
with a mock ensures the tests run without nil pointer dereferences.
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.
Actionable comments posted: 3
🧹 Nitpick comments (4)
pkg/store/azure_keyvault_store.go (2)
23-29
: Consider adding validation for stackDelimiter.The
KeyVaultStore
struct looks good, but consider adding validation in the constructor to ensurestackDelimiter
is not empty when initialized.
125-146
: Add retry mechanism for transient failures.The Get operation should implement a retry mechanism for handling transient failures in Azure Key Vault operations.
+func (s *KeyVaultStore) Get(stack string, component string, key string) (interface{}, error) { + const maxRetries = 3 + var lastErr error + + for i := 0; i < maxRetries; i++ { + value, err := s.tryGet(stack, component, key) + if err == nil { + return value, nil + } + + var respErr *azcore.ResponseError + if errors.As(err, &respErr) && respErr.StatusCode >= 500 { + lastErr = err + time.Sleep(time.Duration(i+1) * time.Second) + continue + } + return nil, err + } + return nil, fmt.Errorf("failed after %d retries: %w", maxRetries, lastErr) +}pkg/store/azure_keyvault_store_test.go (2)
18-21
: Return mock response data in SetSecret.The mock
SetSecret
method should return meaningful response data for better test coverage.func (m *MockKeyVaultClient) SetSecret(ctx context.Context, name string, parameters azsecrets.SetSecretParameters, options *azsecrets.SetSecretOptions) (azsecrets.SetSecretResponse, error) { args := m.Called(ctx, name, parameters) - return azsecrets.SetSecretResponse{}, args.Error(1) + if args.Get(0) == nil { + return azsecrets.SetSecretResponse{}, args.Error(1) + } + return args.Get(0).(azsecrets.SetSecretResponse), args.Error(1) }
66-110
: Add test cases for complex stack paths.The
TestKeyVaultStore_getKey
should include additional test cases for:
- Stack paths with multiple segments
- Component paths with nested structures
- Keys containing special characters
+ { + name: "complex path", + stack: "prod-east", + component: "service/api/v1", + key: "database.credentials", + expected: "prefix-prod-east-service-api-v1-database-credentials", + wantErr: false, + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/store/azure_keyvault_store.go
(1 hunks)pkg/store/azure_keyvault_store_test.go
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
pkg/store/azure_keyvault_store.go (1)
17-21
: LGTM! Well-defined interface for mocking.The
KeyVaultClient
interface is well-designed for testing purposes, making it easy to mock Azure Key Vault operations.
💥 This pull request now has conflicts. Could you fix it @jamengual? 🙏 |
This PR was closed due to inactivity and merge conflicts. 😭 |
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.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pkg/store/azure_keyvault_store.go (2)
109-109
: 🛠️ Refactor suggestionAdd context timeout for Azure operations.
The Set operation uses
context.Background()
without a timeout. Consider adding a reasonable timeout to prevent hanging operations.- _, err = s.client.SetSecret(context.Background(), secretName, params, nil) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + _, err = s.client.SetSecret(ctx, secretName, params, nil)
137-137
: 🛠️ Refactor suggestionAdd context timeout for Get operation.
Similar to the Set operation, the Get operation should also use a context with timeout.
- resp, err := s.client.GetSecret(context.Background(), secretName, "", nil) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + resp, err := s.client.GetSecret(ctx, secretName, "", nil)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod
(5 hunks)pkg/store/azure_keyvault_store.go
(1 hunks)pkg/store/azure_keyvault_store_test.go
(1 hunks)pkg/store/registry.go
(1 hunks)website/docs/core-concepts/projects/configuration/stores.mdx
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/store/registry.go
- go.mod
- pkg/store/azure_keyvault_store_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/store/azure_keyvault_store.go (2)
pkg/store/store.go (1)
Store
(6-9)pkg/store/errors.go (12)
ErrVaultURLRequired
(21-21)ErrCreateClient
(42-42)ErrStackDelimiterNotSet
(11-11)ErrEmptyStack
(8-8)ErrEmptyComponent
(9-9)ErrEmptyKey
(10-10)ErrGetKey
(12-12)ErrValueMustBeString
(41-41)ErrPermissionDenied
(47-47)ErrSetParameter
(17-17)ErrResourceNotFound
(46-46)ErrAccessSecret
(45-45)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build (windows-latest, windows)
🔇 Additional comments (6)
pkg/store/azure_keyvault_store.go (6)
13-16
: LGTM! Good use of constants for status codes.Defining these HTTP status code constants improves readability and maintainability.
18-22
: Clean interface definition for better testability.The
AzureKeyVaultClient
interface is well-designed for mocking in tests.
24-36
: Well-structured store implementation with clear configuration options.The store struct and options are well-defined with appropriate field types.
41-74
: Constructor correctly handles validation and defaults.The
NewAzureKeyVaultStore
function properly validates required fields and sets sensible defaults for optional parameters.
76-82
: Good reuse of common key generation logic.Delegating to the common
getKey
function avoids code duplication across store implementations.
151-154
: LGTM! Good defensive programming.Properly handling the case where the secret value could be nil avoids potential nil pointer dereference.
<dt>`stores.[store_name].type`</dt> | ||
<dd>Must be set to either `google-secret-manager` or its alias `gsm`</dd> | ||
|
||
<dt>`stores.[store_name].options`</dt> | ||
<dd>A map of options specific to the store type. For Google Secret Manager, the following options are supported:</dd> |
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.
Fix documentation type and options description.
The current documentation incorrectly references Google Secret Manager instead of Azure Key Vault.
<dt>`stores.[store_name].type`</dt>
- <dd>Must be set to either `google-secret-manager` or its alias `gsm`</dd>
+ <dd>Must be set to `azure-key-vault`</dd>
<dt>`stores.[store_name].options`</dt>
- <dd>A map of options specific to the store type. For Google Secret Manager, the following options are supported:</dd>
+ <dd>A map of options specific to the store type. For Azure Key Vault, the following options are supported:</dd>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
<dt>`stores.[store_name].type`</dt> | |
<dd>Must be set to either `google-secret-manager` or its alias `gsm`</dd> | |
<dt>`stores.[store_name].options`</dt> | |
<dd>A map of options specific to the store type. For Google Secret Manager, the following options are supported:</dd> | |
<dt>`stores.[store_name].type`</dt> | |
<dd>Must be set to `azure-key-vault`</dd> | |
<dt>`stores.[store_name].options`</dt> | |
<dd>A map of options specific to the store type. For Azure Key Vault, the following options are supported:</dd> |
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.
Good catch by @coderabbitai
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.
@mcalhoun I agree, it was a good observation by the bot. Would you like me to provide any additional suggestions for improving the Azure Key Vault documentation section beyond fixing the incorrect references to Google Secret Manager?
### Azure Key Vault | ||
|
||
```yaml | ||
stores: | ||
dev/azure-key-vault: | ||
type: azure-key-vault | ||
options: | ||
|
||
prod/azure-key-vault: | ||
type: azure-key-vault | ||
options: | ||
``` | ||
|
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.
🛠️ Refactor suggestion
Configuration example needs to be completed with actual options.
The code example for Azure Key Vault stores is empty. Add the required and optional parameters to match the pattern used in other store examples.
stores:
dev/azure-key-vault:
type: azure-key-vault
options:
+ vault_url: https://dev-keyvault.vault.azure.net
+ prefix: atmos/dev # Optional
prod/azure-key-vault:
type: azure-key-vault
options:
+ vault_url: https://prod-keyvault.vault.azure.net
+ prefix: atmos/prod # Optional
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
### Azure Key Vault | |
```yaml | |
stores: | |
dev/azure-key-vault: | |
type: azure-key-vault | |
options: | |
prod/azure-key-vault: | |
type: azure-key-vault | |
options: | |
``` | |
### Azure Key Vault | |
|
||
<dt>`stores.[store_name].options`</dt> | ||
<dd>A map of options specific to the store type. For Google Secret Manager, the following options are supported:</dd> | ||
</dl> |
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.
🛠️ Refactor suggestion
Add missing Azure Key Vault options documentation.
The documentation is missing descriptions for the available Azure Key Vault options.
<dd>A map of options specific to the store type. For Azure Key Vault, the following options are supported:</dd>
+
+ <dt>`stores.[store_name].options.vault_url (required)`</dt>
+ <dd>The URL of the Azure Key Vault instance (e.g., https://my-keyvault.vault.azure.net).</dd>
+
+ <dt>`stores.[store_name].options.prefix (optional)`</dt>
+ <dd>A prefix path that will be added to all keys stored or retrieved from Azure Key Vault. For example if the prefix
+ is `atmos/infra-live/`, and if the stack is `plat-us2-dev`, the component is `vpc`, and the key is `vpc_id`, the full path
+ would be `atmos/infra-live/plat-us2-dev/vpc/vpc_id`.</dd>
+
+ <dt>`stores.[store_name].options.stack_delimiter (optional)`</dt>
+ <dd>
+ The delimiter that atmos is using to delimit stacks in the key path. This defaults to `-`. This is used to build the
+ key path for the store.
+ </dd>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
</dl> | |
<dd>A map of options specific to the store type. For Azure Key Vault, the following options are supported:</dd> | |
<dt>`stores.[store_name].options.vault_url (required)`</dt> | |
<dd>The URL of the Azure Key Vault instance (e.g., https://my-keyvault.vault.azure.net).</dd> | |
<dt>`stores.[store_name].options.prefix (optional)`</dt> | |
<dd>A prefix path that will be added to all keys stored or retrieved from Azure Key Vault. For example if the prefix | |
is `atmos/infra-live/`, and if the stack is `plat-us2-dev`, the component is `vpc`, and the key is `vpc_id`, the full path | |
would be `atmos/infra-live/plat-us2-dev/vpc/vpc_id`.</dd> | |
<dt>`stores.[store_name].options.stack_delimiter (optional)`</dt> | |
<dd> | |
The delimiter that atmos is using to delimit stacks in the key path. This defaults to `-`. This is used to build the | |
key path for the store. | |
</dd> | |
</dl> |
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.
another good suggestion by @coderabbitai
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.
@mcalhoun Thank you for the feedback!
I notice there are still a couple of additional issues in the Azure Key Vault documentation section:
- The type description incorrectly says "Must be set to either
google-secret-manager
or its aliasgsm
" instead of mentioning Azure Key Vault - The options description incorrectly references Google Secret Manager
- The Authentication section for Azure Key Vault is empty/incomplete
Would you like me to provide a comprehensive suggestion that fixes all these issues as well?
case "azure-key-vault": | ||
var opts AzureKeyVaultStoreOptions | ||
if err := parseOptions(storeConfig.Options, &opts); err != nil { | ||
return nil, fmt.Errorf("failed to parse Key Vault store options: %w", err) | ||
} |
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.
Please keep the case statement in alphabetical order (so move this block up with the other a
stores.
|
||
Currently, the following stores are supported: | ||
|
||
- [Artifactory](https://jfrog.com/artifactory/) | ||
- [AWS SSM Parameter Store](https://docs.aws.amazon.com/systems-manager/latest/userguide/systems-manager-parameter-store.html) | ||
- [Google Secret Manager](https://cloud.google.com/secret-manager) | ||
- [Redis](https://redis.io/) | ||
- [Azure Key Vault](https://azure.microsoft.com/en-us/products/key-vault) |
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 comment above about alphabetical order
<dt>`stores.[store_name].type`</dt> | ||
<dd>Must be set to either `google-secret-manager` or its alias `gsm`</dd> | ||
|
||
<dt>`stores.[store_name].options`</dt> | ||
<dd>A map of options specific to the store type. For Google Secret Manager, the following options are supported:</dd> |
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.
Good catch by @coderabbitai
|
||
<dt>`stores.[store_name].options`</dt> | ||
<dd>A map of options specific to the store type. For Google Secret Manager, the following options are supported:</dd> | ||
</dl> |
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.
another good suggestion by @coderabbitai
Co-authored-by: Matt Calhoun <matt@cloudposse.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@jamengual let us know when the PR is ready for the final review |
💥 This pull request now has conflicts. Could you fix it @jamengual? 🙏 |
This PR was closed due to inactivity and merge conflicts. 😭 |
This pull request introduces support for Azure Key Vault as a new store type in the project, along with the necessary documentation and tests. The most important changes include updating dependencies, adding the Azure Key Vault store implementation, and updating the documentation to reflect the new store type.
Azure Key Vault Store Implementation:
pkg/store/azure_keyvault_store.go
: Added a newKeyVaultStore
implementation to interact with Azure Key Vault, including methods for setting and getting secrets.pkg/store/azure_keyvault_store_test.go
: Added unit tests for theKeyVaultStore
to ensure correct behavior and input validation.pkg/store/registry.go
: Updated the store registry to include the newazure-key-vault
store type and its configuration options.Dependency Updates:
go.mod
: Added several Azure SDK dependencies to support the new Azure Key Vault functionality. [1] [2] [3] [4]Documentation Updates:
website/docs/core-concepts/projects/configuration/stores.mdx
: Updated the documentation to include details on configuring and using the Azure Key Vault store, including examples and authentication methods. [1] [2]Summary by CodeRabbit