fix: don't panic when redact.Set replaces an existing store - #5144
fix: don't panic when redact.Set replaces an existing store#5144ankit090701 wants to merge 1 commit into
Conversation
redact.Set panicked if called a second time in the same process, e.g. when a library consumer constructs and executes cli.Command(id) more than once: AppClioSetupConfig registers an initializer that calls redact.Set(state.RedactStore) on every clio Execute() call, and each call gets a fresh, independently-constructed RedactStore from clio - so a second invocation always panicked with "replace existing redaction store (probably unintentional)". The sibling bus.Set and log.Set singleton packages already just overwrite on repeated calls without complaint; redact.Set is now consistent with that. Redactions added to a store before it's replaced no longer apply to output produced afterwards, but that's expected: each independent command invocation gets its own store. Fixes anchore#2285 Signed-off-by: ankit090701 <ankitanku090701@gmail.com>
|
Thanks for the PR and linking the issue @ankit090701. I'm going to go check and see if we can get a fix done in CLO. Yes the panic is entirely syft's (clio never panics), but the forced re-Set is a clio design consequence.
If we update clio and flip the ownership then the problem disappears instead of being papered over. Something like: // setup_config.go
type SetupConfig struct {
...
RedactStore redact.Store
}
func (c *SetupConfig) WithRedactStore(s redact.Store) *SetupConfig {
c.RedactStore = s
return c
}cc @kzantow for feedback on the potential clio change vs this PR landing in syft. If 👍 just say so and I'll take it on. |
| if store != nil { | ||
| // if someone is trying to set a redaction store and we already have one then something is wrong. The store | ||
| // that we're replacing might already have values in it, so we should never replace it. | ||
| panic("replace existing redaction store (probably unintentional)") |
There was a problem hiding this comment.
This panic is intentional: if a store is replaced, there could be values that should be redacted which no longer get redacted. Removing this panic is not the right fix.
Based on the stack overflow link, it seems like you are using the CLI directly, but you should probably be using Syft as a library instead.
Description
redact.Setpanicked if it was called a second time in the same process. This breaks a documented, intended use case: embedding syft as a library and constructing/executingcli.Command(id)more than once (see the linked issue and https://stackoverflow.com/questions/77387892/unable-to-call-anchore-syft-library-command-multiple-times-when-embedded-in-go-a).AppClioSetupConfig(cmd/syft/internal/clio_setup_config.go) registers a clio initializer that callsredact.Set(state.RedactStore)on everyExecute(). Each call tocli.Command(id)gets its own freshly-constructedclio.State, and thus its own newRedactStore- so the second time a consumer executes a command in the same process,redact.Setalways panicked with"replace existing redaction store (probably unintentional)".Looking at the sibling singleton packages in
internal/,bus.Setandlog.Setboth just overwrite their package-level singleton on repeated calls, no questions asked -redact.Setis the only one of the three that panics on replace. This PR brings it in line with its siblings.I confirmed this is still present on current
mainby reproducing the exact stack trace from the issue viacli.Command(id)+Execute()called twice in a row (executing thescansubcommand against a temp dir, twice) - it panics atinternal/redact/redact.go:11through the identical code path described in the issue (clio.(*application).runInitializers->PostLoad->fangs.postLoad/loadConfig->setupCommand-> cobraExecute).Type of change
Checklist
Testing
TestSet_ReplacingExistingStoreDoesNotPanic(internal/redact/redact_test.go), which sets a store, adds a redaction, replaces the store, and confirms: (a) no panic, (b) the new store doesn't inherit redactions from the replaced one, (c) the new store works normally going forward.redact.gochange: the test fails reproducing the exact panic message from this issue.cli.Command(id)twice, executingscan dir:<tmp> -o jsonboth times: panics with the identical stack trace as the issue on unmodified code; passes cleanly with the fix.internal/...andcmd/syft/...suites. The only failures are pre-existing and environmental (this sandbox has nodockerorzipbinaries available, and runs as root which bypasses a permission-denied test ininternal/cache) - unrelated tointernal/redact, which has no relationship to any of those packages.Issue references
Fixes #2285