This is an automated email from the ASF dual-hosted git repository.
pearl11594 pushed a commit to branch nsx-integration-fixes
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit f228c7afe6c5ddde4a6a75b31cf3c0c3e0ee324f
Author: Nicolas Vazquez <nicovazque...@gmail.com>
AuthorDate: Wed Apr 10 11:05:50 2024 -0300
NSX: Fix concurrency issues on port forwarding rules deletion (#37)
* Fix concurrency issues on port forwarding rules deletion
* Refactor objectExists
* Fix unit test
* Fix test
* Small fixes
---
.../resourcedetail/FirewallRuleDetailVO.java | 4 +
.../main/java/org/apache/cloudstack/NsxAnswer.java | 10 ++
.../apache/cloudstack/resource/NsxResource.java | 20 ++--
.../apache/cloudstack/service/NsxApiClient.java | 47 ++++++---
.../org/apache/cloudstack/service/NsxElement.java | 108 ++++++++++++++-------
.../apache/cloudstack/service/NsxServiceImpl.java | 5 +-
.../apache/cloudstack/service/NsxElementTest.java | 5 +-
7 files changed, 140 insertions(+), 59 deletions(-)
diff --git
a/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/FirewallRuleDetailVO.java
b/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/FirewallRuleDetailVO.java
index 636d889fafe..1149d0b13e7 100644
---
a/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/FirewallRuleDetailVO.java
+++
b/engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/FirewallRuleDetailVO.java
@@ -79,4 +79,8 @@ public class FirewallRuleDetailVO implements ResourceDetail {
public boolean isDisplay() {
return display;
}
+
+ public void setValue(String value) {
+ this.value = value;
+ }
}
diff --git
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/NsxAnswer.java
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/NsxAnswer.java
index 0820465a6b6..a667adda794 100644
---
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/NsxAnswer.java
+++
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/NsxAnswer.java
@@ -20,6 +20,9 @@ import com.cloud.agent.api.Answer;
import com.cloud.agent.api.Command;
public class NsxAnswer extends Answer {
+
+ private boolean objectExists;
+
public NsxAnswer(final Command command, final boolean success, final
String details) {
super(command, success, details);
}
@@ -28,4 +31,11 @@ public class NsxAnswer extends Answer {
super(command, e);
}
+ public boolean isObjectExistent() {
+ return objectExists;
+ }
+
+ public void setObjectExists(boolean objectExisted) {
+ this.objectExists = objectExisted;
+ }
}
diff --git
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java
index cd1d481b9f8..42ee24436ea 100644
---
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java
+++
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java
@@ -385,16 +385,21 @@ public class NsxResource implements ServerResource {
cmd.getNetworkResourceId(), cmd.isResourceVpc());
try {
String privatePort = cmd.getPrivatePort();
- String service = privatePort.contains("-") ?
nsxApiClient.getServicePath(ruleName, privatePort, cmd.getProtocol(), null,
null) :
- nsxApiClient.getNsxInfraServices(ruleName, privatePort,
cmd.getProtocol(), null, null);
+ LOGGER.debug(String.format("Checking if rule %s exists on Tier 1
Gateway: %s", ruleName, tier1GatewayName));
if (nsxApiClient.doesPfRuleExist(ruleName, tier1GatewayName)) {
- logger.debug(String.format("Port forward rule for port: %s
exits on NSX, not adding it again", privatePort));
- return new NsxAnswer(cmd, true, null);
+ String msg = String.format("Port forward rule for port: %s
(%s) exits on NSX, not adding it again", ruleName, privatePort);
+ LOGGER.debug(msg);
+ NsxAnswer answer = new NsxAnswer(cmd, true, msg);
+ answer.setObjectExists(true);
+ return answer;
}
+ String service = privatePort.contains("-") ?
nsxApiClient.getServicePath(ruleName, privatePort, cmd.getProtocol(), null,
null) :
+ nsxApiClient.getNsxInfraServices(ruleName, privatePort,
cmd.getProtocol(), null, null);
nsxApiClient.createPortForwardingRule(ruleName, tier1GatewayName,
cmd.getNetworkResourceName(), cmd.getPublicIp(),
cmd.getVmIp(), cmd.getPublicPort(), service);
} catch (Exception e) {
- logger.error(String.format("Failed to add NSX port forward rule %s
for network: %s", ruleName, cmd.getNetworkResourceName()));
+ String msg = String.format("Failed to add NSX port forward rule %s
for network: %s", ruleName, cmd.getNetworkResourceName());
+ LOGGER.error(msg, e);
return new NsxAnswer(cmd, new
CloudRuntimeException(e.getMessage()));
}
return new NsxAnswer(cmd, true, null);
@@ -415,8 +420,9 @@ public class NsxResource implements ServerResource {
nsxApiClient.deleteNatRule(cmd.getService(), cmd.getPrivatePort(),
cmd.getProtocol(),
cmd.getNetworkResourceName(), tier1GatewayName, ruleName);
} catch (Exception e) {
- logger.error(String.format("Failed to add NSX static NAT rule %s
for network: %s", ruleName, cmd.getNetworkResourceName()));
- return new NsxAnswer(cmd, new
CloudRuntimeException(e.getMessage()));
+ String msg = String.format("Failed to delete NSX rule %s for
network %s: due to %s", ruleName, cmd.getNetworkResourceName(), e.getMessage());
+ LOGGER.error(msg, e);
+ return new NsxAnswer(cmd, new CloudRuntimeException(msg));
}
return new NsxAnswer(cmd, true, null);
}
diff --git
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java
index d443b0e14e7..b814af686ac 100644
---
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java
+++
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java
@@ -84,6 +84,7 @@ import org.apache.cloudstack.utils.NsxControllerUtils;
import org.apache.commons.collections.CollectionUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
+import org.apache.commons.lang3.BooleanUtils;
import java.util.ArrayList;
import java.util.List;
@@ -526,24 +527,37 @@ public class NsxApiClient {
}
}
+ protected void deletePortForwardingNatRuleService(String ruleName, String
privatePort, String protocol) {
+ String svcName = getServiceName(ruleName, privatePort, protocol, null,
null);
+ try {
+ Services services = (Services) nsxService.apply(Services.class);
+ com.vmware.nsx_policy.model.Service servicePFRule =
services.get(svcName);
+ if (servicePFRule != null && !servicePFRule.getMarkedForDelete()
&& !BooleanUtils.toBoolean(servicePFRule.getIsDefault())) {
+ services.delete(svcName);
+ }
+ } catch (Error error) {
+ String msg = String.format("Cannot find service %s associated to
rule %s, skipping its deletion: %s",
+ svcName, ruleName, error.getMessage());
+ logger.debug(msg);
+ }
+ }
+
public void deleteNatRule(Network.Service service, String privatePort,
String protocol, String networkName, String tier1GatewayName, String ruleName) {
try {
NatRules natService = (NatRules) nsxService.apply(NatRules.class);
- logger.debug(String.format("Deleting NSX static NAT rule %s for
tier-1 gateway %s (network: %s)", ruleName, tier1GatewayName, networkName));
- // delete NAT rule
- natService.delete(tier1GatewayName, NatId.USER.name(), ruleName);
- if (service == Network.Service.PortForwarding) {
- String svcName = getServiceName(ruleName, privatePort,
protocol, null, null);
- // Delete service
- Services services = (Services)
nsxService.apply(Services.class);
- services.delete(svcName);
+ logger.debug(String.format("Deleting NSX NAT rule %s for tier-1
gateway %s (network: %s)", ruleName, tier1GatewayName, networkName));
+ PolicyNatRule natRule = natService.get(tier1GatewayName,
NatId.USER.name(), ruleName);
+ if (natRule != null && !natRule.getMarkedForDelete()) {
+ logger.debug(String.format("Deleting rule %s from Tier 1
Gateway %s", ruleName, tier1GatewayName));
+ natService.delete(tier1GatewayName, NatId.USER.name(),
ruleName);
}
} catch (Error error) {
- ApiError ae = error.getData()._convertTo(ApiError.class);
- String msg = String.format("Failed to delete NSX Static NAT rule
%s for tier-1 gateway %s (VPC: %s), due to %s",
- ruleName, tier1GatewayName, networkName,
ae.getErrorMessage());
- logger.error(msg);
- throw new CloudRuntimeException(msg);
+ String msg = String.format("Cannot find NAT rule with name %s: %s,
skipping deletion", ruleName, error.getMessage());
+ logger.debug(msg);
+ }
+
+ if (service == Network.Service.PortForwarding) {
+ deletePortForwardingNatRuleService(ruleName, privatePort,
protocol);
}
}
@@ -577,9 +591,14 @@ public class NsxApiClient {
try {
NatRules natService = (NatRules) nsxService.apply(NatRules.class);
PolicyNatRule rule = natService.get(tier1GatewayName, NAT_ID,
ruleName);
+ logger.debug(String.format("Rule %s from Tier 1 GW %s: %s",
ruleName, tier1GatewayName,
+ rule == null ? "null" : rule.getId() + " " +
rule.getPath()));
return !Objects.isNull(rule);
} catch (Error error) {
- logger.debug(String.format("Found a port forward rule named: %s on
NSX", ruleName));
+ String msg = String.format("Error checking if port forwarding rule
%s exists on Tier 1 Gateway %s: %s",
+ ruleName, tier1GatewayName, error.getMessage());
+ Throwable throwable = error.getCause();
+ logger.error(msg, throwable);
return false;
}
}
diff --git
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxElement.java
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxElement.java
index d09049700e5..f3f4b1c135b 100644
---
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxElement.java
+++
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxElement.java
@@ -91,6 +91,8 @@ import com.cloud.utils.Pair;
import com.cloud.utils.component.AdapterBase;
import com.cloud.utils.db.QueryBuilder;
import com.cloud.utils.db.SearchCriteria;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.vm.NicProfile;
import com.cloud.vm.ReservationContext;
@@ -98,7 +100,9 @@ import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachineProfile;
import com.cloud.vm.dao.VMInstanceDao;
import net.sf.ehcache.config.InvalidConfigurationException;
+import org.apache.cloudstack.NsxAnswer;
import org.apache.cloudstack.StartupNsxCommand;
+import org.apache.cloudstack.api.ApiConstants;
import
org.apache.cloudstack.api.command.admin.internallb.ConfigureInternalLoadBalancerElementCmd;
import
org.apache.cloudstack.api.command.admin.internallb.CreateInternalLoadBalancerElementCmd;
import
org.apache.cloudstack.api.command.admin.internallb.ListInternalLoadBalancerElementsCmd;
@@ -108,6 +112,8 @@ import org.apache.cloudstack.resource.NsxNetworkRule;
import org.apache.cloudstack.resource.NsxOpObject;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
+import org.apache.cloudstack.resourcedetail.FirewallRuleDetailVO;
+import org.apache.cloudstack.resourcedetail.dao.FirewallRuleDetailsDao;
import org.springframework.stereotype.Component;
import javax.inject.Inject;
@@ -160,6 +166,8 @@ public class NsxElement extends AdapterBase implements
DhcpServiceProvider, Dns
VirtualRouterProviderDao vrProviderDao;
@Inject
PhysicalNetworkServiceProviderDao pNtwkSvcProviderDao;
+ @Inject
+ FirewallRuleDetailsDao firewallRuleDetailsDao;
protected Logger logger = LogManager.getLogger(getClass());
@@ -527,45 +535,77 @@ public class NsxElement extends AdapterBase implements
DhcpServiceProvider, Dns
return false;
}
+ protected synchronized boolean applyPFRulesInternal(Network network,
List<PortForwardingRule> rules) {
+ return Transaction.execute((TransactionCallback<Boolean>) status -> {
+ boolean result = true;
+ for (PortForwardingRule rule : rules) {
+ IPAddressVO publicIp =
ApiDBUtils.findIpAddressById(rule.getSourceIpAddressId());
+ UserVm vm =
ApiDBUtils.findUserVmById(rule.getVirtualMachineId());
+ if (vm == null && rule.getState() !=
FirewallRule.State.Revoke) {
+ continue;
+ }
+ NsxOpObject nsxObject = getNsxOpObject(network);
+ String publicPort = getPublicPortRange(rule);
+
+ String privatePort = getPrivatePFPortRange(rule);
+
+ NsxNetworkRule networkRule = new NsxNetworkRule.Builder()
+ .setDomainId(nsxObject.getDomainId())
+ .setAccountId(nsxObject.getAccountId())
+ .setZoneId(nsxObject.getZoneId())
+ .setNetworkResourceId(nsxObject.getNetworkResourceId())
+
.setNetworkResourceName(nsxObject.getNetworkResourceName())
+ .setVpcResource(nsxObject.isVpcResource())
+ .setVmId(Objects.nonNull(vm) ? vm.getId() : 0)
+ .setVmIp(Objects.nonNull(vm) ?
vm.getPrivateIpAddress() : null)
+ .setPublicIp(publicIp.getAddress().addr())
+ .setPrivatePort(privatePort)
+ .setPublicPort(publicPort)
+ .setRuleId(rule.getId())
+
.setProtocol(rule.getProtocol().toUpperCase(Locale.ROOT))
+ .build();
+ FirewallRuleDetailVO ruleDetail =
firewallRuleDetailsDao.findDetail(rule.getId(), ApiConstants.FOR_NSX);
+ if (Arrays.asList(FirewallRule.State.Add,
FirewallRule.State.Active).contains(rule.getState())) {
+ if ((ruleDetail == null && FirewallRule.State.Add ==
rule.getState()) || (ruleDetail != null &&
!ruleDetail.getValue().equalsIgnoreCase("true"))) {
+ LOGGER.debug(String.format("Creating port forwarding
rule on NSX for VM %s to ports %s - %s",
+ vm.getUuid(), rule.getDestinationPortStart(),
rule.getDestinationPortEnd()));
+ NsxAnswer answer =
nsxService.createPortForwardRule(networkRule);
+ boolean pfRuleResult = answer.getResult();
+ if (pfRuleResult && !answer.isObjectExistent()) {
+ LOGGER.debug(String.format("Port forwarding rule
%s created on NSX, adding detail on firewall rules details", rule.getId()));
+ if (ruleDetail == null && FirewallRule.State.Add
== rule.getState()) {
+ LOGGER.debug(String.format("Adding new
firewall detail for rule %s", rule.getId()));
+ firewallRuleDetailsDao.addDetail(rule.getId(),
ApiConstants.FOR_NSX, "true", false);
+ } else {
+ LOGGER.debug(String.format("Updating firewall
detail for rule %s", rule.getId()));
+ ruleDetail.setValue("true");
+
firewallRuleDetailsDao.update(ruleDetail.getId(), ruleDetail);
+ }
+ }
+ result &= pfRuleResult;
+ }
+ } else if (rule.getState() == FirewallRule.State.Revoke) {
+ if (ruleDetail != null &&
ruleDetail.getValue().equalsIgnoreCase("true")) {
+ boolean pfRuleResult =
nsxService.deletePortForwardRule(networkRule);
+ if (pfRuleResult) {
+ LOGGER.debug(String.format("Updating firewall rule
detail %s for rule %s, set to false", ruleDetail.getId(), rule.getId()));
+ ruleDetail.setValue("false");
+ firewallRuleDetailsDao.update(ruleDetail.getId(),
ruleDetail);
+ }
+ result &= pfRuleResult;
+ }
+ }
+ }
+ return result;
+ });
+ }
+
@Override
public boolean applyPFRules(Network network, List<PortForwardingRule>
rules) throws ResourceUnavailableException {
if (!canHandle(network, Network.Service.PortForwarding)) {
return false;
}
- boolean result = true;
- for (PortForwardingRule rule : rules) {
- IPAddressVO publicIp =
ApiDBUtils.findIpAddressById(rule.getSourceIpAddressId());
- UserVm vm = ApiDBUtils.findUserVmById(rule.getVirtualMachineId());
- if (vm == null && rule.getState() != FirewallRule.State.Revoke) {
- continue;
- }
- NsxOpObject nsxObject = getNsxOpObject(network);
- String publicPort = getPublicPortRange(rule);
-
- String privatePort = getPrivatePFPortRange(rule);
-
- NsxNetworkRule networkRule = new NsxNetworkRule.Builder()
- .setDomainId(nsxObject.getDomainId())
- .setAccountId(nsxObject.getAccountId())
- .setZoneId(nsxObject.getZoneId())
- .setNetworkResourceId(nsxObject.getNetworkResourceId())
- .setNetworkResourceName(nsxObject.getNetworkResourceName())
- .setVpcResource(nsxObject.isVpcResource())
- .setVmId(Objects.nonNull(vm) ? vm.getId() : 0)
- .setVmIp(Objects.nonNull(vm) ? vm.getPrivateIpAddress() :
null)
- .setPublicIp(publicIp.getAddress().addr())
- .setPrivatePort(privatePort)
- .setPublicPort(publicPort)
- .setRuleId(rule.getId())
- .setProtocol(rule.getProtocol().toUpperCase(Locale.ROOT))
- .build();
- if (Arrays.asList(FirewallRule.State.Add,
FirewallRule.State.Active).contains(rule.getState())) {
- result &= nsxService.createPortForwardRule(networkRule);
- } else if (rule.getState() == FirewallRule.State.Revoke) {
- result &= nsxService.deletePortForwardRule(networkRule);
- }
- }
- return result;
+ return applyPFRulesInternal(network, rules);
}
public Pair<VpcVO, NetworkVO> getVpcOrNetwork(Long vpcId, long networkId) {
diff --git
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxServiceImpl.java
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxServiceImpl.java
index f8880826a3f..19f29ce62ea 100644
---
a/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxServiceImpl.java
+++
b/plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxServiceImpl.java
@@ -139,14 +139,13 @@ public class NsxServiceImpl implements NsxService {
return result.getResult();
}
- public boolean createPortForwardRule(NsxNetworkRule netRule) {
+ public NsxAnswer createPortForwardRule(NsxNetworkRule netRule) {
// TODO: if port doesn't exist in default list of services, create a
service entry
CreateNsxPortForwardRuleCommand createPortForwardCmd = new
CreateNsxPortForwardRuleCommand(netRule.getDomainId(),
netRule.getAccountId(), netRule.getZoneId(),
netRule.getNetworkResourceId(),
netRule.getNetworkResourceName(), netRule.isVpcResource(),
netRule.getVmId(), netRule.getRuleId(),
netRule.getPublicIp(), netRule.getVmIp(),
netRule.getPublicPort(), netRule.getPrivatePort(), netRule.getProtocol());
- NsxAnswer result =
nsxControllerUtils.sendNsxCommand(createPortForwardCmd, netRule.getZoneId());
- return result.getResult();
+ return nsxControllerUtils.sendNsxCommand(createPortForwardCmd,
netRule.getZoneId());
}
public boolean deletePortForwardRule(NsxNetworkRule netRule) {
diff --git
a/plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxElementTest.java
b/plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxElementTest.java
index ff7fa5427ee..59930d68d10 100644
---
a/plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxElementTest.java
+++
b/plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxElementTest.java
@@ -61,6 +61,7 @@ import com.cloud.vm.dao.UserVmDao;
import com.cloud.vm.dao.VMInstanceDao;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.resource.NsxNetworkRule;
+import org.apache.cloudstack.resourcedetail.dao.FirewallRuleDetailsDao;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
@@ -124,6 +125,8 @@ public class NsxElementTest {
private VpcOfferingServiceMapDao vpcOfferingServiceMapDao;
@Mock
LoadBalancerVMMapDao lbVmMapDao;
+ @Mock
+ FirewallRuleDetailsDao firewallRuleDetailsDao;
NsxElement nsxElement;
ReservationContext reservationContext;
@@ -148,6 +151,7 @@ public class NsxElementTest {
nsxElement.vmInstanceDao = vmInstanceDao;
nsxElement.vpcDao = vpcDao;
nsxElement.lbVmMapDao = lbVmMapDao;
+ nsxElement.firewallRuleDetailsDao = firewallRuleDetailsDao;
Field field = ApiDBUtils.class.getDeclaredField("s_ipAddressDao");
field.setAccessible(true);
@@ -279,7 +283,6 @@ public class NsxElementTest {
IPAddressVO ipAddress = new IPAddressVO(new Ip("10.1.13.10"), 1L, 1L,
1L,false);
when(ApiDBUtils.findIpAddressById(anyLong())).thenReturn(ipAddress);
when(nsxElement.canHandle(networkVO, service)).thenReturn(true);
-
when(nsxService.deletePortForwardRule(any(NsxNetworkRule.class))).thenReturn(true);
assertTrue(nsxElement.applyPFRules(networkVO, List.of(rule)));
}
