8000 Add Azure Key Vault support for Atmos Stores by jamengual · Pull Request #963 · cloudposse/atmos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Closed
wants to merge 38 commits into from
Closed

Conversation

jamengual
Copy link
Contributor
@jamengual jamengual commented Jan 21, 2025

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:

Dependency Updates:

  • go.mod: Added several Azure SDK dependencies to support the new Azure Key Vault functionality. [1] [2] [3] [4]

Documentation Updates:

Summary by CodeRabbit

  • New Features
    • Added support for Azure Key Vault as a secret store, enabling secure storage and retrieval of secrets using Azure Key Vault.
  • Bug Fixes
    • Improved error handling for secret operations including access denied and not found cases.
  • Tests
    • Introduced comprehensive unit tests for Azure Key Vault integration, covering secret management operations and input validation.
  • Documentation
    • Updated Atmos stores documentation to include Azure Key Vault configuration, authentication methods, and usage examples.

@jamengual jamengual requested a review from a team as a code owner January 21, 2025 21:12
@mergify mergify bot added the triage Needs triage label Jan 21, 2025

This comment was marked as outdated.

Copy link
Contributor
@coderabbitai coderabbitai bot left a 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 the secretName.

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 the Set 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:

  1. Use typographic quotes (") instead of straight quotes (") for the parameters.
  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 78ee9c6 and f834f8b.

⛔ 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)

Copy link
Member
@mcalhoun mcalhoun left a 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.

Copy link
Contributor
@coderabbitai coderabbitai bot left a 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 issue

Handle errors returned by getKey function

The 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 names

In the getKey function, replacing multiple consecutive dashes using strings.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: Use defer to ensure the context is canceled properly

In the Get method, calling cancel() immediately after GetSecret might not cover all execution paths, especially if future modifications are made. Using defer 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 in Set method

Currently, 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

📥 Commits

Reviewing files that changed from the base of the PR and between f834f8b and 5223d81.

📒 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 client

In TestKeyVaultStore_InputValidation, initializing the client with a mock ensures the tests run without nil pointer dereferences.

Copy link
Contributor
@coderabbitai coderabbitai bot left a 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 ensure stackDelimiter 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5223d81 and 44fbfd4.

📒 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.

@osterman osterman added minor New features that do not break anything and removed triage Needs triage labels Jan 22, 2025
Copy link
mergify bot commented Feb 20, 2025

💥 This pull request now has conflicts. Could you fix it @jamengual? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Feb 20, 2025
@mergify mergify bot closed this Feb 20, 2025
Copy link
mergify bot commented Feb 20, 2025

This PR was closed due to inactivity and merge conflicts. 😭
Please resolve the conflicts and reopen if necessary.

@mergify mergify bot removed the conflict This PR has conflicts label Feb 20, 2025
@aknysh aknysh changed the title Adding Azure Keyvault support for Stores Add Azure Key Vault support for Atmos Stores Apr 25, 2025
Copy link
Contributor
@coderabbitai coderabbitai bot left a 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 suggestion

Add 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 suggestion

Add 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1c8a3f1 and 09e2449.

⛔ 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.

Comment on lines +277 to +281
<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>
Copy link
Contributor
@coderabbitai coderabbitai bot Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
F438
<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>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch by @coderabbitai

Copy link
Contributor

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?

Comment on lines 260 to 272
### Azure Key Vault

```yaml
stores:
dev/azure-key-vault:
type: azure-key-vault
options:

prod/azure-key-vault:
type: azure-key-vault
options:
```

Copy link
Contributor

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.

Suggested change
### 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>
Copy link
Contributor
@coderabbitai coderabbitai bot Apr 25, 2025

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.

Suggested change
</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>

Copy link
Member

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

Copy link
Contributor

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:

  1. The type description incorrectly says "Must be set to either google-secret-manager or its alias gsm" instead of mentioning Azure Key Vault
  2. The options description incorrectly references Google Secret Manager
  3. 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?

Comment on lines +61 to +65
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)
}
Copy link
Member

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.

A377

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)
Copy link
Member

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

Comment on lines +277 to +281
<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>
Copy link
Member

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>
Copy link
Member

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>
@aknysh
Copy link
Member
aknysh commented Apr 29, 2025

@jamengual let us know when the PR is ready for the final review

Copy link
mergify bot commented May 9, 2025

💥 This pull request now has conflicts. Could you fix it @jamengual? 🙏

@mergify mergify bot added the conflict This PR has conflicts label May 9, 2025
Copy link
mergify bot commented May 9, 2025

This PR was closed due to inactivity and merge conflicts. 😭
Please resolve the conflicts and reopen if necessary.

@mergify mergify bot closed this May 9, 2025
@mergify mergify bot removed the conflict This PR has conflicts label May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor New features that do not break anything size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0