Skip to content

Commit 1fac7e2

Browse files
winterhazelwinterhazel
committed
Refactor giant if & add unit tests
1 parent c7568f1 commit 1fac7e2

File tree

3 files changed

+145
-24
lines changed

3 files changed

+145
-24
lines changed

‎engine/components-api/src/main/java/com/cloud/network/rules/FirewallManager.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,18 @@ public interface FirewallManager extends FirewallService {
3333
* 1. one to one nat ip forwarding
3434
* 2. port forwarding
3535
* 3. load balancing
36-
*
36+
* <p>
3737
* and conflicts are detected between those two rules. In this case, it
3838
* is possible for both rules to be rolled back when, technically, we should
3939
* and the user can simply re-add one of the rules themselves.
4040
*
41-
* @param newRule
42-
* the new rule created.
41+
* @param newRule the new rule created.
4342
* @throws NetworkRuleConflictException
4443
*/
4544
void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflictException;
4645

4746
void validateFirewallRule(Account caller, IPAddressVO ipAddress, Integer portStart, Integer portEnd, String proto, Purpose purpose, FirewallRuleType type,
48-
Long networkid, FirewallRule.TrafficType trafficType);
47+
Long networkid, FirewallRule.TrafficType trafficType);
4948

5049
boolean applyRules(List<? extends FirewallRule> rules, boolean continueOnError, boolean updateRulesInDB) throws ResourceUnavailableException;
5150

@@ -73,7 +72,7 @@ void validateFirewallRule(Account caller, IPAddressVO ipAddress, Integer portSta
7372
// throws NetworkRuleConflictException;
7473

7574
FirewallRule createRuleForAllCidrs(long ipAddrId, Account caller, Integer startPort, Integer endPort, String protocol, Integer icmpCode, Integer icmpType,
76-
Long relatedRuleId, long networkId) throws NetworkRuleConflictException;
75+
Long relatedRuleId, long networkId) throws NetworkRuleConflictException;
7776

7877
boolean revokeAllFirewallRulesForNetwork(long networkId, long userId, Account caller) throws ResourceUnavailableException;
7978

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

Lines changed: 46 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
4141
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
4242
import org.apache.cloudstack.network.RoutedIpv4Manager;
43+
import org.apache.commons.lang3.ObjectUtils;
4344
import org.springframework.stereotype.Component;
4445

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

414+
if (ObjectUtils.anyNull(rule.getSourceCidrList(), newRule.getSourceCidrList())) {
415+
continue;
416+
}
417+
413418
_firewallDao.loadDestinationCidrs(rule);
414419
_firewallDao.loadDestinationCidrs((FirewallRuleVO) newRule);
415420

416-
if (rule.getSourceCidrList() == null || newRule.getSourceCidrList() == null) {
417-
continue;
418-
}
419421
duplicatedCidrs = (detectConflictingCidrs(rule.getSourceCidrList(), newRule.getSourceCidrList()) && detectConflictingCidrs(rule.getDestinationCidrList(), newRule.getDestinationCidrList()));
420422
}
421423

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

