feat(system-tests): run firewall_correctness_test on the local backend - #11217
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Enable firewall_correctness_test to run on the local backend by avoiding an overly-broad local firewall bootstrap whitelist and by reading existing global firewall rules before proposing changes.
Changes:
- Remove
backend = "farm"pinning forfirewall_correctness_testand adjust it to hash against existing global firewall rules. - Narrow the local backend’s seeded firewall whitelist from group-wide ULA space to the driver’s specific
/128sources, with an opt-in “group-wide” expansion for tests that need it. - Update Rosetta local setup to explicitly request the wider whitelist needed for a universal VM to reach replica
:8080.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rs/tests/networking/firewall/firewall_correctness_test.rs | Reads current global firewall rules before submitting a proposal so the test can run on local backend. |
| rs/tests/networking/firewall/BUILD.bazel | Drops backend = "farm" restriction for firewall_correctness_test. |
| rs/tests/financial_integrations/rosetta/rosetta_test_lib/setup.rs | Opts Rosetta tests into a group-wide firewall whitelist needed for a non-driver VM. |
| rs/tests/driver/src/driver/test_env_api.rs | Adds TopologySnapshot::firewall_rules helper to retrieve registered firewall rules for hashing/proposals. |
| rs/tests/driver/src/driver/local_backend.rs | Centralizes driver IPv6 sources and exposes /128 prefixes; adds a group ULA constant. |
| rs/tests/driver/src/driver/ic.rs | Introduces API to add extra firewall whitelist prefixes/ports (and a group-wide shorthand). |
| rs/tests/driver/src/driver/bootstrap.rs | Switches local bootstrap whitelist to driver /128s plus optional extras; factors whitelisted ports into a constant. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
rs/tests/driver/src/driver/bootstrap.rs:85
LOCAL_WHITELISTED_PORTSis documented as mirroring the firewall template’sdefault_rules, but it currently omits port 9314 (present in the template’s default rule ports) and includes 9324 (not in the template’s default rule ports). This makes the comment misleading and can also unintentionally block the driver from reaching node services that are expected to be reachable on 9314 via the local whitelist.
/// The ports the Local backend whitelists for the test driver on every node,
/// mirroring the ports the firewall template's `default_rules` open to Farm's
/// management prefixes. `ic-prep` always adds 8080 on top of these.
const LOCAL_WHITELISTED_PORTS: &[u32] = &[
22, 2497, 4100, 7070, 9090, 9091, 9100, 9324, 19100, 19523, 19531,
19a457e to
b78a249
Compare
…ackend `with_api_boundary_nodes_playnet` gives API boundary nodes a domain name and a certificate for it that the nodes trust. Both halves came from Farm, so every test using it — and therefore every test with a cloud engine subnet, which requires it — was pinned to `backend = "farm"`. Stand up an equivalent locally: * Turn the group's `dnsmasq` into its DNS server. It already ran on the group bridge as an RA/DHCPv4 daemon with `--port=0`; dropping that and adding `--no-resolv --no-hosts` makes it a hermetic resolver that answers from an `--addn-hosts` file (new `LocalBackend::add_dns_record`) and from a `--synth-domain` mirroring the public `nip.io` wildcard service. GuestOS has no name-server knob and boots with `IPv6AcceptRA=no`, so rather than reconfiguring the guests, `create_group` assigns the four addresses GuestOS is hard-coded to query to the bridge. Inside the backend's own network namespace those addresses are free and no query can escape, so every node gets a working resolver without touching IC-OS. * Add `InternetComputer::setup_api_bn_local_playnet`, which issues an ephemeral CA plus a leaf covering the API boundary nodes' domains and registers those domains with the group's `dnsmasq`. `bootstrap` serves the leaf from `ic-boundary` through the existing `ic_boundary_tls_cert` mechanism. * Let the replica trust that CA. `nns_delegation_manager` built its root store from the compiled-in public roots only, which no test-issued certificate can satisfy. It now also honours `extra_api_boundary_node_trust_anchors_pem`, a new dev-only `GuestOSDevSettings` field that is unset in production, leaving the public roots as the only anchors there. Drops `backend = "farm"` from `canister_http_socks_test`, `cloud_engine_canister_sig_test`, `xnet_cloud_engine_isolation_test`, `nns_delegation_branch_nns_version_test` and `delete_subnet_test`. The `cpus` of `canister_http_socks_test` was understated: its comment omitted the four cloud engine nodes. Two tests stay on Farm for unrelated reasons, now recorded accurately: `nns_delegation_mainnet_nns_version_test` runs the mainnet GuestOS, whose replica predates the new field; and `firewall_correctness_test` asserts that port 8080 is closed between certain nodes, which the local backend cannot satisfy because `ic-prep` always adds 8080 to the `fd00::/8` rule the driver needs to reach the nodes. That test did assume the global firewall rule set starts out empty, which it does not on the local backend, so it now reads the current rules through the new `TopologySnapshot::firewall_rules`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Url::parse` was handed the whole comma-separated list, which yields a single `Url` whose host is `icp-api.io,https` and whose path is the remainder. That re-serialises to `https://icp-api.io,https//icp0.io,https://ic0.app` — not a valid URL, and not the three the fixture meant to carry. Split it into three, and regenerate the v1.16.0 fixtures, which this branch introduces. The older fixtures keep the malformed value: they are historical records that must stay byte-for-byte as generated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…kend `start_ra_daemon`/`stop_ra_daemon` became `start_dnsmasq`/`stop_dnsmasq` when the daemon took on DNS, but a cross-reference in `canister_http_socks_test` still pointed at the old name, and three comments still called it the RA daemon even though it now serves RA, DHCPv4 and DNS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es plumbing `firewall_correctness_test` stays `backend = "farm"`, and on Farm the global firewall scope starts out empty, so reading the current rules and passing them as the proposal's `previous_rules` buys nothing there. It was a remnant of trying to also enable the test on the local backend, where the backend seeds a global rule so the driver can reach the nodes. Move it, and the `TopologySnapshot::firewall_rules` accessor it needed, to the follow-up that actually enables the test locally. Both files are byte-identical to master again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… local upgrades `assert_assigned_replica_version` allows a node 600s to come up on a new replica version. On Farm that covers a whole GuestOS upgrade cycle; on the local backend it does not. This test's 10 VMs ask for 60 vCPUs and 40 GiB of guest RAM from a single host, and one cycle was measured at ~9 min there: ~90s to download the ~580 MiB update image, ~240s for `manageboot.sh` to `tar`-unpack it into the guest's tmpfs `/tmp`, ~35s to `dd` it onto the inactive slot and ~150s to reboot. Step 5's deadline expired 79s after the orchestrator came back up on the new version, before any replica had bound :8080, so the test panicked with "Replica did reboot, but never came back online!" -- which is only what `assert_assigned_replica_version_with_time` prints when its last poll errored. Wait 20 min per node instead, and raise the per-test timeout from 30 to 50 min: `ImageUpgrader::execute_upgrade` deletes the update image after installing and never checks whether the target version already sits on the inactive slot, so the roll back in Step 7 and the roll forward in Step 9 each pay for the full cycle again. A local run now passes in 2300s, still well inside the `test_timeout = "eternal"` hour the BUILD file gives the action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`firewall_correctness_test` asserts that port 8080 is closed from cloud engine nodes to non-cloud-engine nodes. `init_ic` seeds a global registry firewall rule so the test driver can reach the nodes once the orchestrator's nftables ruleset is live — the driver's source addresses lie outside the nodes' `/64`, so nothing whitelists them by default. That rule whitelisted the group's whole ULA range `fd00::/8` and `ic-prep` always adds 8080 to it, so since the nodes are addressed out of that same range it opened 8080 between all of them. Whitelist the driver's own three addresses as `/128`s instead of the range they live in. Node<->node traffic is then governed by the registry's node-whitelisting rules alone, exactly as on Farm. Nothing in the IC needs the wider range: non-cloud-engine nodes reach the NNS on `:8080` through those same rules, cloud engine nodes reach it through an API boundary node's `:443`, and XNet `:2497` comes from the all-nodes rule. A VM in the group other than the driver can need it, though. Such a VM shares the nodes' `/64`, which the GuestOS firewall already accepts on 7070/9090/9091/9100/19100/19522/19531 (plus 9314 on cloud engines and 9324 on API boundary nodes), so only 22, 2497, 4100, 8080 and 19523 become unreachable. `InternetComputer::with_extra_firewall_whitelist` widens the whitelist for those cases, adding to the driver's prefixes and ports rather than replacing them. Of the local tests that put a VM in the group, only the rosetta ones need it: they run `ic-rosetta-api --ic-url http://[node]:8080` on a universal VM. The driver's three addresses now come from a single `group_driver_ipv6s`, shared by `create_group`, `delete_group` and the whitelist, so a fourth one cannot fall out of the firewall rule unnoticed. The test itself reads the current global rules through the new `TopologySnapshot::firewall_rules` rather than assuming that scope starts out empty, which it does not on this backend. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Address review feedback on the local backend's firewall whitelist.
The prefixes and ports render into anonymous nftables sets
(`ip6 saddr { ... }`), and `nft` rejects a set with a repeated element, taking
the whole ruleset down with it. Since `with_extra_firewall_whitelist` adds to
the driver's own entries, a test could introduce a duplicate easily enough — by
calling it twice, or by passing a port that is already whitelisted. Deduplicate
both lists, which also lets `Itertools::join` replace the map/collect/join
dance.
Also record why `firewall_correctness_test` still inserts its rule at position
0 now that the scope is not necessarily empty: both that rule and the seeded
one are `Allow`, so their relative order cannot change the outcome, and 0 is
the only valid position when the scope does start out empty, as on Farm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b78a249 to
5a2afab
Compare
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
firewall_correctness_testasserts that port 8080 is closed from cloud engine nodes to non-cloud-engine nodes. It was the last test pinned tobackend = "farm"by the local backend's firewall setup rather than by a missing feature.Why it could not run locally
init_icseeds aGlobal-scope registry firewall rule so the test driver can reach the nodes once the orchestrator's nftables ruleset is live — the driver's source addresses lie outside the nodes'/64, so nothing whitelists them by default. That rule whitelisted the group's whole ULA rangefd00::/8, andic-prepalways adds 8080 to it. The nodes are addressed out of that same range, so the rule opened 8080 between all of them and the assertion could not hold.The change
The rule now whitelists the driver's own three addresses (management, journald-streaming, file server) as
/128s instead of the range they live in. Node↔node traffic is then governed by the registry's node whitelisting rules alone, exactly as on Farm.Nothing in the IC needs the wider range:
:8080through those same whitelisting rules;:443—get_node_api_urlspicks API BN URLs by reward type precisely "since NNS nodes would not accept our connections due to firewall rules";:8080through the whitelisting rules, and XNet:2497comes from the all-nodes rule.What can need it is a VM in the group other than the driver. Such a VM shares the nodes'
/64, which the GuestOS firewall already accepts on 7070/9090/9091/9100/19100/19522/19531 (plus 9314 on cloud engines and 9324 on API boundary nodes) — so only the remaining whitelisted ports (22, 2497, 4100, 8080, 19523) become unreachable.InternetComputer::with_extra_firewall_whitelist(and thewith_group_wide_firewall_whitelistshorthand) widens the whitelist for those cases; the extra prefixes and ports are added to the driver's, so a caller cannot lock the driver out.Sweeping the local tests that put a VM in the group, exactly one needs it: the rosetta tests run
ic-rosetta-api --ic-url http://[node]:8080on a universal VM. The others are all covered by the nodes' own/64or talk to an API boundary node on:443; the Vector VM is not started on this backend at all.The driver's three addresses now come from a single
LocalBackend::group_driver_ipv6s, shared bycreate_group,delete_groupand the whitelist, so a fourth one cannot silently fall out of the firewall rule.Finally, the test reads the current global rules through the new
TopologySnapshot::firewall_rulesrather than assuming that scope starts out empty, which it does not on this backend.Verification
backend = "farm"is dropped fromfirewall_correctness_test, which now passes on the local backend (296s, 5 IC node VMs plus an API boundary node).Run locally alongside it, all passing:
firewall_priority_test_localandfirewall_max_connections_test_local— the other two tests over this subsystem;rosetta_test_local— the one test that needs the widened whitelist;cloud_engine_canister_sig_test_local— a cloud engine subnet replicating its registry through an API boundary node rather than through the NNS replica's:8080.upgrade_downgrade_app_subnet_test_localwas not run locally — two attempts died for lack of disk on the machine I verified on, and it is left to CI. It is the interesting case for a released GuestOS: the whitelist stays aGlobal-scope registry rule, which every GuestOS version honours regardless of the image it boots, and only the rule'sipv6_prefixeschange — from one/8to three/128s, which render into the sameip6 saddr { ... }nftables rule.On Farm nothing changes: the
Globalscope is empty there, so the rules the test now reads back arevec![]— exactly what it passed before — and the narrowed whitelist is seeded only on the local backend.firewall_correctness_testandfirewall_correctness_test_head_nnswere run on Farm at the parent commit and pass; CI re-runs them here.