Skip to content

Commit 6d33ef1

Browse files
nvazqueznvazquez
committed
Add unit tests for detectConflicts method and smoke tests
1 parent a0755a1 commit 6d33ef1

File tree

4 files changed

+381
-36
lines changed

4 files changed

+381
-36
lines changed

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

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -400,17 +400,9 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
400400
assert (rules.size() >= 1);
401401
}
402402

403-
NetworkVO newRuleNetwork = _networkDao.findById(newRule.getNetworkId());
404-
if (newRuleNetwork == null) {
405-
throw new InvalidParameterValueException("Unable to create firewall rule as cannot find network by id=" + newRule.getNetworkId());
406-
}
407-
boolean isNewRuleOnVpcNetwork = newRuleNetwork.getVpcId() != null;
408-
boolean isVpcConserveModeEnabled = false;
409-
if (isNewRuleOnVpcNetwork) {
410-
Vpc vpc = _vpcMgr.getActiveVpc(newRuleNetwork.getVpcId());
411-
VpcOfferingVO vpcOffering = vpc != null ? vpcOfferingDao.findById(vpc.getVpcOfferingId()) : null;
412-
isVpcConserveModeEnabled = vpcOffering != null && vpcOffering.isConserveMode();
413-
}
403+
NetworkVO newRuleNetwork = getNewRuleNetwork(newRule);
404+
boolean newRuleIsOnVpcNetwork = isNewRuleOnVpcNetwork(newRuleNetwork);
405+
boolean vpcConserveModeEnabled = isVpcConserveModeEnabled(newRuleNetwork);
414406

