diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 5f4a7d12af..bf621a23cb 100644 --- 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, +} + /// 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, -} - -/** - * 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, -} - -impl FromIterator for VpcFirewallRuleUpdateResult { - fn from_iter(iter: T) -> Self - where - T: IntoIterator, - { - Self { - rules: iter - .into_iter() - .map(|rule| (rule.identity.name.clone(), rule)) - .collect(), - } - } + pub rules: Vec, } /// Firewall rule priority. This is a value from 0 to 65535, with rules with @@ -2191,9 +2168,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 +2179,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 +2189,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 +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 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/db/model.rs b/nexus/src/db/model.rs index f3a8511bbc..3e0605b6ce 100644 --- 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 index ef45f82820..71773a3903 100644 --- 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 index d1a4455d2a..a36f638cbc 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,11 @@ 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)?)) + .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>>, 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(); @@ -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 index c6b04e7b93..ed94aae23f 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::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 { 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,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?; @@ -1762,14 +1758,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( diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index a4b0aea817..ef9d8245aa 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -9,14 +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::collections::HashMap; 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, @@ -40,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)); @@ -54,17 +52,14 @@ 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)); // 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 +74,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 +90,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { direction: VpcFirewallRuleDirection::Inbound, priority: VpcFirewallRulePriority(10), }, - ); + ]; let update_params = VpcFirewallRuleUpdateParams { rules: new_rules.clone() }; client @@ -111,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"); @@ -122,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 57797b2eab..c0bf5e2ee1 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" } } } @@ -5175,27 +5144,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": [ @@ -5294,6 +5242,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", @@ -5321,6 +5277,7 @@ "description", "direction", "filters", + "name", "priority", "status", "targets" @@ -5329,16 +5286,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", + "VpcFirewallRules": { + "description": "Collection of a [`Vpc`]'s firewall rules", "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",