bhouse-nexthop commented on PR #14359:
URL: https://github.com/apache/cloudstack/pull/14359#issuecomment-6730972923
### Test report: protocol validation on every rule API, on a running zone
**Environment**
- A CloudStack 24.0 (main) build with this PR applied, alongside some of my
other open PRs. The code on the paths below is the same as on 4.22.
- Three KVM hosts on Ubuntu 24.04, nested virtualisation; Ceph RBD primary
storage; system VM template 4.22.0.
- Networks: a VPC with an ACL'd tier, an isolated network on
`DefaultIsolatedNetworkOfferingWithSourceNatService`, a dual-stack isolated
network, and a routed isolated network.
Each command was tried with `protocol=cp`, `tcp,udp` and `dp,ic`, then with
`TCP`, `Udp`, `ICMP` and `All`.
| Command | `cp` / `tcp,udp` / `dp,ic` | Valid names |
|---|---|---|
| `createNetworkACL` (VPC) | refused: `Invalid protocol [cp]. Expected one
of: [tcp,udp,icmp,all]` (likewise for the other two) | `TCP`, `Udp`, `ICMP`,
`All` and the lower-case forms accepted |
| `updateNetworkACLItem` | refused with the same message (job result 431).
The stored rule is unchanged (`tcp 22-22`) | `TCP`, `Udp`, `ICMP`, `All`
accepted on a rule without ports |
| `createFirewallRule` (isolated network public IP) | refused: `Protocol cp
is not supported in zone 1` | `TCP`, `Udp`, `ICMP` accepted |
| `createEgressFirewallRule` | refused | `TCP`, `Udp`, `ICMP`, `All`
accepted. `All` is matched against `"tcp,udp,icmp, all"`, the egress list with
a space, which the helper trims |
| `createPortForwardingRule` | refused | `TCP`, `Udp` accepted |
| `createIpv6FirewallRule` (dual-stack isolated network) | refused:
`Protocol cp is not supported in zone` | `TCP`, `Udp`, `ICMP` accepted; egress
`All` accepted |
| `createRoutingFirewallRule` (routed isolated network) | refused | `TCP`,
`Udp`, `ICMP` accepted; egress `All` accepted |
Valid names that are refused, the same as on stock:
- `All` on an **ingress** firewall, IPv6 firewall or routing firewall rule.
The provider's ingress list is `tcp,udp,icmp`, and stock refuses `all` too.
- `ICMP` and `All` on port forwarding. The port forwarding list is `tcp,udp`.
- `updateNetworkACLItem protocol=ICMP` or `protocol=All` on a rule that
still has ports, refused for the ports ("Can't specify start/end port when
protocol is ICMP"). This is #14357's partial-update behaviour, not this PR's.
**Without the PR** (from the code; there is no toggle): stock checks
`supportedProtocols.contains(protocol.toLowerCase())`, so it accepts `cp`,
`tcp,udp` and `dp,ic`. The router then fails to load such a rule (`iptables ...
-p cp -m cp` is rejected) while the API shows it `Active`.
**Result:** pass. Every invalid protocol is refused, and every valid name is
accepted in any case wherever the provider supports it.
**Main has a fifth call site**
- Main has a fifth substring check, which 4.22 lacks:
`FirewallManagerImpl.validateFirewallRuleForVpc()`, from #12706 (firewall rules
on VPC public IPs). It came after this PR branched.
- In the lab, with a VPC offering that has the Firewall service, this build
accepted `cp`, `tcp,udp` and `dp,ic` as firewall rules on the VPC public IP.
They were stored `Active`, but the router never loaded them.
- I tested a one-line follow-up for main, which uses this PR's helper
(`NetUtils.isProtocolInList`), in the lab build. All three are now refused with
`Protocol cp is not supported in VPC 5` (likewise for the other two). `TCP`,
`Udp` and `ICMP` are accepted and loaded on the router: `iptables-save -t
mangle` shows the three `FIREWALL_<public ip>` rules and nothing else. I'll
open it against main once this PR is merged forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
