test(#537): pin locked-down egress rule count - #543
Merged
Conversation
Add direct `lengthEqual` assertions on spec.egress for the two locked-down cases so a stray appended rule is caught, not just an index shift. Correction vs the review suggestion: the locked-down case is 4 rules, not 3. `egressProxy.enabled` DEFAULTS TRUE (values.yaml — only `routeWorkloads` defaults false), so the egress-proxy permit rule renders even when the case doesn't set it: DNS + MySQL + requests-proxy + egress-proxy. Both locked-down cases assert count 4. Fast-follow to the #537 review (non-blocking nit, deferred at merge). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
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 2a18e5c. Configure here.
saqlainsyed007
approved these changes
Aug 3, 2026
saqlainsyed007
left a comment
Contributor
There was a problem hiding this comment.
APPROVE — clean.
Verified the fast-follow against the template and values.yaml at head:
egressProxy.enableddoes default true in values.yaml (onlyrouteWorkloadsdefaults false), so your suggestedcount: 3was indeed off by one — the egress-proxy permit rule (template rule 5, gated on.Values.egressProxy.enabled) renders even in the case that doesn't set egressProxy.count: 4(DNS + MySQL + requests-proxy + egress-proxy) is the correct pin for both locked-down cases. Good catch on the correction, and the inline comment documenting the default is exactly the right call for the next reader.- The
lengthEqualonspec.egressexercises the real rendered policy (not vacuous), and the counts are consistent with rule 2 dropping whenallowExternalHttps: false(MySQL shifts to egress[1], requests-proxy to egress[2], egress-proxy at egress[3]). - Positional identity is pinned across the two cases (egress[1]=mysql, egress[2]=requests-proxy, egress[3]=egress-proxy), so the count assertion complements the identity checks rather than replacing them — a swapped rule at a pinned index still fails. No brittleness-vs-value gap worth blocking.
Consistent with the #537 change it follows. Ship it.
Contributor
Author
|
/fr-pass |
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
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.
Fast-follow to #537 — @saqlainsyed007's test-robustness nit
Your suggestion on #537: assert the locked-down egress shape directly (
lengthEqualonspec.egress) so a stray appended rule is caught, not just an index shift. Applied to both locked-down cases (deferred at merge to keep the approval).One correction worth flagging: you suggested
count: 3for the pure locked-down case (DNS + MySQL + requests-proxy). It's actually 4 —egressProxy.enableddefaultstrueinvalues.yaml(onlyrouteWorkloadsdefaults false), so the egress-proxy permit rule renders even when the case doesn't set it:DNS + MySQL + requests-proxy + egress-proxy. Both locked-down cases therefore assertcount: 4, and I added a comment explaining the default so the next reader isn't surprised.helm unittest— netpol suite 13/13, full chart 322/322.Targets
develop.🤖 Generated with Claude Code
Note
Low Risk
Test-only changes to Helm unit tests; no runtime or template behavior is modified.
Overview
Helm unittest hardening for locked-down training
NetworkPolicycases: both tests now assertspec.egresshas exactly 4 rules vialengthEqual, so an extra egress rule can’t land without failing the suite (follow-up to review on #537).Comments document why the count is 4 (DNS, MySQL, requests-proxy, egress-proxy):
egressProxy.enableddefaults true in chart values, so the egress-proxy permit rule is present even when a test case doesn’t setegressProxy.Reviewed by Cursor Bugbot for commit 2a18e5c. Bugbot is set up for automated code reviews on this repo. Configure here.