Skip to content

Commit d797356

Browse files
hsato03Henrique Sato
hsato03
and
Henrique Sato
authored
Handle public IP race conditions (#9234)
* Lock public IP * Release IP if ID is not null * Fix NPEs Co-authored-by: Henrique Sato <henrique.sato@scclouds.com.br>
1 parent 063dc60 commit d797356

6 files changed

Lines changed: 336 additions & 326 deletions

File tree

‎server/src/main/java/com/cloud/network/IpAddressManagerImpl.java‎

Lines changed: 43 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -725,50 +725,59 @@ protected boolean cleanupIpResources(long ipId, long userId, Account caller) {
725725
@Override
726726
@DB
727727
public boolean disassociatePublicIpAddress(long addrId, long userId, Account caller) {
728-
729728
boolean success = true;
730-
IPAddressVO ipToBeDisassociated = _ipAddressDao.findById(addrId);
731729

732-
PublicIpQuarantine publicIpQuarantine = null;
733-
// Cleanup all ip address resources - PF/LB/Static nat rules
734-
if (!cleanupIpResources(addrId, userId, caller)) {
735-
success = false;
736-
s_logger.warn("Failed to release resources for ip address id=" + addrId);
737-
}
730+
try {
731+
IPAddressVO ipToBeDisassociated = _ipAddressDao.acquireInLockTable(addrId);
738732

739-
IPAddressVO ip = markIpAsUnavailable(addrId);
740-
if (ip == null) {
741-
return true;
742-
}
733+
if (ipToBeDisassociated == null) {
734+
s_logger.error(String.format("Unable to acquire lock on public IP %s.", addrId));
735+
throw new CloudRuntimeException("Unable to acquire lock on public IP.");
736+
}
743737

744-
if (s_logger.isDebugEnabled()) {
745-
s_logger.debug("Releasing ip id=" + addrId + "; sourceNat = " + ip.isSourceNat());
746-
}
738+
PublicIpQuarantine publicIpQuarantine = null;
739+
// Cleanup all ip address resources - PF/LB/Static nat rules
740+
if (!cleanupIpResources(addrId, userId, caller)) {
741+
success = false;
742+
s_logger.warn("Failed to release resources for ip address id=" + addrId);
743+
}
747744

748-
if (ip.getAssociatedWithNetworkId() != null) {
749-
Network network = _networksDao.findById(ip.getAssociatedWithNetworkId());
750-
try {
751-
if (!applyIpAssociations(network, rulesContinueOnErrFlag)) {
752-
s_logger.warn("Unable to apply ip address associations for " + network);
753-
success = false;
745+
IPAddressVO ip = markIpAsUnavailable(addrId);
746+
if (ip == null) {
747+
return true;
748+
}
749+
750+
if (s_logger.isDebugEnabled()) {
751+
s_logger.debug("Releasing ip id=" + addrId + "; sourceNat = " + ip.isSourceNat());
752+
}
753+
754+
if (ip.getAssociatedWithNetworkId() != null) {
755+
Network network = _networksDao.findById(ip.getAssociatedWithNetworkId());
756+
try {
757+
if (!applyIpAssociations(network, rulesContinueOnErrFlag)) {
758+
s_logger.warn("Unable to apply ip address associations for " + network);
759+
success = false;
760+
}
761+
} catch (ResourceUnavailableException e) {
762+
throw new CloudRuntimeException("We should never get to here because we used true when applyIpAssociations", e);
754763
}
755-
} catch (ResourceUnavailableException e) {
756-
throw new CloudRuntimeException("We should never get to here because we used true when applyIpAssociations", e);
764+
} else if (ip.getState() == State.Releasing) {
765+
publicIpQuarantine = addPublicIpAddressToQuarantine(ipToBeDisassociated, caller.getDomainId());
766+
_ipAddressDao.unassignIpAddress(ip.getId());
757767
}
758-
} else if (ip.getState() == State.Releasing) {
759-
publicIpQuarantine = addPublicIpAddressToQuarantine(ipToBeDisassociated, caller.getDomainId());
760-
_ipAddressDao.unassignIpAddress(ip.getId());
761-
}
762768

763-
annotationDao.removeByEntityType(AnnotationService.EntityType.PUBLIC_IP_ADDRESS.name(), ip.getUuid());
769+
annotationDao.removeByEntityType(AnnotationService.EntityType.PUBLIC_IP_ADDRESS.name(), ip.getUuid());
764770

765-
if (success) {
766-
if (ip.isPortable()) {
767-
releasePortableIpAddress(addrId);
771+
if (success) {
772+
if (ip.isPortable()) {
773+
releasePortableIpAddress(addrId);
774+
}
775+
s_logger.debug("Released a public ip id=" + addrId);
776+
} else if (publicIpQuarantine != null) {
777+
removePublicIpAddressFromQuarantine(publicIpQuarantine.getId(), "Public IP address removed from quarantine as there was an error while disassociating it.");
768778
}
769-
s_logger.debug("Released a public ip id=" + addrId);
770-
} else if (publicIpQuarantine != null) {
771-
removePublicIpAddressFromQuarantine(publicIpQuarantine.getId(), "Public IP address removed from quarantine as there was an error while disassociating it.");
779+
} finally {
780+
_ipAddressDao.releaseFromLockTable(addrId);
772781
}
773782

774783
return success;

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1612,6 +1612,10 @@ public boolean checkIpForService(IpAddress userIp, Service service, Long network
16121612
}
16131613

16141614
NetworkVO network = _networksDao.findById(networkId);
1615+
if (network == null) {
1616+
throw new CloudRuntimeException("Could not find network associated with public IP.");
1617+
}
1618+
16151619
NetworkOfferingVO offering = _networkOfferingDao.findById(network.getNetworkOfferingId());
16161620
if (offering.getGuestType() != GuestType.Isolated) {
16171621
return true;

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

Lines changed: 53 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -194,57 +194,54 @@ public FirewallRule createIngressFirewallRule(FirewallRule rule) throws NetworkR
194194
return createFirewallRule(sourceIpAddressId, caller, rule.getXid(), rule.getSourcePortStart(), rule.getSourcePortEnd(), rule.getProtocol(),
195195
rule.getSourceCidrList(), null, rule.getIcmpCode(), rule.getIcmpType(), null, rule.getType(), rule.getNetworkId(), rule.getTrafficType(), rule.isDisplay());
196196
}
197+
197198
//Destination CIDR capability is currently implemented for egress rules only. For others, the field is passed as null.
198199
@DB
199-
protected FirewallRule createFirewallRule(final Long ipAddrId, Account caller, final String xId, final Integer portStart, final Integer portEnd,
200-
final String protocol, final List<String> sourceCidrList, final List<String> destCidrList, final Integer icmpCode, final Integer icmpType, final Long relatedRuleId,
201-
final FirewallRule.FirewallRuleType type,
202-
final Long networkId, final FirewallRule.TrafficType trafficType, final Boolean forDisplay) throws NetworkRuleConflictException {
203-
200+
protected FirewallRule createFirewallRule(final Long ipAddrId, Account caller, final String xId, final Integer portStart, final Integer portEnd, final String protocol,
201+
final List<String> sourceCidrList, final List<String> destCidrList, final Integer icmpCode, final Integer icmpType, final Long relatedRuleId,
202+
final FirewallRule.FirewallRuleType type, final Long networkId, final FirewallRule.TrafficType trafficType, final Boolean forDisplay) throws NetworkRuleConflictException {
204203
IPAddressVO ipAddress = null;
205-
if (ipAddrId != null) {
206-
// this for ingress firewall rule, for egress id is null
207-
ipAddress = _ipAddressDao.findById(ipAddrId);
208-
// Validate ip address
209-
if (ipAddress == null && type == FirewallRule.FirewallRuleType.User) {
210-
throw new InvalidParameterValueException("Unable to create firewall rule; " + "couldn't locate IP address by id in the system");
211-
}
212-
_networkModel.checkIpForService(ipAddress, Service.Firewall, null);
213-
}
204+
try {
205+
// Validate ip address
206+
if (ipAddrId != null) {
207+
// this for ingress firewall rule, for egress id is null
208+
ipAddress = _ipAddressDao.acquireInLockTable(ipAddrId);
209+
if (ipAddress == null) {
210+
throw new InvalidParameterValueException("Unable to create firewall rule; " + "couldn't locate IP address by id in the system");
211+
}
212+
_networkModel.checkIpForService(ipAddress, Service.Firewall, null);
213+
}
214214

215-
validateFirewallRule(caller, ipAddress, portStart, portEnd, protocol, Purpose.Firewall, type, networkId, trafficType);
215+
validateFirewallRule(caller, ipAddress, portStart, portEnd, protocol, Purpose.Firewall, type, networkId, trafficType);
216216

217-
// icmp code and icmp type can't be passed in for any other protocol rather than icmp
218-
if (!protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO) && (icmpCode != null || icmpType != null)) {
219-
throw new InvalidParameterValueException("Can specify icmpCode and icmpType for ICMP protocol only");
220-
}
217+
// icmp code and icmp type can't be passed in for any other protocol rather than icmp
218+
if (!protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO) && (icmpCode != null || icmpType != null)) {
219+
throw new InvalidParameterValueException("Can specify icmpCode and icmpType for ICMP protocol only");
220+
}
221221

222-
if (protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO) && (portStart != null || portEnd != null)) {
223-
throw new InvalidParameterValueException("Can't specify start/end port when protocol is ICMP");
224-
}
222+
if (protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO) && (portStart != null || portEnd != null)) {
223+
throw new InvalidParameterValueException("Can't specify start/end port when protocol is ICMP");
224+
}
225225

226-
Long accountId = null;
227-
Long domainId = null;
226+
Long accountId = null;
227+
Long domainId = null;
228228

229-
if (ipAddress != null) {
230-
//Ingress firewall rule
231-
accountId = ipAddress.getAllocatedToAccountId();
232-
domainId = ipAddress.getAllocatedInDomainId();
233-
} else if (networkId != null) {
234-
//egress firewall rule
229+
if (ipAddress != null) {
230+
//Ingress firewall rule
231+
accountId = ipAddress.getAllocatedToAccountId();
232+
domainId = ipAddress.getAllocatedInDomainId();
233+
} else if (networkId != null) {
234+
//egress firewall rule
235235
Network network = _networkModel.getNetwork(networkId);
236236
accountId = network.getAccountId();
237237
domainId = network.getDomainId();
238-
}
238+
}
239239

240-
final Long accountIdFinal = accountId;
241-
final Long domainIdFinal = domainId;
242-
return Transaction.execute(new TransactionCallbackWithException<FirewallRuleVO, NetworkRuleConflictException>() {
243-
@Override
244-
public FirewallRuleVO doInTransaction(TransactionStatus status) throws NetworkRuleConflictException {
245-
FirewallRuleVO newRule =
246-
new FirewallRuleVO(xId, ipAddrId, portStart, portEnd, protocol.toLowerCase(), networkId, accountIdFinal, domainIdFinal, Purpose.Firewall,
247-
sourceCidrList, destCidrList, icmpCode, icmpType, relatedRuleId, trafficType);
240+
final Long accountIdFinal = accountId;
241+
final Long domainIdFinal = domainId;
242+
return Transaction.execute((TransactionCallbackWithException<FirewallRuleVO, NetworkRuleConflictException>) status -> {
243+
FirewallRuleVO newRule = new FirewallRuleVO(xId, ipAddrId, portStart, portEnd, protocol.toLowerCase(), networkId, accountIdFinal, domainIdFinal, Purpose.Firewall,
244+
sourceCidrList, destCidrList, icmpCode, icmpType, relatedRuleId, trafficType);
248245
newRule.setType(type);
249246
if (forDisplay != null) {
250247
newRule.setDisplay(forDisplay);
@@ -261,8 +258,12 @@ public FirewallRuleVO doInTransaction(TransactionStatus status) throws NetworkRu
261258
CallContext.current().putContextParameter(FirewallRule.class, newRule.getId());
262259

263260
return newRule;
261+
});
262+
} finally {
263+
if (ipAddrId != null) {
264+
_ipAddressDao.releaseFromLockTable(ipAddrId);
264265
}
265-
});
266+
}
266267
}
267268

268269
@Override
@@ -668,9 +669,19 @@ public boolean applyIngressFwRules(long ipId, Account caller) throws ResourceUna
668669
}
669670

670671
@Override
672+
@DB
671673
public boolean applyIngressFirewallRules(long ipId, Account caller) throws ResourceUnavailableException {
672-
List<FirewallRuleVO> rules = _firewallDao.listByIpAndPurpose(ipId, Purpose.Firewall);
673-
return applyFirewallRules(rules, false, caller);
674+
try {
675+
IPAddressVO ipAddress = _ipAddressDao.acquireInLockTable(ipId);
676+
if (ipAddress == null) {
677+
s_logger.error(String.format("Unable to acquire lock for public IP [%s].", ipId));
678+
throw new CloudRuntimeException("Unable to acquire lock for public IP.");
679+
}
680+
List<FirewallRuleVO> rules = _firewallDao.listByIpAndPurpose(ipId, Purpose.Firewall);
681+
return applyFirewallRules(rules, false, caller);
682+
} finally {
683+
_ipAddressDao.releaseFromLockTable(ipId);
684+
}
674685
}
675686

676687
@Override

0 commit comments

Comments
 (0)