From d9207c09fd1ecc0bddbb5db7281142bfdf22dac5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 4 Feb 2022 15:18:59 -0500 Subject: [PATCH 1/3] use vec of firewall rules instead of hashmap --- common/src/api/external/mod.rs | 53 ++++++------- nexus/src/db/model.rs | 7 +- nexus/src/nexus.rs | 78 ++++++++++--------- nexus/tests/integration_tests/vpc_firewall.rs | 12 +-- openapi/nexus.json | 37 +++++++-- 5 files changed, 101 insertions(+), 86 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index f748e04af7..a585a2173e 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -28,7 +28,7 @@ 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; @@ -1416,10 +1416,10 @@ pub struct 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,24 +1442,16 @@ 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, + pub rules: Vec, } /** * 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, + pub rules: Vec, } impl FromIterator for VpcFirewallRuleUpdateResult { @@ -1467,12 +1459,7 @@ impl FromIterator for VpcFirewallRuleUpdateResult { where T: IntoIterator, { - Self { - rules: iter - .into_iter() - .map(|rule| (rule.identity.name.clone(), rule)) - .collect(), - } + Self { rules: iter.into_iter().collect() } } } @@ -2191,9 +2178,10 @@ 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" } ], @@ -2201,8 +2189,9 @@ mod test { "action": "allow", "priority": 65534, "description": "allow inbound traffic between instances" - }, - "rule2": { + }, + { + "name": "rule2", "status": "disabled", "direction": "outbound", "targets": [ { "type": "vpc", "value": "default" } ], @@ -2210,16 +2199,17 @@ mod test { "action": "deny", "priority": 65533, "description": "second rule" - } - } - "#; + } + ] + }"#; let params = serde_json::from_str::(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 +2229,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/model.rs b/nexus/src/db/model.rs index 424f3f11bc..fdffeef609 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1761,13 +1761,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(), }, ); @@ -1805,9 +1804,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/nexus.rs b/nexus/src/nexus.rs index d93fc1c3ca..0f1c968125 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -2516,41 +2516,47 @@ impl TestInterfaces for Nexus { lazy_static! { 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/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index a4b0aea817..2b9ba0faab 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -12,7 +12,6 @@ use omicron_common::api::external::{ VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams, }; use omicron_nexus::external_api::views::Vpc; -use std::collections::HashMap; use std::convert::TryFrom; use uuid::Uuid; @@ -61,10 +60,9 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { assert!(is_default_firewall_rules(&rules)); // Modify one VPC's firewall - let mut new_rules = HashMap::new(); - new_rules.insert( - "deny-all-incoming".parse().unwrap(), + let new_rules = vec![ VpcFirewallRuleUpdate { + name: "deny-all-incoming".parse().unwrap(), action: VpcFirewallRuleAction::Deny, description: "test desc".to_string(), status: VpcFirewallRuleStatus::Disabled, @@ -79,10 +77,8 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { direction: VpcFirewallRuleDirection::Inbound, priority: VpcFirewallRulePriority(100), }, - ); - new_rules.insert( - "allow-icmp".parse().unwrap(), VpcFirewallRuleUpdate { + name: "allow-icmp".parse().unwrap(), action: VpcFirewallRuleAction::Allow, description: "allow icmp".to_string(), status: VpcFirewallRuleStatus::Enabled, @@ -97,7 +93,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { direction: VpcFirewallRuleDirection::Inbound, priority: VpcFirewallRulePriority(10), }, - ); + ]; let update_params = VpcFirewallRuleUpdateParams { rules: new_rules.clone() }; client diff --git a/openapi/nexus.json b/openapi/nexus.json index 1224f180a0..1d39c41bec 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5277,6 +5277,14 @@ } ] }, + "name": { + "description": "name of the rule, unique to this VPC", + "allOf": [ + { + "$ref": "#/components/schemas/Name" + } + ] + }, "priority": { "description": "the relative priority of this rule", "type": "integer", @@ -5304,6 +5312,7 @@ "description", "direction", "filters", + "name", "priority", "status", "targets" @@ -5312,16 +5321,32 @@ "VpcFirewallRuleUpdateParams": { "description": "Updateable properties of a `Vpc`'s firewall Note that VpcFirewallRules are implicitly created along with a Vpc, so there is no explicit creation.", "type": "object", - "additionalProperties": { - "$ref": "#/components/schemas/VpcFirewallRuleUpdate" - } + "properties": { + "rules": { + "type": "array", + "items": { + "$ref": "#/components/schemas/VpcFirewallRuleUpdate" + } + } + }, + "required": [ + "rules" + ] }, "VpcFirewallRuleUpdateResult": { "description": "Response to an update replacing `Vpc`'s firewall", "type": "object", - "additionalProperties": { - "$ref": "#/components/schemas/VpcFirewallRule" - } + "properties": { + "rules": { + "type": "array", + "items": { + "$ref": "#/components/schemas/VpcFirewallRule" + } + } + }, + "required": [ + "rules" + ] }, "VpcResultsPage": { "description": "A single page of results", From b71b40a0d1b141dfc5155f11879308ceb2d11780 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 4 Feb 2022 16:22:59 -0500 Subject: [PATCH 2/3] don't paginate rules endpoint, use same response view for GET and PUT --- common/src/api/external/mod.rs | 18 +++--- nexus/src/db/datastore.rs | 4 +- nexus/src/external_api/http_entrypoints.rs | 14 ++--- nexus/src/nexus.rs | 13 ++-- nexus/tests/integration_tests/vpc_firewall.rs | 28 ++++----- openapi/nexus.json | 62 ++----------------- 6 files changed, 37 insertions(+), 102 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index a585a2173e..34d9c15409 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1415,6 +1415,14 @@ 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, +} + /// A single rule in a VPC firewall #[derive(Clone, Debug, Deserialize, PartialEq, Serialize, JsonSchema)] pub struct VpcFirewallRuleUpdate { @@ -1446,15 +1454,7 @@ pub struct VpcFirewallRuleUpdateParams { pub rules: Vec, } -/** - * Response to an update replacing `Vpc`'s firewall - */ -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct VpcFirewallRuleUpdateResult { - pub rules: Vec, -} - -impl FromIterator for VpcFirewallRuleUpdateResult { +impl FromIterator for VpcFirewallRules { fn from_iter(iter: T) -> Self where T: IntoIterator, diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index cc14293d58..51dfbd54cc 100644 --- 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 { 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/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d1a4455d2a..8b13b93a8d 100644 --- 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,15 +1459,13 @@ async fn subnets_ips_get( }] async fn vpc_firewall_rules_get( rqctx: Arc>>, - query_params: Query, path_params: Path, -) -> Result>, HttpError> { +) -> Result, 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 @@ -1476,14 +1473,12 @@ async fn vpc_firewall_rules_get( &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)?)) + Ok(HttpResponseOk(rules)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1500,8 +1495,9 @@ async fn vpc_firewall_rules_put( rqctx: Arc>>, path_params: Path, router_params: TypedBody, -) -> Result, HttpError> { +) -> Result, 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(); diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 0f1c968125..1345d21e3f 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -51,7 +51,7 @@ 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::VpcFirewallRules; use omicron_common::api::external::VpcRouterKind; use omicron_common::api::internal::nexus; use omicron_common::api::internal::nexus::DiskRuntimeState; @@ -1735,16 +1735,13 @@ impl Nexus { organization_name: &Name, project_name: &Name, vpc_name: &Name, - pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { 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( @@ -1753,7 +1750,7 @@ impl Nexus { project_name: &Name, vpc_name: &Name, params: &VpcFirewallRuleUpdateParams, - ) -> UpdateResult { + ) -> UpdateResult { let vpc = self .project_lookup_vpc(organization_name, project_name, vpc_name) .await?; diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index 2b9ba0faab..ef9d8245aa 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -9,13 +9,13 @@ use omicron_common::api::external::{ VpcFirewallRuleAction, VpcFirewallRuleDirection, VpcFirewallRuleFilter, VpcFirewallRuleHostFilter, VpcFirewallRulePriority, VpcFirewallRuleProtocol, VpcFirewallRuleStatus, VpcFirewallRuleTarget, - VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams, + VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams, VpcFirewallRules, }; use omicron_nexus::external_api::views::Vpc; use std::convert::TryFrom; use uuid::Uuid; -use dropshot::test_util::{object_delete, object_get, objects_list_page}; +use dropshot::test_util::{object_delete, object_get}; use nexus_test_utils::resource_helpers::{ create_organization, create_project, create_vpc, @@ -39,11 +39,10 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { let default_vpc_url = format!("{}/default", vpcs_url); let default_vpc = object_get::(client, &default_vpc_url).await; - let default_vpc_firewall = format!("{}/default/firewall/rules", vpcs_url); - let rules = - objects_list_page::(client, &default_vpc_firewall) - .await - .items; + let default_vpc_firewall = format!("{}/firewall/rules", default_vpc_url); + let rules = object_get::(client, &default_vpc_firewall) + .await + .rules; assert!(rules.iter().all(|r| r.vpc_id == default_vpc.identity.id)); assert!(is_default_firewall_rules(&rules)); @@ -53,9 +52,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { format!("{}/{}/firewall/rules", vpcs_url, other_vpc); let vpc2 = create_vpc(&client, &org_name, &project_name, &other_vpc).await; let rules = - objects_list_page::(client, &other_vpc_firewall) - .await - .items; + object_get::(client, &other_vpc_firewall).await.rules; assert!(rules.iter().all(|r| r.vpc_id == vpc2.identity.id)); assert!(is_default_firewall_rules(&rules)); @@ -107,10 +104,9 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { .unwrap(); // Make sure the firewall is changed - let rules = - objects_list_page::(client, &default_vpc_firewall) - .await - .items; + let rules = object_get::(client, &default_vpc_firewall) + .await + .rules; assert!(!is_default_firewall_rules(&rules)); assert_eq!(rules.len(), new_rules.len()); assert_eq!(rules[0].identity.name, "allow-icmp"); @@ -118,9 +114,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { // Make sure the other firewall is unchanged let rules = - objects_list_page::(client, &other_vpc_firewall) - .await - .items; + object_get::(client, &other_vpc_firewall).await.rules; assert!(is_default_firewall_rules(&rules)); // DELETE is unspported diff --git a/openapi/nexus.json b/openapi/nexus.json index 1d39c41bec..0d84e07cb0 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -1729,36 +1729,6 @@ "summary": "List firewall rules for a VPC.", "operationId": "vpc_firewall_rules_get", "parameters": [ - { - "in": "query", - "name": "limit", - "schema": { - "nullable": true, - "description": "Maximum number of items returned by a single call", - "type": "integer", - "format": "uint32", - "minimum": 1 - }, - "style": "form" - }, - { - "in": "query", - "name": "page_token", - "schema": { - "nullable": true, - "description": "Token returned by previous call to retreive the subsequent page", - "type": "string" - }, - "style": "form" - }, - { - "in": "query", - "name": "sort_by", - "schema": { - "$ref": "#/components/schemas/NameSortMode" - }, - "style": "form" - }, { "in": "path", "name": "organization_name", @@ -1793,13 +1763,12 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/VpcFirewallRuleResultsPage" + "$ref": "#/components/schemas/VpcFirewallRules" } } } } - }, - "x-dropshot-pagination": true + } }, "put": { "tags": [ @@ -1852,7 +1821,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/VpcFirewallRuleUpdateResult" + "$ref": "#/components/schemas/VpcFirewallRules" } } } @@ -5158,27 +5127,6 @@ "ICMP" ] }, - "VpcFirewallRuleResultsPage": { - "description": "A single page of results", - "type": "object", - "properties": { - "items": { - "description": "list of items on this page of results", - "type": "array", - "items": { - "$ref": "#/components/schemas/VpcFirewallRule" - } - }, - "next_page": { - "nullable": true, - "description": "token used to fetch the next page of results (if any)", - "type": "string" - } - }, - "required": [ - "items" - ] - }, "VpcFirewallRuleStatus": { "type": "string", "enum": [ @@ -5333,8 +5281,8 @@ "rules" ] }, - "VpcFirewallRuleUpdateResult": { - "description": "Response to an update replacing `Vpc`'s firewall", + "VpcFirewallRules": { + "description": "Collection of a [`Vpc`]'s firewall rules", "type": "object", "properties": { "rules": { From 6990c11a5b5e5df639c7cec0a52e0dc480b41f75 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 4 Feb 2022 17:35:27 -0500 Subject: [PATCH 3/3] don't need the FromIterator, nexus things should return models --- common/src/api/external/mod.rs | 10 ---------- nexus/src/external_api/http_entrypoints.rs | 13 +++++++------ nexus/src/nexus.rs | 12 ++---------- 3 files changed, 9 insertions(+), 26 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 34d9c15409..d090b0a010 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -34,7 +34,6 @@ 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; @@ -1454,15 +1453,6 @@ pub struct VpcFirewallRuleUpdateParams { pub rules: Vec, } -impl FromIterator for VpcFirewallRules { - fn from_iter(iter: T) -> Self - where - T: IntoIterator, - { - Self { rules: iter.into_iter().collect() } - } -} - /// Firewall rule priority. This is a value from 0 to 65535, with rules with /// lower values taking priority over higher values. #[derive( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 8b13b93a8d..a36f638cbc 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1474,11 +1474,10 @@ async fn vpc_firewall_rules_get( &path.project_name, &path.vpc_name, ) - .await? - .into_iter() - .map(|rule| rule.into()) - .collect(); - Ok(HttpResponseOk(rules)) + .await?; + Ok(HttpResponseOk(VpcFirewallRules { + rules: rules.into_iter().map(|rule| rule.into()).collect(), + })) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1510,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 index 1345d21e3f..6ac332e9ee 100644 --- 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::VpcFirewallRules; use omicron_common::api::external::VpcRouterKind; use omicron_common::api::internal::nexus; use omicron_common::api::internal::nexus::DiskRuntimeState; @@ -1750,7 +1749,7 @@ impl Nexus { project_name: &Name, vpc_name: &Name, params: &VpcFirewallRuleUpdateParams, - ) -> UpdateResult { + ) -> UpdateResult> { let vpc = self .project_lookup_vpc(organization_name, project_name, vpc_name) .await?; @@ -1758,14 +1757,7 @@ impl Nexus { 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(