Skip to content

Commit ea8b85a

Browse files
murali-reddymurali-reddy
committed
CLOUDSTACK-234: create/delete firewa/lb/pf rule: send ip assoc command
only on first rule is created on the IP and last rule is revoked on the IP Current suboptima logic of IP Assoc - On associate IP to GuestNetwork there is an IPAssoc command sent to corresponding network service providers of the network - On every rule apply on IP associated with the network send IP assoc to the network service providers - On every rule deletion on IP associated with a network sernd IP assoc command to the network service providers With this fix logic of IP assoc is changed as below which eliminates executio of unnessary and expensive IpAssocCommand resource command - On associate IP to GuestNetwork, associate IP only to the network, Untill any service is associated with the IP dont send IP Assoc - On creation of first rule on the IP send IPAssoc to corresponding network service provider. Since IP is used for a service, IPAssoc need to be sent to correpondign service provider - On deletion of last rule on the IP send IPAssoc to corresponding network service provider. When last rule is deleted, IP has no service associated with it, so send IP assoc to service provider to remove the IP association
1 parent 836215c commit ea8b85a

File tree

8 files changed

+136
-49
lines changed

8 files changed

+136
-49
lines changed

‎engine/schema/src/com/cloud/network/dao/FirewallRulesDao.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,9 @@ public interface FirewallRulesDao extends GenericDao<FirewallRuleVO, Long> {
5454
List<FirewallRuleVO> listByIpAndNotRevoked(long ipAddressId);
5555

5656
long countRulesByIpId(long sourceIpId);
57-
57+
58+
long countRulesByIpIdAndState(long sourceIpId, FirewallRule.State state);
59+
5860
List<FirewallRuleVO> listByNetworkPurposeTrafficTypeAndNotRevoked(long networkId, FirewallRule.Purpose purpose, FirewallRule.TrafficType trafficType);
5961
List<FirewallRuleVO> listByNetworkPurposeTrafficType(long networkId, FirewallRule.Purpose purpose, FirewallRule.TrafficType trafficType);
6062

‎engine/schema/src/com/cloud/network/dao/FirewallRulesDaoImpl.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ protected FirewallRulesDaoImpl() {
100100
RulesByIpCount = createSearchBuilder(Long.class);
101101
RulesByIpCount.select(null, Func.COUNT, RulesByIpCount.entity().getId());
102102
RulesByIpCount.and("ipAddressId", RulesByIpCount.entity().getSourceIpAddressId(), Op.EQ);
103+
RulesByIpCount.and("state", RulesByIpCount.entity().getState(), Op.EQ);
103104
RulesByIpCount.done();
104105
}
105106

@@ -328,6 +329,16 @@ public boolean remove(Long id) {
328329
return result;
329330
}
330331

332+
@Override
333+
public long countRulesByIpIdAndState(long sourceIpId, FirewallRule.State state) {
334+
SearchCriteria<Long> sc = RulesByIpCount.create();
335+
sc.setParameters("ipAddressId", sourceIpId);
336+
if (state != null) {
337+
sc.setParameters("state", state);
338+
}
339+
return customSearch(sc, null).get(0);
340+
}
341+
331342
@Override
332343
public List<FirewallRuleVO> listByIpAndPurposeWithState(Long ipId, Purpose purpose, State state) {
333344
SearchCriteria<FirewallRuleVO> sc = AllFieldsSearch.create();

‎server/src/com/cloud/network/NetworkManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ boolean associateIpAddressListToAccount(long userId, long accountId, long zoneId
191191

192192
public String acquireGuestIpAddress(Network network, String requestedIp);
193193

194-
boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError) throws ResourceUnavailableException;
194+
boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException;
195195

196196
boolean reallocate(VirtualMachineProfile<? extends VMInstanceVO> vm,
197197
DataCenterDeployment dest) throws InsufficientCapacityException, ConcurrentOperationException;

‎server/src/com/cloud/network/NetworkManagerImpl.java‎

Lines changed: 114 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -627,45 +627,41 @@ public PublicIp assignDedicateIpAddress(Account owner, Long guestNtwkId, Long vp
627627
@Override
628628
public boolean applyIpAssociations(Network network, boolean continueOnError) throws ResourceUnavailableException {
629629
List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(), null);
630-
List<PublicIp> publicIps = new ArrayList<PublicIp>();
631-
if (userIps != null && !userIps.isEmpty()) {
632-
for (IPAddressVO userIp : userIps) {
633-
PublicIp publicIp = PublicIp.createFromAddrAndVlan(userIp, _vlanDao.findById(userIp.getVlanId()));
634-
publicIps.add(publicIp);
635-
}
636-
}
637-
638-
boolean success = applyIpAssociations(network, false, continueOnError, publicIps);
630+
boolean success = true;
639631

640-
if (success) {
641-
for (IPAddressVO addr : userIps) {
642-
if (addr.getState() == IpAddress.State.Allocating) {
643-
markPublicIpAsAllocated(addr);
644-
} else if (addr.getState() == IpAddress.State.Releasing) {
645-
// Cleanup all the resources for ip address if there are any, and only then un-assign ip in the
646-
// system
647-
if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount())) {
648-
s_logger.debug("Unassiging ip address " + addr);
649-
_ipAddressDao.unassignIpAddress(addr.getId());
650-
} else {
651-
success = false;
652-
s_logger.warn("Failed to release resources for ip address id=" + addr.getId());
653-
}
632+
// CloudStack will take a lazy approach to associate an acquired public IP to a network service provider as
633+
// it will not know what service an acquired IP will be used for. An IP is actually associated with a provider when first
634+
// rule is applied. Similarly when last rule on the acquired IP is revoked, IP is not associated with any provider
635+
// but still be associated with the account. At this point just mark IP as allocated or released.
636+
for (IPAddressVO addr : userIps) {
637+
if (addr.getState() == IpAddress.State.Allocating) {
638+
addr.setAssociatedWithNetworkId(network.getId());
639+
markPublicIpAsAllocated(addr);
640+
} else if (addr.getState() == IpAddress.State.Releasing) {
641+
// Cleanup all the resources for ip address if there are any, and only then un-assign ip in the system
642+
if (cleanupIpResources(addr.getId(), Account.ACCOUNT_ID_SYSTEM, _accountMgr.getSystemAccount())) {
643+
_ipAddressDao.unassignIpAddress(addr.getId());
644+
} else {
645+
success = false;
646+
s_logger.warn("Failed to release resources for ip address id=" + addr.getId());
654647
}
655648
}
656649
}
657650

658651
return success;
659652
}
660653

661-
662654

655+
// CloudStack will take a lazy approach to associate an acquired public IP to a network service provider as
656+
// it will not know what a acquired IP will be used for. An IP is actually associated with a provider when first
657+
// rule is applied. Similarly when last rule on the acquired IP is revoked, IP is not associated with any provider
658+
// but still be associated with the account. Its up to caller of this function to decide when to invoke IPAssociation
663659
@Override
664-
public boolean applyIpAssociations(Network network, boolean rulesRevoked, boolean continueOnError,
660+
public boolean applyIpAssociations(Network network, boolean postApplyRules, boolean continueOnError,
665661
List<? extends PublicIpAddress> publicIps) throws ResourceUnavailableException {
666662
boolean success = true;
667663

668-
Map<PublicIpAddress, Set<Service>> ipToServices = _networkModel.getIpToServices(publicIps, rulesRevoked, true);
664+
Map<PublicIpAddress, Set<Service>> ipToServices = _networkModel.getIpToServices(publicIps, postApplyRules, true);
669665
Map<Provider, ArrayList<PublicIpAddress>> providerToIpList = _networkModel.getProviderToIpList(network, ipToServices);
670666

671667
for (Provider provider : providerToIpList.keySet()) {
@@ -2943,11 +2939,10 @@ public boolean applyRules(List<? extends FirewallRule> rules, FirewallRule.Purpo
29432939
publicIps.add(publicIp);
29442940
}
29452941
}
2946-
2947-
// rules can not programmed unless IP is associated with network
2948-
// service provider, so run IP assoication for
2949-
// the network so as to ensure IP is associated before applying
2950-
// rules (in add state)
2942+
}
2943+
// rules can not programmed unless IP is associated with network service provider, so run IP assoication for
2944+
// the network so as to ensure IP is associated before applying rules (in add state)
2945+
if (checkIfIpAssocRequired(network, false, publicIps)) {
29512946
applyIpAssociations(network, false, continueOnError, publicIps);
29522947
}
29532948

@@ -2961,15 +2956,65 @@ public boolean applyRules(List<? extends FirewallRule> rules, FirewallRule.Purpo
29612956
success = false;
29622957
}
29632958

2964-
if (!(rules.get(0).getPurpose() == FirewallRule.Purpose.Firewall && trafficType == FirewallRule.TrafficType.Egress)) {
2965-
// if all the rules configured on public IP are revoked then
2966-
// dis-associate IP with network service provider
2959+
// if there are no active rules associated with a public IP, then public IP need not be associated with a provider.
2960+
// This IPAssoc ensures, public IP is dis-associated after last active rule is revoked.
2961+
if (checkIfIpAssocRequired(network, true, publicIps)) {
29672962
applyIpAssociations(network, true, continueOnError, publicIps);
29682963
}
29692964

29702965
return success;
29712966
}
29722967

2968+
// An IP association is required in below cases
2969+
// 1.there is at least one public IP associated with the network on which first rule (PF/static NAT/LB) is being applied.
2970+
// 2.last rule (PF/static NAT/LB) on the public IP has been revoked. So the public IP should not be associated with any provider
2971+
boolean checkIfIpAssocRequired(Network network, boolean postApplyRules, List<PublicIp> publicIps) {
2972+
for (PublicIp ip : publicIps) {
2973+
if (ip.isSourceNat()) {
2974+
continue;
2975+
} else if (ip.isOneToOneNat()) {
2976+
continue;
2977+
} else {
2978+
Long totalCount = null;
2979+
Long revokeCount = null;
2980+
Long activeCount = null;
2981+
Long addCount = null;
2982+
2983+
totalCount = _firewallDao.countRulesByIpId(ip.getId());
2984+
if (postApplyRules) {
2985+
revokeCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Revoke);
2986+
} else {
2987+
activeCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active);
2988+
addCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Add);
2989+
}
2990+
2991+
if (totalCount == null || totalCount.longValue() == 0L) {
2992+
continue;
2993+
}
2994+
2995+
if (postApplyRules) {
2996+
2997+
if (revokeCount != null && revokeCount.longValue() == totalCount.longValue()) {
2998+
s_logger.trace("All rules are in Revoke state, have to dis-assiciate IP from the backend");
2999+
return true;
3000+
}
3001+
} else {
3002+
if (activeCount != null && activeCount > 0) {
3003+
continue;
3004+
} else if (addCount != null && addCount.longValue() == totalCount.longValue()) {
3005+
s_logger.trace("All rules are in Add state, have to assiciate IP with the backend");
3006+
return true;
3007+
} else {
3008+
continue;
3009+
}
3010+
}
3011+
}
3012+
}
3013+
3014+
// there are no IP's corresponding to this network that need to be associated with provider
3015+
return false;
3016+
}
3017+
29733018
public class NetworkGarbageCollector implements Runnable {
29743019

29753020
@Override
@@ -3535,7 +3580,8 @@ public String acquireGuestIpAddress(Network network, String requestedIp) {
35353580

35363581

35373582
@Override
3538-
public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError) throws ResourceUnavailableException {
3583+
public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError, boolean forRevoke)
3584+
throws ResourceUnavailableException {
35393585
Network network = _networksDao.findById(staticNats.get(0).getNetworkId());
35403586
boolean success = true;
35413587

@@ -3554,9 +3600,11 @@ public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean con
35543600
}
35553601
}
35563602

3557-
// static NAT rules can not programmed unless IP is associated with network service provider, so run IP
3558-
// association for the network so as to ensure IP is associated before applying rules (in add state)
3559-
applyIpAssociations(network, false, continueOnError, publicIps);
3603+
// static NAT rules can not programmed unless IP is associated with source NAT service provider, so run IP
3604+
// association for the network so as to ensure IP is associated before applying rules
3605+
if (checkStaticNatIPAssocRequired(network, false, forRevoke, publicIps)) {
3606+
applyIpAssociations(network, false, continueOnError, publicIps);
3607+
}
35603608

35613609
// get provider
35623610
StaticNatServiceProvider element = getStaticNatProviderForNetwork(network);
@@ -3587,12 +3635,38 @@ public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean con
35873635
}
35883636
}
35893637

