bhouse-nexthop commented on PR #14357:
URL: https://github.com/apache/cloudstack/pull/14357#issuecomment-6727080322
### Test report: protocol-number and upper-case ICMP rules, and the six
partial-update cases
This adds to @Damans227's test (protocol 6 and 17, ICMP 8/0, egress, a
reboot). It covers the residual cases, plus @weizhouapache's six partial-update
scenarios run against a live management server and router.
**Environment**
- A CloudStack 24.0 (main) build with this PR applied, alongside some of my
other open PRs.
- Why the result carries over to 4.22:
- The PR's changes are on the management server (`NetworkACLServiceImpl`,
the create and update commands, `SetNetworkACLCommand`, `NetUtils`), and they
apply cleanly to main.
- The router side is system VM template 4.22.0, as on 4.22.
- The VPC offering has no Firewall service, as on 4.22.
- Three KVM hosts on Ubuntu 24.04, nested virtualisation; Ceph RBD primary
storage.
- System VM template 4.22.0 (Debian 12, nft 1.0.6).
- A dual-stack VPC: tier T1 holds the server V1, with the test list; tier T2
holds the client V2. There is also an external IPv6 host.
**1. Rendering on the router**
| Rule (API) | Stored | Router |
|---|---|---|
| `protocol=ICMP icmptype=3 icmpcode=1`, 0.0.0.0/0 | `ICMP` 3/1 | `-p icmp
-m icmp --icmp-type 3/1 -j ACCEPT` |
| `protocol=ICMP icmptype=3 icmpcode=1` from the external /64, then `update
protocol=1 icmptype=128 icmpcode=0` | `icmp` 128/0 | `ip6 saddr fd00:30::/64
icmpv6 type echo-request icmpv6 code no-route accept`. ping6 from the external
host to V1: 2/2 |
| `protocol=tcp` (no ports), then `update protocol=icmp` with no type or
code | `icmp`, type and code null | `-p icmp -m icmp --icmp-type any -j
ACCEPT`. ping from V2: 2/2 |
| `protocol=006 startport=22 endport=22` | `006` 22-22 | `-p tcp -m tcp
--dport 22 -j ACCEPT`. tcp 22 from V2 connects, tcp 8080 does not |
No ACL apply failed: the agent log on the router's host has no `ruleParts`
or `Problem occurred` line after these changes.
**2. The six partial-update cases, default `partialupgrade=true`**
Each case starts from a fresh rule from T2's CIDR with action allow: either
tcp 80-80 or icmp 8/0.
| # | Stored | Update | Result | Rule afterwards |
|---|---|---|---|---|
| 1 | tcp 80-80 | `protocol=icmp` | rejected: "Can't specify start/end port
when protocol is ICMP" | tcp 80-80 (unchanged) |
| 2 | tcp 80-80 | `protocol=udp` | accepted | udp 80-80 (ports kept) |
| 3 | tcp 80-80 | `protocol=udp startport=53 endport=53` | accepted | udp
53-53 |
| 4 | icmp 8/0 | `protocol=tcp` | rejected: "Can specify icmpCode and
icmpType for ICMP protocol only" | icmp 8/0 (unchanged) |
| 5 | icmp 8/0 | `protocol=tcp icmptype=8` | rejected, same message | icmp
8/0 (unchanged) |
| 6 | icmp 8/0 | `protocol=1 icmptype=0 icmpcode=0` | accepted | icmp 0/0 |
This matches the table in the review thread: rejected / ports kept / new
ports / rejected / rejected / new type.
**3. The same cases with `partialupgrade=false`**
When `cidrlist`, `action` and `traffictype` are passed as well, every field
is replaced from the call:
| # | Update | Rule afterwards |
|---|---|---|
| 1 | `protocol=icmp` | icmp, type/code -1 (all ICMP), no ports |
| 2 | `protocol=udp` | udp, no ports |
| 3 | `protocol=udp startport=53 endport=53` | udp 53-53 |
| 4 | `protocol=tcp` | tcp, no ports, no ICMP type |
| 5 | `protocol=tcp icmptype=8` | accepted; tcp, the `icmptype=8` silently
dropped |
| 6 | `protocol=1 icmptype=0 icmpcode=0` | icmp 0/0 |
**Not part of this PR, but worth knowing.** A `partialupgrade=false` call
that leaves out `cidrlist`, `action` or `traffictype` breaks the rule. The
update fails with:
```
Cannot invoke "java.util.List.size()" because "cidrs" is null
(NetworkACLItemCidrsDaoImpl.updateCidrs)
```
- By then, `NetworkACLItemDaoImpl.update()` has already written the row with
those fields set to null.
- `listNetworkACLs` for that list then fails with `Cannot invoke
"...TrafficType.toString()" because ... getTrafficType() is null`, until the
rule is updated again with every field.
That path (`transferDataToNetworkAclRulePojo()` and the DAO update) is the
same on main without this PR. This PR only adds its port check ahead of it, so
it is an existing bug, and it should get an issue of its own. It does mean that
`partialupgrade=false` is a "clean replace" only when the caller passes every
field. That corrects what I said in my reply in the review thread, where I
described it as a clean replace without that condition.
**Result:** pass, against the residual cases and the six-case table.
**Not covered**
- The UI: the start and end port fields in the ACL rule form.
- A router with an older template.
- Egress ICMP rules.
--
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]
