feat(firewall): open XNet port to all nodes in the network#10263
feat(firewall): open XNet port to all nodes in the network#10263pierugo-dfinity wants to merge 6 commits into
Conversation
| trusted_tcp_ports_for_node_whitelist: [22, 4100, 8080], | ||
| trusted_udp_ports_for_node_whitelist: [4100], | ||
| untrusted_tcp_ports_for_node_whitelist: [2497], | ||
| untrusted_udp_ports_for_node_whitelist: [], |
There was a problem hiding this comment.
I'm not sure I understand what trusted/untrusted means in this context. Maybe something like this?
| trusted_tcp_ports_for_node_whitelist: [22, 4100, 8080], | |
| trusted_udp_ports_for_node_whitelist: [4100], | |
| untrusted_tcp_ports_for_node_whitelist: [2497], | |
| untrusted_udp_ports_for_node_whitelist: [], | |
| whitelisted_node_tcp_ports: [22, 4100, 8080], | |
| whitelisted_node_udp_ports: [4100], | |
| all_node_tcp_ports: [2497], | |
| all_node_udp_ports: [], |
I think that would also be closer to the existing naming in the orchestrator
There was a problem hiding this comment.
Right, renamed here. I kept the "whitelist" at the end in contrast to the next field ports_for_http_adapter_blacklist
pierugo-dfinity
force-pushed
the
pierugo/firewall/open-xnet-to-all-nodes
branch
from
d9cd7d0 to
712017d
Compare
| // Insert the whitelisting rules at the top of the list (highest priority) | ||
| tcp_rules.insert(0, tcp_node_whitelisting_rule); | ||
| udp_rules.insert(0, udp_node_whitelisting_rule); | ||
| // Insert the ic-http-adapter rule at the top of the list (highest priority) |
There was a problem hiding this comment.
The comment here suggests that the order matters. Should we preserve it, and also add one above in get_node_whitelisting_rules?
| whitelisted_nodes_tcp_ports_whitelist: [22, 4100, 8080], | ||
| whitelisted_nodes_udp_ports_whitelist: [4100], | ||
| all_nodes_tcp_ports_whitelist: [2497], | ||
| all_nodes_udp_ports_whitelist: [], |
There was a problem hiding this comment.
Is there any system test we could adapt to test this?
There was a problem hiding this comment.
I added the firewall_correctness_test system test and moved firewall-related system tests into their own folder (with common functionality in its own crate ic_firewall_system_test_utils. LMK what you think of it: cbd870a.
Btw, by writing it, I realized that port 4100 now uses UDP and we can thus remove TCP from the list of allowed rules: 521da89.
| static TCP_PORTS_CLOSED: &[u32] = &[23, 24, 25]; | ||
|
|
||
| static CLOUD_ENGINE_NODE_REWARD_TYPES: &[NodeRewardType] = &[ |
There was a problem hiding this comment.
| static TCP_PORTS_CLOSED: &[u32] = &[23, 24, 25]; | |
| static CLOUD_ENGINE_NODE_REWARD_TYPES: &[NodeRewardType] = &[ | |
| const TCP_PORTS_CLOSED: &[u32] = &[23, 24, 25]; | |
| const CLOUD_ENGINE_NODE_REWARD_TYPES: &[NodeRewardType] = &[ |
| NodeRewardType::Type4dot1, | ||
| NodeRewardType::Type4dot2, | ||
| NodeRewardType::Type4dot3, | ||
| NodeRewardType::Type4dot3, |
There was a problem hiding this comment.
| NodeRewardType::Type4dot3, | |
| NodeRewardType::Type4dot4, |
More generally, I think by now we have multiple of these definitions (along with is_cloud_engine(), and similar) across the repository. Eventually I think we should have a single definition somewhere (i.e. in rs/protobuf/src/registry/node.rs), to reduce duplication and to make sure they don't diverge.
| * Ports in `trusted_tcp_ports_for_node_whitelist` should only be reachable from | ||
| whitelisted nodes. | ||
| * Ports in `untrusted_tcp_ports_for_node_whitelist` should be reachable from all IC |
There was a problem hiding this comment.
Are these still the old names?
| async fn add_dummy_registry_rule(nns_node: &IcNodeSnapshot, log: &Logger) { | ||
| let ipv6_prefixes = get_config().firewall.unwrap().default_rules[0] | ||
| .ipv6_prefixes | ||
| .clone(); | ||
| // This is actually not a dummy rule. It still allows inbound SSH such that the test driver can | ||
| // connect to the nodes and perform the test. |
There was a problem hiding this comment.
Maybe something like this?
| async fn add_dummy_registry_rule(nns_node: &IcNodeSnapshot, log: &Logger) { | |
| let ipv6_prefixes = get_config().firewall.unwrap().default_rules[0] | |
| .ipv6_prefixes | |
| .clone(); | |
| // This is actually not a dummy rule. It still allows inbound SSH such that the test driver can | |
| // connect to the nodes and perform the test. | |
| async fn add_ssh_only_registry_rule(nns_node: &IcNodeSnapshot, log: &Logger) { | |
| let ipv6_prefixes = get_config().firewall.unwrap().default_rules[0] | |
| .ipv6_prefixes | |
| .clone(); |
| // Wait for a while to make sure that all nodes have updated their firewall rules according | ||
| // to the new registry configuration. | ||
| tokio::time::sleep(std::time::Duration::from_secs(20)).await; |
There was a problem hiding this comment.
This seems like it could be flaky. Alternatively maybe we could retry one of the tcp connections until it succeeds, or check the firewall_registry_version metric of all nodes
| tags = [ | ||
| "colocate", | ||
| ], |
There was a problem hiding this comment.
These used to be "long_test"s. They aren't anymore, is that intentional?
| execute_add_firewall_rules_proposal( | ||
| log, | ||
| nns_node, | ||
| FirewallRulesScope::Global, |
There was a problem hiding this comment.
Did the scope change intentionally?
3 participants
In an effort to allow XNet connections between cloud engines and other subnets, this PR opens port 2497 (the XNet port) to all nodes in the network.
This is done by splitting the current
[tcp/udp]_ports_for_node_whitelistconfig field into twotrusted_[tcp/udp]_ports_for_node_whitelistanduntrusted_[tcp/udp]_ports_for_node_whitelist. The trusted ports are open only to "whitelisted" nodes (i.e., non-type4nodes for non-type4nodes, and all nodes fortype4nodes, as dictated by theis_whitelistedfunction infirewall.rs), while the untrusted ports are always open to all nodes.