chore(seal-check): adopt e2e-common lib + pin probe HOST (#541 fast-follow) - #566
Conversation
…ollow) Promised on the #541 review threads, now that #550 (the shared lib) and #541 (the seal-check script) are both on develop: - e2e-seal-check.sh now sources scripts/tests/lib/e2e-common.sh and uses e2e_isolate_env / e2e_install_prereqs, matching the other e2e-*.sh (drops the inlined isolation-env + install block). Keeps its own NS=$CLUSTER_NAME and local fail() for its assertions. - Harden the positive control (Saqlain nit): pin networkPolicy.training. enforcementProbeHost to a single $HOST var the install passes AND the positive control targets, so the probe and the control can never drift onto different hosts (was: HOST hardcoded while the probe used the chart default). shellcheck --severity=error/warning clean; helm template renders the probe Job with HOST=1.1.1.1 pinned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c263ac7. Configure here.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Reviewed at high effort (8 finder angles + cross-file/removed-behavior trace). This holds up well — clean, well-scoped fast-follow.
What I verified:
- The deleted inlined isolation + prereq block is faithfully reproduced by
e2e_isolate_env/e2e_install_prereqs; the only net changes are the intentionalexport USERandfail→errorfor the docker check (bothexit 1, both matching the siblinge2e-*.sh). - Sourcing contract respected:
e2e_install_prereqs(resolveshas/error/install_*at call time) runs aftercommon.sh+setup-linux.share sourced, identical ordering toe2e-cluster.sh. - The
HOSTpinning does what it claims —--set …enforcementProbeHost="$HOST"and the positive control both derive from the singleHOST=1.1.1.1, closing the hardcoded-vs-chart-default drift. e2e-common.shitself is untouched, so no other consumer of the shared helpers is affected.
No correctness issues. One optional maintainability nit inline — non-blocking; LGTM.
saqlainsyed007
left a comment
There was a problem hiding this comment.
LGTM — clean fast-follow. Verified the shared-lib adoption preserves the inlined behavior (only the intentional export USER and fail→error deltas), the sourcing/call ordering matches the sibling e2e scripts, and the single HOST=1.1.1.1 now feeds both the Helm --set and the positive control so the probe/control can no longer drift. The one inline note is optional/non-blocking.
|
/fr-pass |
#541 fast-follow (promised on review)
Now that #550 (shared
e2e-common.sh) and #541 (the seal-check runner) are both on develop:e2e-seal-check.shadopts the shared lib — sourcesscripts/tests/lib/e2e-common.shand usese2e_isolate_env/e2e_install_prereqslike the othere2e-*.sh(drops the inlined isolation-env + install block). Keeps its ownNS=$CLUSTER_NAMEand localfail().$HOSTvar that the install pins via--set networkPolicy.training.enforcementProbeHost="$HOST"AND the positive control targets — so the enforcement probe and the positive control can never drift onto different hosts (was:HOSThardcoded while the probe used the chart default).Verified:
shellcheck --severity=error/warningclean;helm templaterenders the probe Job withHOST=1.1.1.1pinned +TIMEOUT=240. Targetsdevelop.🤖 Generated with Claude Code
Note
Low Risk
Test-harness refactor and explicit Helm value for probe host; no production runtime or auth changes.
Overview
e2e-seal-check.shnow follows the same bring-up pattern as the othere2e-*.shscripts by sourcingscripts/tests/lib/e2e-common.shand callinge2e_isolate_env tbsealande2e_install_prereqsinstead of inlined cluster isolation and kubectl/k3d/helm install logic. Script-specific pieces (NS=$CLUSTER_NAME, localfail(), chart install, and seal-check flow) are unchanged.The egress probe host is a single
$HOST(1.1.1.1) passed to Helm via--set networkPolicy.training.enforcementProbeHost="$HOST", so the chart enforcement Job and the positive-control curl indefaultalways target the same host—avoiding drift between a hardcoded control and the chart default.Reviewed by Cursor Bugbot for commit c263ac7. Bugbot is set up for automated code reviews on this repo. Configure here.