Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nagaboinaramgopal wants to merge 1 commit into
base: main
Choose a base branch
from
+17 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());

@DaanHoogland DaanHoogland Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 DaanHoogland Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nagaboinaramgopal nagaboinaramgopal Sep 4, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 DaanHoogland Sep 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nagaboinaramgopal Sep 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

String forDisplay = (String) ruleMap.getOrDefault(ApiConstants.FOR_DISPLAY, "true");

// Create ACL rule using the service
Expand Down
16 changes: 16 additions & 0 deletions server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@
import com.cloud.utils.net.NetUtils;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.api.ServerApiException;
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd;
import org.mockito.ArgumentCaptor;
import org.springframework.test.util.ReflectionTestUtils;
import org.apache.cloudstack.api.command.user.network.MoveNetworkAclItemCmd;
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLItemCmd;
import org.apache.cloudstack.api.command.user.network.UpdateNetworkACLListCmd;
Expand Down Expand Up @@ -1506,4 +1509,17 @@ public void validateAclAssociatedToVpcTestNullVpcShouldThrowInvalidParameterValu

networkAclServiceImpl.validateAclAssociatedToVpc(networkMockVpcMockId, accountMock, SOME_UUID);
}

@Test
public void createACLRuleFromMapDefaultsTrafficTypeToIngress() {
Map<String, Object> ruleMap = new HashMap<>();
ruleMap.put(ApiConstants.PROTOCOL, "tcp");
Mockito.doReturn(Mockito.mock(NetworkACLItem.class)).when(networkAclServiceImpl).createNetworkACLItem(Mockito.any());

ReflectionTestUtils.invokeMethod(networkAclServiceImpl, "createACLRuleFromMap", ruleMap, 1L);

ArgumentCaptor<CreateNetworkACLCmd> captor = ArgumentCaptor.forClass(CreateNetworkACLCmd.class);
Mockito.verify(networkAclServiceImpl).createNetworkACLItem(captor.capture());
Assert.assertEquals(NetworkACLItem.TrafficType.Ingress, captor.getValue().getTrafficType());
}
}