Skip to content

Commit

Permalink
Refactor giant if & add unit tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@winterhazel
winterhazel committed Oct 25, 2024
1 parent c7568f1 commit 8ad3a1b
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 19 deletions.
62 changes: 46 additions & 16 deletions server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
import org.apache.cloudstack.network.RoutedIpv4Manager;
import org.apache.commons.lang3.ObjectUtils;
import org.springframework.stereotype.Component;

import com.cloud.configuration.Config;
Expand Down Expand Up @@ -410,12 +411,13 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
_firewallDao.loadSourceCidrs(rule);
_firewallDao.loadSourceCidrs((FirewallRuleVO)newRule);

if (ObjectUtils.anyNull(rule.getSourceCidrList(), newRule.getSourceCidrList())) {
continue;

Check warning on line 415 in server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java#L415 

Added line #L415 was not covered by tests
}

_firewallDao.loadDestinationCidrs(rule);
_firewallDao.loadDestinationCidrs((FirewallRuleVO) newRule);

if (rule.getSourceCidrList() == null || newRule.getSourceCidrList() == null) {
continue;
}
duplicatedCidrs = (detectConflictingCidrs(rule.getSourceCidrList(), newRule.getSourceCidrList()) && detectConflictingCidrs(rule.getDestinationCidrList(), newRule.getDestinationCidrList()));
}

Expand All @@ -424,7 +426,7 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
_firewallDao.loadSourceCidrs(rule);
_firewallDao.loadSourceCidrs((FirewallRuleVO) newRule);

Check warning on line 427 in server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java#L426-L427 

Added lines #L426 - L427 were not covered by tests

