bhouse-nexthop commented on code in PR #14357:
URL: https://github.com/apache/cloudstack/pull/14357#discussion_r4360255850
##########
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 good question. It already works that way when the protocol is
given. This check runs after `transferDataToNetworkAclRulePojo()`, so on a
partial update `networkACLItemVo.getSourcePortStart()` already holds the
command's port if one was passed, and the stored port if not. That is "the
command's value, then the rule's value".
When the protocol is *not* given, I check only the ports in the command on
purpose. Using the rule's stored ports there would re-check ports the caller
didn't touch. A rule stored before this change with ports on, for example,
protocol `47` would then be rejected for any edit at all, even changing only
`reason` or `number`, until someone removes the ports. So in that case only
ports passed in this call are checked against the stored protocol. A protocol
change is always checked against the ports the rule ends up with
(`validatePortsAreUsableWithProtocolOnUpdateTestStoredPortsKeptWhenProtocolNotGiven`
/ `...TestProtocolChangedOntoStoredPorts` cover both).
If you would rather we reject any edit of such a rule until its ports are
fixed, I can switch it to always use the rule's values. It's a one-line change.
--
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]
