Repository navigation
Add firewall rules for Services on tiers of a VPC with the Firewall service - #115
adestis-mf wants to merge 2 commits into
Conversation
…ervice Since CloudStack 4.23 a VPC offering can include the Firewall service. A tier lists the service only if its own network offering includes it, so on the other tiers the controller only added a Network ACL rule, and the VPC's virtual router kept dropping traffic to the Service's public IP for lack of a firewall rule. From CloudStack 4.23 on, the controller reads the VPC of such a tier with listVPCs, before it associates an IP. If the VPC has the Firewall service, it manages a firewall rule on the public IP in addition to the Network ACL rule, with spec.loadBalancerSourceRanges applied, and deletes it with the Service or an obsolete rule. A firewall rule that cannot be deleted, or a VPC that cannot be read, keeps the load balancer rule for the retry, like an ACL rule. An account that may not call listVPCs keeps the previous behaviour, and a warning is logged once per VPC. The README says how to allow it and what to do before upgrading. deleteFirewallRule now returns the first failed delete instead of the result of the last one, so a later success does not hide it. Fixes apache#114
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
What changed in this PR
Adds support for managing CloudStack firewall rules on VPC tiers where the VPC offering includes the Firewall service (CloudStack ≥ 4.23), ensuring Services become reachable by opening the port at the VPC virtual router.
Changes:
- Detects whether a tier’s VPC has the Firewall service (via
listVPCs/GetVPCByID) and manages a matching firewall rule alongside Network ACL rules. - Improves deletion behavior by returning the first firewall-rule delete error (so failures aren’t masked by later successes).
- Expands unit tests and updates README with required permissions, troubleshooting, and upgrade guidance.
| File | Description |
|---|---|
| cloudstack_loadbalancer.go | Adds VPC Firewall detection/caching, new deletion/pruning behavior, and firewall-rule reconciliation for VPC tiers with Firewall service. |
| cloudstack_loadbalancer_test.go | Adds unit tests covering VPC lookup behavior and new create/prune/delete paths and error cases. |
| cloudstack.go | Adds shared state (vpcReadRefused) to dedupe “missing listVPCs” warnings once per VPC. |
| README.md | Documents new listVPCs permission requirement, behavior changes for VPC tiers, troubleshooting, and upgrade notes. |
Review effort: Lite
| // vpcReadRefused holds the VPCs the account may not read, so that is logged once per VPC. | ||
| vpcReadRefused *sync.Map |
There was a problem hiding this comment.
The map only gets an entry when CloudStack refuses listVPCs for a VPC, that is when the account's role lacks the permission. The VPCs come from the networks of the cluster's nodes and of its load balancer rules, so they are the account's own VPCs, usually one. A TTL would bring back the warning periodically, which the map is there to avoid, so I'd keep it as it is.
| switch { | ||
| case err != nil && isNotAllowed(err): | ||
| if lb.firstVPCReadRefusal(network.Vpcid) { | ||
| klog.Warningf("Cannot read VPC %v to check for the Firewall service, so no firewall rules are managed there; allow the account to call listVPCs: %v", network.Vpcid, err) | ||
| } else { | ||
| klog.V(4).Infof("Cannot read VPC %v to check for the Firewall service for load balancer %v: %v", network.Vpcid, lb.name, err) |
There was a problem hiding this comment.
isNotAllowed is the existing helper on main, used for the stickiness policies and for taking over ACL rules; this PR adds one call site. A structural check isn't possible with cloudstack-go v2.19.1: CSError.Error() returns an error rather than a string, so CSError doesn't implement error, and the client only returns the formatted error. The code survives only in the message. The refusal of listVPCs came back in this form in the live test. If cloudstack-go exposes the error type later, all call sites can switch together.
| name: cs.GetLoadBalancerName(context.TODO(), "", service), | ||
| projectID: cs.projectID, | ||
| rules: make(map[string]*cloudstack.LoadBalancerRule), | ||
| vpcReadRefused: cs.vpcReadRefused, | ||
| version: cs.version, | ||
| } |
There was a problem hiding this comment.
Good point. vpcReadRefused is now a sync.Map value in CSCloud, whose zero value is ready to use, and each sync gets a pointer to it, so the warning is logged once per VPC however CSCloud is built (466eeab, with a test that builds it as a literal). Initializing it lazily in getLoadBalancer would be an unsynchronized write, as syncs can run concurrently.
The set of VPCs the account may not read was a *sync.Map that only newCSCloud set, so a CSCloud built otherwise logged the warning on every sync. Make it a sync.Map value, whose zero value is ready to use, and give each sync a pointer to it.
Fixes #114
Since CloudStack 4.23 (apache/cloudstack#12706) a VPC offering can include the Firewall service. A tier lists the service only if its own offering does, so on the other tiers the controller only added a Network ACL rule, and the VPC's virtual router kept the Service's port closed.
Changes
listVPCs, once per sync and before an IP is associated. If the VPC has the Firewall service, it manages a firewall rule on the public IP next to the ACL rule, withspec.loadBalancerSourceRangesapplied, and deletes it with the Service or an obsolete rule. If that fails, the load balancer rule is kept for the retry, as for ACL rules.listVPCs(error 432), the controller behaves as before and logs a warning once per VPC.deleteFirewallRulenow returns the first failed delete instead of the result of the last one.Isolated networks, VPCs without the Firewall service and CloudStack before 4.23 behave as before.
The project role that CKS creates allows neither
listVPCsnorlistNetworkACLLists; apache/cloudstack#14690 adds them.A few colleagues and I worked on this, since LoadBalancer Services in VPCs with the Firewall service matter for our cloud environment.
Testing
go test ./...,go vetandgolangci-lintpass.listVPCsthe controller logged the warning (error 432) and the port stayed closed. With it, the firewall rule was created and the Service answered from outside. Source ranges, a port change and deleting the Service worked as expected, and the cluster's own rules were left alone.