Skip to content

Commit ea0e9d8

Browse files
winterhazelwinterhazel
committed
Refactor giant if & add unit tests
1 parent becdebe commit ea0e9d8

File tree

2 files changed

+142
-20
lines changed

2 files changed

+142
-20
lines changed

‎server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java‎

Lines changed: 46 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.apache.cloudstack.context.CallContext;
3434
import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
3535
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
36+
import org.apache.commons.lang3.ObjectUtils;
3637
import org.apache.log4j.Logger;
3738
import org.springframework.stereotype.Component;
3839

@@ -399,12 +400,13 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
399400
_firewallDao.loadSourceCidrs(rule);
400401
_firewallDao.loadSourceCidrs((FirewallRuleVO)newRule);
401402

403+
if (ObjectUtils.anyNull(rule.getSourceCidrList(), newRule.getSourceCidrList())) {
404+
continue;
405+
}
406+
402407
_firewallDao.loadDestinationCidrs(rule);
403408
_firewallDao.loadDestinationCidrs((FirewallRuleVO) newRule);
404409

405-
if (rule.getSourceCidrList() == null || newRule.getSourceCidrList() == null) {
406-
continue;
407-
}
408410
duplicatedCidrs = (detectConflictingCidrs(rule.getSourceCidrList(), newRule.getSourceCidrList()) && detectConflictingCidrs(rule.getDestinationCidrList(), newRule.getDestinationCidrList()));
409411
}
410412

@@ -413,7 +415,7 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
413415
_firewallDao.loadSourceCidrs(rule);
414416
_firewallDao.loadSourceCidrs((FirewallRuleVO) newRule);
415417

