Represent resolved firewall filter_hosts as a HashSet (#8315)

Previously, we have been adding one `filter_host` entry for *every NIC*
in the VPC when using a `VpcFirewallRuleHostFilter::Vpc(...)`. This PR
fixes this in two ways:

* We no longer linear-scan the NIC list to attempt to resolve
`vpc_name`->`Vni` mappings per rule. We build a `HashMap` of these early
on.
* This isn't *practically* a problem given that we're only working in
terms of a single VPC today, but the principle is correct for when we do
support e.g. VPC peering.
* `filter_hosts` is now encoded as a `HashSet` in nexus and sled-agent,
which ensures this property holds even under users' attempts to
introduce duplicate host specifiers.

Closes #8305.

Author: FelixMcFelix

clients/sled-agent-client/src/lib.rs

@@ -75,6 +75,7 @@ progenitor::generate_api!(
         PortFec = omicron_common::api::internal::shared::PortFec,
         PortSpeed = omicron_common::api::internal::shared::PortSpeed,
         RouterId = omicron_common::api::internal::shared::RouterId,
+        ResolvedVpcFirewallRule = omicron_common::api::internal::shared::ResolvedVpcFirewallRule,
         ResolvedVpcRoute = omicron_common::api::internal::shared::ResolvedVpcRoute,
         ResolvedVpcRouteSet = omicron_common::api::internal::shared::ResolvedVpcRouteSet,
         RouterTarget = omicron_common::api::internal::shared::RouterTarget,

common/src/api/internal/nexus.rs

@@ -241,7 +241,9 @@ pub struct ProducerRegistrationResponse {
 /// A `HostIdentifier` represents either an IP host or network (v4 or v6),
 /// or an entire VPC (identified by its VNI). It is used in firewall rule
 /// host filters.
-#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema)]
+#[derive(
+    Clone, Debug, Deserialize, Serialize, Eq, PartialEq, Hash, JsonSchema,
+)]
 #[serde(tag = "type", content = "value", rename_all = "snake_case")]
 pub enum HostIdentifier {
     Ip(oxnet::IpNet),

common/src/api/internal/shared.rs

@@ -784,12 +784,12 @@ pub struct ResolvedVpcRoute {
 }
 
 /// VPC firewall rule after object name resolution has been performed by Nexus
-#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, JsonSchema)]
 pub struct ResolvedVpcFirewallRule {
     pub status: external::VpcFirewallRuleStatus,
     pub direction: external::VpcFirewallRuleDirection,
     pub targets: Vec<NetworkInterface>,
-    pub filter_hosts: Option<Vec<HostIdentifier>>,
+    pub filter_hosts: Option<HashSet<HostIdentifier>>,
     pub filter_ports: Option<Vec<external::L4PortRange>>,
     pub filter_protocols: Option<Vec<external::VpcFirewallRuleProtocol>>,
     pub action: external::VpcFirewallRuleAction,

nexus/networking/src/firewall_rules.rs

@@ -22,6 +22,7 @@ use omicron_common::api::external::Error;
 use omicron_common::api::external::ListResultVec;
 use omicron_common::api::internal::nexus::HostIdentifier;
 use omicron_common::api::internal::shared::NetworkInterface;
+use omicron_common::api::internal::shared::ResolvedVpcFirewallRule;
 use oxnet::IpNet;
 use slog::Logger;
 use slog::debug;
@@ -48,7 +49,7 @@ pub async fn resolve_firewall_rules_for_sled_agent(
     vpc: &db::model::Vpc,
     rules: &[db::model::VpcFirewallRule],
     log: &Logger,
-) -> Result<Vec<sled_agent_client::types::ResolvedVpcFirewallRule>, Error> {
+) -> Result<Vec<ResolvedVpcFirewallRule>, Error> {
     // Collect the names of instances, subnets, and VPCs that are either
     // targets or host filters. We have to find the sleds for all the
     // targets, and we'll need information about the IP addresses or
@@ -133,6 +134,7 @@ pub async fn resolve_firewall_rules_for_sled_agent(
     }
 
     let mut vpc_interfaces: NicMap = HashMap::new();
+    let mut vpc_vni_map = HashMap::new();
     for vpc_name in &vpcs {
         if let Ok((.., authz_vpc)) = LookupPath::new(opctx, datastore)
             .project_id(vpc.project_id)
@@ -144,6 +146,7 @@ pub async fn resolve_firewall_rules_for_sled_agent(
                 .derive_vpc_network_interface_info(opctx, &authz_vpc)
                 .await?
             {
+                vpc_vni_map.insert(&vpc_name.0, iface.vni);
                 vpc_interfaces
                     .entry(vpc_name.0.clone())
                     .or_insert_with(Vec::new)
@@ -328,7 +331,7 @@ pub async fn resolve_firewall_rules_for_sled_agent(
                         AllowedSourceIps::List(list) => Some(
                             list.iter()
                                 .copied()
-                                .map(|ip| HostIdentifier::Ip(ip).into())
+                                .map(HostIdentifier::Ip)
                                 .collect(),
                         ),
                     }
@@ -355,50 +358,40 @@ pub async fn resolve_firewall_rules_for_sled_agent(
             // There are host filters, but we don't need to apply the allowlist
             // to this VPC either, so insert the rules as-is.
             (Some(hosts), None) => {
-                let mut host_addrs = Vec::with_capacity(hosts.len());
+                let mut host_addrs = HashSet::with_capacity(hosts.len());
                 for host in hosts {
                     match &host.0 {
                         external::VpcFirewallRuleHostFilter::Instance(name) => {
                             for interface in instance_interfaces
                                 .get(&name)
                                 .unwrap_or(&no_interfaces)
                             {
-                                host_addrs.push(
-                                    HostIdentifier::Ip(IpNet::host_net(
-                                        interface.ip,
-                                    ))
-                                    .into(),
-                                )
+                                host_addrs.insert(HostIdentifier::Ip(
+                                    IpNet::host_net(interface.ip),
+                                ));
                             }
                         }
                         external::VpcFirewallRuleHostFilter::Subnet(name) => {
                             for subnet in subnet_networks
                                 .get(name)
                                 .unwrap_or(&no_networks)
                             {
-                                host_addrs.push(
-                                    HostIdentifier::Ip(IpNet::from(*subnet))
-                                        .into(),
-                                );
+                                host_addrs.insert(HostIdentifier::Ip(
+                                    IpNet::from(*subnet),
+                                ));
                             }
                         }
                         external::VpcFirewallRuleHostFilter::Ip(addr) => {
-                            host_addrs.push(
-                                HostIdentifier::Ip(IpNet::host_net(*addr))
-                                    .into(),
-                            )
+                            host_addrs.insert(HostIdentifier::Ip(
+                                IpNet::host_net(*addr),
+                            ));
                         }
                         external::VpcFirewallRuleHostFilter::IpNet(net) => {
-                            host_addrs.push(HostIdentifier::Ip(*net).into())
+                            host_addrs.insert(HostIdentifier::Ip(*net));
                         }
                         external::VpcFirewallRuleHostFilter::Vpc(name) => {
-                            for interface in vpc_interfaces
-                                .get(name)
-                                .unwrap_or(&no_interfaces)
-                            {
-                                host_addrs.push(
-                                    HostIdentifier::Vpc(interface.vni).into(),
-                                )
+                            if let Some(vni) = vpc_vni_map.get(name) {
+                                host_addrs.insert(HostIdentifier::Vpc(*vni));
                             }
                         }
                     }
@@ -414,25 +407,23 @@ pub async fn resolve_firewall_rules_for_sled_agent(
         let filter_ports = rule
             .filter_ports
             .as_ref()
-            .map(|ports| ports.iter().map(|v| v.0.into()).collect());
+            .map(|ports| ports.iter().map(|v| v.0).collect());
 
         let filter_protocols = rule
             .filter_protocols
             .as_ref()
-            .map(|protocols| protocols.iter().map(|v| v.0.into()).collect());
+            .map(|protocols| protocols.iter().map(|v| v.0).collect());
 
-        sled_agent_rules.push(
-            sled_agent_client::types::ResolvedVpcFirewallRule {
-                status: rule.status.0.into(),
-                direction: rule.direction.0.into(),
-                targets,
-                filter_hosts,
-                filter_ports,
-                filter_protocols,
-                action: rule.action.0.into(),
-                priority: rule.priority.0.0,
-            },
-        );
+        sled_agent_rules.push(ResolvedVpcFirewallRule {
+            status: rule.status.0,
+            direction: rule.direction.0,
+            targets,
+            filter_hosts,
+            filter_ports,
+            filter_protocols,
+            action: rule.action.0,
+            priority: rule.priority.0,
+        });
     }
     debug!(
         log,

nexus/src/app/vpc.rs

@@ -27,6 +27,7 @@ use omicron_common::api::external::ServiceIcmpConfig;
 use omicron_common::api::external::UpdateResult;
 use omicron_common::api::external::VpcFirewallRuleUpdateParams;
 use omicron_common::api::external::http_pagination::PaginatedBy;
+use omicron_common::api::internal::shared::ResolvedVpcFirewallRule;
 use std::sync::Arc;
 use uuid::Uuid;
 
@@ -262,8 +263,7 @@ impl super::Nexus {
         opctx: &OpContext,
         vpc: &db::model::Vpc,
         rules: &[db::model::VpcFirewallRule],
-    ) -> Result<Vec<sled_agent_client::types::ResolvedVpcFirewallRule>, Error>
-    {
+    ) -> Result<Vec<ResolvedVpcFirewallRule>, Error> {
         nexus_networking::resolve_firewall_rules_for_sled_agent(
             &self.db_datastore,
             opctx,

openapi/sled-agent.json

@@ -6500,7 +6500,8 @@
             "type": "array",
             "items": {
               "$ref": "#/components/schemas/HostIdentifier"
-            }
+            },
+            "uniqueItems": true
           },
           "filter_ports": {
             "nullable": true,