bhouse-nexthop commented on code in PR #14357:
URL: https://github.com/apache/cloudstack/pull/14357#discussion_r4348818609
##########
api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java:
##########
@@ -99,11 +101,15 @@ public Long getId() {
}
public String getProtocol() {
- if (protocol != null) {
- return protocol.trim();
- } else {
+ if (protocol == null) {
return null;
}
+ String p = protocol.trim();
+ // As on create, protocol number 1 is ICMP so that it keeps its icmp
type and code
Review Comment:
Agreed, it only restated the code. The comment is gone; the method is now a
one-line call into the `NetUtils` helper weizhou asked for (see the reply
below), so nothing is left to explain.
##########
api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java:
##########
@@ -99,11 +101,15 @@ public Long getId() {
}
public String getProtocol() {
- if (protocol != null) {
- return protocol.trim();
- } else {
+ if (protocol == null) {
return null;
}
+ String p = protocol.trim();
+ // As on create, protocol number 1 is ICMP so that it keeps its icmp
type and code
+ if (StringUtils.isNumeric(p) && Integer.parseInt(p) ==
NetUtils.ICMP_PROTO_NUMBER) {
+ p = NetUtils.ICMP_PROTO;
+ }
+ return p;
Review Comment:
Done. `NetUtils.icmpProtocolNumberAsName(String)` now trims the protocol and
turns protocol number `1` into `icmp`. `CreateNetworkACLCmd.getProtocol()` and
`UpdateNetworkACLItemCmd.getProtocol()` both call it, so create and update
can't drift apart. It also uses `NumberUtils.toInt`, so a numeric protocol too
large for an `int` is passed through as before rather than throwing
`NumberFormatException` from the getter. Tests are in `NetUtilsTest`.
##########
server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java:
##########
@@ -116,6 +117,8 @@ public class NetworkACLServiceImpl extends ManagerBase
implements NetworkACLServ
private VpcManager vpcManager;
private String supportedProtocolsForAclRules = "tcp,udp,icmp,all";
+ private static final int TCP_PROTO_NUMBER = 6;
+ private static final int UDP_PROTO_NUMBER = 17;
Review Comment:
Moved: `TCP_PROTO_NUMBER` and `UDP_PROTO_NUMBER` are now in `NetUtils` next
to `ICMP_PROTO_NUMBER`, and the private copies in `NetworkACLServiceImpl` and
`SetNetworkACLCommand` are gone.
--
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]
