refactor: unify SecretVar env/vault fields into single SecretRef/FromSecret - #4598
refactor: unify SecretVar env/vault fields into single SecretRef/FromSecret#4598BearTS wants to merge 1 commit into
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughSummary by CodeRabbitRelease Notes
Walkthrough
ChangesSecretVar Unified Reference Model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BearTS
changed the title
Confidence Score: 2/5Not safe to merge — seven test files that were not included in the PR still reference removed SecretVar methods and fields, so The production code sweep is thorough and the unified abstraction is clean, but the test suite across framework/configstore and transports/bifrost-http will fail to compile because IsFromEnv(), IsFromVault(), .FromEnv, .EnvVar, and .VaultRef are all removed without updating the seven test files that rely on them. There is also a silent data-loss path for vault secrets stored in the old JSON object format, and the config schema has not been updated to document the new field names. framework/configstore/tables/virtualkey_secretvar_test.go, framework/configstore/tables/encryption_test.go, framework/configstore/clientconfig_redaction_test.go, framework/configstore/encryption_test.go, transports/bifrost-http/lib/config_test.go, transports/bifrost-http/lib/ctx_test.go, scripts/bifrost-migration-cli/modelconformance_test.go — all reference removed fields/methods and will not compile. transports/config.schema.json needs new secret_ref/from_secret entries. core/schemas/secretvar.go needs vault_var/from_vault backward compat added to secretVarCompat. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Input: string or JSON"] --> B{JSON object?\nhas 'value' +\n'secret_ref' or 'env_var'}
B -- Yes --> C{New format?\nsecret_ref != ''\nor from_secret=true}
C -- Yes --> D["Set SecretRef=secret_ref\nFromSecret=true"]
C -- No --> E{Backward compat?\nfrom_env=true\n&& env_var != ''}
E -- Yes --> F["Set SecretRef='env.'+env_var\nFromSecret=true\nResolve env var"]
E -- No --> G{Legacy format?\nvalue == env_var\nstarts with 'env.'}
G -- Yes --> H["Set SecretRef=env_var\nFromSecret=true\nResolve env var"]
G -- No --> I["Resolve SecretRef\nif vault. or env."]
B -- No --> J{Plain string\nstarts with 'vault.'}
J -- Yes --> K["SecretRef=val\nFromSecret=true\nLookupVault"]
J -- No --> L{Plain string\nstarts with 'env.'}
L -- Yes --> M["SecretRef=val\nFromSecret=true\nos.LookupEnv"]
L -- No --> N["Literal value\nSecretRef=''\nFromSecret=false"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Input: string or JSON"] --> B{JSON object?\nhas 'value' +\n'secret_ref' or 'env_var'}
B -- Yes --> C{New format?\nsecret_ref != ''\nor from_secret=true}
C -- Yes --> D["Set SecretRef=secret_ref\nFromSecret=true"]
C -- No --> E{Backward compat?\nfrom_env=true\n&& env_var != ''}
E -- Yes --> F["Set SecretRef='env.'+env_var\nFromSecret=true\nResolve env var"]
E -- No --> G{Legacy format?\nvalue == env_var\nstarts with 'env.'}
G -- Yes --> H["Set SecretRef=env_var\nFromSecret=true\nResolve env var"]
G -- No --> I["Resolve SecretRef\nif vault. or env."]
B -- No --> J{Plain string\nstarts with 'vault.'}
J -- Yes --> K["SecretRef=val\nFromSecret=true\nLookupVault"]
J -- No --> L{Plain string\nstarts with 'env.'}
L -- Yes --> M["SecretRef=val\nFromSecret=true\nos.LookupEnv"]
L -- No --> N["Literal value\nSecretRef=''\nFromSecret=false"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| FromSecret: e.FromSecret, | ||
| } | ||
| } | ||
|
|
||
| // UnmarshalJSON unmarshals the value from JSON. | ||
| func (e *SecretVar) UnmarshalJSON(data []byte) error { | ||
| val := string(data) | ||
| // Cleanup string if required | ||
| // Use strconv.Unquote to properly handle JSON string escape sequences | ||
| // This converts "\"{\\\"key\\\":\\\"value\\\"}\"" to "{\"key\":\"value\"}" | ||
| if unquoted, err := strconv.Unquote(val); err == nil { | ||
| val = unquoted | ||
| } | ||
| // Check if the incoming data is a valid JSON object matching the SecretVar schema. | ||
| if sonic.Valid(data) { | ||
| valueNode, _ := sonic.Get(data, "value") | ||
| envNode, _ := sonic.Get(data, "env_var") | ||
| if valueNode.Exists() && envNode.Exists() { | ||
| // Use a type alias to avoid infinite recursion (alias doesn't inherit methods) | ||
| type secretVarAlias SecretVar | ||
| var secretVar secretVarAlias | ||
| if err := sonic.Unmarshal(data, &secretVar); err == nil { | ||
| e.Val = secretVar.Val | ||
| e.FromEnv = secretVar.FromEnv | ||
| e.EnvVar = secretVar.EnvVar | ||
| e.FromVault = secretVar.FromVault | ||
| e.VaultRef = secretVar.VaultRef | ||
| secretRefNode, _ := sonic.Get(data, "secret_ref") | ||
| if valueNode.Exists() && (envNode.Exists() || secretRefNode.Exists()) { | ||
| type secretVarCompat struct { | ||
| Val string `json:"value"` | ||
| SecretRef string `json:"secret_ref"` | ||
| FromSecret bool `json:"from_secret"` | ||
| // backward compat: env_var/from_env (shipped) | ||
| EnvVar string `json:"env_var"` | ||
| FromEnv bool `json:"from_env"` | ||
| } | ||
| var raw secretVarCompat | ||
| if err := sonic.Unmarshal(data, &raw); err == nil { | ||
| e.Val = raw.Val | ||
|
|
||
| // Explicit vault reference: {from_vault: true, vault_var: "vault.path"} | ||
| if e.FromVault && e.VaultRef != "" { | ||
| if !strings.HasPrefix(e.VaultRef, "vault.") { | ||
| e.VaultRef = "vault." + e.VaultRef | ||
| // New format | ||
| if raw.SecretRef != "" || raw.FromSecret { | ||
| e.SecretRef = raw.SecretRef | ||
| e.FromSecret = raw.FromSecret | ||
| } else if raw.FromEnv && raw.EnvVar != "" { | ||
| // Backward compat: env | ||
| ref := raw.EnvVar | ||
| if !strings.HasPrefix(ref, "env.") { | ||
| ref = "env." + ref | ||
| } | ||
| e.Val = e.VaultRef | ||
| if vaultValue, ok := LookupVault(e.VaultRef); ok { | ||
| e.Val = vaultValue | ||
| e.SecretRef = ref | ||
| e.FromSecret = true | ||
| if envValue, ok := os.LookupEnv(strings.TrimPrefix(ref, "env.")); ok { | ||
| e.Val = envValue | ||
| } else { | ||
| e.Val = "" | ||
| } | ||
| return nil | ||
| } | ||
| // Old format: value == env_var == "env.XXX" | ||
| if strings.HasPrefix(e.Val, "env.") && e.Val == e.EnvVar { | ||
| } else if strings.HasPrefix(raw.Val, "env.") && raw.Val == raw.EnvVar { | ||
| // Old format: value == env_var == "env.XXX" | ||
| e.SecretRef = raw.EnvVar | ||
| e.FromSecret = true | ||
| e.Val = "" | ||
| envValue, ok := os.LookupEnv(strings.TrimPrefix(e.EnvVar, "env.")) | ||
| if ok { | ||
| if envValue, ok := os.LookupEnv(strings.TrimPrefix(raw.EnvVar, "env.")); ok { | ||
| e.Val = envValue | ||
| } | ||
| e.FromEnv = true | ||
| return nil | ||
| } | ||
| // New format: value is empty, from_env=true, env_var holds the reference | ||
| if e.Val == "" && e.FromEnv && strings.HasPrefix(e.EnvVar, "env.") { | ||
| if envValue, ok := os.LookupEnv(strings.TrimPrefix(e.EnvVar, "env.")); ok { | ||
|
|
||
| // Resolve references | ||
| if e.FromSecret && strings.HasPrefix(e.SecretRef, "vault.") { | ||
| e.Val = e.SecretRef | ||
| if vaultValue, ok := LookupVault(e.SecretRef); ok { | ||
| e.Val = vaultValue |
There was a problem hiding this comment.
The secretVarCompat struct includes EnvVar/FromEnv for old env-format backward compat, but has no VaultRef/FromVault fields. A record stored in the old format {"value":"","env_var":"","from_env":false,"vault_var":"vault.bifrost/key","from_vault":true} will match the valueNode.Exists() && envNode.Exists() guard (because env_var is present, even if empty) and enter the unmarshal block, but every condition thereafter falls through with an empty SecretRef and FromSecret=false. The vault reference is silently lost, causing the field to behave as an empty literal value rather than resolving the secret.
The old test cases for this exact input ("struct form with from_vault") were removed without a replacement. Adding VaultVar string and FromVault bool to secretVarCompat and mapping them to SecretRef/FromSecret would close this gap.
1 participant
Summary
Unifies the
SecretVarstruct's dual-track secret sourcing (FromEnv/EnvVar+FromVault/VaultRef) into a single, source-agnostic pair of fields:FromSecret boolandSecretRef string. This eliminates branching throughout the codebase wherever env and vault references were handled separately, and establishes a cleaner abstraction for any future secret backend.Changes
FromEnv/EnvVar/FromVault/VaultReffields onSecretVarwithFromSecret boolandSecretRef stringSecretRefnow holds the full prefixed reference (e.g.env.MY_VARorvault.path/to/secret) regardless of backendIsFromEnv()andIsFromVault()methods removed; replaced by a singleIsFromSecret()methodUnmarshalJSON,NewSecretVar, andScanso existing stored records using the oldenv_var/from_envJSON format continue to deserialize correctlyStoreVaultSecretVarnow setsSecretRefto"vault." + path(previously it stored the raw path inVaultRef)BeforeSave/AfterFindhooks, HTTP config handlers, and pluginsisSecretVarObjectandmarshalSecretVarObjectin the HTTP plugin handler updated to recognize both the newsecret_ref/from_secretkeys and the legacyenv_var/from_envkeysTestSecretVar_UnmarshalJSON_FullStructurereplaced withTestSecretVar_UnmarshalJSON_BackwardCompatto explicitly cover the old formatType of change
Affected areas
How to test
go test ./core/schemas/... ./core/providers/... ./framework/configstore/... ./transports/... ./plugins/...Verify that existing configurations using
env.VARandvault.path/to/secretreferences continue to resolve correctly after the schema change. Confirm that records persisted with the oldenv_var/from_envJSON format deserialize into the new fields without data loss.Breaking changes
The
SecretVarstruct's exported fields have changed. Any code outside this repository that directly reads or writesEnvVar,FromEnv,VaultRef, orFromVaultwill need to migrate toSecretRefandFromSecret. The JSON serialization format changes fromenv_var/from_env/vault_var/from_vaulttosecret_ref/from_secret; backward-compatible deserialization is provided for reading old records, but newly written records will use the new format.Related issues
N/A
Security considerations
Secret reference metadata (
SecretRef) is preserved through redaction and serialization paths so that env/vault references remain identifiable in API responses without exposing resolved values. TheFullyRedactedandRedactedmethods carrySecretRef/FromSecretthrough unchanged, maintaining the same security properties as before.Checklist
docs/contributing/README.mdand followed the guidelines