Nathan.gallet/helm deterministic rendering#214
Nathan.gallet/helm deterministic rendering#214gh-worker-dd-mergequeue-cf854d[bot] merged 9 commits into
Conversation
… sprig functions Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Add test case for randAscii substitution in TestApplyDeterministicSubstitutions. Clean up dead code in makeDeterministic by removing unnecessary reassignments and nil-checks that depend on pointer side-effects. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ld access The `\bnow\b` pattern used to match the Sprig `now` function was too broad. Word boundaries fire between non-word characters ($, .) and word characters (n), causing false positives on `$now` variables and `.Values.now` field accesses. After a match is found, check if it is preceded by $ or .; if so, skip it. This ensures only actual function calls to `now` are substituted. Add test cases covering both false-positive cases: - `$now` variable reference should remain unchanged - `.Values.now` field access should remain unchanged Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Moved the guard logic for the 'now' pattern from a hardcoded index check into the deterministicPattern struct itself. This eliminates the maintainability issue where reordering patterns could silently change which pattern gets guarded. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
🎯 Code Coverage (details) 🔗 Commit SHA: 213e607 | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0700f51dd2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| var replacements []replacement | ||
|
|
||
| for _, p := range deterministicPatterns { | ||
| matches := p.re.FindAllStringIndex(s, -1) |
There was a problem hiding this comment.
When a chart template contains literal text that happens to mention one of these tokens, this scans the entire file and rewrites it even though it is not a Sprig call. For example, a ConfigMap script/comment containing randAlphaNum 8, or a template string such as include "uuidv4", is corrupted before Helm renders it (the latter can even produce an invalid template with nested quotes), so the scanner may analyze a manifest that the chart would never render or fail to render the chart at all. The replacement logic should be limited to actual template actions and avoid quoted strings/comments.
Useful? React with 👍 / 👎.
…string args
Substitutions are now scoped to {{ ... }} action blocks via templateActionSpans/inAnySpan,
preventing rewrites of literal text (e.g. shell scripts in ConfigMap data). A shared guard
notPrecededByQuoteVarOrField is applied to all patterns so string arguments like
`include "uuidv4"` are not treated as function calls.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Replace the single-byte quote guard with insideQuotedStringInSpan, which
counts unescaped double-quotes from the opening {{ to the match position.
An odd count means the match is inside a string literal and should be skipped.
This prevents patterns like {{ required "set this now" .Values.foo }} and
{{ printf "prefix randAlphaNum 8" }} from being corrupted by substitution.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
|
||
| // deterministicPatterns maps each non-deterministic sprig function regex to a replacement generator. | ||
| // The generator receives the line number of the match. | ||
| var deterministicPatterns = []deterministicPattern{ |
There was a problem hiding this comment.
I don't think a chart using {{ randAlphaNum .Values.len }} will be matched here
…tic substitution
- Exclude {{/* ... */}} comment blocks from template action spans so function
names mentioned in comments are never rewritten
- Extend argument pattern from \d+ to [\w$.]+ so calls like
randAlphaNum .Values.suffixLen and randAlphaNum $len are replaced
- Add randBytes pattern (was missing entirely)
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
2 participants
Summary
Helm charts using non-deterministic Sprig functions (
randAlphaNum,uuidv4,now, etc.) in resource names produce a different rendered name on every scan, generating a newDATADOG_FINGERPRINTeach time and re-reporting the same finding as new indefinitely.Root cause:
engine.Engine(used internally byaction.Install.Run) pulls its funcmap fromsprig.TxtFuncMap()with no injection point —funcMap()andinitFunMap()are unexported, andEnginehas noFuncMapfield.Fix: Pre-process each chart template's raw bytes before rendering, replacing non-deterministic function calls with stable, line-number-seeded stubs:
randAlphaNum N,randAlpha N,randAscii N"ddscan{lineNum:04d}"randNumeric N"{lineNum:08d}"uuidv4"00000000-0000-0000-{lineNum:04d}-{lineNum:012d}"now(toDate "2006-01-02" "2000-01-01")— returnstime.Time, works in any pipelineStubs are seeded by line number so two resources in the same file using the same function produce distinct, stable names across every scan. Replacements are applied back-to-front by byte offset so earlier match positions stay valid during mutation. The
nowguard skips matches preceded by$or.to avoid corrupting$nowvariable declarations and.Values.nowfield accesses.Crypto functions (
genSignedCert,genCA, etc.) are deferred — they return structs, not strings, and rarely appear in resource names.Changes
pkg/resolver/helm/helm.go— addsdeterministicPatternstruct,deterministicPatternstable,applyDeterministicSubstitutions, andmakeDeterministic(mirrorssetIDpattern, recurses into sub-charts).makeDeterministicis called beforesetIDinrunInstall.pkg/resolver/helm/helm_deterministic_test.go— unit tests for each function type + two-lines distinctness + no-match passthrough; integration test callsResolvetwice and asserts byte-identical output.test/fixtures/test_helm_nondeterministic/— minimal fixture chart exercising all covered function types.Testing
All existing
TestHelm_Resolvecases remain green.TestHelm_DeterministicRenderingcallsResolvetwice on the new fixture and assertsrequire.Equal(first.File, second.File).