427-
if (rule.getSourceCidrList() == null || newRule.getSourceCidrList() == null) {
429+
if (ObjectUtils.anyNull(rule.getSourceCidrList(), newRule.getSourceCidrList())) {
428430
continue;
429431
}
430432

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

465467
if (!notNullPorts) {
466468
continue;
467-
} else if (!oneOfRulesIsFirewall &&
468-
!((bothRulesFirewall || bothRulesPortForwarding) && !duplicatedCidrs) &&
469-
((rule.getSourcePortStart().intValue() <= newRule.getSourcePortStart().intValue() &&
470-
rule.getSourcePortEnd().intValue() >= newRule.getSourcePortStart().intValue()) ||
471-
(rule.getSourcePortStart().intValue() <= newRule.getSourcePortEnd().intValue() &&
472-
rule.getSourcePortEnd().intValue() >= newRule.getSourcePortEnd().intValue()) ||
473-
(newRule.getSourcePortStart().intValue() <= rule.getSourcePortStart().intValue() &&
474-
newRule.getSourcePortEnd().intValue() >= rule.getSourcePortStart().intValue()) ||
475-
(newRule.getSourcePortStart().intValue() <= rule.getSourcePortEnd().intValue() &&
476-
newRule.getSourcePortEnd().intValue() >= rule.getSourcePortEnd().intValue()))) {
477-
//Above else if conditions checks for the conflicting port ranges.
478-
469+
} else if (checkIfRulesHaveConflictingPortRanges(newRule, rule, oneOfRulesIsFirewall, bothRulesFirewall, bothRulesPortForwarding, duplicatedCidrs)) {
479470
// we allow port forwarding rules with the same parameters but different protocols
480471
boolean allowPf =
481472
(rule.getPurpose() == Purpose.PortForwarding && newRule.getPurpose() == Purpose.PortForwarding && !newRule.getProtocol().equalsIgnoreCase(
@@ -502,6 +493,45 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
502493
}
503494
}
504495

496+
protected boolean checkIfRulesHaveConflictingPortRanges(FirewallRule newRule, FirewallRule rule, boolean oneOfRulesIsFirewall, boolean bothRulesFirewall, boolean bothRulesPortForwarding, boolean duplicatedCidrs) {
497+
String rulesAsString = String.format("[%s] and [%s]", rule, newRule);
498+
499+
if (oneOfRulesIsFirewall) {
500+
logger.debug("Only one of the rules (%s) is firewall; therefore, their port ranges will not conflict.",
501+
rulesAsString);
502+
return false;
503+
}
504+
505+
if ((bothRulesFirewall || bothRulesPortForwarding) && !duplicatedCidrs) {
506+
logger.debug("Both rules (%s) are firewall/port forwarding, but they do not have duplicated CIDRs; therefore, their port ranges will not conflict.",
507+
rulesAsString);
508+
return false;
509+
}
510+
511+
if (rule.getSourcePortStart() <= newRule.getSourcePortStart() && rule.getSourcePortEnd() >= newRule.getSourcePortStart()) {
512+
logger.debug("Rules (%s) have conflicting port ranges.", rulesAsString);
513+
return true;
514+
}
515+
516+
if (rule.getSourcePortStart() <= newRule.getSourcePortEnd() && rule.getSourcePortEnd() >= newRule.getSourcePortEnd()) {
517+
logger.debug("Rules (%s) have conflicting port ranges.", rulesAsString);
518+
return true;
519+
}
520+
521+
if (newRule.getSourcePortStart() <= rule.getSourcePortStart() && newRule.getSourcePortEnd() >= rule.getSourcePortStart()) {
522+
logger.debug("Rules (%s) have conflicting port ranges.", rulesAsString);
523+
return true;
524+
}
525+
526+
if (newRule.getSourcePortStart() <= rule.getSourcePortEnd() && newRule.getSourcePortEnd() >= rule.getSourcePortEnd()) {
527+
logger.debug("Rules (%s) have conflicting port ranges.", rulesAsString);
528+
return true;
529+
}
530+
531+
logger.debug("Rules (%s) do not have conflicting port ranges.", rulesAsString);
532+
return false;
533+
}
534+
505535
@Override
506536
public void validateFirewallRule(Account caller, IPAddressVO ipAddress, Integer portStart, Integer portEnd, String proto, Purpose purpose, FirewallRuleType type,
507537
Long networkId, FirewallRule.TrafficType trafficType) {

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

Lines changed: 95 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,26 +27,26 @@
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.logging.log4j.Logger;
4139
import org.apache.logging.log4j.LogManager;
4240
import org.junit.After;
41+
import org.junit.Assert;
4342
import org.junit.Before;
4443
import org.junit.Ignore;
4544
import org.junit.Test;
4645
import org.junit.runner.RunWith;
4746
import org.mockito.InjectMocks;
4847
import org.mockito.Mock;
4948
import org.mockito.MockitoAnnotations;
49+
import org.mockito.Spy;
5050
import org.mockito.junit.MockitoJUnitRunner;
5151

5252
import java.util.ArrayList;
@@ -111,19 +111,42 @@ public void testInjected() {
111111
@Mock
112112
FirewallRulesDao _firewallDao;
113113

114+
@Spy
114115
@InjectMocks
115-
FirewallManager _firewallMgr = new FirewallManagerImpl();
116+
FirewallManagerImpl _firewallMgr;
117+
118+
FirewallRule fwRule50to150;
119+
FirewallRule fwRule100to200;
120+
FirewallRule fwRule151to200;
121+
122+
FirewallRule pfRule50to150;
123+
FirewallRule pfRule100to200;
124+
FirewallRule pfRule151to200;
125+
116126

117127
@Before
118128
public void initMocks() {
119129
closeable = MockitoAnnotations.openMocks(this);
130+
131+
fwRule50to150 = createFirewallRule(50, 150, Purpose.Firewall);
132+
fwRule100to200 = createFirewallRule(100, 150, Purpose.Firewall);
133+
fwRule151to200 = createFirewallRule(151, 200, Purpose.Firewall);
134+
135+
pfRule50to150 = createFirewallRule(50, 150, Purpose.PortForwarding);
136+
pfRule100to200 = createFirewallRule(100, 150, Purpose.PortForwarding);
137+
pfRule151to200 = createFirewallRule(151, 200, Purpose.PortForwarding);
120138
}
121139

122140
@After
123141
public void tearDown() throws Exception {
124142
closeable.close();
125143
}
126144

145+
private FirewallRule createFirewallRule(int startPort, int endPort, Purpose purpose) {
146+
return new FirewallRuleVO("xid", 1L, startPort, endPort, "TCP", 2, 3, 4, purpose, new ArrayList<>(),
147+
new ArrayList<>(), 5, 6, null, FirewallRule.TrafficType.Ingress);
148+
}
149+
127150
@Ignore("Requires database to be set up")
128151
@Test
129152
public void testApplyRules() {
@@ -218,6 +241,75 @@ public void testDetectRulesConflict() {
218241
}
219242
}
220243

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

305+
Assert.assertFalse(result);
306+
}
222307

308+
@Test
309+
public void checkIfRulesHaveConflictingPortRangesTestBothRulesArePortForwardingAndDuplicatedCidrsAndNoRangeConflictReturnsFalse()
310+
{
311+
boolean result = _firewallMgr.checkIfRulesHaveConflictingPortRanges(pfRule50to150, pfRule151to200, false, false, true, true);
312+
313+
Assert.assertFalse(result);
314+
}
223315
}

0 commit comments

Comments
 (0)