feature: add envfile support
#2042
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in Changes
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
internal/cli/serve/command.go (5)
81-84: Wrap the error when loading the environment fileWhen loading the environment file using
godotenv.Load(envFile), any error is returned directly. For better error context and diagnosis, wrap the error to include additional information.Apply this diff to wrap the error:
err = godotenv.Load(envFile) if err != nil { - return "", err + return "", errors.E(op, fmt.Errorf("could not load environment file: %w", err)) }
28-53: Improve robustness of environment variable parsingIn the
expandEnvVarsfunction, slicing operations likematch[2:len(match)-1]may cause index out-of-range errors if unexpected patterns are encountered. Consider adding checks to ensure the slices are safe and handle any edge cases.Alternatively, you can simplify environment variable expansion by using Go's built-in
os.Expandoros.ExpandEnvfunctions, which handle variable substitution and default values safely.Apply this diff to use
os.Expand:-func expandEnvVars(input string) string { - return envVarPattern.ReplaceAllStringFunc(input, func(match string) string { - // ... existing code ... - }) +func expandEnvVars(input string) string { + return os.Expand(input, func(varName string) string { + return os.Getenv(varName) + }) }
71-74: Handle nested configurations when locating 'envfile'The current implementation checks for
envfileat the root level of the configuration map. If the configuration uses nested structures, this approach might missenvfiledeclarations located deeper in the hierarchy. Consider recursively searching the configuration map to locateenvfileentries.
5-16: Organize and optimize imports for better readabilityThe import statements can be organized into standard library, third-party, and local packages to enhance readability. Also, remove any unused imports to clean up the code.
Arrange the imports as follows:
import ( + "fmt" + "os" + "os/signal" + "path/filepath" + "regexp" + "strings" + "syscall" + "github.com/joho/godotenv" "github.com/roadrunner-server/endure/v2" "github.com/roadrunner-server/roadrunner/v2024/container" "github.com/roadrunner-server/roadrunner/v2024/internal/meta" "github.com/roadrunner-server/roadrunner/v2024/internal/sdnotify" - "gopkg.in/yaml.v3" - "os" - "os/signal" - "path/filepath" - "regexp" - "strings" - "syscall" + "gopkg.in/yaml.v3" + configImpl "github.com/roadrunner-server/config/v5" "github.com/roadrunner-server/errors" "github.com/spf13/cobra" )
115-124: Ensure temporary config file is always cleaned upThe temporary config file is scheduled for removal with a
deferstatement, but only iflen(tempFile) > 0. To prevent any potential file leakage, ensure that the deferred removal is called in all cases where a temporary file is created.Move the
deferstatement immediately after the temporary file is created:tempFile, err := processConfig(*cfgFile) if err != nil { return errors.E(op, err) } if len(tempFile) > 0 { *cfgFile = tempFile + defer func() { + _ = os.Remove(tempFile) + }() - defer func() { - _ = os.Remove(tempFile) - }() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
internal/cli/serve/command.go(2 hunks)
🔇 Additional comments (1)
internal/cli/serve/command.go (1)
87-88: Verify that environment variable expansion is secure
Environment variable expansion might introduce security risks if untrusted input is used or if the environment variables contain sensitive data. Confirm that the configuration files are from trusted sources and that the expanded content does not expose or log sensitive information.
Run the following script to check for any occurrences of potentially sensitive variables:
✅ Verification successful
Let me analyze the expandEnvVars function and its usage to better understand the security implications.
Let me check the complete implementation of the environment variable expansion and the pattern used.
Let me check how the expanded content is used after processing and if there's any logging of the configuration.
Based on the gathered information, I can now provide a final response.
Environment variable expansion implementation appears secure
The implementation shows good security practices:
- Uses a well-defined regex pattern that only matches valid environment variable formats (
${var},$var,${var:-default}) - Expanded content is only written to a temporary file with restricted permissions (0644)
- No logging of expanded configuration content
- Configuration is read from a file and processed in memory without exposing sensitive data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of sensitive environment variables in the config file.
# Sensitive variable patterns (e.g., passwords, tokens, secrets)
sensitive_patterns='(PASSWORD|PASS|TOKEN|SECRET|KEY)'
# Search the expanded configuration content
grep -E "$sensitive_patterns" "$tempFile" && echo "Sensitive variables found in the configuration." || echo "No sensitive variables found."
Length of output: 206
Script:
#!/bin/bash
# Check the implementation of expandEnvVars and its usage
ast-grep --pattern 'func expandEnvVars($_) {
$$$
}'
# Also check where this function is used
rg "expandEnvVars" -A 3
Length of output: 765
Script:
#!/bin/bash
# Check the environment variable pattern and full implementation
rg "envVarPattern" -B 2 -A 10
# Also check how the content is being read and used
rg "content.*:=.*" -A 3 --context-separator="" internal/cli/serve/command.go
Length of output: 1353
Script:
#!/bin/bash
# Check how the expanded content is used
rg "expandedContent" -B 2 -A 5
# Check for any logging of configuration content
rg -i "log|print|fmt.+content" internal/cli/serve/command.go
Length of output: 1781
internal/cli/serve/command.go
Outdated
| tmpFile, err := os.CreateTemp("", "rr-processed-*.yaml") | ||
| if err != nil { | ||
| return "", errors.E(op, fmt.Errorf("could not create temporary config file: %w", err)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Close the temporary file to prevent resource leakage
The temporary file tmpFile is created with os.CreateTemp but is never closed, which can lead to file descriptor leakage. Ensure that you close the file after writing to it.
Apply this diff to close the temporary file:
tmpFile, err := os.CreateTemp("", "rr-processed-*.yaml")
if err != nil {
return "", errors.E(op, fmt.Errorf("could not create temporary config file: %w", err))
}
+defer func() {
+ _ = tmpFile.Close()
+}()
if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0644); err != nil {
return "", errors.E(op, fmt.Errorf("could not write processed config: %w", err))
}Alternatively, you can write directly to the temporary file and avoid using os.WriteFile:
tmpFile, err := os.CreateTemp("", "rr-processed-*.yaml")
if err != nil {
return "", errors.E(op, fmt.Errorf("could not create temporary config file: %w", err))
}
+defer func() {
+ _ = tmpFile.Close()
+}()
-if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0644); err != nil {
+if _, err = tmpFile.Write([]byte(expandedContent)); err != nil {
return "", errors.E(op, fmt.Errorf("could not write processed config: %w", err))
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
internal/cli/serve/command.go (2)
23-24: Consider pre-compiling the regex pattern at init time.The
envVarPatternregex is complex and used frequently. While it's already package-level, consider initializing it in aninit()function to ensure proper error handling during compilation.-var envVarPattern = regexp.MustCompile(`\$\{([^{}:\-]+)(?::-([^{}]+))?\}|\$([A-Za-z0-9_]+)`) +var envVarPattern *regexp.Regexp + +func init() { + var err error + envVarPattern, err = regexp.Compile(`\$\{([^{}:\-]+)(?::-([^{}]+))?\}|\$([A-Za-z0-9_]+)`) + if err != nil { + panic(fmt.Sprintf("failed to compile env var pattern: %v", err)) + } +}
115-125: Improve error context in processConfig error handling.While the error handling is functional, it could provide more context about the operation that failed.
// Process config and get temporary file path tempFile, err := processConfig(*cfgFile) if err != nil { - return errors.E(op, err) + return errors.E(op, fmt.Errorf("failed to process config file %q: %w", *cfgFile, err)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
internal/cli/serve/command.go(2 hunks)
🔇 Additional comments (1)
internal/cli/serve/command.go (1)
27-54: LGTM! The environment variable expansion logic is robust.
The implementation correctly handles:
- Variables with default values
${VAR:-default} - Simple variables
${VAR}or$VAR - Fallback to original string if variable not found
internal/cli/serve/command.go
Outdated
| if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0644); err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
Review security implications of temporary file permissions.
The temporary configuration file is created with 0644 permissions, which might expose sensitive environment variables to other users on the system. Consider using more restrictive permissions.
-if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0644); err != nil {
+if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0600); err != nil {
return "", err
}📝 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.
| if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0644); err != nil { | |
| return "", err | |
| } | |
| if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0600); err != nil { | |
| return "", err | |
| } |
internal/cli/serve/command.go
Outdated
| if !filepath.IsAbs(envFile) { | ||
| envFile = filepath.Join(filepath.Dir(cfgFile), envFile) | ||
| } | ||
|
|
||
| err = godotenv.Load(envFile) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add existence check for the env file before loading.
The code should verify if the env file exists before attempting to load it. This would provide a more user-friendly error message.
if !filepath.IsAbs(envFile) {
envFile = filepath.Join(filepath.Dir(cfgFile), envFile)
}
+if _, err := os.Stat(envFile); err != nil {
+ if os.IsNotExist(err) {
+ return "", fmt.Errorf("env file does not exist: %s", envFile)
+ }
+ return "", fmt.Errorf("error accessing env file: %w", err)
+}
+
err = godotenv.Load(envFile)
if err != nil {
return "", err
}📝 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.
| if !filepath.IsAbs(envFile) { | |
| envFile = filepath.Join(filepath.Dir(cfgFile), envFile) | |
| } | |
| err = godotenv.Load(envFile) | |
| if err != nil { | |
| return "", err | |
| } | |
| if !filepath.IsAbs(envFile) { | |
| envFile = filepath.Join(filepath.Dir(cfgFile), envFile) | |
| } | |
| if _, err := os.Stat(envFile); err != nil { | |
| if os.IsNotExist(err) { | |
| return "", fmt.Errorf("env file does not exist: %s", envFile) | |
| } | |
| return "", fmt.Errorf("error accessing env file: %w", err) | |
| } | |
| err = godotenv.Load(envFile) | |
| if err != nil { | |
| return "", err | |
| } |
internal/cli/serve/command.go
Outdated
| // Create temporary file with processed content | ||
| tmpFile, err := os.CreateTemp("", "rr-processed-*.yaml") | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer func() { | ||
| _ = tmpFile.Close() | ||
| }() | ||
|
|
||
| if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0644); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return tmpFile.Name(), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure temporary file cleanup on all error paths.
While the temporary file is properly closed with defer, it should be cleaned up if an error occurs during writing.
// Create temporary file with processed content
tmpFile, err := os.CreateTemp("", "rr-processed-*.yaml")
if err != nil {
return "", err
}
+tmpName := tmpFile.Name()
defer func() {
_ = tmpFile.Close()
+ if err != nil {
+ _ = os.Remove(tmpName)
+ }
}()
-if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0644); err != nil {
+if err = os.WriteFile(tmpName, []byte(expandedContent), 0644); err != nil {
return "", err
}
-return tmpFile.Name(), nil
+return tmpName, nil📝 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.
| // Create temporary file with processed content | |
| tmpFile, err := os.CreateTemp("", "rr-processed-*.yaml") | |
| if err != nil { | |
| return "", err | |
| } | |
| defer func() { | |
| _ = tmpFile.Close() | |
| }() | |
| if err = os.WriteFile(tmpFile.Name(), []byte(expandedContent), 0644); err != nil { | |
| return "", err | |
| } | |
| return tmpFile.Name(), nil | |
| } | |
| // Create temporary file with processed content | |
| tmpFile, err := os.CreateTemp("", "rr-processed-*.yaml") | |
| if err != nil { | |
| return "", err | |
| } | |
| tmpName := tmpFile.Name() | |
| defer func() { | |
| _ = tmpFile.Close() | |
| if err != nil { | |
| _ = os.Remove(tmpName) | |
| } | |
| }() | |
| if err = os.WriteFile(tmpName, []byte(expandedContent), 0644); err != nil { | |
| return "", err | |
| } | |
| return tmpName, nil | |
| } |
rustatian
added
the
C-enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2042 +/- ##
==========================================
- Coverage 35.95% 33.66% -2.29%
==========================================
Files 17 17
Lines 1018 1090 +72
==========================================
+ Hits 366 367 +1
- Misses 621 690 +69
- Partials 31 33 +2 ☔ View full report in Codecov by Sentry. |
|
I see that you support envfiles, but there are very few environment variables supported now, and it is not used to replace the configuration in rr.yml, my implementation is a more general scheme, can be more flexible, I think it is necessary, otherwise I need to maintain multiple rr.yml configuration files |
|
All envs are supported and used to replace placeholders, see configuration (config) plugin. Your implementation has several critical errors:
|
|
Reason for This PR
It is easier to configure environment variables in the.env file, Reduce the maintenance of configuration files
Description of Changes
It is implemented by variable substitution at the entry.
add .env file:
add variable in rr.yml:
run
rr serve -c rr.yml, then a temp config file will be used by rr :License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Summary by CodeRabbit