bhouse-nexthop commented on PR #14643:
URL: https://github.com/apache/cloudstack/pull/14643#issuecomment-6752445514
### Test report: full updates of a network ACL rule
**Summary:** an incomplete `updateNetworkACLItem partialupgrade=false` is
refused before anything is written, and the rule and `listNetworkACLs` stay
intact. `protocol=tcp icmptype=8` is refused. Complete full updates still work,
including changing an ICMP rule to tcp, which clears the ICMP fields.
**Environment**
- A CloudStack 24.0 (main) build with this PR, alongside some of my other
open PRs. One management server.
- The PR targets 4.22; it wasn't run on a 4.22 build.
`NetworkACLServiceImpl` and `NetworkACLItemDaoImpl` have the same update path
on both branches.
- A new ACL list in a VPC, with two rules:
- rule 10: tcp 22, `10.0.0.0/8`, Ingress, Allow;
- rule 20: icmp type 8 code 0, `10.0.0.0/8`, Ingress, Allow.
**Results**
| # | Call (`partialupgrade=false` unless noted) | Result |
|---|---|---|
| 1 | rule 10: `protocol=tcp startport=80 endport=80` (no `cidrlist`, no
`traffictype`) | refused, 431 |
| 2 | rule 10: `cidrlist=... traffictype=Ingress startport=80 endport=80`
(no `protocol`) | refused, 431 |
| 3 | rule 10: `protocol=tcp cidrlist=... startport=80 endport=80` (no
`traffictype`) | refused, 431 |
| 4 | rule 10: complete, with `protocol=tcp icmptype=8` | refused, 431 |
| 5 | rule 10, partial update: `traffictype=egres` | refused, 431 |
| 6 | rule 10: `protocol=udp 53-53 cidrlist=10.1.0.0/16,192.168.0.0/24
traffictype=Egress action=Deny` | OK |
| 7 | rule 20 (icmp): `protocol=tcp 443-443 cidrlist=10.0.0.0/8
traffictype=Ingress`, no ICMP fields | OK |
**Refusal messages**
```
1-3: The protocol, cidrlist and traffictype must be given when the ACL rule
is updated with partialupgrade=false
4: Can specify icmpCode and icmpType for ICMP protocol only
5: Invalid traffic type egres
```
**Nothing is written by a refused call.** The rule's row and CIDRs
(`network_acl_item`, `network_acl_item_cidrs`), before call 1 and after calls
1-4:
```
number protocol start end icmp_type icmp_code traffic_type action cidrs
10 tcp 22 22 NULL NULL Ingress Allow 10.0.0.0/8
```
`listNetworkACLs aclid=<list>` returned both rules, and `listNetworkACLs
listall=true` succeeded (6 rules), after the refusals. Before this PR, call 1
wrote the rule with a null traffic type and CIDR list, failed with an NPE, and
left `listNetworkACLs` failing.
**Complete full updates**
- After call 6: `10 udp 53 53 NULL NULL Egress Deny
10.1.0.0/16,192.168.0.0/24`. Every field was replaced, and both CIDRs were
stored.
- After call 7: `20 tcp 443 443 NULL NULL Ingress Allow 10.0.0.0/8`. The
ICMP type and code were cleared, since none was given.
- The management server log for the whole run has no `NullPointerException`.
**Result:** pass.
**Not covered**
- The ACL list wasn't attached to a tier, so no rule was pushed to a router.
The refusals happen before anything is applied, and a complete full update goes
through the same apply path as before.
- A failure while writing the CIDRs, which the PR now rolls back in one
transaction: I had no way to make that happen live. It is covered by the PR's
reasoning.
- The UI's edit dialog.
--
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]
