bhouse-nexthop commented on code in PR #14357:
URL: https://github.com/apache/cloudstack/pull/14357#discussion_r4377045008
##########
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 does this answer your question, or would you rather I switch
to always using the rule's stored ports? Either way works for me. This is the
only thing left open here: @Damans227 has approved, and the latest Trillian run
(tid-17255) passed 147/149. The two errors, in `test_primary_storage` and
`test_primary_storage_scope`, also showed up on #14360, which doesn't touch
storage. I'd like to get it into 4.22.2 if possible.
--
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]