3590-
// if all the rules configured on public IP are revoked then, dis-associate IP with network service provider
3591-
applyIpAssociations(network, true, continueOnError, publicIps);
3638+
// if the static NAT rules configured on public IP is revoked then, dis-associate IP with static NAT service provider
3639+
if (checkStaticNatIPAssocRequired(network, true, forRevoke, publicIps)) {
3640+
applyIpAssociations(network, true, continueOnError, publicIps);
3641+
}
35923642

35933643
return success;
35943644
}
35953645

3646+
// checks if there are any public IP assigned to network, that are marked for one-to-one NAT that
3647+
// needs to be associated/dis-associated with static-nat provider
3648+
boolean checkStaticNatIPAssocRequired(Network network, boolean postApplyRules, boolean forRevoke, List<PublicIp> publicIps) {
3649+
for (PublicIp ip : publicIps) {
3650+
if (ip.isOneToOneNat()) {
3651+
Long activeFwCount = null;
3652+
activeFwCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active);
3653+
3654+
if (!postApplyRules && !forRevoke) {
3655+
if (activeFwCount > 0) {
3656+
continue;
3657+
} else {
3658+
return true;
3659+
}
3660+
} else if (postApplyRules && forRevoke) {
3661+
return true;
3662+
}
3663+
} else {
3664+
continue;
3665+
}
3666+
}
3667+
return false;
3668+
}
3669+
35963670
@DB
35973671
@Override
35983672
public boolean reallocate(VirtualMachineProfile<? extends VMInstanceVO> vm, DataCenterDeployment dest) throws InsufficientCapacityException, ConcurrentOperationException {

‎server/src/com/cloud/network/NetworkModelImpl.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ Set<Purpose> getPublicIpPurposeInRules(PublicIpAddress ip, boolean includeRevoke
273273
}
274274

275275
@Override
276-
public Map<PublicIpAddress, Set<Service>> getIpToServices(List<? extends PublicIpAddress> publicIps, boolean rulesRevoked, boolean includingFirewall) {
276+
public Map<PublicIpAddress, Set<Service>> getIpToServices(List<? extends PublicIpAddress> publicIps, boolean postApplyRules, boolean includingFirewall) {
277277
Map<PublicIpAddress, Set<Service>> ipToServices = new HashMap<PublicIpAddress, Set<Service>>();
278278

279279
if (publicIps != null && !publicIps.isEmpty()) {
@@ -331,7 +331,7 @@ public Map<PublicIpAddress, Set<Service>> getIpToServices(List<? extends PublicI
331331
// IP is not being used for any purpose so skip IPAssoc to network service provider
332332
continue;
333333
} else {
334-
if (rulesRevoked) {
334+
if (postApplyRules) {
335335
// no active rules/revoked rules are associated with this public IP, so remove the
336336
// association with the provider
337337
if (ip.isSourceNat()) {

‎server/src/com/cloud/network/rules/RulesManagerImpl.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -964,7 +964,7 @@ public boolean applyStaticNatsForNetwork(long networkId, boolean continueOnError
964964
}
965965

966966
try {
967-
if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) {
967+
if (!_networkMgr.applyStaticNats(staticNats, continueOnError, false)) {
968968
return false;
969969
}
970970
} catch (ResourceUnavailableException ex) {
@@ -1307,7 +1307,7 @@ protected boolean applyStaticNatForIp(long sourceIpId, boolean continueOnError,
13071307

13081308
if (staticNats != null && !staticNats.isEmpty()) {
13091309
try {
1310-
if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) {
1310+
if (!_networkMgr.applyStaticNats(staticNats, continueOnError, forRevoke)) {
13111311
return false;
13121312
}
13131313
} catch (ResourceUnavailableException ex) {
@@ -1334,7 +1334,7 @@ public boolean applyStaticNatForNetwork(long networkId, boolean continueOnError,
13341334
s_logger.debug("Found " + staticNats.size() + " static nats to disable for network id " + networkId);
13351335
}
13361336
try {
1337-
if (!_networkMgr.applyStaticNats(staticNats, continueOnError)) {
1337+
if (!_networkMgr.applyStaticNats(staticNats, continueOnError, forRevoke)) {
13381338
return false;
13391339
}
13401340
} catch (ResourceUnavailableException ex) {

‎server/test/com/cloud/network/MockNetworkManagerImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ public String acquireGuestIpAddress(Network network, String requestedIp) {
323323

324324

325325
@Override
326-
public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError) throws ResourceUnavailableException {
326+
public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException {
327327
// TODO Auto-generated method stub
328328
return false;
329329
}

‎server/test/com/cloud/vpc/MockNetworkManagerImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -987,7 +987,7 @@ public String acquireGuestIpAddress(Network network, String requestedIp) {
987987
* @see com.cloud.network.NetworkManager#applyStaticNats(java.util.List, boolean)
988988
*/
989989
@Override
990-
public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError)
990+
public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError, boolean forRevoke)
991991
throws ResourceUnavailableException {
992992
// TODO Auto-generated method stub
993993
return false;

0 commit comments

Comments
 (0)