server: fix ClassCastException importing an ACL rule without a traffic type

[nagaboinaramgopal]

createACLRuleFromMap passed the enum NetworkACLItem.TrafficType.Ingress as the getOrDefault default for a String variable, so importing a rule that omits the optional traffictype threw ClassCastException and every such rule was silently rejected. Use the enum's string form so the intended Ingress default applies.

Tested: new unit test createACLRuleFromMapDefaultsTrafficTypeToIngress (fails before, passes after); NetworkACLServiceImplTest green.

[DaanHoogland]

I do not think this is needed, the object can be passed as is and the string cast will take care toString() is called on the result.

[DaanHoogland]

@nagaboinaramgopal , did you encounter an issue that made you implement this?

[nagaboinaramgopal]

Thanks @DaanHoogland. I dug into this one a bit and the catch is the default value. When traffictype is left out of the rule map, getOrDefault(TRAFFIC_TYPE, TrafficType.Ingress) returns the TrafficType enum constant, and casting an enum to String with (String) throws a ClassCastException rather than converting it, so the rule drops into the errors list. The two neighbouring defaults on the same lines are Strings ("deny" and "true"), which is why only the traffic type trips. The test createACLRuleFromMapDefaultsTrafficTypeToIngress covers the no-traffictype path. If you would rather, I can switch it to String.valueOf(...) or lift the default into a constant, whatever reads cleanest to you.

[DaanHoogland]

best reading ;) this would be an implementation of toString in the enum. But to be honest, if this is not solving a real live issue, … we are poor on test-resources so again, is this a real live issue?

[nagaboinaramgopal]

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.

[nagaboinaramgopal]

14051-evidence.zip