bhouse-nexthop commented on code in PR #14357:
URL: https://github.com/apache/cloudstack/pull/14357#discussion_r4502940911
##########
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 thanks for writing out the cases. Here's how each one behaves
on this branch with the default `partialupgrade=true`. Take a stored rule with
`tcp` and ports 80-80, or `icmp` with type 8 code 0:
| Stored | Update | Result today |
|---|---|---|
| ports | protocol that can't carry ports (`icmp`, `all`, `47`), no ports
passed | rejected: the stored ports are checked against the new protocol |
| ports | new protocol that can carry ports (`tcp` to `udp`), no ports
passed | ports kept |
| ports | new protocol with new ports | new ports applied, checked against
the new protocol |
| icmp type/code | protocol other than icmp, no type/code passed | rejected
("Can specify icmpCode and icmpType for ICMP protocol only") |
| icmp type/code | protocol other than icmp, type/code passed | rejected,
same check |
| icmp type/code | `icmp` again (or `1`) with a different type/code | new
type/code applied |
With `partialupgrade=false`, every field is replaced from the call, so
stored ports and ICMP type/code are dropped unless they're passed again. That
already gives the "clean up, then check only what the API was given" behaviour
when it's wanted.
I'd rather not change the partial-update semantics in this PR. Silently
dropping stored ports on a protocol change would turn a call that's rejected
today into one that succeeds with a wider rule than the caller may expect:
`tcp` 22 changed to `all` would end up allowing everything. It would also
change the default update path right before the 4.22.2 freeze. Callers who want
a clean replace can use `partialupgrade=false`. If you think the partial path
should clear them too, I can open a separate issue for it so it gets its own
review.
--
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]