416-
if (rule.getSourceCidrList() == null || newRule.getSourceCidrList() == null) {
418+
if (ObjectUtils.anyNull(rule.getSourceCidrList(), newRule.getSourceCidrList())) {
417419
continue;
418420
}
419421

@@ -453,18 +455,7 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
453455

454456
if (!notNullPorts) {
455457
continue;
456-
} else if (!oneOfRulesIsFirewall &&
457-
!((bothRulesFirewall || bothRulesPortForwarding) && !duplicatedCidrs) &&
458-
((rule.getSourcePortStart().intValue() <= newRule.getSourcePortStart().intValue() &&
459-
rule.getSourcePortEnd().intValue() >= newRule.getSourcePortStart().intValue()) ||
460-
(rule.getSourcePortStart().intValue() <= newRule.getSourcePortEnd().intValue() &&
461-
rule.getSourcePortEnd().intValue() >= newRule.getSourcePortEnd().intValue()) ||
462-
(newRule.getSourcePortStart().intValue() <= rule.getSourcePortStart().intValue() &&
463-
newRule.getSourcePortEnd().intValue() >= rule.getSourcePortStart().intValue()) ||
464-
(newRule.getSourcePortStart().intValue() <= rule.getSourcePortEnd().intValue() &&
465-
newRule.getSourcePortEnd().intValue() >= rule.getSourcePortEnd().intValue()))) {
466-
//Above else if conditions checks for the conflicting port ranges.
467-
458+
} else if (checkIfRulesHaveConflictingPortRanges(newRule, rule, oneOfRulesIsFirewall, bothRulesFirewall, bothRulesPortForwarding, duplicatedCidrs)) {
468459
// we allow port forwarding rules with the same parameters but different protocols
469460
boolean allowPf =
470461
(rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
@@ -491,6 +482,45 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
491482
}
492483
}
493484

485+
protected boolean checkIfRulesHaveConflictingPortRanges(FirewallRule newRule, FirewallRule rule, boolean oneOfRulesIsFirewall, boolean bothRulesFirewall, boolean bothRulesPortForwarding, boolean duplicatedCidrs) {
486+
String rulesAsString = String.format("[%s] and [%s]", rule, newRule);
487+
488+
if (oneOfRulesIsFirewall) {
489+
s_logger.debug(String.format("Only one of the rules (%s) is firewall; therefore, their port ranges will not conflict.",
490+
rulesAsString));
491+
return false;
492+
}
493+
494+
if ((bothRulesFirewall || bothRulesPortForwarding) && !duplicatedCidrs) {
495+
s_logger.debug(String.format("Both rules (%s) are firewall/port forwarding, but they do not have duplicated CIDRs; therefore, their port ranges will not conflict.",
496+
rulesAsString));
497+
return false;
498+
}
499+
500+
if (rule.getSourcePortStart() <= newRule.getSourcePortStart() && rule.getSourcePortEnd() >= newRule.getSourcePortStart()) {
501+
s_logger.debug(String.format("Rules (%s) have conflicting port ranges.", rulesAsString));
502+
return true;
503+
}
504+
505+
if (rule.getSourcePortStart() <= newRule.getSourcePortEnd() && rule.getSourcePortEnd() >= newRule.getSourcePortEnd()) {
506+
s_logger.debug(String.format("Rules (%s) have conflicting port ranges.", rulesAsString));
507+
return true;
508+
}
509+
510+
if (newRule.getSourcePortStart() <= rule.getSourcePortStart() && newRule.getSourcePortEnd() >= rule.getSourcePortStart()) {
511+
s_logger.debug(String.format("Rules (%s) have conflicting port ranges.", rulesAsString));
512+
return true;
513+
}
514+
515+
if (newRule.getSourcePortStart() <= rule.getSourcePortEnd() && newRule.getSourcePortEnd() >= rule.getSourcePortEnd()) {
516+
s_logger.debug(String.format("Rules (%s) have conflicting port ranges.", rulesAsString));
517+
return true;
518+
}
519+
520+
s_logger.debug(String.format("Rules (%s) do not have conflicting port ranges.", rulesAsString));
521+
return false;
522+
}
523+
494524
@Override
495525
public void validateFirewallRule(Account caller, IPAddressVO ipAddress, Integer portStart, Integer portEnd, String proto, Purpose purpose, FirewallRuleType type,
496526
Long networkId, FirewallRule.TrafficType trafficType) {

‎server/src/test/java/com/cloud/network/firewall/FirewallManagerTest.java‎

Lines changed: 96 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,25 +27,25 @@
2727
import com.cloud.network.element.FirewallServiceProvider;
2828
import com.cloud.network.element.VirtualRouterElement;
2929
import com.cloud.network.element.VpcVirtualRouterElement;
30-
import com.cloud.network.rules.FirewallManager;
3130
import com.cloud.network.rules.FirewallRule;
3231
import com.cloud.network.rules.FirewallRule.Purpose;
3332
import com.cloud.network.rules.FirewallRuleVO;
3433
import com.cloud.network.vpc.VpcManager;
3534
import com.cloud.user.AccountManager;
3635
import com.cloud.user.DomainManager;
3736
import com.cloud.utils.component.ComponentContext;
38-
import junit.framework.Assert;
3937
import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
4038
import org.apache.log4j.Logger;
39+
import org.junit.Assert;
4140
import org.junit.Before;
4241
import org.junit.Ignore;
4342
import org.junit.Test;
4443
import org.junit.runner.RunWith;
4544
import org.mockito.InjectMocks;
4645
import org.mockito.Mock;
4746
import org.mockito.MockitoAnnotations;
48-
import org.mockito.runners.MockitoJUnitRunner;
47+
import org.mockito.Spy;
48+
import org.mockito.junit.MockitoJUnitRunner;
4949

5050
import java.util.ArrayList;
5151
import java.util.Arrays;
@@ -108,12 +108,35 @@ public void testInjected() {
108108
@Mock
109109
FirewallRulesDao _firewallDao;
110110

111+
@Spy
111112
@InjectMocks
112-
FirewallManager _firewallMgr = new FirewallManagerImpl();
113+
FirewallManagerImpl _firewallMgr;
114+
115+
FirewallRule fwRule50to150;
116+
FirewallRule fwRule100to200;
117+
FirewallRule fwRule151to200;
118+
119+
FirewallRule pfRule50to150;
120+
FirewallRule pfRule100to200;
121+
FirewallRule pfRule151to200;
122+
113123

114124
@Before
115125
public void initMocks() {
116126
MockitoAnnotations.initMocks(this);
127+
128+
fwRule50to150 = createFirewallRule(50, 150, Purpose.Firewall);
129+
fwRule100to200 = createFirewallRule(100, 150, Purpose.Firewall);
130+
fwRule151to200 = createFirewallRule(151, 200, Purpose.Firewall);
131+
132+
pfRule50to150 = createFirewallRule(50, 150, Purpose.PortForwarding);
133+
pfRule100to200 = createFirewallRule(100, 150, Purpose.PortForwarding);
134+
pfRule151to200 = createFirewallRule(151, 200, Purpose.PortForwarding);
135+
}
136+
137+
private FirewallRule createFirewallRule(int startPort, int endPort, Purpose purpose) {
138+
return new FirewallRuleVO("xid", 1L, startPort, endPort, "TCP", 2, 3, 4, purpose, new ArrayList<>(),
139+
new ArrayList<>(), 5, 6, null, FirewallRule.TrafficType.Ingress);
117140
}
118141

119142
@Ignore("Requires database to be set up")
@@ -210,6 +233,75 @@ public void testDetectRulesConflict() {
210233
}
211234
}
212235

236+
@Test
237+
public void checkIfRulesHaveConflictingPortRangesTestOnlyOneRuleIsFirewallReturnsFalse()
238+
{
239+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule50to150, pfRule50to150, true, false, false, true);
240+
241+
Assert.assertFalse(result);
242+
}
243+
244+
@Test
245+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesAreFirewallButNoDuplicateCidrsReturnsFalse()
246+
{
247+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule50to150, fwRule50to150, false, true, false, false);
248+
249+
Assert.assertFalse(result);
250+
}
251+
252+
@Test
253+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingButNoDuplicateCidrsReturnsFalse()
254+
{
255+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule50to150, false, false, true, false);
256+
257+
Assert.assertFalse(result);
258+
}
259+
260+
@Test
261+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesAreFirewallAndDuplicatedCidrsAndNewRuleSourceStartPortIsInsideExistingRangeReturnsTrue()
262+
{
263+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule100to200, fwRule50to150, false, true, false, true);
264+
265+
Assert.assertTrue(result);
266+
}
267+
268+
@Test
269+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesAreFirewallAndDuplicatedCidrsAndNewRuleSourceEndPortIsInsideExistingRangeReturnsTrue()
270+
{
271+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule50to150, fwRule100to200, false, true, false, true);
272+
273+
Assert.assertTrue(result);
274+
}
275+
276+
@Test
277+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingAndDuplicatedCidrsAndNewRuleSourceStartPortIsInsideExistingRangeReturnsTrue()
278+
{
279+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule100to200, false, false, true, true);
280+
281+
Assert.assertTrue(result);
282+
}
283+
284+
@Test
285+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingAndDuplicatedCidrsAndNewRuleSourceEndPortIsInsideExistingRangeReturnsTrue()
286+
{
287+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule100to200, false, false, true, true);
288+
289+
Assert.assertTrue(result);
290+
}
291+
292+
@Test
293+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesAreFirewallAndDuplicatedCidrsAndNoRangeConflictReturnsFalse()
294+
{
295+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(fwRule50to150, fwRule151to200, false, true, false, true);
213296

297+
Assert.assertFalse(result);
298+
}
214299

300+
@Test
301+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingAndDuplicatedCidrsAndNoRangeConflictReturnsFalse()
302+
{
303+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule151to200, false, false, true, true);
304+
305+
Assert.assertFalse(result);
306+
}
215307
}

0 commit comments

Comments
 (0)