fix(config): resolve secrets only for selected context#3565
fix(config): resolve secrets only for selected context#3565SergioChan wants to merge 2 commits intogetporter:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Scopes config template-variable discovery (and therefore second-pass ${secret.*} resolution) to only the selected context in multi-context config files, preventing secret lookups for other contexts during normal config loading.
Changes:
- Recomputes
cfg.templateVariablesfrom the extracted selected context config map in multi-context configs. - Adds
listTemplateVariablesForContextto discover template variables within a context-only YAML subtree. - Adds a regression test ensuring
current-context: localdoes not attempt to resolve secrets referenced only under another context.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/config/loader.go | Re-scopes template variable discovery to the selected context to avoid cross-context secret resolution. |
| pkg/config/loader_test.go | Adds a regression test ensuring secrets from unselected contexts are not resolved. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil | ||
| } | ||
|
|
||
| engine := liquid.NewEngine() | ||
| engine.Delims("${", "}", "${%", "%}") | ||
| tmpl, err := engine.ParseTemplate(cfgContents) | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
listTemplateVariablesForContext silently returns nil when yaml.Marshal or ParseTemplate fails. If that happens, loadFinalPass will be skipped and any ${secret.*} in the selected context will remain rendered as empty from the first pass, leading to a subtly broken config without an error. Consider returning an error from listTemplateVariablesForContext (and propagating it from LoadFromViper) or at least logging/handling the failure explicitly so config loading fails fast when template-variable discovery can’t run.
| return nil | |
| } | |
| engine := liquid.NewEngine() | |
| engine.Delims("${", "}", "${%", "%}") | |
| tmpl, err := engine.ParseTemplate(cfgContents) | |
| if err != nil { | |
| return nil | |
| // Log the error and return an empty slice so callers don't silently skip the final pass. | |
| fmt.Printf("error marshaling context config for template variable discovery: %v\n", err) | |
| return []string{} | |
| } | |
| engine := liquid.NewEngine() | |
| engine.Delims("${", "}", "${%", "%}") | |
| tmpl, err := engine.ParseTemplate(cfgContents) | |
| if err != nil { | |
| // Log the error and return an empty slice so callers don't silently skip the final pass. | |
| fmt.Printf("error parsing context template for template variable discovery: %v\n", err) | |
| return []string{} |
Signed-off-by: Sergio <c@rct.ai>
SergioChan
force-pushed
the
fix-context-secret-scope-3558
branch
from
a0b2fb9 to
0d94259
Compare
|
Pushed a follow-up commit with DCO sign-off so the DCO check should pass now. No code changes were introduced in this update (commit was amended to add |
|
@SergioChan Thanks for the PR, could you please create an integration test for this too. I have been working on #3561, and found a lot of gotchas that is hard to validate in a unit test. |
Signed-off-by: sergiochan <yuheng9211@qq.com>
|
Thanks for the review — good call. I added an integration test to cover this behavior:
What it verifies:
Commit pushed to this PR branch:
Quick local validation performed:
Happy to adjust the test style/scope if you'd prefer a different integration path. |
|
Thanks for calling that out — I added an integration test in commit The new case exercises selected-context secret resolution behavior so the regression is validated beyond unit-level coverage. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cfgContents, err := yaml.Marshal(contextConfigMap) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| engine := liquid.NewEngine() | ||
| engine.Delims("${", "}", "${%", "%}") | ||
| tmpl, err := engine.ParseTemplate(cfgContents) | ||
| if err != nil { | ||
| return nil | ||
| } |
| require.NoError(t, err) | ||
| assert.Equal(t, "filesystem", c.Data.DefaultSecretsPlugin) | ||
| } | ||
|
|
|
|
||
| p.Config.DataLoader = config.LoadFromFilesystem() | ||
|
|
||
| _, err := p.Config.Load(ctx, func(ctx context.Context, secretKey string) (string, error) { |
|
@SergioChan Hi, will you have time to take a look at the Copilot comments. |
|
Yep — I can take this on. I’ll go through the Copilot comments carefully and post a focused update so we can keep this implementation simple (and avoid drifting toward #3561 complexity unless needed). |
What does this change
config:block in multi-context config files.${secret.*}references that belong to other contexts.current-context: localdoes not attempt to resolve secrets that are only referenced underprod.This makes
porter config showand normal config loading work when contexts use different secret stores.What issue does it fix
Closes #3558
Notes for the reviewer
cfg.templateVariablesis now recomputed from the selected context map so second-pass secret resolution only evaluates relevant keys.Checklist