Use Vec instead of HashMap for firewall rules, don't paginate (#668)

* use vec of firewall rules instead of hashmap * don't paginate rules endpoint, use same response view for GET and PUT * don't need the FromIterator, nexus things should return models
oxidecomputer · Feb 7, 2022 · f615ee4 · f615ee4
1 parent 65abb10
commit f615ee4
Show file tree

Hide file tree

Showing 8 changed files with 141 additions and 208 deletions.
diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs
@@ -28,13 +28,12 @@ use schemars::JsonSchema;
 use serde::Deserialize;
 use serde::Serialize;
 use serde_with::{DeserializeFromStr, SerializeDisplay};
-use std::collections::{BTreeMap, HashMap};
+use std::collections::BTreeMap;
 use std::convert::TryFrom;
 use std::fmt::Debug;
 use std::fmt::Display;
 use std::fmt::Formatter;
 use std::fmt::Result as FormatResult;
-use std::iter::FromIterator;
 use std::net::IpAddr;
 use std::num::{NonZeroU16, NonZeroU32};
 use std::str::FromStr;
@@ -1415,11 +1414,19 @@ pub struct VpcFirewallRule {
     pub vpc_id: Uuid,
 }
 
+/**
+ * Collection of a [`Vpc`]'s firewall rules
+ */
+#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
+pub struct VpcFirewallRules {
+    pub rules: Vec<VpcFirewallRule>,
+}
+
 /// A single rule in a VPC firewall
