bhouse-nexthop commented on code in PR #14357:
URL: https://github.com/apache/cloudstack/pull/14357#discussion_r4517132058
##########
server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java:
##########
@@ -690,6 +692,33 @@ protected void
validateSourceStartAndEndPorts(NetworkACLItemVO networkACLItemVO)
}
}
+ /**
+ * Ports are only applied to TCP and UDP, so they cannot be given with any
other protocol number:
+ * the rule would be applied to the whole protocol, ignoring them.
+ */
+ protected void validatePortsAreUsableWithProtocol(String protocol, Integer
sourcePortStart, Integer sourcePortEnd) {
+ if ((sourcePortStart == null && sourcePortEnd == null) ||
!StringUtils.isNumeric(protocol)) {
+ return;
+ }
+ int protoNumber = NumberUtils.toInt(protocol, -1);
+ if (protoNumber != NetUtils.TCP_PROTO_NUMBER && protoNumber !=
NetUtils.UDP_PROTO_NUMBER) {
+ throw new InvalidParameterValueException(String.format("Start and
end port can only be given for TCP or UDP (protocol number %d or %d), not for
protocol number [%s]",
+ NetUtils.TCP_PROTO_NUMBER, NetUtils.UDP_PROTO_NUMBER,
protocol));
+ }
+ }
+
+ /**
+ * On update the ports a rule already carries are only checked when the
protocol is given too, so
+ * that a rule stored with ports on another protocol number can still be
edited otherwise, while a
+ * protocol change cannot leave them on it.
+ */
+ protected void
validatePortsAreUsableWithProtocolOnUpdate(UpdateNetworkACLItemCmd
updateNetworkACLItemCmd, NetworkACLItemVO networkACLItemVo) {
+ boolean protocolGiven =
StringUtils.isNotBlank(updateNetworkACLItemCmd.getProtocol());
+ validatePortsAreUsableWithProtocol(networkACLItemVo.getProtocol(),
+ protocolGiven ? networkACLItemVo.getSourcePortStart() :
updateNetworkACLItemCmd.getSourcePortStart(),
Review Comment:
@weizhouapache agreed, that reads better. Done in
https://github.com/apache/cloudstack/pull/14357/commits/9fc97a4cfa3ebf8e4d14686e5fcd3631a5586dfb:
the ports passed in the command are checked first, and the rule's stored ports
are used only when none was passed and the protocol is given.
It doesn't change behaviour. `transferDataToNetworkAclRulePojo()` runs
before the check and already copies any port passed onto the rule, so both
orders gave the same values. The new order doesn't rely on that call order,
though. There's no ICMP type/code fallback of this kind in the PR, so only the
ports changed. I also added a test for ports passed together with the protocol.
--
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]
