nagaboinaramgopal commented on code in PR #14051:
URL: https://github.com/apache/cloudstack/pull/14051#discussion_r3941655130
##########
server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java:
##########
@@ -1109,7 +1109,7 @@ private NetworkACLItem createACLRuleFromMap(Map<String,
Object> ruleMap, long ac
throw new InvalidParameterValueException("Protocol is required");
}
String action = (String) ruleMap.getOrDefault(ApiConstants.ACTION,
"deny");
- String trafficType = (String)
ruleMap.getOrDefault(ApiConstants.TRAFFIC_TYPE,
NetworkACLItem.TrafficType.Ingress);
+ String trafficType = (String)
ruleMap.getOrDefault(ApiConstants.TRAFFIC_TYPE,
NetworkACLItem.TrafficType.Ingress.toString());
Review Comment:
Thanks @DaanHoogland, good question. Yes, it is real. I set it up on a 4.23
environment and captured the before and after so it is easy to see directly.
Full capture (requests, async job JSON, stack trace) attached as
`14051-evidence.zip`.
Setup: `importNetworkACL` into a normal VPC ACL. The `rules` parameter
documents "id and protocol are must", so a rule without `traffictype` is a
valid call.
On the shipped build, same ACL:
- rule **with** traffictype imports fine, so the path works in general
- rule **without** traffictype fails and is dropped:
```
ERROR NetworkACLServiceImpl Failed to import rule at index 0:
class NetworkACLItem$TrafficType cannot be cast to class java.lang.String
java.lang.ClassCastException
at NetworkACLServiceImpl.createACLRuleFromMap(...)
at NetworkACLServiceImpl.importNetworkACLRules(...)
job result: 530 "Failed to import any ACL rules."
```
The user impact is what makes it worth fixing: in a mixed import, the rows
that leave traffictype out are silently discarded into the errors list while
the others import, so you end up with a partial ACL and no obvious reason why.
The `getOrDefault` default is the `TrafficType` enum, and `(String)` on it
throws before the value is ever used.
After the one-line change (default to `Ingress.toString()`), the exact same
no-traffictype call succeeds and the rule is created defaulting to Ingress. In
the capture I swapped only `NetworkACLServiceImpl.class` into the same build,
nothing else changed.
On the reading: I agree the enum form is nicer, but a `toString()` on the
enum alone would not fix it, since `(String) enumValue` throws before
`toString()` is reached. The minimal working options are `.toString()` on the
default (current) or `String.valueOf(...)`. Happy to lift `Ingress` into a
named default constant if that reads best.
--
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]