-#[derive(Clone, Debug, PartialEq, Deserialize, Serialize, JsonSchema)]
+#[derive(Clone, Debug, Deserialize, PartialEq, Serialize, JsonSchema)]
 pub struct VpcFirewallRuleUpdate {
-    // In an update, the name is encoded as a key in the JSON object, so we
-    // don't include one here
+    /// name of the rule, unique to this VPC
+    pub name: Name,
     /// human-readable free-form text about a resource
     pub description: String,
     /// whether this rule is in effect
@@ -1442,38 +1449,8 @@ pub struct VpcFirewallRuleUpdate {
  * so there is no explicit creation.
  */
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
-// TODO we're controlling the schemars output, but not the serde
-// deserialization here because of surprising behavior; see #449
-#[schemars(deny_unknown_fields)]
 pub struct VpcFirewallRuleUpdateParams {
-    #[serde(flatten)]
-    pub rules: HashMap<Name, VpcFirewallRuleUpdate>,
-}
-
-/**
- * Response to an update replacing `Vpc`'s firewall
- */
-#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
-// TODO we're controlling the schemars output, but not the serde
-// deserialization here because of surprising behavior; see #449
-#[schemars(deny_unknown_fields)]
-pub struct VpcFirewallRuleUpdateResult {
-    #[serde(flatten)]
-    pub rules: HashMap<Name, VpcFirewallRule>,
-}
-
-impl FromIterator<VpcFirewallRule> for VpcFirewallRuleUpdateResult {
-    fn from_iter<T>(iter: T) -> Self
-    where
-        T: IntoIterator<Item = VpcFirewallRule>,
-    {
-        Self {
-            rules: iter
-                .into_iter()
-                .map(|rule| (rule.identity.name.clone(), rule))
-                .collect(),
-        }
-    }
+    pub rules: Vec<VpcFirewallRuleUpdate>,
 }
 
 /// Firewall rule priority. This is a value from 0 to 65535, with rules with
@@ -2191,35 +2168,38 @@ mod test {
 
     #[test]
     fn test_firewall_deserialization() {
-        let json = r#"
-            {
-            "allow-internal-inbound": {
+        let json = r#"{
+            "rules": [
+              {
+                "name": "allow-internal-inbound",
                 "status": "enabled",
                 "direction": "inbound",
                 "targets": [ { "type": "vpc", "value": "default" } ],
                 "filters": {"hosts": [ { "type": "vpc", "value": "default" } ]},
                 "action": "allow",
                 "priority": 65534,
                 "description": "allow inbound traffic between instances"
-            },
-            "rule2": {
+              },
+              {
+                "name": "rule2",
                 "status": "disabled",
                 "direction": "outbound",
                 "targets": [ { "type": "vpc", "value": "default" } ],
                 "filters": {"ports": [ "22-25", "27" ], "protocols": [ "UDP" ]},
                 "action": "deny",
                 "priority": 65533,
                 "description": "second rule"
-            }
-            }
-            "#;
+              }
+            ]
+          }"#;
         let params =
             serde_json::from_str::<VpcFirewallRuleUpdateParams>(json).unwrap();
         assert_eq!(params.rules.len(), 2);
         assert_eq!(
-            params.rules[&Name::try_from("allow-internal-inbound".to_string())
-                .unwrap()],
+            params.rules[0],
             VpcFirewallRuleUpdate {
+                name: Name::try_from("allow-internal-inbound".to_string())
+                    .unwrap(),
                 status: VpcFirewallRuleStatus::Enabled,
                 direction: VpcFirewallRuleDirection::Inbound,
                 targets: vec![VpcFirewallRuleTarget::Vpc(
@@ -2239,8 +2219,9 @@ mod test {
             }
         );
         assert_eq!(
-            params.rules[&Name::try_from("rule2".to_string()).unwrap()],
+            params.rules[1],
             VpcFirewallRuleUpdate {
+                name: Name::try_from("rule2".to_string()).unwrap(),
                 status: VpcFirewallRuleStatus::Disabled,
                 direction: VpcFirewallRuleDirection::Outbound,
                 targets: vec![VpcFirewallRuleTarget::Vpc(

diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs
@@ -2043,13 +2043,13 @@ impl DataStore {
     pub async fn vpc_list_firewall_rules(
         &self,
         vpc_id: &Uuid,
-        pagparams: &DataPageParams<'_, Name>,
     ) -> ListResultVec<VpcFirewallRule> {
         use db::schema::vpc_firewall_rule::dsl;
 
-        paginated(dsl::vpc_firewall_rule, dsl::name, &pagparams)
+        dsl::vpc_firewall_rule
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::vpc_id.eq(*vpc_id))
+            .order(dsl::name.asc())
             .select(VpcFirewallRule::as_select())
             .load_async(self.pool())
             .await

diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs
@@ -1780,13 +1780,12 @@ impl VpcFirewallRule {
     pub fn new(
         rule_id: Uuid,
         vpc_id: Uuid,
-        rule_name: external::Name,
         rule: &external::VpcFirewallRuleUpdate,
     ) -> Self {
         let identity = VpcFirewallRuleIdentity::new(
             rule_id,
             external::IdentityMetadataCreateParams {
-                name: rule_name,
+                name: rule.name.clone(),
                 description: rule.description.clone(),
             },
         );
@@ -1824,9 +1823,7 @@ impl VpcFirewallRule {
         params
             .rules
             .iter()
-            .map(|(name, rule)| {
-                VpcFirewallRule::new(Uuid::new_v4(), vpc_id, name.clone(), rule)
-            })
+            .map(|rule| VpcFirewallRule::new(Uuid::new_v4(), vpc_id, rule))
             .collect()
     }
 }

diff --git a/nexus/src/defaults.rs b/nexus/src/defaults.rs
@@ -13,42 +13,48 @@ use std::net::Ipv6Addr;
 lazy_static! {
     pub static ref DEFAULT_FIREWALL_RULES: external::VpcFirewallRuleUpdateParams =
         serde_json::from_str(r#"{
-            "allow-internal-inbound": {
-                "status": "enabled",
-                "direction": "inbound",
-                "targets": [ { "type": "vpc", "value": "default" } ],
-                "filters": { "hosts": [ { "type": "vpc", "value": "default" } ] },
-                "action": "allow",
-                "priority": 65534,
-                "description": "allow inbound traffic to all instances within the VPC if originated within the VPC"
-            },
-            "allow-ssh": {
-                "status": "enabled",
-                "direction": "inbound",
-                "targets": [ { "type": "vpc", "value": "default" } ],
-                "filters": { "ports": [ "22" ], "protocols": [ "TCP" ] },
-                "action": "allow",
-                "priority": 65534,
-                "description": "allow inbound TCP connections on port 22 from anywhere"
-            },
-            "allow-icmp": {
-                "status": "enabled",
-                "direction": "inbound",
-                "targets": [ { "type": "vpc", "value": "default" } ],
-                "filters": { "protocols": [ "ICMP" ] },
-                "action": "allow",
-                "priority": 65534,
-                "description": "allow inbound ICMP traffic from anywhere"
-            },
-            "allow-rdp": {
-                "status": "enabled",
-                "direction": "inbound",
-                "targets": [ { "type": "vpc", "value": "default" } ],
-                "filters": { "ports": [ "3389" ], "protocols": [ "TCP" ] },
-                "action": "allow",
-                "priority": 65534,
-                "description": "allow inbound TCP connections on port 3389 from anywhere"
-            }
+            "rules": [
+                {
+                    "name": "allow-internal-inbound",
+                    "status": "enabled",
+                    "direction": "inbound",
+                    "targets": [ { "type": "vpc", "value": "default" } ],
+                    "filters": { "hosts": [ { "type": "vpc", "value": "default" } ] },
+                    "action": "allow",
+                    "priority": 65534,
+                    "description": "allow inbound traffic to all instances within the VPC if originated within the VPC"
+                },
+                {
+                    "name": "allow-ssh",
+                    "status": "enabled",
+                    "direction": "inbound",
+                    "targets": [ { "type": "vpc", "value": "default" } ],
+                    "filters": { "ports": [ "22" ], "protocols": [ "TCP" ] },
+                    "action": "allow",
+                    "priority": 65534,
+                    "description": "allow inbound TCP connections on port 22 from anywhere"
+                },
+                {
+                    "name": "allow-icmp",
+                    "status": "enabled",
+                    "direction": "inbound",
+                    "targets": [ { "type": "vpc", "value": "default" } ],
+                    "filters": { "protocols": [ "ICMP" ] },
+                    "action": "allow",
+                    "priority": 65534,
+                    "description": "allow inbound ICMP traffic from anywhere"
+                },
+                {
+                    "name": "allow-rdp",
+                    "status": "enabled",
+                    "direction": "inbound",
+                    "targets": [ { "type": "vpc", "value": "default" } ],
+                    "filters": { "ports": [ "3389" ], "protocols": [ "TCP" ] },
+                    "action": "allow",
+                    "priority": 65534,
+                    "description": "allow inbound TCP connections on port 3389 from anywhere"
+                }
+            ]
         }"#).unwrap();
 }
 

diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs
@@ -52,9 +52,8 @@ use omicron_common::api::external::RouterRouteCreateParams;
 use omicron_common::api::external::RouterRouteKind;
 use omicron_common::api::external::RouterRouteUpdateParams;
 use omicron_common::api::external::Saga;
-use omicron_common::api::external::VpcFirewallRule;
 use omicron_common::api::external::VpcFirewallRuleUpdateParams;
-use omicron_common::api::external::VpcFirewallRuleUpdateResult;
+use omicron_common::api::external::VpcFirewallRules;
 use omicron_common::api::external::VpcRouter;
 use omicron_common::api::external::VpcRouterKind;
 use ref_cast::RefCast;
@@ -1460,30 +1459,25 @@ async fn subnets_ips_get(
 }]
 async fn vpc_firewall_rules_get(
     rqctx: Arc<RequestContext<Arc<ServerContext>>>,
-    query_params: Query<PaginatedByName>,
     path_params: Path<VpcPathParam>,
-) -> Result<HttpResponseOk<ResultsPage<VpcFirewallRule>>, HttpError> {
+) -> Result<HttpResponseOk<VpcFirewallRules>, HttpError> {
     // TODO: Check If-Match and fail if the ETag doesn't match anymore.
     // Without this check, if firewall rules change while someone is listing
     // the rules, they will see a mix of the old and new rules.
     let apictx = rqctx.context();
     let nexus = &apictx.nexus;
-    let query = query_params.into_inner();
     let path = path_params.into_inner();
     let handler = async {
         let rules = nexus
             .vpc_list_firewall_rules(
                 &path.organization_name,
                 &path.project_name,
                 &path.vpc_name,
-                &data_page_params_for(&rqctx, &query)?
-                    .map_name(|n| Name::ref_cast(n)),
             )
-            .await?
-            .into_iter()
-            .map(|rule| rule.into())
-            .collect();
-        Ok(HttpResponseOk(ScanByName::results_page(&query, rules)?))
+            .await?;
+        Ok(HttpResponseOk(VpcFirewallRules {
+            rules: rules.into_iter().map(|rule| rule.into()).collect(),
+        }))
     };
     apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
 }
@@ -1500,8 +1494,9 @@ async fn vpc_firewall_rules_put(
     rqctx: Arc<RequestContext<Arc<ServerContext>>>,
     path_params: Path<VpcPathParam>,
     router_params: TypedBody<VpcFirewallRuleUpdateParams>,
-) -> Result<HttpResponseOk<VpcFirewallRuleUpdateResult>, HttpError> {
+) -> Result<HttpResponseOk<VpcFirewallRules>, HttpError> {
     // TODO: Check If-Match and fail if the ETag doesn't match anymore.
+    // TODO: limit size of the ruleset because the GET endpoint is not paginated
     let apictx = rqctx.context();
     let nexus = &apictx.nexus;
     let path = path_params.into_inner();
@@ -1514,7 +1509,9 @@ async fn vpc_firewall_rules_put(
                 &router_params.into_inner(),
             )
             .await?;
-        Ok(HttpResponseOk(rules))
+        Ok(HttpResponseOk(VpcFirewallRules {
+            rules: rules.into_iter().map(|rule| rule.into()).collect(),
+        }))
     };
     apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
 }

diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs
@@ -51,7 +51,6 @@ use omicron_common::api::external::RouterRouteKind;
 use omicron_common::api::external::RouterRouteUpdateParams;
 use omicron_common::api::external::UpdateResult;
 use omicron_common::api::external::VpcFirewallRuleUpdateParams;
-use omicron_common::api::external::VpcFirewallRuleUpdateResult;
 use omicron_common::api::external::VpcRouterKind;
 use omicron_common::api::internal::nexus;
 use omicron_common::api::internal::nexus::DiskRuntimeState;
@@ -1736,16 +1735,13 @@ impl Nexus {
         organization_name: &Name,
         project_name: &Name,
         vpc_name: &Name,
-        pagparams: &DataPageParams<'_, Name>,
     ) -> ListResultVec<db::model::VpcFirewallRule> {
         let vpc = self
             .project_lookup_vpc(organization_name, project_name, vpc_name)
             .await?;
-        let subnets = self
-            .db_datastore
-            .vpc_list_firewall_rules(&vpc.id(), pagparams)
-            .await?;
-        Ok(subnets)
+        let rules =
+            self.db_datastore.vpc_list_firewall_rules(&vpc.id()).await?;
+        Ok(rules)
     }
 
     pub async fn vpc_update_firewall_rules(
@@ -1754,22 +1750,15 @@ impl Nexus {
         project_name: &Name,
         vpc_name: &Name,
         params: &VpcFirewallRuleUpdateParams,
-    ) -> UpdateResult<VpcFirewallRuleUpdateResult> {
+    ) -> UpdateResult<Vec<db::model::VpcFirewallRule>> {
         let vpc = self
             .project_lookup_vpc(organization_name, project_name, vpc_name)
             .await?;
         let rules = db::model::VpcFirewallRule::vec_from_params(
             vpc.id(),
             params.clone(),
         );
-        let result = self
-            .db_datastore
-            .vpc_update_firewall_rules(&vpc.id(), rules)
-            .await?
-            .into_iter()
-            .map(|rule| rule.into())
-            .collect();
-        Ok(result)
+        self.db_datastore.vpc_update_firewall_rules(&vpc.id(), rules).await
     }
 
     pub async fn vpc_list_subnets(