Skip to content

Config: Add support for environment variable expansion. #111

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.

Sign up for GitHub

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

Open
ggreer wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Config: Add support for environment variable expansion. #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions pkg/bsql/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
"errors"
"fmt"
"os"
"regexp"
"slices"
"strings"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -66,7 +69,7 @@
User string `yaml:"user" json:"user"`

// Password is the database password used for authentication
Password string `yaml:"password" json:"password"` //nolint:gosec // We need a password to connect to the database.

Check failure on line 72 in pkg/bsql/config.go

View workflow job for this annotation

GitHub Actions / verify / lint 

directive `//nolint:gosec // We need a password to connect to the database.` is unused for linter "gosec" (nolintlint)

// Params contains additional connection parameters (e.g., {"sslmode": "disable", "timeout": "30s"})
Params map[string]string `yaml:"params" json:"params"`
Expand Down Expand Up @@ -500,15 +503,52 @@
return config, nil
}

type LookupFunc func(key string) (string, bool)

func expandEnvironmentVariables(_ context.Context, data []byte, lookup LookupFunc) ([]byte, error) {
var missingVars []string
envVarRegex := regexp.MustCompile(`\${([^}]+)}`)

dataStr := envVarRegex.ReplaceAllStringFunc(string(data), func(match string) string {
envVar := match[2 : len(match)-1]
if envVal, exists := lookup(envVar); exists {
if strings.ContainsAny(envVal, "\n\r") {
// Quote multi-line values so they remain a single YAML scalar.
escaped := strings.ReplaceAll(envVal, `\`, `\\`)
escaped = strings.ReplaceAll(escaped, `"`, `\"`)
escaped = strings.ReplaceAll(escaped, "\n", `\n`)
escaped = strings.ReplaceAll(escaped, "\r", `\r`)
return `"` + escaped + `"`
}
return envVal
}
if !slices.Contains(missingVars, envVar) {
missingVars = append(missingVars, envVar)
}
return match
})
Comment on lines +512 to +529
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 6, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Expand parsed YAML scalars, not raw file text.

This replacement runs before YAML parsing, so it also rewrites ${...} inside comments, keys, and already-quoted values. That breaks valid configs in common cases—for example, a commented example like # ${DB_PASSWORD} becomes a hard startup error, and a quoted placeholder with a multiline secret produces invalid YAML. Please expand only yaml.Node string scalar values after parsing.

Possible direction 
- dataStr := envVarRegex.ReplaceAllStringFunc(string(data), func(match string) string {
-   ...
- })
- return []byte(dataStr), nil
+ var root yaml.Node
+ if err := yaml.Unmarshal(data, &root); err != nil {
+   return nil, err
+ }
+
+ // Walk the parsed tree and expand only scalar string nodes.
+ // Then marshal the modified node tree back to bytes.
+ walkStringScalars(&root, func(value string) (string, error) {
+   return expandString(value, lookup)
+ })
+
+ out, err := yaml.Marshal(&root)
+ if err != nil {
+   return nil, err
+ }
+ return out, nil
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/bsql/config.go` around lines 512 - 529, The current
envVarRegex.ReplaceAllStringFunc run on dataStr mutates the raw file text
(including comments, keys, and already-quoted scalars); instead parse the YAML
into a yaml.Node and perform the ${...} expansion only on node.Value for string
scalar nodes. Replace the ReplaceAllStringFunc usage by: unmarshal into a root
*yaml.Node, recursively walk nodes and for nodes where Kind==yaml.ScalarNode
(and/or Tag=="!!str") run the envVarRegex replacement using lookup and append to
missingVars as before; when a replacement contains newlines or needs escaping,
set the node.Style to yaml.DoubleQuotedStyle and escape backslashes, quotes and
newline characters in node.Value so the resulting node remains valid YAML;
finally marshal the node back to bytes (replacing dataStr) and preserve the
existing missingVars collection and lookup semantics.


if len(missingVars) > 0 {
return nil, fmt.Errorf("missing environment variables: %s", strings.Join(missingVars, ", "))
}
Comment on lines +531 to +533
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 6, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Prefix the new errors with baton-sql.

Both new error messages omit the required connector prefix. Please make these baton-sql: missing environment variables: ... and baton-sql: failed to expand environment variables: %w.

Suggested diff 
- return nil, fmt.Errorf("missing environment variables: %s", strings.Join(missingVars, ", "))
+ return nil, fmt.Errorf("baton-sql: missing environment variables: %s", strings.Join(missingVars, ", "))

- return nil, fmt.Errorf("failed to expand environment variables: %w", err)
+ return nil, fmt.Errorf("baton-sql: failed to expand environment variables: %w", err)

As per coding guidelines, **/*.go: Always wrap errors with %w format verb and include connector prefix (baton-*) in error messages.

Also applies to: 547-548

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/bsql/config.go` around lines 531 - 533, Update the error messages in
pkg/bsql/config.go that validate/expand environment variables to include the
connector prefix and proper wrapping: change the "missing environment variables"
return to prefix with "baton-sql: " (e.g., "baton-sql: missing environment
variables: ...") and modify the error returned when expansion fails to include
the "baton-sql: " prefix and use %w to wrap the original error (e.g.,
"baton-sql: failed to expand environment variables: %w"). Locate the two error
returns used after env validation/expansion (the missingVars join and the
expansion failure) and update their fmt.Errorf calls accordingly.


return []byte(dataStr), nil
}

// LoadConfigFromFile reads a YAML configuration file from the given path and parses its content into a Config struct.
func LoadConfigFromFile(path string) (*Config, error) {
func LoadConfigFromFile(ctx context.Context, path string) (*Config, error) {
data, err := os.ReadFile(path)
if err != nil {
return nil, err
}

// First pass: expand environment variables in string nodes
replacedData, err := expandEnvironmentVariables(ctx, data, os.LookupEnv)
if err != nil {
return nil, fmt.Errorf("failed to expand environment variables: %w", err)
}
config := &Config{}
err = yaml.Unmarshal(data, config)
err = yaml.Unmarshal(replacedData, config)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/connector/connector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (c *Connector) Validate(ctx context.Context) (annotations.Annotations, erro

// New returns a new instance of the connector.
func New(ctx context.Context, configFilePath string) (*Connector, error) {
c, err := bsql.LoadConfigFromFile(configFilePath)
c, err := bsql.LoadConfigFromFile(ctx, configFilePath)
if err != nil {
return nil, err
}
Expand Down
Loading