415407
for (FirewallRuleVO rule : rules) {
416408
if (rule.getId() == newRule.getId()) {
@@ -461,10 +453,10 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
461453

462454
// Checking if the rule applied is to the same network that is passed in the rule.
463455
// (except for VPCs with conserve mode = true)
464-
if ((!isNewRuleOnVpcNetwork || !isVpcConserveModeEnabled)
456+
if ((!newRuleIsOnVpcNetwork || !vpcConserveModeEnabled)
465457
&& rule.getNetworkId() != newRule.getNetworkId() && rule.getState() != State.Revoke) {
466458
String errMsg = String.format("New rule is for a different network than what's specified in rule %s", rule.getXid());
467-
if (isNewRuleOnVpcNetwork) {
459+
if (newRuleIsOnVpcNetwork) {
468460
errMsg += String.format(" - VPC id=%s is not using conserve mode", newRuleNetwork.getVpcId());
469461
}
470462
throw new NetworkRuleConflictException(errMsg);
@@ -516,6 +508,27 @@ public void detectRulesConflict(FirewallRule newRule) throws NetworkRuleConflict
516508
}
517509
}
518510

511+
protected boolean isVpcConserveModeEnabled(NetworkVO newRuleNetwork) {
512+
if (isNewRuleOnVpcNetwork(newRuleNetwork)) {
513+
Vpc vpc = _vpcMgr.getActiveVpc(newRuleNetwork.getVpcId());
514+
VpcOfferingVO vpcOffering = vpc != null ? vpcOfferingDao.findById(vpc.getVpcOfferingId()) : null;
515+
return vpcOffering != null && vpcOffering.isConserveMode();
516+
}
517+
return false;
518+
}
519+
520+
protected boolean isNewRuleOnVpcNetwork(NetworkVO newRuleNetwork) {
521+
return newRuleNetwork.getVpcId() != null;
522+
}
523+
524+
protected NetworkVO getNewRuleNetwork(FirewallRule newRule) {
525+
NetworkVO newRuleNetwork = _networkDao.findById(newRule.getNetworkId());
526+
if (newRuleNetwork == null) {
527+
throw new InvalidParameterValueException("Unable to create firewall rule as cannot find network by id=" + newRule.getNetworkId());
528+
}
529+
return newRuleNetwork;
530+
}
531+
519532
protected boolean checkIfRulesHaveConflictingPortRanges(FirewallRule newRule, FirewallRule rule, boolean oneOfRulesIsFirewall, boolean bothRulesFirewall, boolean bothRulesPortForwarding, boolean duplicatedCidrs) {
520533
String rulesAsString = String.format("[%s] and [%s]", rule, newRule);
521534

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

Lines changed: 76 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,10 @@
3232
import com.cloud.network.rules.FirewallRule;
3333
import com.cloud.network.rules.FirewallRule.Purpose;
3434
import com.cloud.network.rules.FirewallRuleVO;
35+
import com.cloud.network.vpc.Vpc;
3536
import com.cloud.network.vpc.VpcManager;
37+
import com.cloud.network.vpc.VpcOfferingVO;
38+
import com.cloud.network.vpc.dao.VpcOfferingDao;
3639
import com.cloud.user.AccountManager;
3740
import com.cloud.user.DomainManager;
3841
import com.cloud.utils.component.ComponentContext;
@@ -81,6 +84,8 @@ public class FirewallManagerTest {
8184
FirewallRulesDao _firewallDao;
8285
@Mock
8386
NetworkDao _networkDao;
87+
@Mock
88+
VpcOfferingDao vpcOfferingDao;
8489

8590
@Spy
8691
@InjectMocks
@@ -168,54 +173,102 @@ public void testApplyFWRules() {
168173
}
169174
}
170175

171-
@Test
172-
public void testDetectRulesConflict() {
173-
List<FirewallRuleVO> ruleList = new ArrayList<FirewallRuleVO>();
174-
FirewallRuleVO rule1 = spy(new FirewallRuleVO("rule1", 3, 500, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
175-
FirewallRuleVO rule2 = spy(new FirewallRuleVO("rule2", 3, 1701, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
176-
FirewallRuleVO rule3 = spy(new FirewallRuleVO("rule3", 3, 4500, "UDP", 1, 2, 1, Purpose.Vpn, null, null, null, null));
176+
private List<FirewallRuleVO> createExistingFirewallListRulesList(long existingNetworkId) {
177+
List<FirewallRuleVO> ruleList = new ArrayList<>();
178+
FirewallRuleVO rule1 = spy(new FirewallRuleVO("rule1", 3, 500, "UDP", existingNetworkId, 2, 1, Purpose.Vpn, null, null, null, null));
179+
FirewallRuleVO rule2 = spy(new FirewallRuleVO("rule2", 3, 1701, "UDP", existingNetworkId, 2, 1, Purpose.Vpn, null, null, null, null));
180+
FirewallRuleVO rule3 = spy(new FirewallRuleVO("rule3", 3, 4500, "UDP", existingNetworkId, 2, 1, Purpose.Vpn, null, null, null, null));
177181

178182
List<String> sString = Arrays.asList("10.1.1.1/24","192.168.1.1/24");
179183
List<String> dString1 = Arrays.asList("10.1.1.1/25");
180-
List<String> dString2 = Arrays.asList("10.1.1.128/25");
181184

182-
FirewallRuleVO rule4 = spy(new FirewallRuleVO("rule4", 3L, 10, 20, "TCP", 1, 2, 1, Purpose.Firewall, sString, dString1, null, null,
185+
FirewallRuleVO rule4 = spy(new FirewallRuleVO("rule4", 3L, 10, 20, "TCP", existingNetworkId, 2, 1, Purpose.Firewall, sString, dString1, null, null,
183186
null, FirewallRule.TrafficType.Egress));
184187

188+
when(rule1.getId()).thenReturn(1L);
189+
when(rule2.getId()).thenReturn(2L);
190+
when(rule3.getId()).thenReturn(3L);
191+
when(rule4.getId()).thenReturn(4L);
192+
185193
ruleList.add(rule1);
186194
ruleList.add(rule2);
187195
ruleList.add(rule3);
188196
ruleList.add(rule4);
189197

190-
FirewallManagerImpl firewallMgr = (FirewallManagerImpl)_firewallMgr;
198+
return ruleList;
199+
}
191200

192-
when(firewallMgr._firewallDao.listByIpAndPurposeAndNotRevoked(3,null)).thenReturn(ruleList);
193-
when(rule1.getId()).thenReturn(1L);
194-
when(rule2.getId()).thenReturn(2L);
195-
when(rule3.getId()).thenReturn(3L);
196-
when(rule4.getId()).thenReturn(4L);
201+
private List<FirewallRule> createNewRuleList(long newNetworkId) {
202+
List<String> sString = Arrays.asList("10.1.1.1/24","192.168.1.1/24");
203+
List<String> dString2 = Arrays.asList("10.1.1.128/25");
197204

198-
FirewallRule newRule1 = new FirewallRuleVO("newRule1", 3, 500, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
199-
FirewallRule newRule2 = new FirewallRuleVO("newRule2", 3, 1701, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
200-
FirewallRule newRule3 = new FirewallRuleVO("newRule3", 3, 4500, "TCP", 1, 2, 1, Purpose.PortForwarding, null, null, null, null);
201-
FirewallRule newRule4 = new FirewallRuleVO("newRule4", 3L, 15, 25, "TCP", 1, 2, 1, Purpose.Firewall, sString, dString2, null, null,
205+
FirewallRule newRule1 = new FirewallRuleVO("newRule1", 3, 500, "TCP", newNetworkId, 2, 1, Purpose.PortForwarding, null, null, null, null);
206+
FirewallRule newRule2 = new FirewallRuleVO("newRule2", 3, 1701, "TCP", newNetworkId, 2, 1, Purpose.PortForwarding, null, null, null, null);
207+
FirewallRule newRule3 = new FirewallRuleVO("newRule3", 3, 4500, "TCP", newNetworkId, 2, 1, Purpose.PortForwarding, null, null, null, null);
208+
FirewallRule newRule4 = new FirewallRuleVO("newRule4", 3L, 15, 25, "TCP", newNetworkId, 2, 1, Purpose.Firewall, sString, dString2, null, null,
202209
null, FirewallRule.TrafficType.Egress);
210+
return Arrays.asList(newRule1, newRule2, newRule3, newRule4);
211+
}
212+
213+
@Test
214+
public void testDetectRulesConflictIsolatedNetwork() {
215+
List<FirewallRuleVO> ruleList = createExistingFirewallListRulesList(1L);
216+
when(_firewallMgr._firewallDao.listByIpAndPurposeAndNotRevoked(3,null)).thenReturn(ruleList);
217+
218+
List<FirewallRule> newRuleList = createNewRuleList(1L);
203219

204220
NetworkVO networkVO = Mockito.mock(NetworkVO.class);
205-
when(firewallMgr._networkDao.findById(1L)).thenReturn(networkVO);
221+
when(_firewallMgr._networkDao.findById(1L)).thenReturn(networkVO);
206222
when(networkVO.getVpcId()).thenReturn(null);
207223

208224
try {
209-
firewallMgr.detectRulesConflict(newRule1);
210-
firewallMgr.detectRulesConflict(newRule2);
211-
firewallMgr.detectRulesConflict(newRule3);
212-
firewallMgr.detectRulesConflict(newRule4);
225+
for (FirewallRule newRule : newRuleList) {
226+
_firewallMgr.detectRulesConflict(newRule);
227+
}
213228
}
214229
catch (NetworkRuleConflictException ex) {
215230
Assert.fail();
216231
}
217232
}
218233

234+
private void testDetectRulesConflictVpcBase(boolean vpcConserveMode) throws NetworkRuleConflictException {
235+
long existingNetworkId = 1L;
236+
long newNetworkId = 2L;
237+
long vpcId = 10L;
238+
239+
List<FirewallRuleVO> ruleList = createExistingFirewallListRulesList(existingNetworkId);
240+
when(_firewallMgr._firewallDao.listByIpAndPurposeAndNotRevoked(3,null)).thenReturn(ruleList);
241+
242+
List<FirewallRule> newRuleList = createNewRuleList(newNetworkId);
243+
244+
NetworkVO newNetworkVO = Mockito.mock(NetworkVO.class);
245+
Vpc vpc = Mockito.mock(Vpc.class);
246+
VpcOfferingVO vpcOffering = Mockito.mock(VpcOfferingVO.class);
247+
248+
when(_firewallMgr._networkDao.findById(2L)).thenReturn(newNetworkVO);
249+
when(newNetworkVO.getVpcId()).thenReturn(vpcId);
250+
when(_vpcMgr.getActiveVpc(vpcId)).thenReturn(vpc);
251+
when(vpc.getVpcOfferingId()).thenReturn(1L);
252+
when(vpcOfferingDao.findById(1L)).thenReturn(vpcOffering);
253+
when(vpcOffering.isConserveMode()).thenReturn(vpcConserveMode);
254+
255+
for (FirewallRule newRule : newRuleList) {
256+
_firewallMgr.detectRulesConflict(newRule);
257+
}
258+
}
259+
260+
@Test
261+
public void testDetectRulesConflictVpcConserveMode() throws NetworkRuleConflictException {
262+
// When VPC conserve mode is enabled, rules can be created for multiple network tiers
263+
testDetectRulesConflictVpcBase(true);
264+
}
265+
266+
@Test(expected = NetworkRuleConflictException.class)
267+
public void testDetectRulesConflictVpcConserveModeFalse() throws NetworkRuleConflictException {
268+
// When VPC conserve mode is disabled, an exception should be thrown when attempting to create rules on different network tiers
269+
testDetectRulesConflictVpcBase(false);
270+
}
271+
219272
@Test
220273
public void checkIfRulesHaveConflictingPortRangesTestOnlyOneRuleIsFirewallReturnsFalse()
221274
{

0 commit comments

Comments
 (0)