Skip to content

test(netpol): characterize the locked-down egress policy (RFC-0003 D6, #199-1) - #537

Merged
LukasWodka merged 1 commit into
developfrom
test/199-netpol-lockdown-tests
Aug 3, 2026
Merged

test(netpol): characterize the locked-down egress policy (RFC-0003 D6, #199-1)#537
LukasWodka merged 1 commit into
developfrom
test/199-netpol-lockdown-tests

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

#199, PR 1 of the egress-lockdown work — test-only, no behavior change. It's the regression net so the actual default flip (#199-2) is a green-to-green change.

The egress mechanism is already fully shipped (squid gateway, the allowExternalHttps gate, jobs-manager proxy injection, the seal-check enforcement probe — client-runtime#102 / client#378). #199 is just making deny-by-default the shipped default. Flipping allowExternalHttps to false drops the rule-2 external-HTTPS hole, so the egress rule indices shift.

This PR pins the current behavior and characterizes the locked-down state:

  • Pins allowExternalHttps: true in the three cases that assert the rule-2 hole or a rule-2-dependent index (external-443, MySQL, OpenShift) — so they survive the flip.
  • Two new locked-down cases (allowExternalHttps: false): the 0.0.0.0/0 rule is gone (MySQL shifts egress[2] → egress[1]); the unconditional requests-proxy egress (8888) survives so pods still POST results/FLOPs; the egress-proxy allowlist rule (3128) renders under egressProxy.enabled.

Tests

13 network-policy tests (was 11), 322 helm suite green.

Track: #199 — PR-1 (tests) lands anytime · PR-2 (the 4-line flip) is gated on the dev→staging→prod fleet pin-rollout. Epic: backend#1151 (D6).

Note on assignee: per your epic-ownership call I've assigned this to you; the client-repo convention is saadqbal — reassign if you'd rather follow the repo default.

🤖 Generated with Claude Code


Note

Low Risk
Only Helm unit tests in network_policy_test.yaml; no template or runtime behavior changes.

Overview
Test-only prep for RFC-0003 D6 / #199: no chart behavior changes; this is the regression net before allowExternalHttps defaults to false in a follow-up PR.

Existing network-policy cases that assert the rule-2 external HTTPS hole (0.0.0.0/0 at egress[1]) or rules that sit after it (MySQL at egress[2], OpenShift except blocks) now explicitly set allowExternalHttps: true so they stay valid when the default changes.

Two new cases with allowExternalHttps: false document the locked-down shape: the broad external-443 rule is absent (MySQL moves to egress[1]), requests-proxy on 8888 remains, and with egressProxy.enabled the egress-proxy rule on 3128 appears at egress[3].

Reviewed by Cursor Bugbot for commit 31325bd. Bugbot is set up for automated code reviews on this repo. Configure here.

…#199-1)

Test-only, no behavior change — the regression net for the egress-lockdown
default flip (#199-2). The egress mechanism is fully shipped (client-runtime#102
/ #378); #199 is just flipping allowExternalHttps to false by default, which
drops the rule-2 external-HTTPS hole and shifts the remaining egress indices.

- Pin allowExternalHttps: true in the three cases that assert the rule-2 hole
  or the MySQL rule at a rule-2-dependent index (external-443, MySQL, OpenShift),
  so they keep passing once the default flips.
- Add two locked-down cases (allowExternalHttps: false): the 0.0.0.0/0 rule is
  gone (MySQL shifts to egress[1]), the UNCONDITIONAL requests-proxy egress
  (8888) survives so pods can still POST results/FLOPs, and the egress-proxy
  allowlist rule (3128) renders when egressProxy.enabled.

13 network-policy tests (was 11), 322 helm suite green. Lands first so #199-2
is a green-to-green flip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka LukasWodka self-assigned this Aug 3, 2026
@LukasWodka

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 31325bd. Configure here.

@saqlainsyed007 saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary — APPROVE ✅

Test-only PR (#199-1); the regression net so the allowExternalHttps default flip (#199-2) is a green-to-green change. Verified locally.

What I checked

  • Ran helm unittest client -f tests/network_policy_test.yaml13 passed.
  • Walked the render order in templates/network-policy-training.yaml: DNS(0) → external-HTTPS(1, gated on allowExternalHttps) → MySQL → requests-proxy → egress-proxy(gated on egressProxy.enabled).
    • allowExternalHttps: false ⇒ MySQL → egress[1], requests-proxy → egress[2]
    • + egressProxy.enabled ⇒ egress-proxy → egress[3], port 3128 ✓
  • The three allowExternalHttps: true pins land on exactly the rule-2-dependent cases (external-443, MySQL@egress[2], OpenShift except-block@egress[1]); comments are accurate.

Assessment

  • Correctness: assertions match the template exactly.
  • Conventions: matches the existing suite's style; comments explain why each pin exists — good for the next reader hitting the index shift.
  • No template/runtime change, so risk is confined to CI. Nothing blocking.

One non-blocking robustness nit inline.

Comment thread client/tests/network_policy_test.yaml
@LukasWodka
LukasWodka merged commit 4d9cc1c into develop Aug 3, 2026
31 of 32 checks passed
LukasWodka added a commit that referenced this pull request Aug 3, 2026
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>
@LukasWodka

Copy link
Copy Markdown
Contributor Author

/fr-pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants