diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/firewall/CreateEgressFirewallRuleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/firewall/CreateEgressFirewallRuleCmd.java index 3bb140011c4..dab7d10ddd3 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/firewall/CreateEgressFirewallRuleCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/firewall/CreateEgressFirewallRuleCmd.java @@ -20,6 +20,7 @@ package org.apache.cloudstack.api.command.user.firewall; import java.util.ArrayList; import java.util.List; +import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.apache.cloudstack.acl.RoleType; @@ -242,42 +243,17 @@ public class CreateEgressFirewallRuleCmd extends BaseAsyncCreateCmd implements F @Override public void create() { - if (getSourceCidrList() != null) { - String guestCidr = _networkService.getNetwork(getNetworkId()).getCidr(); + validateCidrs(); - for (String cidr : getSourceCidrList()) { - if (!NetUtils.isValidIp4Cidr(cidr) && !NetUtils.isValidIp6Cidr(cidr)) { - throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Source cidrs formatting error " + cidr); - } - if (cidr.equals(NetUtils.ALL_IP4_CIDRS)) { - continue; - } - if (!NetUtils.isNetworkAWithinNetworkB(cidr, guestCidr)) { - throw new ServerApiException(ApiErrorCode.PARAM_ERROR, cidr + " is not within the guest cidr " + guestCidr); - } - } - } - - //Destination CIDR formatting check. Since it's optional param, no need to set a default as in the case of source. - if(destCidrList != null){ - for(String cidr : destCidrList){ - if(!NetUtils.isValidIp4Cidr(cidr) && !NetUtils.isValidIp6Cidr(cidr)) { - throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Destination cidrs formatting error" + cidr); - } - } - } - - if (getProtocol().equalsIgnoreCase(NetUtils.ALL_PROTO)) { + if (NetUtils.ALL_PROTO.equalsIgnoreCase(getProtocol())) { if (getSourcePortStart() != null && getSourcePortEnd() != null) { - throw new InvalidParameterValueException("Do not pass ports to protocol ALL, protocol ALL do not require ports. Unable to create " + - "firewall rule for the network id=" + networkId); + throw new InvalidParameterValueException(String.format("Unable to create firewall rule for the network id=[%d] due to: ports must not be informed to protocol ALL, as it does not require them.", networkId)); } } if (getVpcId() != null) { - throw new InvalidParameterValueException("Unable to create firewall rule for the network id=" + networkId + - " as firewall egress rule can be created only for non vpc networks."); - } + throw new InvalidParameterValueException(String.format("Unable to create firewall rule for the network id=[%d] as firewall egress rule can be created only for non VPC networks.", networkId)); + } try { FirewallRule result = _firewallService.createEgressFirewallRule(this); @@ -286,12 +262,47 @@ public class CreateEgressFirewallRuleCmd extends BaseAsyncCreateCmd implements F setEntityUuid(result.getUuid()); } } catch (NetworkRuleConflictException ex) { - s_logger.info("Network rule conflict: " + ex.getMessage()); - s_logger.trace("Network Rule Conflict: ", ex); + String message = "Network rule conflict: "; + if (!s_logger.isTraceEnabled()) { + s_logger.info(message + ex.getMessage()); + } else { + s_logger.trace(message, ex); + } throw new ServerApiException(ApiErrorCode.NETWORK_RULE_CONFLICT_ERROR, ex.getMessage()); } } + protected void validateCidrs() { + if (getSourceCidrList() != null) { + String guestCidr = _networkService.getNetwork(getNetworkId()).getCidr(); + + for (String cidr : getSourceCidrList()) { + cidr = StringUtils.trim(cidr); + isValidIpCidr(cidr, "Source"); + if (cidr.equals(NetUtils.ALL_IP4_CIDRS)) { + continue; + } + if (!NetUtils.isNetworkAWithinNetworkB(cidr, guestCidr)) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR,String.format("[%s] is not within the guest CIDR [%s]. ", cidr, guestCidr)); + } + } + } + + //Destination CIDR formatting check. Since it's optional param, no need to set a default as in the case of source. + if(destCidrList != null){ + for(String cidr : destCidrList){ + cidr = StringUtils.trim(cidr); + isValidIpCidr(cidr, "Destination"); + } + } + } + + private void isValidIpCidr(String cidr, String sourceOrDestination) { + if (!NetUtils.isValidIp4Cidr(cidr) && !NetUtils.isValidIp6Cidr(cidr)) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR,String.format("%s CIDRs formatting error [%s].", sourceOrDestination, cidr)); + } + } + @Override public String getEventType() { return EventTypes.EVENT_FIREWALL_EGRESS_OPEN; diff --git a/api/src/test/java/org/apache/cloudstack/api/command/user/firewall/CreateEgressFirewallRuleCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/user/firewall/CreateEgressFirewallRuleCmdTest.java new file mode 100644 index 00000000000..6dcecc32a14 --- /dev/null +++ b/api/src/test/java/org/apache/cloudstack/api/command/user/firewall/CreateEgressFirewallRuleCmdTest.java @@ -0,0 +1,317 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.api.command.user.firewall; + +import com.cloud.network.Network; +import com.cloud.network.NetworkService; +import com.cloud.utils.net.NetUtils; +import org.apache.cloudstack.api.ServerApiException; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import java.util.ArrayList; + +@PrepareForTest(NetUtils.class) +@RunWith(PowerMockRunner.class) +public class CreateEgressFirewallRuleCmdTest { + + @Mock + NetworkService networkServiceMock; + + @Spy + @InjectMocks + CreateEgressFirewallRuleCmd cmdMock = new CreateEgressFirewallRuleCmd(); + + @Test + public void validateCidrsTestValidCidrs(){ + ArrayList listMock = new ArrayList<>(); + String cidrMock = "10.1.1.0/24"; + listMock.add(cidrMock); + cmdMock.setSourceCidrList(listMock); + cmdMock.setDestCidrList(listMock); + + long networkIdMock = 1234; + Mockito.doReturn(networkIdMock).when(cmdMock).getNetworkId(); + + Network networkMock = Mockito.mock(Network.class); + Mockito.doReturn(networkMock).when(networkServiceMock).getNetwork(networkIdMock); + + Mockito.doReturn(cidrMock).when(networkMock).getCidr(); + + PowerMockito.mockStatic(NetUtils.class); + PowerMockito.when(NetUtils.isValidIp4Cidr(cidrMock)).thenReturn(true); + PowerMockito.when(NetUtils.isValidIp6Cidr(cidrMock)).thenReturn(false); + PowerMockito.when(NetUtils.isNetworkAWithinNetworkB(cidrMock,cidrMock)).thenReturn(true); + + cmdMock.validateCidrs(); + + Mockito.verify(cmdMock, Mockito.atLeast(2)).getSourceCidrList(); + Mockito.verify(cmdMock).getNetworkId(); + Mockito.verify(networkServiceMock).getNetwork(networkIdMock); + Mockito.verify(networkMock).getCidr(); + + PowerMockito.verifyStatic(NetUtils.class, Mockito.times(2)); + NetUtils.isValidIp4Cidr(cidrMock); + NetUtils.isValidIp6Cidr(cidrMock); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isNetworkAWithinNetworkB(cidrMock,cidrMock); + } + + @Test + public void validateCidrsTestCidrsBeginningWithWhiteSpace(){ + ArrayList listMock = new ArrayList<>(); + String cidrMock = " 10.1.1.0/24"; + listMock.add(cidrMock); + cmdMock.setSourceCidrList(listMock); + cmdMock.setDestCidrList(listMock); + + long networkIdMock = 1234; + Mockito.doReturn(networkIdMock).when(cmdMock).getNetworkId(); + + Network networkMock = Mockito.mock(Network.class); + Mockito.doReturn(networkMock).when(networkServiceMock).getNetwork(networkIdMock); + + Mockito.doReturn("10.1.1.0/24").when(networkMock).getCidr(); + + PowerMockito.mockStatic(NetUtils.class); + PowerMockito.when(NetUtils.isValidIp4Cidr(Mockito.eq("10.1.1.0/24"))).thenReturn(true); + PowerMockito.when(NetUtils.isValidIp6Cidr(Mockito.eq("10.1.1.0/24"))).thenReturn(false); + PowerMockito.when(NetUtils.isNetworkAWithinNetworkB(Mockito.eq("10.1.1.0/24"), Mockito.eq("10.1.1.0/24"))).thenReturn(true); + + cmdMock.validateCidrs(); + + Mockito.verify(cmdMock, Mockito.atLeast(2)).getSourceCidrList(); + Mockito.verify(cmdMock).getNetworkId(); + Mockito.verify(networkServiceMock).getNetwork(networkIdMock); + Mockito.verify(networkMock).getCidr(); + + PowerMockito.verifyStatic(NetUtils.class, Mockito.times(2)); + NetUtils.isValidIp4Cidr(Mockito.eq("10.1.1.0/24")); + NetUtils.isValidIp6Cidr(Mockito.eq("10.1.1.0/24")); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isNetworkAWithinNetworkB(Mockito.eq("10.1.1.0/24"), Mockito.eq("10.1.1.0/24")); + } + + @Test (expected = ServerApiException.class) + public void validateCidrsTestInvalidSourceCidr(){ + ArrayList listMock = new ArrayList<>(); + String sourceCidrMock = "aaaa"; + listMock.add(sourceCidrMock); + cmdMock.setSourceCidrList(listMock); + cmdMock.setDestCidrList(listMock); + + long networkIdMock = 1234; + Mockito.doReturn(networkIdMock).when(cmdMock).getNetworkId(); + + Network networkMock = Mockito.mock(Network.class); + Mockito.doReturn(networkMock).when(networkServiceMock).getNetwork(networkIdMock); + + Mockito.doReturn("10.1.1.0/24").when(networkMock).getCidr(); + + PowerMockito.mockStatic(NetUtils.class); + PowerMockito.when(NetUtils.isValidIp4Cidr(sourceCidrMock)).thenReturn(false); + PowerMockito.when(NetUtils.isValidIp6Cidr(sourceCidrMock)).thenReturn(false); + + cmdMock.validateCidrs(); + + Mockito.verify(cmdMock, Mockito.atLeast(2)).getSourceCidrList(); + Mockito.verify(cmdMock).getNetworkId(); + Mockito.verify(networkServiceMock).getNetwork(networkIdMock); + Mockito.verify(networkMock).getCidr(); + + PowerMockito.verifyStatic(NetUtils.class, Mockito.times(1)); + NetUtils.isValidIp4Cidr(sourceCidrMock); + NetUtils.isValidIp6Cidr(sourceCidrMock); + } + + @Test (expected = ServerApiException.class) + public void validateCidrsTestInvalidDestinationCidr(){ + ArrayList listSourceMock = new ArrayList<>(); + String sourceCidrMock = "10.1.1.0/24"; + listSourceMock.add(sourceCidrMock); + cmdMock.setSourceCidrList(listSourceMock); + + ArrayList listDestMock = new ArrayList<>(); + String destCidrMock = "aaaa"; + listDestMock.add(destCidrMock); + cmdMock.setDestCidrList(listDestMock); + + long networkIdMock = 1234; + Mockito.doReturn(networkIdMock).when(cmdMock).getNetworkId(); + + Network networkMock = Mockito.mock(Network.class); + Mockito.doReturn(networkMock).when(networkServiceMock).getNetwork(networkIdMock); + + Mockito.doReturn(sourceCidrMock).when(networkMock).getCidr(); + + PowerMockito.mockStatic(NetUtils.class); + PowerMockito.when(NetUtils.isValidIp4Cidr(Mockito.eq(sourceCidrMock))).thenReturn(true); + PowerMockito.when(NetUtils.isValidIp6Cidr(Mockito.eq(sourceCidrMock))).thenReturn(false); + PowerMockito.when(NetUtils.isNetworkAWithinNetworkB(sourceCidrMock, sourceCidrMock)).thenReturn(true); + + PowerMockito.when(NetUtils.isValidIp4Cidr(Mockito.eq(destCidrMock))).thenReturn(false); + PowerMockito.when(NetUtils.isValidIp6Cidr(Mockito.eq(destCidrMock))).thenReturn(false); + + cmdMock.validateCidrs(); + + Mockito.verify(cmdMock, Mockito.atLeast(2)).getSourceCidrList(); + Mockito.verify(cmdMock).getNetworkId(); + Mockito.verify(networkServiceMock).getNetwork(networkIdMock); + Mockito.verify(networkMock).getCidr(); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isValidIp4Cidr(sourceCidrMock); + NetUtils.isValidIp6Cidr(sourceCidrMock); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isNetworkAWithinNetworkB(sourceCidrMock,sourceCidrMock); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isValidIp4Cidr(destCidrMock); + NetUtils.isValidIp6Cidr(destCidrMock); + } + + @Test + public void validateCidrsTestSourceCidrEqualsAllIp4(){ + ArrayList listSourceMock = new ArrayList<>(); + String sourceCidrMock = "0.0.0.0/0"; + listSourceMock.add(sourceCidrMock); + cmdMock.setSourceCidrList(listSourceMock); + + ArrayList listDestMock = new ArrayList<>(); + String destCidrMock = "10.1.1.0/24"; + listDestMock.add(destCidrMock); + cmdMock.setDestCidrList(listDestMock); + + long networkIdMock = 1234; + Mockito.doReturn(networkIdMock).when(cmdMock).getNetworkId(); + + Network networkMock = Mockito.mock(Network.class); + Mockito.doReturn(networkMock).when(networkServiceMock).getNetwork(networkIdMock); + + Mockito.doReturn(sourceCidrMock).when(networkMock).getCidr(); + + PowerMockito.mockStatic(NetUtils.class); + PowerMockito.when(NetUtils.isValidIp4Cidr(Mockito.eq(sourceCidrMock))).thenReturn(true); + PowerMockito.when(NetUtils.isValidIp6Cidr(Mockito.eq(sourceCidrMock))).thenReturn(false); + + PowerMockito.when(NetUtils.isValidIp4Cidr(Mockito.eq(destCidrMock))).thenReturn(true); + PowerMockito.when(NetUtils.isValidIp6Cidr(Mockito.eq(destCidrMock))).thenReturn(false); + + cmdMock.validateCidrs(); + + Mockito.verify(cmdMock, Mockito.atLeast(2)).getSourceCidrList(); + Mockito.verify(cmdMock).getNetworkId(); + Mockito.verify(networkServiceMock).getNetwork(networkIdMock); + Mockito.verify(networkMock).getCidr(); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isValidIp4Cidr(sourceCidrMock); + NetUtils.isValidIp6Cidr(sourceCidrMock); + + PowerMockito.verifyStatic(NetUtils.class, Mockito.never()); + NetUtils.isNetworkAWithinNetworkB(sourceCidrMock,sourceCidrMock); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isValidIp4Cidr(destCidrMock); + NetUtils.isValidIp6Cidr(destCidrMock); + } + + @Test(expected = ServerApiException.class) + public void validateCidrsTestSourceCidrNotWithinNetwork() { + ArrayList listMock = new ArrayList<>(); + String cidrMock = "10.1.1.0/24"; + listMock.add(cidrMock); + cmdMock.setSourceCidrList(listMock); + cmdMock.setDestCidrList(listMock); + + long networkIdMock = 1234; + Mockito.doReturn(networkIdMock).when(cmdMock).getNetworkId(); + + Network networkMock = Mockito.mock(Network.class); + Mockito.doReturn(networkMock).when(networkServiceMock).getNetwork(networkIdMock); + + Mockito.doReturn(cidrMock).when(networkMock).getCidr(); + + PowerMockito.mockStatic(NetUtils.class); + PowerMockito.when(NetUtils.isValidIp4Cidr(cidrMock)).thenReturn(true); + PowerMockito.when(NetUtils.isValidIp6Cidr(cidrMock)).thenReturn(false); + PowerMockito.when(NetUtils.isNetworkAWithinNetworkB(cidrMock, cidrMock)).thenReturn(false); + + cmdMock.validateCidrs(); + + Mockito.verify(cmdMock, Mockito.atLeast(2)).getSourceCidrList(); + Mockito.verify(cmdMock).getNetworkId(); + Mockito.verify(networkServiceMock).getNetwork(networkIdMock); + Mockito.verify(networkMock).getCidr(); + + PowerMockito.verifyStatic(NetUtils.class, Mockito.times(1)); + NetUtils.isValidIp4Cidr(cidrMock); + NetUtils.isValidIp6Cidr(cidrMock); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isNetworkAWithinNetworkB(cidrMock, cidrMock); + } + + @Test + public void validateCidrsTestNullDestinationCidr(){ + ArrayList listSourceMock = new ArrayList<>(); + String sourceCidrMock = "10.1.1.0/24"; + listSourceMock.add(sourceCidrMock); + cmdMock.setSourceCidrList(listSourceMock); + + long networkIdMock = 1234; + Mockito.doReturn(networkIdMock).when(cmdMock).getNetworkId(); + + Network networkMock = Mockito.mock(Network.class); + Mockito.doReturn(networkMock).when(networkServiceMock).getNetwork(networkIdMock); + + Mockito.doReturn(sourceCidrMock).when(networkMock).getCidr(); + + PowerMockito.mockStatic(NetUtils.class); + PowerMockito.when(NetUtils.isValidIp4Cidr(Mockito.eq(sourceCidrMock))).thenReturn(true); + PowerMockito.when(NetUtils.isValidIp6Cidr(Mockito.eq(sourceCidrMock))).thenReturn(false); + PowerMockito.when(NetUtils.isNetworkAWithinNetworkB(sourceCidrMock, sourceCidrMock)).thenReturn(true); + + cmdMock.validateCidrs(); + + Mockito.verify(cmdMock, Mockito.atLeast(2)).getSourceCidrList(); + Mockito.verify(cmdMock).getNetworkId(); + Mockito.verify(networkServiceMock).getNetwork(networkIdMock); + Mockito.verify(networkMock).getCidr(); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isValidIp4Cidr(sourceCidrMock); + NetUtils.isValidIp6Cidr(sourceCidrMock); + + PowerMockito.verifyStatic(NetUtils.class); + NetUtils.isNetworkAWithinNetworkB(sourceCidrMock,sourceCidrMock); + + PowerMockito.verifyStatic(NetUtils.class, Mockito.never()); + NetUtils.isValidIp4Cidr(null); + NetUtils.isValidIp6Cidr(null); + } +}