refactor: consolidate shell port-spec parsing into shared parse_port_specs()#6037
Conversation
…) shell function - Add parse_port_specs() to setup-iptables.sh mirroring TypeScript parseValidPortSpecs() in src/host-iptables-validation.ts: takes a comma-separated input + label, validates each entry via is_valid_port_spec(), warns and skips invalid ones, populates a caller nameref array (bash 4.3+). - Refactor allow_service_ports_to_ip(): remove the duplicated inline regex validator since HSP_PORTS is now pre-validated by parse_port_specs(). - Refactor allow_host_access_to_gateway(): replace the inline IFS=',' read + trim + is_valid_port_spec loop with parse_port_specs. - Refactor configure_http_dnat(): replace the inline IFS=',' read + trim + is_valid_port_spec loop and the dead range/single-port if/else with parse_port_specs. - Refactor configure_host_access_rules(): replace the bare IFS=',' read for AWF_HOST_SERVICE_PORTS with parse_port_specs so validation happens at parse time. - Extend tests/setup-iptables-port-spec.test.sh: add 9 new test cases for parse_port_specs (empty input, single port, multiple ports, whitespace trimming, port ranges, invalid-spec filtering, warning emission, all-invalid input), 62 total. Closes #6015
There was a problem hiding this comment.
Pull request overview
This PR refactors containers/agent/setup-iptables.sh to consolidate duplicated comma-separated port-spec parsing/validation into a single shared parse_port_specs() helper, keeping the shell behavior aligned with the TypeScript port-spec validator and reducing divergence risk across iptables rule paths.
Changes:
- Introduces
parse_port_specs()(bash nameref-based) to trim, validate, and filter port specs with consistent warning behavior. - Replaces multiple bespoke
IFS=',' read+ trim + validate loops withparse_port_specs()in host access and DNAT configuration paths. - Expands shell unit tests to cover
parse_port_specs()behavior (empty input, whitespace trimming, ranges, mixed validity, and warning emission).
Show a summary per file
| File | Description |
|---|---|
| containers/agent/setup-iptables.sh | Adds shared parse_port_specs() and refactors multiple iptables configuration paths to reuse it. |
| tests/setup-iptables-port-spec.test.sh | Adds extraction and functional tests for parse_port_specs() alongside existing port-spec fixtures. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| for _pps_trimmed in "${_pps_entries[@]}"; do | ||
| _pps_trimmed=$(echo "$_pps_trimmed" | xargs) | ||
| if [ -z "$_pps_trimmed" ]; then | ||
| continue |
|
@copilot address review feedback |
Fixed in the latest commit. Replaced _pps_trimmed="${_pps_trimmed#"${_pps_trimmed%%[! ]*}"}"
_pps_trimmed="${_pps_trimmed%"${_pps_trimmed##*[! ]}"}"This strips leading and trailing spaces without spawning a subprocess or risk of |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
✅ Contribution Check completed successfully! PR follows the applicable CONTRIBUTING.md guidelines; no contribution-check comment needed. |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
❌ Security Guard failed. Please review the logs for details. |
|
✅ Build Test Suite completed successfully! |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Smoke Claude passed |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.98% | 98.98% | ➡️ +0.00% |
| Statements | 98.94% | 98.94% | ➡️ +0.00% |
| Functions | 99.44% | 99.44% | ➡️ +0.00% |
| Branches | 95.64% | 95.60% | 📉 -0.04% |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
Smoke Test: Copilot BYOK (Direct) Mode ✅ PASS
Running in direct BYOK mode via Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test: Services Connectivity
Result: FAIL — Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔍 Smoke Test Results
PR: refactor: consolidate shell port-spec parsing into shared parse_port_specs() Overall: PASS 🎉 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔬 Smoke Test: Copilot PAT Auth
Overall: PARTIAL — Template vars unresolved; pre-computed outputs unavailable. Auth mode: PAT (COPILOT_GITHUB_TOKEN) Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
✅ fix: use tag-only image ref in rootless permission repair Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"
- "registry.npmjs.org"See Network Configuration for more information.
|
Smoke Test: Claude Engine Validation
Overall Result: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test Results (Gemini)
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Chroot Version Comparison
Overall: FAILED — Python and Node.js versions differ between host and chroot. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
Running in direct BYOK mode (AWF_AUTH_TYPE=github-oidc + AWF_AUTH_AZURE_* + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) authenticated via Microsoft Entra Overall: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🔬 OTEL Tracing Smoke Test Results
All 5 scenarios pass. OTEL tracing integration is functional. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) Merged PRs: ${{ steps.smoke-data.outputs.SMOKE_PR_DATA }}
Overall: PASS cc Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Port validation logic (
is_valid_port_specsemantics +IFS=',' readparse loop) was duplicated three times insetup-iptables.sh, diverging from each other and from the TypeScriptparseValidPortSpecs()insrc/host-iptables-validation.ts. The inline validator inallow_service_ports_to_ip()used a different regex that didn't support ranges;configure_http_dnat()had a deadif/elseon*"-"*where both branches executed identicaliptablescommands.Changes
containers/agent/setup-iptables.shparse_port_specs <result_array_name> <input> <label>— a bash 4.3+ nameref function that directly mirrors TypeScriptparseValidPortSpecs(): trims whitespace, validates each entry viais_valid_port_spec(), warns-and-skips invalid specs, populates the caller's array with valid resultsallow_service_ports_to_ip(): drop duplicated inline regex —HSP_PORTSis now pre-validated at parse timeallow_host_access_to_gateway()/configure_http_dnat(): replace identicalIFS=',' read+ trim + validate loops with a singleparse_port_specscall; remove dead range/single-portif/elsein the DNAT pathconfigure_host_access_rules(): replace bareIFS=',' readforAWF_HOST_SERVICE_PORTSwithparse_port_specsso both port-env-var consumers validate at the same layertests/setup-iptables-port-spec.test.shrun_parse_port_specs/run_parse_port_specs_warningstest helpers using function extraction (same isolation pattern as the existingis_valid_port_spectests)