test(apps): anchor index-addressed negative asserts to what they name - #3606
Conversation
notContains passes whenever its path resolves to something that simply lacks the forbidden content, and a different element at the same index qualifies. These guards would go green if the Role's rule order changed or a nodePool were added ahead of the one they name, while the resource they forbid came back on the element they meant. Assert the addressed element before asserting what it must not have. The Kafka Role rules have no identifier, so the anchor is the in-cluster bootstrap Service that rule must keep. The OpenSearch pool does have one, `component`, and that is what is pinned: roles are shared between pools, so a role-based anchor would be satisfied by a second data-bearing pool inserted ahead of this one. Each anchor was checked against the shift it exists to catch, and against the same shift with the anchors removed: without them the blocks stay green while the forbidden content is back on the element they name. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe tests add positive assertions that anchor checks to the intended Kafka RBAC rule and OpenSearch node pool before evaluating negative conditions. ChangesTest assertion anchoring
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
Summary
Test-only hardening. Four negative asserts that addressed an element purely by index (rules[0] in three Kafka RBAC cases, spec.nodePools[0] in one OpenSearch case) now carry a positive anchor that pins the index to the element the assertion means, so the guard fails correctly if a reorder/insert moves the forbidden content onto that element instead of passing spuriously.
Findings
Verified against the rendered templates at the PR head:
packages/apps/kafka/templates/dashboard-resourcemap.yaml:11-15—rules[0]is theservicesrule;<release>-kafka-bootstrapis emitted unconditionally,<release>-kafka-external-bootstraponly whenexternalis true. So the newcontains rules[0].resourceNames: test-kafka-kafka-bootstrapanchor is always satisfied on a legitimate render (no false-fail) and correctly re-pins index 0. If the Role rules are reordered, the anchor fails and the test goes red. All three Kafka cases (external=false; external=false+tls=true; external=false+tls=false) are re-anchored consistently.packages/apps/opensearch/templates/opensearch.yaml:45-46— there is a single staticnodePools[0]with the literalcomponent: nodes;master → cluster_manager. Theequal spec.nodePools[0].component: nodesanchor targets static content (no false-fail) and pins index 0; a pool inserted ahead of it fails the anchor. The comment's rationale for anchoring oncomponentrather than a role (roles are shared across pools) is correct and forward-looking.- The
contains/equalanchors are not no-ops: they fail when the pinned index's content changes, which is exactly the regression class being closed. The anchor also indirectly guards document reordering (it requiresdocumentIndex: 0to still be the Role / OpenSearch CR). - Consistent with sibling PR #3605 and with the pre-existing anchored asserts already in the Kafka suite (e.g. "grants access to internal bootstrap service").
Caveats
- Phase 5b: N/A — test-only diff, no chart output change, no existing-customer upgrade or fresh-install surface.
Nits (non-blocking)
packages/apps/kafka/tests/dashboard-resourcemap_test.yaml— the same ~8-line rationale comment is duplicated verbatim across the three Kafka cases. Per-assert locality is defensible, but a single shared note (or a one-line pointer) would reduce the paste. Not a blocker.
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
Test-only hardening of four index-addressed negative assertions across two helm-unittest suites; each rewritten assert was independently mutation-tested against the exact regression it targets and confirmed to catch it, with the pre-fix shape confirmed to pass vacuously under the same mutation.
Verification detail
Diff is exactly two files, +42/-0, both under packages/*/tests/*.yaml (confirmed via git diff --stat against merge-base c6eb5849, and files_file). No template, values, schema, RBAC, migration, or CRD file is touched.
Kafka (packages/apps/kafka/tests/dashboard-resourcemap_test.yaml, 3 asserts). packages/apps/kafka/templates/dashboard-resourcemap.yaml:6-16 renders a single Role with three ordered rules[]; rules[0] is the services rule whose resourceNames always contains <release>-kafka-bootstrap and conditionally <release>-kafka-external-bootstrap. The three notContains: rules[0].resourceNames: <release>-kafka-external-bootstrap guards (external=false; external=false+tls=true; external=false+tls=false) now carry a paired contains: rules[0].resourceNames: <release>-kafka-bootstrap anchor at the same documentIndex/path.
Reproduced both failure classes the PR claims to guard against, by mutating the checked-out template and re-running helm unittest packages/apps/kafka:
- Doc-order swap (RoleBinding emitted before Role): post-fix test file — all three guards plus the pre-existing "grants access to internal bootstrap service" case go RED (
containsfails with "unknown path rules[0].resourceNames", sincedocumentIndex: 0now resolves to the RoleBinding). Pre-fix test file (anchor lines removed,notContainsonly) on the identical mutated template — all three guards stay GREEN (28/35 passing incl. these 3), i.e. vacuous pass reproduced. - Rule-order swap inside the Role (secrets rule moved to index 0): post-fix — all three guards go RED. Pre-fix (anchor removed) — all three guards stay GREEN (31/35 passing), vacuous pass reproduced again.
Both mutations independently confirm claims (a) anchored to a named element (the bootstrap Service name, the only available identifier since rules carry none), (b) the assert genuinely reddens under the regression it targets, and it is not a vacuous restatement of pre-existing coverage.
OpenSearch (packages/apps/opensearch/tests/opensearch_test.yaml, 1 assert). packages/apps/opensearch/templates/opensearch.yaml:45-46 currently renders a single static nodePools[0] with component: nodes. The "disables master role when configured" guard (notContains: spec.nodePools[0].roles: cluster_manager) now carries a paired equal: spec.nodePools[0].component: nodes anchor, deliberately keyed on component (a per-pool identifier) rather than roles (shared across pools, so a role-based anchor would not distinguish pools).
Reproduced the scenario the PR body describes ("a nodePool inserted ahead of this one, roles shared between pools") by mutating opensearch.yaml to insert a second, data-bearing ingest-pool node pool ahead of nodes, and injecting the actual regression this test exists to catch (cluster_manager rendered unconditionally in the nodes pool regardless of nodeRoles.master):
- Post-fix test file: "disables master role when configured" goes RED (
equalfails:nodePools[0].componentis nowingest-pool, notnodes). - Pre-fix test file (anchor line removed,
notContainsonly) on the identical mutated template: the same case stays GREEN (49/58 passing, this case not in the failing set) even though thenodespool genuinely carriescluster_managerdespitenodeRoles.master: false— vacuous pass reproduced.
Confirms the same three properties for the fourth assert, and specifically confirms the PR's stated rationale for choosing component over a role-based anchor (a role-based anchor would have been satisfied by the inserted data-bearing pool's shared data/ingest roles and would not have caught the shift).
Framework fit. helm-unittest evaluates every entry in an asserts: list independently (no short-circuit on an earlier failure), so pairing a contains/equal anchor alongside the pre-existing notContains in the same list strictly adds a check; it cannot suppress or weaken the original assertion. Verified directly: in every mutation above, the original notContains assertion was still present and still (spuriously) green in the pre-fix run, while the added anchor alone carried the correctly-red result post-fix.
No coverage weakened. All three restored-to-baseline runs (git status --short, git diff --stat against the working tree after each mutation experiment) confirm the repo was returned to the exact PR-head state with zero residual diff. Grepped both touched test files plus the rest of the kafka/opensearch tests/ trees for other notContains/notEqual usages: one additional case in the same Kafka file (grants access to clients CA cert secret, line 101) was already paired with its own contains anchor pre-PR and is correctly left untouched; two cases in kafka_test.yaml assert notContains against spec.kafka.listeners with a full-object content matcher and no index, which is not the index-addressed pattern this PR targets and is correctly out of scope. The PR body's stated exclusions ("already paired," "pure-negative tests elsewhere … tracked as their own sweep") check out against the tree.
Claims. "kafka 35/35, opensearch 58/58" reproduced exactly on the unmutated tree. "Additive only, no chart output changes" confirmed (+42/-0, zero template/values files in the diff). release-note fenced block present (NONE). Single commit, Conventional-Commits-formatted, signed off.
Mechanical sweep: ran fail-open (\|\| true, 2>/dev/null) and RBAC-verb greps against the diff; no hits (diff contains no shell or RBAC content, only helm-unittest YAML).
Phase 5b/5c: N/A. No chart/values/schema/migration/RBAC/CRD file is touched; there is no rendered-output change to analyze for upgrade or fresh-install impact, and no configuration corner to render.
Caveats
- The scope is intentionally narrow: the PR explicitly does not extend to index-addressed negative asserts in other charts, stating they are "tracked as their own sweep." Confirmed no other adjacent case in these same two suites was missed by this exclusion (see above).
Nits (non-blocking)
packages/apps/kafka/tests/dashboard-resourcemap_test.yaml— the same six-line rationale comment is duplicated verbatim across all three anchored Kafka cases. A single shared note (or a one-line pointer per case) would avoid the repetition; not a blocker since these are test fixtures, not rendered manifests.
What this PR does
Four negative assertions in two suites address an element by index.
notContainspasses whenever its path resolves to something that simply lacks the forbidden content, and a different element at the same index qualifies. So if the Kafka Role's rule order changed, or a nodePool were added ahead of the one the OpenSearch test names, those guards would go green while the resource they forbid was live on the element they meant.This asserts the addressed element before asserting what it must not have. The Kafka Role rules carry no identifier, so the anchor is the in-cluster bootstrap Service that rule must keep. The OpenSearch pool does have one,
component, and that is what is pinned rather than a role: roles are shared between pools, so a role-based anchor would be satisfied by a second data-bearing pool inserted ahead of this one.The anchors turn out to cover more than the ordering shift they were written for.
containsfails on a path that does not resolve, wherenotContainspasses, so a positive assert on the same path also catches the document-index half of the same defect. Swap the Role and RoleBinding documents in the Kafka template and all three guards go red, where before they stayed green.What this PR does not do
It does not raise the guards above the index. A
rules[0]-scoped assertion cannot express "the Role must not grant this Service at all", because a grant reappearing on a rule appended further down satisfies both the anchor and the negative. That ceiling is inherent to index-scoped assertions and is unchanged here.It does not touch the two cases in these files that were already paired with a positive assert on the same path, and it does not extend to the pure-negative tests elsewhere in the same suites, which are tracked as their own sweep.
Tests
The change is itself a test fix. Both suites pass: kafka 35/35, opensearch 58/58.
Each anchor was checked against the shift it exists to catch and against the same shift with the anchor removed. Without them the blocks stay green while the forbidden content is live on the element they name; with them the blocks fail. The OpenSearch mutation deliberately inserts a data-bearing pool, which is what distinguishes the
componentanchor from a role-based one.Screenshots
None. This changes test files.
Downstream repositories
Walked the trigger map against the diff. Two helm-unittest files, additive only, no chart output changes.
Release note
Summary by CodeRabbit