Firewall rules not updated for renamed VPC

[sudomateo]

Perhaps this should be in https://github.com/oxidecomputer/omicron but I experienced it in the web console so I'll file it here.

1. Create a VPC named k8s.
2. Observe the default allow-icmp firewall rule targets the k8s VPC.
3. Rename the k8s VPC to ms-k8s.
4. Observe the default allow-icmp firewall rule still targets the old k8s VPC.
5. Attempt to update the allow-icmp firewall rule.
   1. Delete the k8s VPC target.
   2. Add the ms-k8s VPC target.
   3. Click "Update Rule" and note the Cross-VPC firewall host filter unsupported error.
6. Refresh the VPC firewall rules list and note the allow-icmp rule was indeed updated with the ms-k8s VPC target.

Here's what the JavaScript console showed when I clicked "Update Rule".

API URL:        https://oxide.sys.r3.oxide-preview.com/v1/vpc-firewall-rules?project=matthewsanabria&vpc=ms-k8s
Request ID:     21647ed3-cef9-4b39-9068-c74fbe1d6925
Error code:     InvalidRequest
Error message:  Cross-VPC firewall host filter unsupported

I'd do more digging on this but I'm in the middle of a few things. I wanted to file the issue though so I didn't forget about it.

[david-crespo]

Almost certainly an Omicron thing unless something in the console explains how it could modify the thing despite the 400. But I’m pretty sure we just make a single PUT with the list of rules.

[sudomateo]

I believe so too. I'll throw an LLM agent at it to see if it finds something. I'll transfer it to omicron afterwards.

[david-crespo]

Got it already:

• DB write before validation — vpc_update_firewall_rules persists the new rules to the DB, then calls the sled push (which performs cross-VPC validation) only after the transaction succeeds.
  

  omicron/nexus/src/app/vpc.rs

  Lines 182 to 202 in 4bb7276

  	pub(crate) async fn vpc_update_firewall_rules(
  	&self,
  	opctx: &OpContext,
  	vpc_lookup: &lookup::Vpc<'_>,
  	params: &VpcFirewallRuleUpdateParams,
  	) -> UpdateResult<Vec<db::model::VpcFirewallRule>> {
  	let (.., authz_vpc, db_vpc) =
  	vpc_lookup.fetch_for(authz::Action::Modify).await?;
  	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)
  	.await?;
  	self.send_sled_agents_firewall_rules(opctx, &db_vpc, &rules, &[])
  	.await?;
  	Ok(rules)
  	}

• Cross-VPC target check (too late) — inside resolve_firewall_rules_for_sled_agent, the rule target is compared against the current VPC name. If the VPC was renamed and old rules still reference the old name, this returns InvalidRequest.
  

  omicron/nexus/networking/src/firewall_rules.rs

  Lines 114 to 123 in 4bb7276

  	external::VpcFirewallRuleTarget::Vpc(name) => {
  	if name != vpc.name() {
  	return Err(FirewallRulesError::Lookup(
  	Error::invalid_request(
  	"cross-VPC firewall target unsupported",
  	),
  	));
  	}
  	vpcs.insert(name.clone().into());
  	}

• Cross-VPC host filter check (too late) — same problem for host filters.
  

  omicron/nexus/networking/src/firewall_rules.rs

  Lines 139 to 148 in 4bb7276

  	external::VpcFirewallRuleHostFilter::Vpc(name) => {
  	if name != vpc.name() {
  	return Err(FirewallRulesError::Lookup(
  	Error::invalid_request(
  	"cross-VPC firewall host filter unsupported",
  	),
  	));
  	}
  	vpcs.insert(name.clone().into());
  	}

[sudomateo]

Ha, I also got the same thing. Makes sense why my last rule modification worked--there were no more rules referencing the old VPC name.

Probably worth having calls to vpc_update walk the rules and rewrite the old VPC name with the new VPC name.

[david-crespo]

Yeah, I think that is the real fix. In the meantime I am drafting a PR to at least error sooner so you can't have it both error and DB write.