Add support to load config values and secrets from external sources #762
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=error msg="Running error: context loading failed: no go files to analyze: running Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Please see the CodeRabbit review for details on how to improve shell parsing. This current implementation will break with quoted strings, and @CodeRabbit proposes how to avoid that issue. |
|
Tip For best results, initiate chat on the files or code changes. @osterman: You're correct. Using import (
"github.com/mattn/go-shellwords"
// other imports
)
// ...
parsedArgs, err := shellwords.Parse(cmd)
if err != nil {
return err
}
cmdName := parsedArgs[0]
cmdArgs := slices.Concat(parsedArgs[1:], args)
// Proceed with cmdName and cmdArgs as beforeThis approach ensures that quoted strings are handled correctly when parsing the command and its arguments. |
|
@coderabbitai full review |
|
ok, i see. updated to use a proper shell parser instead. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
internal/exec/template_funcs_component.go (1)
149-158: Consider a more robust command execution framework!To improve maintainability and reliability, consider:
- Creating a dedicated command executor interface:
type CommandExecutor interface { Execute(ctx context.Context, cmd string, args ...string) error }
- Implementing specific executors for different command types:
type TerraformExecutor struct { workingDir string // ... other config }This would:
- Separate concerns between command parsing and execution
- Make it easier to add support for new command types
- Improve testing and validation
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
internal/exec/template_funcs_component.go(2 hunks)
🔇 Additional comments (2)
internal/exec/template_funcs_component.go (2)
10-10: Excellent choice of dependency, warrior!
The go-shellwords package is a battle-tested solution for parsing shell commands, making it a strong choice for this task.
158-158: Verify impact on existing Terraform/OpenTofu commands!
Given the command parsing changes, we should verify that existing Terraform/OpenTofu commands continue to work as expected.
Let's scan the codebase for existing command patterns:
✅ Verification successful
Let me gather more information about the command parsing changes to ensure a thorough verification.
Let me check one more critical aspect of the command handling to complete our verification.
Command parsing changes are safe and properly implemented!
The verification shows that:
- Command parsing correctly extracts the binary name (terraform/tofu) from the last element of parsed arguments
- The implementation maintains compatibility with both Terraform and OpenTofu commands
- Configuration files support various command formats including absolute paths and version-specific binaries
- Command execution context properly handles the parsed binary name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Terraform/OpenTofu command patterns in the codebase
# to ensure compatibility with the new parsing logic
echo "Searching for Terraform command patterns..."
rg -A 2 "command.*terraform|command.*tofu" --type yaml
echo "Searching for command section definitions in Go code..."
rg "CommandSectionName.*=.*" --type go
Length of output: 24219
Script:
#!/bin/bash
# Search for the command parsing logic and its usage
echo "Searching for command parsing implementation..."
rg -A 5 "NewTerraform.*cmdName" --type go
echo "Searching for terraform command configuration..."
ast-grep --pattern 'func $_($$) (string, error) {
$$$
command := $$$
$$$
}'
Length of output: 720
Script:
#!/bin/bash
# Search for the command name resolution logic
echo "Searching for command name resolution..."
rg -B 5 -A 5 "cmdName.*:=.*" --type go
echo "Checking terraform command execution context..."
ast-grep --pattern 'type $_ struct {
$$$
command string
$$$
}'
Length of output: 1647
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
internal/exec/template_funcs_vals.go (2)
10-12: Consider adding documentation for the valsLogWriter struct.While the implementation is straightforward, adding documentation would help other developers understand the purpose and usage of this type.
+// valsLogWriter implements io.Writer interface to provide logging capabilities +// during vals operations using the provided CLI configuration type valsLogWriter struct { cliConfig schema.CliConfiguration }
19-22: Consider enhancing error handling and type safety.While the implementation is functional, there are opportunities for improvement:
- The return type
anymight benefit from more specific typing- Consider adding validation for the
refparameter- Error wrapping could provide more context
-func valsFunc(cliConfig schema.CliConfiguration, ref string) (any, error) { +func valsFunc(cliConfig schema.CliConfiguration, ref string) (any, error) { + if ref == "" { + return nil, fmt.Errorf("empty reference string provided") + } vlw := valsLogWriter{cliConfig} - return vals.Get(ref, vals.Options{LogOutput: vlw}) + result, err := vals.Get(ref, vals.Options{LogOutput: vlw}) + if err != nil { + return nil, fmt.Errorf("failed to get value for ref %q: %w", ref, err) + } + return result, nil }internal/exec/template_funcs.go (2)
41-43: Add documentation for the new Vals method.This method appears to be a significant addition to the template functions. Adding documentation would help users understand:
- The purpose of the method
- The expected format of the
refparameter- The possible return types
- Example usage
Consider adding a doc comment like this:
+// Vals processes the given reference string and returns the corresponding value. +// The reference format should follow <specify-format-here>. +// Returns the processed value or an error if the reference is invalid. func (f AtmosFuncs) Vals(ref string) (any, error) {
41-43: Consider adding input validation and more specific return types.The current implementation might benefit from:
- Validation of the
refparameter before passing it tovalsFunc- A more specific return type than
anyto improve type safetyConsider something like:
-func (f AtmosFuncs) Vals(ref string) (any, error) { +func (f AtmosFuncs) Vals(ref string) (interface{}, error) { + if ref == "" { + return nil, fmt.Errorf("reference string cannot be empty") + } return valsFunc(f.cliConfig, ref) }Note: If you know the specific types that
valsFunccan return, consider creating a custom type or using a more specific interface.go.mod (1)
Line range hint
3-3: Invalid Go version specified.The Go version
1.23is not valid. The latest stable version of Go is 1.22.Apply this change:
-go 1.23 +go 1.22
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(5 hunks)internal/exec/template_funcs.go(1 hunks)internal/exec/template_funcs_vals.go(1 hunks)
🔇 Additional comments (6)
internal/exec/template_funcs_vals.go (3)
1-8: LGTM! Clean package structure and imports.
The package organization and import declarations are well-structured. Good use of package aliasing for utils to avoid naming conflicts.
14-17: LGTM! Clean Write implementation.
The Write method correctly implements the io.Writer interface and properly handles logging through the CLI configuration.
1-22: Verify integration with shell command execution.
The vals implementation looks solid, but let's verify its integration with the shell command execution feature.
✅ Verification successful
Integration with shell command execution is properly implemented
The vals integration is correctly implemented and registered in the template function system:
valsFuncis properly integrated throughtemplate_funcs.goand made available in the template function map- The function is registered and accessible through the template system via
FuncMapintemplate_funcs.go - The implementation includes proper error handling and logging through
valsLogWriter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper integration between vals and shell command execution
# Test 1: Look for usage of valsFunc in shell command execution
echo "Checking for valsFunc usage in shell command context..."
rg -l "valsFunc.*exec" --type go
# Test 2: Look for potential test files
echo "Checking for related test files..."
fd -e go -g "*vals*test.go"
Length of output: 264
Script:
#!/bin/bash
# Let's broaden our search to understand the integration better
# Check where valsFunc is used in the codebase
echo "Checking all usages of valsFunc..."
rg "valsFunc" -A 5 --type go
# Check for vals package imports and usage
echo "Checking vals package imports..."
rg "github.com/helmfile/vals" --type go
# Look for template function registrations
echo "Checking template function registrations..."
rg "template.*Funcs|FuncMap" -A 5 --type go
Length of output: 3841
internal/exec/template_funcs.go (1)
41-43: Verify the integration with vals package.
The function seems to integrate with the newly added github.com/helmfile/vals package mentioned in the summary. Let's verify the usage pattern.
✅ Verification successful
Integration with vals package is properly implemented
The vals integration is clean and follows best practices:
- Uses official
github.com/helmfile/valspackage v0.37.8 - Implements proper logging through
valsLogWriterstruct - Correctly utilizes
vals.Get()with appropriate options - Maintains clean separation of concerns with vals-specific code in
template_funcs_vals.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how vals is used in the codebase
# Expected: Find implementation of valsFunc and its usage patterns
# Look for valsFunc implementation
echo "=== Searching for valsFunc implementation ==="
rg -A 5 "func valsFunc"
# Look for other Vals usage
echo "=== Searching for other Vals usage patterns ==="
rg -A 3 "\.Vals\("
Length of output: 563
Script:
#!/bin/bash
# Let's verify the vals package integration more thoroughly
# Check the vals package import
echo "=== Checking vals package import ==="
rg "github.com/helmfile/vals"
# Look for the valsLogWriter implementation
echo "=== Checking valsLogWriter implementation ==="
rg -A 5 "type valsLogWriter"
# Check for any vals-related configuration or setup
echo "=== Checking vals-related configuration ==="
rg -A 5 "vals\.Options"
# Check for any test cases related to vals integration
echo "=== Checking vals-related tests ==="
rg -A 5 "TestVals" --type go
Length of output: 1272
go.mod (2)
27-27: Dependencies look good for shell command execution.
The addition of github.com/mattn/go-shellwords is appropriate for shell command parsing, which aligns with the PR's objective of adding support for shell command execution with arguments. This is a well-maintained library commonly used for shell command parsing in Go.
Also applies to: 33-33
Line range hint 51-367: Indirect dependency updates look reasonable.
The updates to indirect dependencies appear to be routine maintenance updates, including security patches and feature improvements. These changes maintain compatibility with the direct dependencies.
|
Ok, I will revert the command execution related changes. The end goal would be to support loading secrets from external sources, like 1Password, etc. hence my initial intention to add support for a more complex shell command, however it would be a much cleaner approach to integrate vals as a template function. What do you think? |
speier
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
go.mod (1)
104-124: Consider consolidating AWS SDK versions.The mix of AWS SDK v1 (aws-sdk-go) and v2 (aws-sdk-go-v2) could lead to inconsistent behavior. Consider standardizing on v2 where possible.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod(5 hunks)
🔇 Additional comments (3)
go.mod (3)
27-27: LGTM on adding vals library!
The addition of github.com/helmfile/vals v0.37.8 aligns perfectly with the PR objective of supporting external secrets loading.
66-73: Verify Azure SDK version compatibility.
The addition of multiple Azure SDK components (azcore, azidentity, keyvault, etc.) suggests integration with Azure services. Ensure these versions are compatible with each other and the minimum supported Go version.
#!/bin/bash
# Description: Check if Azure SDK versions are compatible with the Go version
# Test: Look for any version constraints or compatibility issues
rg -A 2 "go \d+\.\d+" ./README.md
rg "go\s+\d+\.\d+" ./go.mod345-351: Verify compatibility with updated Google Cloud dependencies.
The significant updates to Google Cloud dependencies (api, genproto, grpc) might require additional testing to ensure compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
internal/exec/template_funcs.go (1)
42-44: Enhance documentation with example usage.While the documentation clearly explains the reference format, adding an example would make it more user-friendly. Consider adding an example like:
// Example: ref+1password://item/vault/item-path#field
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
internal/exec/template_funcs.go(2 hunks)internal/exec/template_funcs_vals.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/template_funcs_vals.go
🔇 Additional comments (2)
internal/exec/template_funcs.go (2)
12-12: LGTM! Import is properly organized.
The addition of the "fmt" package import is well-placed and necessary for error handling in the new functionality.
45-51: 🛠️ Refactor suggestion
Verify reference format validation.
The method handles sensitive data from external sources. While empty ref validation is good, consider adding validation for the reference format to prevent potential security issues.
Let's check if the valsFunc implementation includes proper validation:
Consider adding format validation before delegating to valsFunc:
func (f AtmosFuncs) Vals(ref string) (interface{}, error) {
if ref == "" {
return nil, fmt.Errorf("vals reference code cannot be empty")
}
+
+ // Validate reference format
+ if !strings.HasPrefix(ref, "ref+") {
+ return nil, fmt.Errorf("invalid vals reference format: must start with 'ref+'")
+ }
return valsFunc(f.cliConfig, ref)
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx (2)
21-27: Consider adding credential security best practicesWhile the environment variables are well documented, consider adding security recommendations:
- Use credential rotation policies
- Leverage environment-specific credentials
- Consider using credential management tools
To use [vals](https://github.com/helmfile/vals), you need to set specific environment variables based on your chosen backend: - For AWS backends: `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY` - For GCP backends: `GOOGLE_APPLICATION_CREDENTIALS` - For Azure backends: `AZURE_CLIENT_ID`, `AZURE_CLIENT_SECRET` Refer to each backend's documentation for the complete list of required credentials. + +### Security Best Practices + +When managing credentials: +- Implement regular credential rotation +- Use different credentials per environment +- Consider using credential management tools like AWS IAM roles or Vault
42-54: Add validation and troubleshooting informationThe example is clear, but users would benefit from knowing how to validate their configuration and troubleshoot common issues.
For example, to retrieve a secret from 1Password, we could do the following: <File> ```yaml components: terraform: dockerconfig: vars: auths: ghcr.io: auth: '{{ atmos.Vals "ref+op://infra/ghcr/token" }}'Note: 1Password backend expects the
OP_SERVICE_ACCOUNT_TOKENenvironment variable to be set prior to running Atmos.
+
+### Validation
+
+To validate your configuration:
+1. Ensure the secret exists in 1Password
+2. Verify the service account token has necessary permissions
+3. Test withatmos describe stack --stack=<your-stack>in a secure environment
+
+### Common Issues
+
+- Invalid token format
+- Insufficient permissions
+- Secret path not found<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [style] ~54-~54: ‘prior to’ might be wordy. Consider a shorter alternative. Context: ...T_TOKEN` environment variable to be set prior to running Atmos. ## Expression syntax R... (EN_WORDINESS_PREMIUM_PRIOR_TO) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2b1723074e62557c34af1f2965ebaa6a6e79ba07 and d30fce742d0168506ae2a15a5ffcb9a23962f835. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (2)</summary> * `go.mod` (5 hunks) * `website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx</summary> [style] ~54-~54: ‘prior to’ might be wordy. Consider a shorter alternative. Context: ...T_TOKEN` environment variable to be set prior to running Atmos. ## Expression syntax R... (EN_WORDINESS_PREMIUM_PRIOR_TO) --- [duplication] ~89-~89: Possible typo: you repeated a word Context: ...s://github.com/helmfile/vals#gitlab) - [1Password](https://github.com/helmfile/vals#1password) - [1Password Connect](https://github.com/helmfile/va... (ENGLISH_WORD_REPEAT_RULE) </details> </details> </details> <details> <summary>🔇 Additional comments (4)</summary> <details> <summary>go.mod (4)</summary> `27-27`: **LGTM: vals integration aligns with PR objectives** The addition of `github.com/helmfile/vals v0.37.8` as a direct dependency aligns perfectly with the PR's goal of supporting external config and secret loading. --- `106-126`: **Consider dependency impact on build time and binary size** The addition of extensive AWS SDK dependencies could impact build times and binary size. Consider using specific AWS SDK modules instead of the full SDK if only certain services are needed. ```shell #!/bin/bash # Description: Check AWS SDK usage patterns # Look for specific AWS service imports echo "Checking for AWS SDK service-specific imports..." rg -l 'github.com/aws/aws-sdk-go-v2/service/' echo "Checking for general AWS SDK usage..." rg -l 'github.com/aws/aws-sdk-go'
334-334: Verify compatibility with updated crypto packageThe update to
golang.org/x/crypto v0.28.0is significant. Ensure all crypto-related functionality continues to work as expected.#!/bin/bash # Description: Check for crypto package usage # Look for potential compatibility issues echo "Checking for crypto package imports..." rg -l 'golang.org/x/crypto' echo "Checking for specific crypto functions..." rg 'crypto\.' -A 2
68-75: Verify the necessity of all Azure SDK dependenciesThe PR introduces multiple Azure SDK dependencies. While these are indirect dependencies likely pulled in by vals, we should verify if all are necessary for the intended functionality.
✅ Verification successful
Azure SDK dependencies are correctly listed as indirect and are necessary for the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Azure SDK dependencies are used in the codebase # Look for Azure-specific imports or usage echo "Checking for Azure SDK usage..." rg -l 'azure-sdk-for-go' echo "Checking for Azure service imports..." rg -l 'github.com/Azure/'Length of output: 290
Script:
#!/bin/bash # Description: Verify Azure SDK dependencies usage in the codebase echo "Checking for Azure SDK usage..." rg -l 'github.com/Azure/azure-sdk-for-go' echo "Checking for specific Azure service imports..." rg -l 'github.com/Azure/'Length of output: 271
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx (2)
33-39: Enhance security best practices with vals-specific guidanceThe current security practices are good but could be more actionable. Consider adding vals-specific recommendations:
### Security Best Practices When managing credentials: - Implement regular credential rotation - Use different credentials per environment - Consider using credential management tools like AWS IAM roles or Vault +- Use read-only credentials when possible for vals references +- Implement least-privilege access for each backend service +- Consider using backend-specific features: + - AWS: Use IAM roles with session tokens + - Vault: Leverage Vault's lease management + - 1Password: Use scoped service accounts
49-59: Add examples for other popular backendsThe current example only shows 1Password usage. Consider adding examples for other commonly used backends:
components: terraform: dockerconfig: vars: auths: ghcr.io: auth: '{{ atmos.Vals "ref+op://infra/ghcr/token" }}' + + # AWS Secrets Manager example + database: + vars: + password: '{{ atmos.Vals "ref+awssecrets://myapp/dev/db#/password" }}' + + # Vault example + api: + vars: + credentials: '{{ atmos.Vals "ref+vault://secret/data/myapp/api#/key" }}' + + # SOPS example + certificates: + vars: + ssl_key: '{{ atmos.Vals "ref+sops://secrets/certificates.yaml#/prod/ssl" }}'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx
[style] ~61-~61: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...T_TOKEN` environment variable to be set prior to running Atmos. ### Validation To vali...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[duplication] ~110-~110: Possible typo: you repeated a word
Context: ...s://github.com/helmfile/vals#gitlab) - 1Password - [1Password Connect](https://github.com/helmfile/va...
(ENGLISH_WORD_REPEAT_RULE)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx (5)
15-17: Enhance security warning with specific alternatives.Consider adding secure alternatives for viewing secrets:
<Admonition type="warning" title="Careful with secrets"> `atmos describe stack` will reveal the secrets in plain text without obfuscation. Make sure not to accidentally run these commands to stdout in your CI/CD pipelines. + For safer alternatives: + - Use `atmos describe stack --stack=<stack> 2>/dev/null | grep -v "secret"` + - Redirect output to a temporary file that's immediately deleted </Admonition>
29-31: Enhance usage example with more context.The current example is too minimal. Consider expanding it with a more realistic use case:
```yaml - {{ atmos.Vals "<reference>" }} +# Example: Loading database credentials from AWS Secrets Manager +component: + database: + vars: + username: {{ atmos.Vals "ref+awssecrets://myapp/prod/db#/username" }} + password: {{ atmos.Vals "ref+awssecrets://myapp/prod/db#/password" }}--- `35-38`: **Add concrete examples to security best practices.** Consider enhancing the practices with specific examples: ```diff When managing credentials: - - Implement regular credential rotation - - Use different credentials per environment - - Consider using credential management tools like AWS IAM roles or Vault + - Implement regular credential rotation (e.g., every 90 days) + - Use different credentials per environment: + ```yaml + dev: ref+vault://secret/dev/myapp + prod: ref+vault://secret/prod/myapp + ``` + - Use credential management tools: + - AWS: Use IAM roles with session tokens + - Vault: Implement token lease and renewal
61-61: Simplify language and emphasize environment variable requirement.-<b>Note</b>: 1Password backend expects the `OP_SERVICE_ACCOUNT_TOKEN` environment variable to be set prior to running Atmos. +<Admonition type="info" title="Required Environment Variable"> + Set `OP_SERVICE_ACCOUNT_TOKEN` before running Atmos with 1Password backend. +</Admonition>🧰 Tools
🪛 LanguageTool
[style] ~61-~61: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...T_TOKEN` environment variable to be set prior to running Atmos. ### Validation To vali...(EN_WORDINESS_PREMIUM_PRIOR_TO)
81-83: Document URI parameters in detail.Add descriptions for each URI component:
ref+BACKEND://PATH[?PARAMS][#FRAGMENT][+]
+ +Where: +- `BACKEND`: The secret backend (e.g., vault, awssecrets) +- `PATH`: Backend-specific path to the secret +- `PARAMS`: Optional query parameters for backend configuration +- `FRAGMENT`: Optional JSON path to extract specific fields +- `+`: Optional suffix to expand YAML/JSON values
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx
[style] ~61-~61: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...T_TOKEN` environment variable to be set prior to running Atmos. ### Validation To vali...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[duplication] ~110-~110: Possible typo: you repeated a word
Context: ...s://github.com/helmfile/vals#gitlab) - 1Password - [1Password Connect](https://github.com/helmfile/va...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~127-~127: Please add a punctuation mark at the end of paragraph.
Context: ...nvalid - Surface backend-specific error messages Handle potential errors in your templa...
(PUNCTUATION_PARAGRAPH_END)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx (4)
29-31: Enhance the usage example with real-world scenarios.Consider expanding the example to show practical use cases for different backends:
- {{ atmos.Vals "<reference>" }} +# AWS Secrets Manager example +aws_secret: {{ atmos.Vals "ref+awssecrets://myapp/production/api-key" }} + +# Vault example with nested key +vault_secret: {{ atmos.Vals "ref+vault://secret/data/myapp#/credentials/key" }} + +# Azure Key Vault example +azure_secret: {{ atmos.Vals "ref+azurekv://myapp-vault/secret-name" }}
35-38: Expand security best practices with concrete examples.Consider adding more specific guidance:
When managing credentials: - Implement regular credential rotation - Use different credentials per environment - Consider using credential management tools like AWS IAM roles or Vault +- Use least privilege principle (e.g., read-only access to specific secrets) +- Implement audit logging for secret access +- Set up alerts for unusual secret access patterns +- Use short-lived credentials where possible
61-61: Improve wording for clarity.-<b>Note</b>: 1Password backend expects the `OP_SERVICE_ACCOUNT_TOKEN` environment variable to be set prior to running Atmos. +<b>Note</b>: 1Password backend requires the `OP_SERVICE_ACCOUNT_TOKEN` environment variable to be set before running Atmos.🧰 Tools
🪛 LanguageTool
[style] ~61-~61: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...T_TOKEN` environment variable to be set prior to running Atmos. ### Validation To vali...(EN_WORDINESS_PREMIUM_PRIOR_TO)
124-127: Add proper punctuation for consistency.The `atmos.Vals` function will: - Return an error if the backend is unreachable - Fail fast if credentials are invalid - - Surface backend-specific error messages + - Surface backend-specific error messages.🧰 Tools
🪛 LanguageTool
[grammar] ~127-~127: Please add a punctuation mark at the end of paragraph.
Context: ...nvalid - Surface backend-specific error messages Handle potential errors in your templa...(PUNCTUATION_PARAGRAPH_END)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx
[style] ~61-~61: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...T_TOKEN` environment variable to be set prior to running Atmos. ### Validation To vali...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[duplication] ~110-~110: Possible typo: you repeated a word
Context: ...s://github.com/helmfile/vals#gitlab) - 1Password - [1Password Connect](https://github.com/helmfile/va...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~127-~127: Please add a punctuation mark at the end of paragraph.
Context: ...nvalid - Surface backend-specific error messages Handle potential errors in your templa...
(PUNCTUATION_PARAGRAPH_END)
🔇 Additional comments (4)
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx (4)
1-13: LGTM! Clear and concise introduction.
The metadata and introduction effectively communicate the purpose and value proposition of the atmos.Vals function.
15-17: Previous review comment is still applicable.
A past review has already suggested improvements to the security warning message.
63-76: Previous review comments are still applicable.
Past reviews have already suggested comprehensive improvements to both the validation and common issues sections.
77-120: LGTM! Comprehensive backend documentation.
The expression syntax is clearly explained, and the backend list is thorough with proper links to detailed documentation.
🧰 Tools
🪛 LanguageTool
[duplication] ~110-~110: Possible typo: you repeated a word
Context: ...s://github.com/helmfile/vals#gitlab) - 1Password - [1Password Connect](https://github.com/helmfile/va...
(ENGLISH_WORD_REPEAT_RULE)
| # Conditional logic based on secret availability | ||
| {{- if $secret := atmos.Vals "ref+vault://secret/myapp" }} | ||
| value: {{ $secret }} | ||
| {{- else }} | ||
| {{- fail "Required secret not available" }} | ||
| {{- end }} | ||
|
|
||
| # Multiple secrets with error aggregation | ||
| {{- $creds := dict }} | ||
| {{- range $key := list "username" "password" "api-key" }} | ||
| {{- $path := printf "ref+vault://secret/myapp/%s" $key }} | ||
| {{- $value := atmos.Vals $path | default (fail (printf "Missing required secret: %s" $key)) }} | ||
| {{- $_ := set $creds $key $value }} | ||
| {{- end }} | ||
|
|
There was a problem hiding this comment.
@aknysh this is only valid on template imports, right?
There was a problem hiding this comment.
Let me review this.
Some Go templates are still valid YAML files, but some are not.
There was a problem hiding this comment.
we need to make a note that the error handling code above is not a valid YAML.
It will work in Atmos stack manifests only when the file extention is not .yaml.
It can be my-stack.tmpl, and then imported as a template (not YAML):
import:
- catalog/my-stack.tmplIf an imported file is not YAML, it will not be validated as YAML
There was a problem hiding this comment.
this example comes from coderabbit suggestion, i don't think you need something like this.
i'm using the function in many places simply like this:
components:
terraform:
argocd:
vars:
slack_token: '{{ atmos.Vals "ref+sops://secrets.enc.yaml#/argocd/slack_token" }}'
There was a problem hiding this comment.
btw switched to SOPS lately because remote backends like 1password can slows down command execution thats true
|
@speier we really appreciate this PR and the initiative, and apologize for the delays in getting it merged. While his implementation aligns with the approach we originally intended to take (and similar to the other template functions), we anticipate it could introduce several challenges. These include issues around:
Additionally, we foresee it raising numerous questions from users. For these reasons, we had previously decided not to implement this approach, giving us some pause now as you've already implemented it. I think we need to discuss internally some more the potential implications and mitigations of this approach. As soon as we release something "into the wild" it becomes picked up, and is something we need to maintain and support. We have a meeting internally today, where we'll discuss this. |
|
@osterman ok, i see. it's your call eventually. i thought this wasn't any different from gomplate really, maybe the documentation needs to be improved tough. but i can understand your decision. |
It is true. And some of the same warts exist with that, as you astutely point out. I think we just need to warn users sufficiently, and provide safeguards. I am uneasy giving users the ability to expose secrets as it defeats the purpose of using a secure backend to begin with, like 1Password. So while gomplate provides some of the same functionality as Vals, we (Cloud Posse) introduced it for other reasons. It's prudent not to rush the implementation, when the consequences are signficant. |
|
In GitHub Actions, any string matching the value of a secret is automatically masked. Perhaps we could adopt a similar approach in |
|
that part i totally understand, atmos describe should mask secrets. (is there any other commands which should mask them?) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
go.mod (2)
Line range hint
3-3: Invalid Go version specifiedThe Go version is set to 1.23 which hasn't been released yet. The latest stable version is Go 1.22.
Update the Go version:
-go 1.23 +go 1.22
68-75: Consider implementing cloud provider feature flagsThe addition of multiple cloud provider SDKs (Azure, AWS, GCP) suggests broad cloud platform support. Consider implementing feature flags to:
- Allow selective compilation of cloud provider support
- Reduce binary size for users who don't need all providers
- Minimize the attack surface when specific providers aren't needed
Also applies to: 107-126
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod(7 hunks)
🔇 Additional comments (2)
go.mod (2)
189-190: Review JWT and Vault version compatibility
The update includes both JWT v4 and v5 libraries, along with HashiCorp Vault API. Ensure there are no compatibility issues between these authentication components.
Also applies to: 229-230
27-27: Verify vals library integration
The addition of github.com/helmfile/vals v0.37.8 aligns with the PR objective to support loading config values and secrets from external sources. However, let's verify its usage across the codebase.
✅ Verification successful
Let me gather more information about the actual implementation of the vals integration.
Let me check one more aspect to verify the complete integration.
Vals library integration is properly implemented
The verification shows a well-structured integration of the vals library:
- Proper initialization with
vals.New()and runtime management - Complete documentation in
website/docs/core-concepts/stacks/templates/functions/atmos.Vals.mdx - Implementation in
internal/exec/template_funcs_vals.gowith error handling - Support for multiple secret backends (AWS, GCP, Vault, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for vals library usage patterns
# Expected: Find template function implementations using vals
# Search for vals import and usage
rg -A 5 "github.com/helmfile/vals"
# Search for potential template function implementations
ast-grep --pattern 'func $_(vals $_) {
$$$
}'
Length of output: 7752
Script:
#!/bin/bash
# Search for the vals template function implementation
rg -A 10 "func.*Vals" internal/exec/template_funcs_vals.go
# Check for actual usage of vals in the codebase
rg -A 5 "vals\." internal/exec/template_funcs_vals.go
# Look for test files related to vals
fd "vals.*test" --exec cat {}
Length of output: 666
Script:
#!/bin/bash
# Search for the template function registration and actual usage
rg -A 5 "atmos\.Vals"
# Look for the actual template function implementation
ast-grep --pattern 'func $_(cliConfig schema.CliConfiguration) interface{} {
$$$
}' internal/exec/template_funcs_vals.go
Length of output: 5211
|
We've opened up #834 which is the intereface we're proposing for natively handling storage backends in atmos. The interface supports read/write, but both are not required. We would like to add support for multiple read-only backends, but it won't be in the initial release. Once the release is made, we'll accept PRs adding backends. |
|
Superseded by: |
what
Integrate vals as a template function.
why
Loading configuration values and secrets from external sources, supporting various backends.
Summary by CodeRabbit
New Features
atmos.Valstemplate function for loading configuration values and secrets from external sources.Updates
Documentation
atmos.Valstemplate function, including usage examples and security best practices.