if (rule.getSourceCidrList() == null || newRule.getSourceCidrList() == null) {
if (ObjectUtils.anyNull(rule.getSourceCidrList(), newRule.getSourceCidrList())) {
continue;
}

Expand Down Expand Up @@ -464,18 +466,7 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict

if (!notNullPorts) {
continue;
} else if (!oneOfRulesIsFirewall &&
!((bothRulesFirewall || bothRulesPortForwarding) && !duplicatedCidrs) &&
((rule.getSourcePortStart().intValue() <= newRule.getSourcePortStart().intValue() &&
rule.getSourcePortEnd().intValue() >= newRule.getSourcePortStart().intValue()) ||
(rule.getSourcePortStart().intValue() <= newRule.getSourcePortEnd().intValue() &&
rule.getSourcePortEnd().intValue() >= newRule.getSourcePortEnd().intValue()) ||
(newRule.getSourcePortStart().intValue() <= rule.getSourcePortStart().intValue() &&
newRule.getSourcePortEnd().intValue() >= rule.getSourcePortStart().intValue()) ||
(newRule.getSourcePortStart().intValue() <= rule.getSourcePortEnd().intValue() &&
newRule.getSourcePortEnd().intValue() >= rule.getSourcePortEnd().intValue()))) {
//Above else if conditions checks for the conflicting port ranges.

} else if (checkIfRulesHaveConflictingPortRanges(newRule, rule, oneOfRulesIsFirewall, bothRulesFirewall, bothRulesPortForwarding, duplicatedCidrs)) {
// we allow port forwarding rules with the same parameters but different protocols
boolean allowPf =
(rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
Expand All @@ -502,6 +493,45 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
}
}

protected boolean checkIfRulesHaveConflictingPortRanges(FirewallRule newRule, FirewallRule rule, boolean oneOfRulesIsFirewall, boolean bothRulesFirewall, boolean bothRulesPortForwarding, boolean duplicatedCidrs) {
String rulesAsString = String.format("[%s] and [%s]", rule, newRule);

if (oneOfRulesIsFirewall) {
logger.debug("Only one of the rules (%s) is firewall; therefore, their port ranges will not conflict.",
rulesAsString);
return false;
}

if ((bothRulesFirewall || bothRulesPortForwarding) && !duplicatedCidrs) {
logger.debug("Both rules (%s) are firewall/port forwarding, but they do not have duplicated CIDRs; therefore, their port ranges will not conflict.",
rulesAsString);
return false;
}

if (rule.getSourcePortStart() <= newRule.getSourcePortStart() && rule.getSourcePortEnd() >= newRule.getSourcePortStart()) {
logger.debug("Rules (%s) have conflicting port ranges.", rulesAsString);
return true;
}

if (rule.getSourcePortStart() <= newRule.getSourcePortEnd() && rule.getSourcePortEnd() >= newRule.getSourcePortEnd()) {
logger.debug("Rules (%s) have conflicting port ranges.", rulesAsString);
return true;
}

if (newRule.getSourcePortStart() <= rule.getSourcePortStart() && newRule.getSourcePortEnd() >= rule.getSourcePortStart()) {
logger.debug("Rules (%s) have conflicting port ranges.", rulesAsString);
return true;

Check warning on line 523 in server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java#L522-L523 

Added lines #L522 - L523 were not covered by tests
}

if (newRule.getSourcePortStart() <= rule.getSourcePortEnd() && newRule.getSourcePortEnd() >= rule.getSourcePortEnd()) {
logger.debug("Rules (%s) have conflicting port ranges.", rulesAsString);
return true;

Check warning on line 528 in server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java#L527-L528 

Added lines #L527 - L528 were not covered by tests
}

logger.debug("Rules (%s) do not have conflicting port ranges.", rulesAsString);
return false;
}

@Override
public void validateFirewallRule(Account caller, IPAddressVO ipAddress, Integer portStart, Integer portEnd, String proto, Purpose purpose, FirewallRuleType type,
Long networkId, FirewallRule.TrafficType trafficType) {
Expand Down
98 changes: 95 additions & 3 deletions server/src/test/java/com/cloud/network/firewall/FirewallManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,26 @@
import com.cloud.network.element.FirewallServiceProvider;
import com.cloud.network.element.VirtualRouterElement;
import com.cloud.network.element.VpcVirtualRouterElement;
import com.cloud.network.rules.FirewallManager;
import com.cloud.network.rules.FirewallRule;
import com.cloud.network.rules.FirewallRule.Purpose;
import com.cloud.network.rules.FirewallRuleVO;
import com.cloud.network.vpc.VpcManager;
import com.cloud.user.AccountManager;
import com.cloud.user.DomainManager;
import com.cloud.utils.component.ComponentContext;
import junit.framework.Assert;
import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.LogManager;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.mockito.Spy;
import org.mockito.junit.MockitoJUnitRunner;

import java.util.ArrayList;
Expand Down Expand Up @@ -111,19 +111,42 @@ public void testInjected() {
@Mock
FirewallRulesDao _firewallDao;

@Spy
@InjectMocks
FirewallManager _firewallMgr = new FirewallManagerImpl();
FirewallManagerImpl _firewallMgr;

FirewallRule fwRule50to150;
FirewallRule fwRule100to200;
FirewallRule fwRule151to200;

FirewallRule pfRule50to150;
FirewallRule pfRule100to200;
FirewallRule pfRule151to200;


@Before
public void initMocks() {
closeable = MockitoAnnotations.openMocks(this);

fwRule50to150 = createFirewallRule(50, 150, Purpose.Firewall);
fwRule100to200 = createFirewallRule(100, 150, Purpose.Firewall);
fwRule151to200 = createFirewallRule(151, 200, Purpose.Firewall);

pfRule50to150 = createFirewallRule(50, 150, Purpose.PortForwarding);
pfRule100to200 = createFirewallRule(100, 150, Purpose.PortForwarding);
pfRule151to200 = createFirewallRule(151, 200, Purpose.PortForwarding);
}

@After
public void tearDown() throws Exception {
closeable.close();
}

private FirewallRule createFirewallRule(int startPort, int endPort, Purpose purpose) {
return new FirewallRuleVO("xid", 1L, startPort, endPort, "TCP", 2, 3, 4, purpose, new ArrayList<>(),
new ArrayList<>(), 5, 6, null, FirewallRule.TrafficType.Ingress);
}

@Ignore("Requires database to be set up")
@Test
public void testApplyRules() {
Expand Down Expand Up @@ -218,6 +241,75 @@ public void testDetectRulesConflict() {
}
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestOnlyOneRuleIsFirewallReturnsFalse()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule50to150, pfRule50to150, true, false, false, true);

Assert.assertFalse(result);
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestBothRulesAreFirewallButNoDuplicateCidrsReturnsFalse()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule50to150, fwRule50to150, false, true, false, false);

Assert.assertFalse(result);
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingButNoDuplicateCidrsReturnsFalse()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule50to150, false, false, true, false);

Assert.assertFalse(result);
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestBothRulesAreFirewallAndDuplicatedCidrsAndNewRuleSourceStartPortIsInsideExistingRangeReturnsTrue()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule100to200, fwRule50to150, false, true, false, true);

Assert.assertTrue(result);
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestBothRulesAreFirewallAndDuplicatedCidrsAndNewRuleSourceEndPortIsInsideExistingRangeReturnsTrue()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule50to150, fwRule100to200, false, true, false, true);

Assert.assertTrue(result);
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingAndDuplicatedCidrsAndNewRuleSourceStartPortIsInsideExistingRangeReturnsTrue()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule100to200, false, false, true, true);

Assert.assertTrue(result);
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingAndDuplicatedCidrsAndNewRuleSourceEndPortIsInsideExistingRangeReturnsTrue()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule100to200, false, false, true, true);

Assert.assertTrue(result);
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestBothRulesAreFirewallAndDuplicatedCidrsAndNoRangeConflictReturnsFalse()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule50to150, fwRule151to200, false, true, false, true);

Assert.assertFalse(result);
}

@Test
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingAndDuplicatedCidrsAndNoRangeConflictReturnsFalse()
{
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule151to200, false, false, true, true);

Assert.assertFalse(result);
}
}

0 comments on commit 8ad3a1b

Please sign in to comment.