fix: hash the full source spec for the cache key - #9970
Open
gangadhar-res wants to merge 3 commits into
Open
Conversation
The source cache key hashed resource.ReportConfig(), which exists to
redact resource configuration for reports and predates the cache.
Eleven plugins omit result-affecting spec fields from ReportConfig
(e.g. shell omits workdir, yaml omits documentindex), so two distinct
sources could silently share a cache entry and the second one received
the first one's value.
Hash the full user-provided spec from ResourceConfig instead, keeping
ReportConfig for its original reporting purpose. Secret-bearing fields
are safe to include since only the SHA256 digest is ever stored or
logged, and the encoding stays deterministic: yaml.v3 decodes specs
into map[string]interface{} and encoding/json sorts map keys. The
resource is still instantiated first so unknown kinds or malformed
specs keep returning the empty-key cache-miss sentinel.
Regression tests cover two shell sources differing only in workdir
(a field ReportConfig omits) both as typed specs and as YAML-decoded
map specs, plus identical specs still sharing a key.
Fixes updatecli#9849
Signed-off-by: Gangadhar Chalapaka <gangadhar@resolve.ai>
Author
|
@olblak Could I have a review on this pr ? thanks |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The source cache key (
pkg/core/cache/source.go) now hashes the full user-provided spec fromResourceConfiginstead ofresource.ReportConfig().Why
ReportConfig()exists to redact resource configuration for reports and predates the cache (#4898). Eleven plugins omit result-affecting spec fields from it (e.g.shellomitsworkdir,yamlomitsdocumentindex), so two distinct sources could silently share a cache entry and the second received the first one's value — no error, no warning, just a wrong value written to the target.This implements option 2 from the issue discussion: the cache key gets its own complete input,
ReportConfigkeeps its original reporting purpose, and no per-plugin changes are needed — the whole bug class is removed at the one call site.Notes:
map[string]interface{}andencoding/jsonmarshals maps with sorted keys.Testing
shellsources differing only inworkdir(a fieldReportConfigomits) get different keys, while identical specs still share one — both as typed specs and as YAML-decoded map specs, plus hash stability across decodes. Both new tests fail onmainby construction.go test ./pkg/core/cache/passes;go build ./...,gofmt,go vet, andgolangci-lintintroduce no new findings.Fixes #9849