From 75f5b67c9974eb614d4253562e0cfbb9e79df281 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Jun 2024 14:45:46 -0500 Subject: [PATCH] 400 instead of 500 on duplicate firewall rule names (#5913) Closes #5725 This does the check in a helper function in the model file, which feels appropriate. I wasn't sure about raising external API validation errors from the model file, but we do that in a bunch of places: ```console $ rg 'external::Error' nexus/db-model nexus/db-model/src/allow_list.rs 17:use omicron_common::api::external::Error; nexus/db-model/src/role_assignment.rs 9:use omicron_common::api::external::Error; nexus/db-model/src/ipv6.rs 17:use omicron_common::api::external::Error; nexus/db-model/src/saga_types.rs 21:use omicron_common::api::external::Error; nexus/db-model/src/vpc_subnet.rs 76: ) -> Result<(), external::Error> { 81: .map_err(|e| external::Error::invalid_request(e.to_string())) ... there were a bunch more ``` --- nexus/db-model/src/vpc_firewall_rule.rs | 34 +++++++++- nexus/src/app/vpc.rs | 5 +- nexus/tests/integration_tests/vpc_firewall.rs | 67 +++++++++++++------ 3 files changed, 82 insertions(+), 24 deletions(-) diff --git a/nexus/db-model/src/vpc_firewall_rule.rs b/nexus/db-model/src/vpc_firewall_rule.rs index 2d19796524..120d3e1473 100644 --- a/nexus/db-model/src/vpc_firewall_rule.rs +++ b/nexus/db-model/src/vpc_firewall_rule.rs @@ -14,6 +14,7 @@ use nexus_types::identity::Resource; use omicron_common::api::external; use serde::Deserialize; use serde::Serialize; +use std::collections::HashSet; use std::io::Write; use uuid::Uuid; @@ -253,15 +254,42 @@ impl VpcFirewallRule { pub fn vec_from_params( vpc_id: Uuid, params: external::VpcFirewallRuleUpdateParams, - ) -> Vec { - params + ) -> Result, external::Error> { + ensure_no_duplicates(¶ms)?; + Ok(params .rules .iter() .map(|rule| VpcFirewallRule::new(Uuid::new_v4(), vpc_id, rule)) - .collect() + .collect()) } } +fn ensure_no_duplicates( + params: &external::VpcFirewallRuleUpdateParams, +) -> Result<(), external::Error> { + // we could do this by comparing set(names).len() to names.len(), but this + // way we can say what the duplicate names are, and that's nice! + let mut names = HashSet::new(); + let mut dupes = HashSet::new(); + for r in params.rules.iter() { + if !names.insert(r.name.clone()) { + // insert returns false if already present + dupes.insert(r.name.clone()); + } + } + + if dupes.is_empty() { + return Ok(()); + } + + let dupes_str = + dupes.iter().map(|d| format!("\"{d}\"")).collect::>().join(", "); + return Err(external::Error::invalid_value( + "rules", + format!("Rule names must be unique. Duplicates: [{}]", dupes_str), + )); +} + impl Into for VpcFirewallRule { fn into(self) -> external::VpcFirewallRule { external::VpcFirewallRule { diff --git a/nexus/src/app/vpc.rs b/nexus/src/app/vpc.rs index 0950e65b83..09d402a940 100644 --- a/nexus/src/app/vpc.rs +++ b/nexus/src/app/vpc.rs @@ -179,7 +179,8 @@ impl super::Nexus { let rules = db::model::VpcFirewallRule::vec_from_params( authz_vpc.id(), params.clone(), - ); + )?; + let rules = self .db_datastore .vpc_update_firewall_rules(opctx, &authz_vpc, rules) @@ -199,7 +200,7 @@ impl super::Nexus { let mut rules = db::model::VpcFirewallRule::vec_from_params( vpc_id, defaults::DEFAULT_FIREWALL_RULES.clone(), - ); + )?; for rule in rules.iter_mut() { for target in rule.targets.iter_mut() { match target.0 { diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index a62019288d..83379bef88 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -5,7 +5,9 @@ use http::method::Method; use http::StatusCode; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; -use nexus_test_utils::resource_helpers::{create_project, create_vpc}; +use nexus_test_utils::resource_helpers::{ + create_project, create_vpc, object_get, object_put_error, +}; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::views::Vpc; use omicron_common::api::external::{ @@ -42,7 +44,9 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { let default_vpc_firewall = format!("/v1/vpc-firewall-rules?vpc=default&{}", project_selector,); - let rules = get_rules(client, &default_vpc_firewall).await; + 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("default", &rules)); @@ -52,7 +56,8 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { let other_vpc_firewall = format!("/v1/vpc-firewall-rules?{}", other_vpc_selector); let vpc2 = create_vpc(&client, &project_name, &other_vpc).await; - let rules = get_rules(client, &other_vpc_firewall).await; + let rules = + object_get::(client, &other_vpc_firewall).await.rules; assert!(rules.iter().all(|r| r.vpc_id == vpc2.identity.id)); assert!(is_default_firewall_rules(other_vpc, &rules)); @@ -111,14 +116,17 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { assert_eq!(updated_rules[1].identity.name, "deny-all-incoming"); // Make sure the firewall is changed - let rules = get_rules(client, &default_vpc_firewall).await; + let rules = object_get::(client, &default_vpc_firewall) + .await + .rules; assert!(!is_default_firewall_rules("default", &rules)); assert_eq!(rules.len(), new_rules.len()); assert_eq!(rules[0].identity.name, "allow-icmp"); assert_eq!(rules[1].identity.name, "deny-all-incoming"); // Make sure the other firewall is unchanged - let rules = get_rules(client, &other_vpc_firewall).await; + let rules = + object_get::(client, &other_vpc_firewall).await.rules; assert!(is_default_firewall_rules(other_vpc, &rules)); // DELETE is unsupported @@ -162,20 +170,6 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { .unwrap(); } -async fn get_rules( - client: &dropshot::test_util::ClientTestContext, - url: &str, -) -> Vec { - NexusRequest::object_get(client, url) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body::() - .unwrap() - .rules -} - fn is_default_firewall_rules( vpc_name: &str, rules: &Vec, @@ -292,3 +286,38 @@ fn is_default_firewall_rules( } true } + +#[nexus_test] +async fn test_firewall_rules_same_name(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + + let project_name = "my-project"; + create_project(&client, &project_name).await; + + let rule = VpcFirewallRuleUpdate { + name: "dupe".parse().unwrap(), + description: "".to_string(), + status: VpcFirewallRuleStatus::Enabled, + direction: VpcFirewallRuleDirection::Inbound, + targets: vec![], + filters: VpcFirewallRuleFilter { + hosts: None, + protocols: None, + ports: None, + }, + action: VpcFirewallRuleAction::Allow, + priority: VpcFirewallRulePriority(65534), + }; + + let error = object_put_error( + client, + &format!("/v1/vpc-firewall-rules?vpc=default&project={}", project_name), + &VpcFirewallRuleUpdateParams { + rules: vec![rule.clone(), rule.clone()], + }, + StatusCode::BAD_REQUEST, + ) + .await; + assert_eq!(error.error_code, Some("InvalidValue".to_string())); + assert_eq!(error.message, "unsupported value for \"rules\": Rule names must be unique. Duplicates: [\"dupe\"]"); +}