nagaboinaramgopal commented on code in PR #14046:
URL: https://github.com/apache/cloudstack/pull/14046#discussion_r4186732600
##########
server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java:
##########
@@ -946,7 +946,7 @@ protected void updateIcmpCodeAndType (boolean
isPartialUpgrade, UpdateNetworkACL
}
private void updateIcmpCodeAndTypeFullUpgrade (Integer icmpCode, Integer
icmpType, NetworkACLItemVO networkACLItemVo) {
- if
(networkACLItemVo.getProtocol().equalsIgnoreCase(NetUtils.ICMP_PROTO)) {
+ if
(NetUtils.ICMP_PROTO.equalsIgnoreCase(networkACLItemVo.getProtocol())) {
Review Comment:
Good question, thanks. You're right the builder itself isn't null-safe:
`SetNetworkACLCommand` does `"icmp".compareTo(aclTO.getProtocol())`, which
would NPE on a null protocol. But a null can't reach the router here. The
`protocol` column is `NOT NULL default 'TCP'` and the JPA field is
`updatable=false`, and `applyNetworkACL` re-reads the rules from the DB
(`listByACL`) before building the command, so the transient null this update
produces is never persisted or sent. The NPE this PR fixes is the reachable
one, upstream in `updateIcmpCodeAndTypeFullUpgrade` before persistence.
One caveat for completeness: an explicitly empty protocol string (a
separate, pre-existing create-path edge) wouldn't NPE but would build a
malformed rule. Happy to harden `SetNetworkACLCommand`/`NetworkACLTO` to treat
a blank protocol as `all` in a follow-up if you think it's worth it.
--
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]
