bhouse-nexthop opened a new issue, #14642: URL: https://github.com/apache/cloudstack/issues/14642
### problem `updateNetworkACLItem` with `partialupgrade=false` replaces every field of the rule with what the call passes. Two cases go wrong. **1. A field left out is written as null, and the rule is left broken.** When `cidrlist` or `traffictype` is left out, `transferDataToNetworkAclRulePojo()` sets them to null. `NetworkACLItemDaoImpl.update()` then writes the rule row and only afterwards updates the CIDRs, outside a transaction: - with `cidrlist` left out, `NetworkACLItemCidrsDaoImpl.updateCidrs()` fails with `Cannot invoke "java.util.List.size()" because "cidrs" is null`. The call fails, but the row has already been committed with the null traffic type. - from then on `listNetworkACLs` for that list fails with `Cannot invoke "...TrafficType.toString()" because ... getTrafficType() is null`, and applying the list fails the same way in `SetNetworkACLCommand`, until the rule is updated again with every field. When `protocol` is left out, the call fails earlier with a NullPointerException in `updateIcmpCodeAndTypeFullUpgrade()`. Nothing is written in that case, but the error is an NPE rather than a message. (#14046 removes that NPE; the rule would then be saved without a protocol, which `createNetworkACL` never allows.) `action` left out is already handled: it becomes Allow, as on create. **2. An ICMP type or code given with another protocol is silently dropped.** `partialupgrade=false protocol=tcp icmptype=8` is accepted, and the rule is saved as tcp without the ICMP type. `createNetworkACL` with the same parameters, and the same update with the default `partialupgrade=true`, both refuse it with "Can specify icmpCode and icmpType for ICMP protocol only". ### versions 4.20 and later for the dropped ICMP type; the null fields since `partialupgrade` was added in 4.12. Seen on 4.22 and main. ### The steps to reproduce the bug 1. In a VPC, create an ACL list and a rule on it: `createNetworkACL aclid=<list> protocol=tcp startport=80 endport=80 cidrlist=10.1.2.0/24 action=allow traffictype=ingress`. 2. `updateNetworkACLItem id=<rule> partialupgrade=false protocol=udp startport=53 endport=53`. It fails with the `cidrs is null` NPE. 3. `listNetworkACLs aclid=<list>` now fails with the `getTrafficType() is null` NPE. 4. `updateNetworkACLItem id=<rule> partialupgrade=false protocol=tcp cidrlist=10.1.2.0/24 action=allow traffictype=ingress` repairs it. 5. On an ICMP rule, `updateNetworkACLItem id=<rule> partialupgrade=false protocol=tcp icmptype=8 cidrlist=10.1.2.0/24 action=allow traffictype=ingress` is accepted, and the rule is tcp with no ICMP type. ### What to do about it? - With `partialupgrade=false`, refuse the call when `protocol`, `cidrlist` or `traffictype` is missing, before anything is written. Defaulting them as create does would silently widen a rule to every address, or turn an egress rule into an ingress rule. - Refuse an invalid `traffictype` on update, as create does, instead of ignoring it. - Keep an ICMP type or code given with another protocol, so that the existing validation refuses it. - Write the rule row and its CIDRs in one transaction, so that a failure on the CIDRs can't leave the rule half updated. -- 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]
