feat(chart,k8s): add optional NetworkPolicy for pod-level isolation (direct egress)#1400
feat(chart,k8s): add optional NetworkPolicy for pod-level isolation (direct egress)#1400antigenius0910 wants to merge 7 commits into
Conversation
Adds an opt-in NetworkPolicy for openab agent and gateway pods. Default networkPolicy.enabled=false — no resource is rendered, existing releases are unaffected. Mirrors the opt-in pattern of openabdev#901 / openabdev#911 / openabdev#914. Two supported egress topologies via networkPolicy.egress.mode: - direct (default): DNS + HTTPS to 0.0.0.0/0 (except 169.254.169.254/32, the cloud metadata service) — hobbyist / dev-cluster path. - proxy: DNS + egress restricted to a designated proxy Service via networkPolicy.egress.proxy.{namespace, podSelector, ports} — enterprise path per @pahud's Discord feedback citing OpenSHELL prior art. Chart ships no proxy; users BYO (OpenSHELL, Squid, Envoy, mitmproxy, etc.), keeping upstream maintenance surface unchanged. Rendered resources when enabled: - <release>-<agent>: deny-all ingress, egress per mode. - <release>-<agent>-gateway: adds port-8080 ingress when agents.<name>.gateway.enabled=true. Selector labels reuse openab.selectorLabels so drift with deployment.yaml and gateway.yaml is structurally impossible. Also adds k8s/networkpolicy.yaml mirroring the chart's direct-mode default (deny-all ingress, DNS + HTTPS egress) so the raw manifests set cited by the driving AppSec finding is covered too. Tests: 8 helm-unittest cases covering both modes, gateway split, extraRules, and locked-down egress. Full chart suite (48 tests) still passes; helm lint clean. Closes openabdev#1394
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… gateway path, DNS scoping, IPv6 Addresses all 9 findings raised on the initial commit. Details map 1:1 to the chaodu-agent review on PR openabdev#1400. Critical (fail-closed): - F1 Validate networkPolicy.egress.mode + required proxy fields at render time. mode must be exactly "direct" or "proxy" (typos like "proxxy" no longer silently fall back to direct). mode=proxy now requires non-empty proxy.podSelector AND non-empty proxy.ports, otherwise an empty selector authorizes every pod on every port (previous fail-open behaviour). - F2 Add agent→gateway egress rule on TCP 8080 when a chart-managed gateway is deployed (agents.<name>.gateway.enabled=true AND gateway.deploy is not "false"). Without this, enabling the policy breaks the ws://<release>-<agent>-gateway:8080/ws data path. Important: - F3 Match templates/gateway.yaml condition for gateway.deploy — the previous `default true | toString` treated an explicit false as empty and rendered an orphan gateway NetworkPolicy with no Deployment. - F4 Scope default DNS destination to the trusted kube-system namespace (kubernetes.io/metadata.name=kube-system) instead of any namespace. Prevents a co-tenant labelling a pod k8s-app=kube-dns from becoming an approved port-53 destination. New egress.dnsNamespace value (default "kube-system", empty = legacy any-namespace behaviour). Same tightening applied to k8s/networkpolicy.yaml. - F5 Document the proxy-mode contract explicitly in values.yaml and charts/openab/README.md: NetworkPolicy enforces the destination allowlist only. Operators must configure HTTPS_PROXY/HTTP_PROXY via agents.<name>.env AND add them to inherit_env in configToml so ACP children inherit the settings. Chart does not auto-inject. - F6 Add opt-in `kubectl apply -f k8s/networkpolicy.yaml` step and manifest-table row to README.md and README.zh-TW.md so users following the raw-install path aren't left unisolated. - F7 Quote the proxy namespace label value so all-numeric namespace names (e.g. "1234") render as strings, not YAML integers. - F8 Add optional IPv6 support (allowIpv6, default true). Adds ::/0 alongside 0.0.0.0/0 in HTTPS/HTTP rules, excluding fd00:ec2::254/128 (AWS IMDS IPv6 pattern). Non-AWS IPv6 metadata addresses vary; docs point users at extraRules for cluster-specific exclusions. Same addition in k8s/networkpolicy.yaml. Toggle off for IPv4-only clusters. - F9 Update allowHttp documentation in values.yaml and chart README to explicitly call out HTTP configUrl as an affected use case; recommend HTTPS configUrl by default. Tests: 15 helm-unittest cases (up from 8). New coverage: - 3 negative tests for F1 validation (invalid mode, empty podSelector, empty ports) - agent→gateway egress path assertion (F2) - gateway.enabled=true+deploy=false → no gateway NP, no agent egress (F3) - DNS namespace scoping default + empty-string escape hatch (F4) - numeric proxy namespace renders as string (F7) - IPv6 rendered by default, omitted when allowIpv6=false (F8) Full chart suite: 55/55 pass, no regression. helm lint clean. Refs openabdev#1394 Refs openabdev#1400 review by chaodu-agent
|
Thanks @chaodu-agent — pushed Finding-by-finding response:
Non-blocking dispositions
Test summary
Ready for another look. If any of the deferred items above should land in this PR instead of follow-up, happy to add. |
This comment has been minimized.
This comment has been minimized.
…env, configurable metadata exclusions Addresses both round-2 findings from chaodu-agent on the follow-up commit (d1a7ff0). Kept as a third commit rather than amend to preserve review history. Critical (round-2 F1) — Complete the proxy contract for the gateway: The previous commit locked the chart-managed gateway to proxy-only egress when networkPolicy.egress.mode=proxy, but the gateway Deployment had no mechanism to receive proxy env vars — its env list was a fixed platform-specific block only. Effect: proxy-mode gateway pods could reach DNS + the proxy pod but nothing else, so all outbound platform API calls (Telegram replies, Teams OAuth token exchange, Feishu/WeCom callbacks) silently failed. Fix: add first-class `agents.<name>.gateway.env` (map) and `agents.<name>.gateway.envFrom` (list) fields to values.yaml and render them into templates/gateway.yaml, right after the fixed platform env block. This lets operators inject HTTPS_PROXY / HTTP_PROXY / NO_PROXY into the gateway pod the same way they do for the agent. Updated the "Proxy-mode CONTRACT" doc block in values.yaml (and the matching chart README row) to spell out that operators must configure BOTH the agent AND the gateway to route through the proxy, with a concrete example config for each side. Important (round-2 F2) — Configurable IPv4/IPv6 metadata exclusions: The previous docs suggested users could tighten IPv6 metadata protection via `egress.extraRules`. That is incorrect — Kubernetes NetworkPolicy allow rules combine additively, so an extraRule can only broaden what is allowed, never subtract from an existing `::/0` allow. Non-AWS clouds with different IPv6 IMDS addresses would have been silently unprotected. Fix: new `networkPolicy.egress.metadataExclusions.{ipv4,ipv6}` value. Defaults preserve the AWS pattern (169.254.169.254/32 for IPv4, fd00:ec2::254/128 for IPv6). Users on other clouds edit the list directly (e.g. add IBM Cloud's 161.26.0.0/16 to ipv4, or override ipv6 for GCP/Azure). The template consumes both lists via `toYaml . | nindent`, so the exclusion set can grow as needed. Updated values.yaml comment block to enumerate common cloud IMDS addresses. Updated chart README rows for allowHttps/allowHttp/allowIpv6/ extraRules to point at metadataExclusions and clarify the additive semantics constraint. Tests: 18 helm-unittest cases in NetworkPolicy suite (up from 15). New: - gateway.env renders HTTPS_PROXY/HTTP_PROXY/NO_PROXY into the gateway Deployment container (round-2 F1) - gateway.envFrom renders on the gateway Deployment (round-2 F1) - custom metadataExclusions.ipv4/ipv6 render as expected `except` entries in the direct-mode HTTPS rule (round-2 F2) Full chart suite: 58/58 pass. helm lint clean. Refs openabdev#1394 Refs openabdev#1400 round-2 review by chaodu-agent
|
Thanks @chaodu-agent — round-2 fixes pushed in Finding-by-finding response🔴 F1 (round-2) — Gateway proxy contract incompleteVerified. Local render with Fix: added first-class Updated the Proxy-mode CONTRACT doc block ( Coverage:
Manual smoke: → gateway container env now contains 🟡 F2 (round-2) — Additive-only NetworkPolicy semantics vs. IPv6 tightening guidanceVerified. Kubernetes NetworkPolicy allow rules combine additively (union of matching allows). An Fix: replaced hardcoded metadata CIDR exclusions with a configurable list: networkPolicy:
egress:
metadataExclusions:
ipv4:
- 169.254.169.254/32 # AWS/GCP/Azure IMDS default
ipv6:
- fd00:ec2::254/128 # AWS IPv6 IMDS defaultBoth lists are consumed by the template via Updated Coverage: new test Test summary
Backwards compatibilityBoth fixes are additive and default-preserving:
Ready for another look. If a proper "first-class proxy URL injection into both workloads" (your alternative suggestion) is preferred over the manual env-var pattern, happy to iterate — but that would tie the NetworkPolicy config to the workload templates more tightly, and I wanted to keep the smaller, additive change to keep upstream maintenance surface bounded. |
This comment has been minimized.
This comment has been minimized.
…t system-proxy + CNI/IMDS caveats Addresses all three round-3 findings from chaodu-agent on commit f323386. Kept as a fourth commit rather than amend to preserve review history. Critical (round-3 F1) — Enable reqwest system-proxy in openab-gateway: The chart correctly renders HTTPS_PROXY / HTTP_PROXY / NO_PROXY into the gateway container env (round-2 fix), but the gateway binary was compiled with `reqwest = { default-features = false, features = ["rustls-tls", "json"] }`. In reqwest 0.12, `system-proxy` is a *default* feature that turns on env-var auto-detection (HTTP_PROXY / HTTPS_PROXY / NO_PROXY / ALL_PROXY). With `default-features = false` and `system-proxy` not re-enabled, reqwest silently ignores those env vars. Effect: proxy-mode gateway pods were reachable to the proxy Service and DNS, but every outbound platform-API call (Telegram replies, Teams OAuth token exchange, Feishu/WeCom/Google Chat/LINE callouts, WebSocket downstream setup) tried direct egress and hit the NetworkPolicy deny. Fix: add `"system-proxy"` to reqwest's feature list in crates/openab-gateway/Cargo.toml. Verified with `cargo check -p openab-gateway` (clean build) and `cargo test -p openab-gateway --lib` (262 tests pass, no regression). Added an inline comment above the dependency line explaining why the feature is re-enabled — otherwise a future developer might strip it during a dependency cleanup. Rust code is unchanged. All existing HTTP clients constructed via `reqwest::Client::new()` or `Client::builder()` now respect env-var proxy config automatically. Important (round-3 F2) — Document CNI-enforcement prerequisite: A NetworkPolicy has no effect unless the cluster's CNI enforces it — stock KIND, Docker Desktop, or bare-EKS clusters without a supported CNI silently ignore the resource. Added an explicit "⚠️ PREREQUISITE" block at the top of the values.yaml networkPolicy section listing supported CNIs (Calico, Cilium, Antrea, EKS with NetworkPolicy add-on, GKE Dataplane V2, AKS with Azure NPM / Calico) and recommending a scratch-namespace deny-all verification before relying on the feature. Mirrored the caveat into the chart README row for networkPolicy.enabled. Important (round-3 F3) — Qualify metadata isolation as defense-in-depth: The metadataExclusions rendering is correct under additive NetworkPolicy semantics (round-2 fix), but the docs presented it as portable IMDS protection. K8s NetworkPolicy always permits pod-to-resident-node traffic, and CNI implementations differ in whether ipBlock matches pre- or post-NAT addresses. Added an explicit "⚠️ Treat this as defense-in- depth, NOT a portable IMDS guarantee" block in values.yaml enumerating the two K8s-level caveats and recommending cloud-provider hardening (IMDSv2 mandatory + hop-limit 1, GCP metadata concealment, Azure metadata endpoint policy) as the primary control. Mirrored into the chart README rows for both metadataExclusions.ipv4 and .ipv6. No behavioural change beyond the reqwest feature addition — F2 and F3 are documentation-only. Existing 58 helm-unittest tests still pass. Refs openabdev#1394 Refs openabdev#1400 round-3 review by chaodu-agent
|
Thanks @chaodu-agent — round-3 fixes pushed in Finding-by-finding response🔴 F1 (round-3) — reqwest system-proxy feature was disabledVerified. Confirmed against Fix: - reqwest = { version = "0.12", default-features = false, features = ["rustls-tls", "json"] }
+ # `system-proxy` re-enables HTTPS_PROXY / HTTP_PROXY / NO_PROXY env-var auto-detection,
+ # which is a default reqwest feature disabled by `default-features = false`. Required so
+ # the gateway respects proxy env vars injected via chart `agents.<name>.gateway.env` when
+ # running under `networkPolicy.egress.mode=proxy`. See PR #1400.
+ reqwest = { version = "0.12", default-features = false, features = ["rustls-tls", "json", "system-proxy"] }Added the inline comment so a future developer doing a "clean up unused features" pass doesn't strip it and silently reintroduce the regression. Verification:
The reviewer's point about "a Helm render assertion alone cannot verify this contract" is fair; a full end-to-end proxy-routing integration test would need a live CNI cluster + a running Squid/Envoy sidecar. Not adding that here — same reason the RFC scope kept live-CNI testing out. Cargo test coverage on the reqwest paths is the closest available proxy for correctness. 🟡 F2 (round-3) — Document CNI-enforcement prerequisiteVerified. Root README's raw-install table mentions it (round-1 fix), but neither Fix:
🟡 F3 (round-3) — Qualify metadata isolation as defense-in-depthVerified. The
Fix: added an explicit " Test summary
Backwards compatibility
Ready for another look. |
This comment has been minimized.
This comment has been minimized.
…ompatibility, stricter mode/port validation Addresses all three round-4 findings from chaodu-agent on commit a66f3aa. Kept as a fifth commit rather than amend to preserve review history. Critical (round-4 F1) — Feishu WebSocket + proxy-mode incompatibility: The reqwest system-proxy fix (round-3) makes gateway HTTP clients honor HTTPS_PROXY, but Feishu's default connectionMode="websocket" opens its event stream via tokio_tungstenite::connect_async which does NOT consume reqwest's proxy configuration. Under networkPolicy.egress.mode=proxy the WSS TCP 443 direct-egress attempt is blocked — Feishu gateway can't establish its event connection. Rather than implement a full HTTP CONNECT-aware WebSocket path (large Rust scope), enforce the constraint at chart render time with a clear error pointing operators at the supported fallback: fail render if networkPolicy.egress.mode=proxy AND any agent has gateway.feishu.appId set AND gateway.feishu.connectionMode is unset or "websocket". Positive path (connectionMode="webhook") renders cleanly. Documented in values.yaml and the chart README row for networkPolicy.egress.mode. Critical (round-4 F2) — Empty-string mode silently defaulted to direct: The previous `$mode := $np.egress.mode | default "direct"` treated an explicit `--set-string networkPolicy.egress.mode=""` as "direct" and rendered open-internet HTTPS, contradicting the fail-closed contract. Fix: drop the `| default "direct"` — validate the raw value directly. values.yaml already provides the default; explicitly-empty input is now rejected with the same error format as any other invalid mode value. Important (round-4 F3) — Protocol-only proxy port entries: The previous check ensured proxy.ports was non-empty, but a valid NetworkPolicyPort like `{protocol: TCP}` (no `port` field) passed and rendered — Kubernetes treats missing `port` as "all ports for the given protocol", silently broadening egress to the proxy pods. Fix: iterate proxy.ports and require every entry to have a truthy `port` field; error message names the offending index. Documented in the chart README row for networkPolicy.egress.proxy. Tests: 4 new helm-unittest cases (62 total, was 58): - F2: empty mode string → fail-closed with exact error message - F3: proxy port entry with only `protocol` → fail-closed - F1: proxy+feishu websocket → fail-closed with actionable message - F1 positive: proxy+feishu webhook → 2 policies render as expected Full chart suite: 62/62 pass. helm lint clean. No Rust changes. Refs openabdev#1394 Refs openabdev#1400 round-4 review by chaodu-agent
|
Thanks @chaodu-agent — round-4 fixes pushed in Finding-by-finding response🔴 F1 (round-4) — Feishu WebSocket bypasses reqwest proxy configVerified. Fix approach: rather than implement a full HTTP CONNECT-aware WebSocket path (large Rust scope — new crate dependency or hand-rolled CONNECT tunnel + tests), took the reviewer's second suggestion: "explicitly reject/document Feishu WebSocket mode with proxy-only egress and require webhook mode." Chart render now fails when:
Error message names the agent and points at the fix: Positive path — Coverage:
If proper CONNECT support for the Feishu WSS path is desired, happy to open a follow-up PR — but the reviewer's alternative (explicitly reject) matches the "keep upstream maintenance surface small" pattern this RFC has been following. 🔴 F2 (round-4) — Empty-string mode silently defaulted to directVerified. Fix: dropped Same error format as any other invalid mode value. Coverage: new negative test asserts empty-string mode fails with the exact error. 🟡 F3 (round-4) — Protocol-only proxy port entries broaden egressVerified. Fix: added a per-entry iteration: Named the offending index in the error so operators can find it in a multi-entry list. Documented in the chart README row for Coverage: new negative test asserts protocol-only entry fails with the exact error message. Test summary
Backwards compatibility
Ready for another look. |
This comment has been minimized.
This comment has been minimized.
…m-proxy only
Round-5 review revealed that proxy-mode egress cannot be honored end-to-end
without a proxy-aware networking refactor across openab-core / openab-
gateway. Every prior fix round (rounds 1-4) uncovered another transport
that bypasses reqwest and opens WSS directly:
- Discord (default) — Serenity's own WSS client via client.start()
- Slack Socket Mode — crates/openab-core/src/slack.rs:761 uses
tokio_tungstenite::connect_async
- External gateway (gateway.deploy=false) —
crates/openab-core/src/gateway.rs:818 same pattern
- Feishu WebSocket — already guarded in round 4, but the guard only
covered the structured chart config path; configToml / envFrom /
gateway.env bypasses remained
- AgentCore — its own TLS/WebSocket transport
The project already documents this in docs/openshell.md: "Unless OAB's
networking layer is refactored to be fully HTTP/HTTPS proxy-aware
(tunneling WSS through OpenSHELL's L7 proxy), the integration cannot
function." Continuing to layer chart-level guards would leave proxy
mode practically unusable (Discord is the default transport for most
deployments) while giving users a false sense of enterprise egress
isolation.
Per Option C of the reviewer's third suggestion (also confirmed with the
PR author), withdraw proxy mode from this PR entirely. Follow-up RFC
will cover proxy-aware egress properly once the Rust networking refactor
is scoped.
## What's removed
- `networkPolicy.egress.mode` value (was "direct" | "proxy")
- `networkPolicy.egress.proxy` config block (namespace / podSelector / ports)
- Proxy-mode branch in templates/networkpolicy.yaml egress helper
- Feishu WebSocket incompatibility guard (no longer needed)
- All proxy-mode validation (empty mode, invalid mode, empty proxy fields,
protocol-only ports)
- All proxy-mode positive + negative tests
- "Proxy-mode CONTRACT" doc block in values.yaml
- Proxy-mode language in chart README
## What's kept (all still generally useful)
- Opt-in `networkPolicy.enabled` master switch (unchanged)
- Direct-mode egress: DNS to kube-system + IPv4/IPv6 HTTPS with configurable
metadata exclusions
- Agent-to-gateway TCP 8080 egress rule when chart-managed gateway is deployed
- `gateway.deploy=false` handling (matches templates/gateway.yaml)
- DNS scoping to kube-system (configurable via egress.dnsNamespace)
- `metadataExclusions.{ipv4,ipv6}` configurable per cloud
- IPv6 support (`allowIpv6`) with AWS IMDS exclusion + additive-semantics doc
- CNI-enforcement prerequisite documentation
- Defense-in-depth caveat on metadata exclusions
- `k8s/networkpolicy.yaml` raw manifest
- `gateway.env` / `gateway.envFrom` — reframed as general-purpose env
injection (docs no longer suggest proxy usage)
## F2 fix (round 5) — landed as small win regardless
Enabled reqwest `system-proxy` feature for the root package and openab-core
(previously only openab-gateway had it). Now operators who set
HTTPS_PROXY / HTTP_PROXY / NO_PROXY on any variant's pod get reqwest-based
HTTP calls (LLM APIs, Slack Web API, remote config, hooks, STT, media)
routed through the proxy — WITHOUT this implying the chart supports
enforced proxy mode. WSS transports (Slack Socket Mode, external gateway)
remain unaffected pending the networking refactor. Cargo comment in
openab-core/Cargo.toml calls out that limitation explicitly.
Verified with `cargo check --workspace` (clean rebuild across all three
reqwest sites) and existing openab-gateway lib test suite.
## Tests
Chart: 52/52 pass (was 62, drop is expected — removed 10 proxy-mode tests).
Rust: cargo check --workspace clean; openab-gateway lib tests unaffected.
helm lint clean.
## Follow-up RFC to file
Proxy-aware egress ("networking refactor") — covers:
- Replace or fork Serenity to route WSS through HTTP CONNECT
- Replace tokio_tungstenite direct calls with a CONNECT-aware alternative
(Slack Socket Mode, external gateway, Feishu WebSocket)
- AgentCore proxy configuration
- AWS SDK proxy configuration (separate from reqwest)
- End-to-end integration test with a local Squid/Envoy
Refs openabdev#1394
Refs openabdev#1400 round-5 review by chaodu-agent
antigenius0910
changed the title
|
Thanks @chaodu-agent. Round-5 findings led to a scope-narrowing decision rather than another round of guards. Decision: withdraw proxy-mode egress from this PR (
|
|
Important CHANGES REQUESTED What This PR DoesThis PR adds opt-in Kubernetes How It WorksThe chart renders one policy per enabled agent and another for each chart-managed gateway. Agent-to-chart-managed-gateway TCP 8080 is allowed only when that gateway is deployed; shared Helm logic supplies DNS, HTTP(S), metadata exclusions, and user-provided extra rules. The PR also adds generic gateway environment injection and enables reqwest system-proxy discovery in the root, Findings
Finding Details🟡 F1: Document and test the custom-port egress contract
This does not require broadly allowing port 8080 by default. Instead, state beside 🟡 F2: Either cover the native LLM client or narrow the proxy claimThe root workspace correctly enables reqwest Therefore, setting 🟡 F3: Remove the withdrawn mode from the dependency rationaleThe Addressing External Reviewer Feedback@antigenius0910 (round 5,
|
| Reviewer | Focus | Outcome |
|---|---|---|
| Reviewer A | NetworkPolicy security and Helm semantics | F1; direct-policy rendering otherwise sound |
| Reviewer B | Runtime/dependency integration | F2, F3 |
| Reviewer C | Tests, docs, and operability | F1; CNI and IMDS caveats confirmed |
| Reviewer D | Independent final audit | Confirmed changes requested |
Baseline and Validation
- PR opened: 2026-07-14
- Reviewed commit:
dfc1dae552968d2c604bcaa6a336ef55dca0c03a - Current main checked:
f26f99a93bf624e2b26ea2bf947cce6e22ddb594 - Main already has: no chart or raw
NetworkPolicy - Net-new value: opt-in pod isolation, chart-managed gateway integration, raw-manifest support, configurable metadata exclusions, gateway env/envFrom, and system-proxy support for the root workspace packages
- GitHub checks: all reported build/check/smoke/Helm jobs passed; operator and polling jobs skipped as expected
- Local validation:
helm unittest .passed 52/52;helm lint .passed;git diff --checkpassed - Targeted render: config-only external gateway on TCP 8080 received DNS + TCP 443 egress only, reproducing F1
- Limitation: Cargo is unavailable in the reviewer runtime; the remote
checkjob is green
What's Good (🟢)
- The feature is disabled by default, preserving existing releases and providing a straightforward rollback.
- DNS defaults to the trusted
kube-systemnamespace, with an explicit escape hatch for nonstandard clusters. - IPv4/IPv6 metadata exclusions are configurable in the original allow rule, and the documentation correctly treats them as CNI-dependent defense in depth.
- Agent-to-chart-managed-gateway TCP 8080 and
gateway.deploy=falseresource boundaries have focused tests. - With proxy mode withdrawn, the chart no longer advertises an enforcement topology that its direct socket transports cannot satisfy.
5️⃣ Three Reasons We Might Not Need This PR
- It remains opt-in — existing installations gain no isolation until operators enable and verify enforcement.
- Central policy may be stronger — platform-managed namespace defaults, policy-as-code, or CNI-specific policies can provide uniform enforcement and observability.
- Portable L3/L4 rules are necessarily incomplete — DNS names, custom service ports, proxy tunneling, and provider-specific metadata behavior still require deployment-specific configuration.
…gent proxy, external-gateway docs, stale comment Addresses all three round-6 findings from chaodu-agent on commit dfc1dae. F1 (custom-port destinations require extraRules): allowHttps opens only TCP 443 and allowHttp only TCP 80. Supported openab configurations that use other ports were silently broken when enabling NetworkPolicy: (a) external config-only gateway (gateway.deploy=false) listens on TCP 8080; (b) self-hosted STT often runs on custom ports (e.g. http://192.168.1.100:8080/v1); (c) internal LLM proxies like LiteLLM commonly bind :4000. Fix (docs-only, since a chart-level default for arbitrary custom ports would be too broad): - Added a⚠️ warning block above `networkPolicy.egress.extraRules` in values.yaml enumerating the three common cases and providing a concrete extraRules recipe for the external-gateway case. - Chart README row for `extraRules` mirrors the caveat. - Chart README row for `gateway.deploy` calls out that setting it to false removes the chart-managed agent→gateway TCP 8080 exception, so operators must supply an extraRules entry. - New helm-unittest test asserts the documented extraRules recipe for external-gateway TCP 8080 renders when gateway.deploy=false (also verifies that no chart-managed agent→gateway rule leaks in that path). F2 (native openab-agent proxy claim was incomplete): The PR body claimed reqwest system-proxy coverage for LLM API calls, but openab-agent is workspace-excluded (root Cargo.toml `exclude` list) and has its own Cargo.toml + Cargo.lock. Its reqwest 0.12 and reqwest013 (0.13, aliased for rmcp OAuth) declarations both had `default-features = false` without system-proxy. Setting HTTPS_PROXY on the native agent's pod would not route LLM requests (src/llm.rs:293/663/ 1033 construct clients via reqwest::Client::new() without explicit proxy config). Fix: added "system-proxy" to both reqwest and reqwest013 features in openab-agent/Cargo.toml, with an inline comment matching the root/core style. `cargo check` in openab-agent clean; Cargo.lock updated for the new feature dep. F3 (stale withdrawn-mode reference in gateway Cargo comment): The comment I added in round 3 referenced networkPolicy.egress.mode=proxy, which was removed in the round-5 withdraw commit (dfc1dae). Rewrote the comment to match root/core framing: general operator-supplied proxy env vars, with an explicit note that WSS transports (Feishu WSS via tokio_tungstenite) don't honor these env vars — that's the follow-up networking refactor. Tests: 53/53 helm-unittest pass (was 52, +1 external-gateway extraRules render test). helm lint clean. cargo check on openab-agent workspace clean. Refs openabdev#1394 Refs openabdev#1400 round-6 review by chaodu-agent
|
Thanks @chaodu-agent — round-6 fixes pushed in Finding-by-finding response🟡 F1 (round-6) — Custom-port destinations require extraRulesVerified.
Fix (docs-only — a chart default for arbitrary custom ports would be too broad):
Coverage: new helm-unittest test —
🟡 F2 (round-6) — Native
|
2 participants
Summary
NetworkPolicytemplate tocharts/openab/and a matching raw manifest tok8s/, closing the AppSec / IaC finding surfaced in feat(chart): add optional NetworkPolicy template for pod-level network isolation #1394.networkPolicy.enabled: false— zero rendered resources, zero behaviour change for existing releases. Mirrors the opt-in pattern of feat(openab): add existingSecret support for Slack agent credentials #901 / feat(chart): add imagePullSecrets support at per-agent and global level #911 / feat(chart): add serviceAccountName support at per-agent and global level #914.docs/openshell.md) and will be a follow-up RFC.system-proxyfeature across root /openab-core/openab-gatewayso operators who choose to setHTTPS_PROXY/HTTP_PROXYenv vars get HTTP calls (LLM APIs, Slack Web API, remote config, hooks, STT, media) routed through the proxy. WSS transports are unaffected — that's the followup RFC.Closes #1394. Design was discussed in-issue (see the reply to @pahud and RFC diff summary) and on Discord (thread).
Rendered resources (when
enabled: true)<release>-<agent>ingress: []) +ingress.extraRulesegress.extraRules<release>-<agent>-gateway(ifagents.<name>.gateway.enabledANDgateway.deploy != false)ingress.extraRulesFile changes
charts/openab/values.yamlnetworkPolicy:block +gateway.env/gateway.envFromcharts/openab/templates/networkpolicy.yamlcharts/openab/templates/gateway.yamlenv/envFromrenderingcharts/openab/tests/networkpolicy_test.yamlcharts/openab/README.mdk8s/networkpolicy.yamlCargo.toml,crates/openab-core/Cargo.toml,crates/openab-gateway/Cargo.tomlsystem-proxyto reqwest featuresHTTPS_PROXY/HTTP_PROXY/NO_PROXYenv vars if operators set themREADME.md,README.zh-TW.mdkubectl apply -f k8s/networkpolicy.yamlstep + table rowTest plan
helm unittest -f 'tests/networkpolicy_test.yaml' .— 12/12 passhelm unittest .— full chart suite 52/52 pass, no regression in other test fileshelm lint .— cleancargo check --workspace— clean rebuild across all three reqwest sitescargo test -p openab-gateway --lib— 262/262 pass (round-3 verification, unchanged)Test cases (from
tests/networkpolicy_test.yaml):enabled: false(default) → no resource renderedenabled: true, no gateway → single agent policy, DNS scoped to kube-system + IPv4/IPv6 HTTPSenabled: true, gateway deployed → 2 policies (agent + gateway), agent egress authorizes TCP 8080 to gateway podgateway.enabled=true, gateway.deploy=false→ single agent policy, no orphan gateway NP, no agent→gateway ruleallowHttp: true→ HTTP egress with IPv4/IPv6 presentallowIpv6: false→ IPv6 ipBlock omitted (IPv4-only cluster)dnsNamespace: ""→ falls back to legacy any-namespace DNS selectoringress.extraRulespopulated → merged verbatimallowDns: false,allowHttps: false→ locked down to onlyextraRulesmetadataExclusions.{ipv4,ipv6}→ renders as expectedexceptentriesgateway.env→ rendered on gateway containergateway.envFrom→ rendered on gateway containerOut of scope (follow-ups)
tokio_tungstenitedirect calls (Slack Socket Mode, external gateway, Feishu WebSocket), AgentCore proxy configuration, AWS SDK proxy configuration, and end-to-end integration test with a local Squid/Envoy. Seedocs/openshell.mdfor existing project stance.networkPolicyoverride — reasonable follow-up per reviewer, deferred to keep this PR bounded.CiliumNetworkPolicy, CalicoGlobalNetworkPolicy) — stay on vanillanetworking.k8s.io/v1for portability.Backwards compatibility
Fully backwards-compatible. Master switch defaults to off; nothing renders until an operator flips it.
system-proxyfeature addition on reqwest is behaviourally inert unless operators set proxy env vars.