feat(instanceidhandler): support CronJob alternate names and template hashes

[shellyco-code]

Overview

This PR resolves the // TODO add case for CronJobs here, or deprecate in instanceidhandler/v1/containerinstance/initializers.go.

When deserializing instance ID strings for CronJob child Jobs (which use alternate names incorporating a calculated pod template hash), GenerateInstanceIDFromString now uses a best-effort heuristic matching the producer's template hash domain (rand.SafeEncodeString(fmt.Sprint(Sum32()))) to populate AlternateName and TemplateHash.

Changes Made

• In containerinstance/initializers.go: Implemented best-effort template hash recovery for Kind == "Job" instance ID strings matching the producer domain. Documented the API contract and acknowledged the tradeoff for standalone literal Jobs whose names happen to end in matching suffixes (such as backup-db or backup-4). Ordinary CronJob workloads (Kind == "CronJob") remain strictly literal.
• In helpers/keys.go: Added IsTemplateHash validating canonical uint32 encodings in the producer domain [4-9bcdf]{1,10} (preventing impossible values like leading-zero 44 or values exceeding math.MaxUint32). Documented the best-effort contextual recovery and ambiguity contract.
• In containerinstance/initializers_test.go & initializers2_test.go: Added generator-to-parser round trip tests, literal Job tradeoff tests (backup-db, backup-4), ordinary CronJob preservation tests (backup-xxxxxxxx), and timestamp negative tests.
• In helpers/keys_test.go: Added test cases for numeric, short, canonical, and non-canonical strings.

How to Test

Run all unit tests:

go test -v ./instanceidhandler/v1/...
go test ./...
Related issues/PRs
Fixes #177

Checklist before requesting a review
 My code follows the style guidelines of this project
 I have performed a self-review of my code
 I have added thorough tests
 New and existing unit tests pass locally with my changes

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 134b7b1b-2234-41fc-9140-18ee4d591fac

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[matthyx]

Request changes: the reconstruction gap in #177 is real, but the suffix classifier introduces two confirmed metadata defects (inline P2 findings). No identity-string or slug regression was established.

Reviewed head 3800cd9dc5a9727c1b4f27a0605a83f263059671 against main at ea00709ca41bb39638aadd662299663a39d9e326.

Need and history. Main's parser still leaves AlternateName/TemplateHash empty for the documented kubevuln-scheduler-b449cf78f example. This is established by source inspection; no baseline execution was performed. #177 explicitly requests this recovery. Merged #102 introduced CronJob hashing; #132 and #137 extend generation. Merged #176, linked to #175, normalizes the producer's input and does not fix deserialization. Its input-mutation objection was resolved with a deep copy before approval. Closed #75 discussed redundant slug tests and educational retention, not rejection of CronJob recovery. Closed #144 concerns draft ECS support, with no recorded substantive rejection relevant here.

Searched paginated all-state PR and issue titles/bodies and indexed issue/PR searches using CronJob, GenerateInstanceIDFromString, AlternateName, template hash, initializers.go, instance ID and unix timestamp; inspected related discussions/reviews. No duplicate or superseding parser fix or prior rejection of this approach found. Searches are repository-scoped and limited to retrieved/indexed material; absence is not proof.

Findings. Two P2 correctness blockers: valid generated numeric hashes lose metadata, and literal CronJob names gain fabricated hash metadata. The producing encoder maps decimal digits to 456789bcdf, preserves length, and can also generate fewer than eight characters. More fundamentally, legacy serialization does not retain whether the name was literal or alternate; suffix syntax cannot prove provenance. Match the producer's full domain and define an explicit contextual/ambiguity contract, with generator-to-parser tests. This need not restore the original timestamp-bearing name, whose loss predates this PR.

Fresh validation. In Docker with dropped capabilities, no-new-privileges, read-only exported source, bounded resources, and no host credentials/sensitive mounts:

• Original go test -count=1 ./instanceidhandler/v1/...: passed.
• Reviewer-added generator/parser regression tests: failed for actual generated backup-8698448884 (parsed hash empty) and ordinary CronJob backup-xxxxxxxx (parsed hash becomes xxxxxxxx).
• go vet ./instanceidhandler/v1/...: passed.
• Changed-file gofmt and diff whitespace checks flag extra blank EOF lines in both helper files; nonblocking.

Latest-head CI reports successful build/Basic-Test; CodeQL is neutral. License Compliance reports error, “2 issues found”; cause is unverified, so not attributed to this patch or infrastructure. Main requires one approval and no status contexts. Full local suite, race testing, full lint and Kubernetes integration were not run. Added tests cover common strings, but miss producer round trips, numeric/short hashes and literal-name negatives.

The four-file scope adds no dependencies and preserves effective serialized names/hashed IDs/slugs; the realistic introduced risk is incorrect public TemplateHash/GetLabels metadata. Independent correctness review requests changes; independent architecture status is BLOCK. No security/concurrency/resource-lifecycle defect identified in this bounded change.

Immediately before submission: open, non-draft, mergeable, unchanged head/base; no existing reviews or inline findings to duplicate. Verdict: request changes. No merge, push, source-branch or settings changes.

[matthyx]

[P2] Do not discard hashes produced by the generator. GenerateInstanceIDFromRuntimeObj uses SafeEncodeString(decimal uint32), which can yield an entirely numeric hash. A Pod owned by Job backup-28677846 with container backup/image:196 generates backup-8698448884; parsing its GetStringFormatted() succeeds but GetTemplateHash() becomes empty instead of 8698448884, also losing the label. The encoder can generate fewer than eight characters, which the regex likewise rejects. Match the complete producing domain and add generator-to-parser regression coverage. Numeric hashes overlap timestamps, so define contextual recovery/ambiguity rather than simply removing this guard.

[matthyx]

[P2] Preserve literal CronJob names instead of inventing hash metadata. A normal CronJob named backup-xxxxxxxx, without owner references or hash labels, has empty TemplateHash when generated from the runtime object. Parsing its serialized ID now sets TemplateHash=xxxxxxxx and emits kubescape.io/instance-template-hash=xxxxxxxx. x is not even in the producer's encoded-decimal alphabet. Even literal names ending in a genuine-looking hash are ambiguous in the existing format. Keep ordinary CronJob names literal and require sufficient provenance/context for inferred Job metadata; add negative and producer round-trip tests.

[shellyco-code]

PTAL now

[matthyx]

Rechecked latest head 8712656221435d6047ffcaef8c31731259f10e3b against unchanged main ea00709ca41bb39638aadd662299663a39d9e326. Needs clarification; no approval yet.

The specific failures in my earlier review are fixed: numeric/short generated hashes are now recognized, ordinary CronJob names stay literal, and producer-to-parser tests were added. Fresh targeted tests, including my original numeric-hash and literal-CronJob reproductions, pass. The earlier formatting findings are also resolved.

Remaining contract question (P2 compatibility risk, not an unintended violation of the revised heuristic): containerinstance/initializers.go:38–42 infers metadata for every Job whose suffix matches. A standalone Job named backup-db or backup-4, with no owner references or hash labels, has an empty template hash from the runtime constructor; serializing and parsing it assigns db or 4, including the public template-hash label. Both suffixes really can be generated, so another regex cannot distinguish them from literal names. This behavior is now explicit in code comments, but differs from the PR description's promise to distinguish arbitrary hyphenated Job names.

Does the maintainer intend the existing public parser to return best-effort hash metadata with these false positives? If yes, document that on the public parsing API, add literal-Job cases acknowledging the tradeoff, and correct helpers/keys.go:156 (“unambiguously template hashes”). If literal Job metadata must remain accurate, recovery needs an explicit option or supplied provenance. This continues the existing thread; I am not duplicating it with another inline blocker.

Nonblocking precision: the regex accepts impossible canonical producer values (44 decodes to leading-zero 00; ffffffffff exceeds uint32). Describe it as an alphabet/length heuristic or validate canonical uint32 encodings.

Validation: credential-free Docker with read-only exported source, dropped capabilities, no-new-privileges and bounded resources. go test -count=1 ./instanceidhandler/v1/... and go vet ./instanceidhandler/v1/... pass; changed-file gofmt and diff whitespace checks pass. Separate literal-Job round-trip assertions reproduce the mismatch above. Full local suite, race testing, full lint, integration testing and baseline execution were not performed.

Latest-head CI run 37200592837 succeeds for builds/Basic-Test. License Compliance remains error (“2 issues found”), cause unverified; it is not required. Main requires one approval and no status contexts.

Need/history remain as established in the previous review (#177; merged #102/#132/#137/#176; closed #75 not a rejection of recovery). Refreshed paginated repository PR/issue history since that review found no competing change. This is bounded repository/API history, not proof no duplicate exists. Five changed files, no dependency additions; strings, hashes and slugs retain the effective name. No new security, concurrency or lifecycle blocker found. Independent correctness recommendation: COMMENT; architecture: WATCH for the intentional metadata tradeoff.

Immediately before submission: open, non-draft, mergeable, same reviewed head/base, no new review concerns to duplicate. Verdict: needs clarification. No source-branch modifications, push, merge or settings changes.

[matthyx]

Approved latest head 68ffc3bfad76eb2e1fd67f9ab26bc78126fe2936 against main at ea00709ca41bb39638aadd662299663a39d9e326. No remaining blockers found.

The clarification from my previous review is addressed: the public parser and PR description now explicitly promise best-effort Job metadata recovery, acknowledge literal-name collisions (backup-db, backup-4), and test that tradeoff. Ordinary CronJobs remain literal. Numeric/short hashes work; canonical validation rejects leading-zero and uint32-overflow encodings. The earlier “unambiguously” claim is corrected. Serialized strings, hashed IDs and slugs retain the effective name.

Fresh validation in credential-free Docker with read-only exported source, dropped capabilities, no-new-privileges and bounded resources:

• go test -count=1 ./instanceidhandler/v1/...: passed, including author regressions, my original numeric-hash/CronJob reproductions, and reviewer encoder checks covering zero, MaxUint32, adjacent boundaries, 10,000 sampled uint32 values, leading zeros and overflow.
• go vet ./instanceidhandler/v1/..., changed-file gofmt and diff whitespace checks: passed.
• Latest-head CI run 37314306556: successful build/Basic-Test. License Compliance still reports error (“2 issues found”), with cause unverified; it is not a required check.

Full local repository suite, race tests, full lint, integration tests and baseline execution were not performed. Necessity on main remains grounded in the parser's source and #177; these validation limits are not material blockers for this bounded parser change.

History remains as recorded in the initial review: merged #102/#132/#137 concern generation; #176 addresses hash normalization, not parsing; closed #75 discussed redundant tests, not rejection of recovery. Refreshed paginated all-state repository PR/issue history since the last review found no competing change. Search is limited to retrieved/indexed repository material, not proof of absence.

Five changed files, no dependencies added. Remaining compatibility limitation: literal Job suffixes can still be inferred as hashes, as now expressly documented; exact provenance cannot be recovered from this legacy string alone. No supported downstream failure or new security/concurrency/lifecycle blocker identified. Independent correctness recommendation: APPROVE; architecture: CLEAR under the clarified contract.

Immediately before submission the PR was open, non-draft, mergeable, with unchanged head/base and no new concerns to duplicate. Applicable rules require one eligible latest-push approval, with no required status contexts or thread-resolution gate; the extra Copilot approval rule does not apply to this human-authored PR. No merge, push, source-branch or settings changes.

[matthyx]

@shellyco-code congrats!

[shellyco-code]

@shellyco-code congrats!

thanks Matthias