NSX: Fix concurrency issues on port forwarding rules deletion (#37)

nvazquez · Pearl1594 · commit f228c7afe6c5 · 2024-04-12T10:08:06.000-04:00
* Fix concurrency issues on port forwarding rules deletion

* Refactor objectExists

* Fix unit test

* Fix test

* Small fixes
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
@@ -79,4 +79,8 @@ public long getResourceId() {
     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
@@ -20,6 +20,9 @@
 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 NsxAnswer(final Command command, final Exception e) {
         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
@@ -385,16 +385,21 @@ private NsxAnswer executeRequest(CreateNsxPortForwardRuleCommand cmd) {
                 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 @@ private NsxAnswer executeRequest(DeleteNsxNatRuleCommand cmd) {
             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
@@ -84,6 +84,7 @@
 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 void createStaticNatRule(String vpcName, String tier1GatewayName,
         }
     }
 
+    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 boolean doesPfRuleExist(String ruleName, String tier1GatewayName) {
         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
@@ -91,14 +91,18 @@
 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;
 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.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 boolean applyStaticNats(Network config, List<? extends StaticNat> rules)
         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
@@ -139,14 +139,13 @@ public boolean deleteStaticNatRule(long zoneId, long domainId, long accountId, L
         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
@@ -61,6 +61,7 @@
 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 void setup() throws NoSuchFieldException, IllegalAccessException {
         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 void testApplyPFRules_delete() throws ResourceUnavailableException {
         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)));
     }
 

Original file line number	Diff line number	Diff line change
`@@ -79,4 +79,8 @@ public long getResourceId() {`
`79`	`79`	`public boolean isDisplay() {`
`80`	`80`	`return display;`
`81`	`81`	`}`
	`82`	`+`
	`83`	`+ public void setValue(String value) {`
	`84`	`+ this.value = value;`
	`85`	`+ }`
`82`	`86`	`}`
Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,9 @@`
`20`	`20`	`import com.cloud.agent.api.Command;`
`21`	`21`
`22`	`22`	`public class NsxAnswer extends Answer {`
	`23`	`+`
	`24`	`+ private boolean objectExists;`
	`25`	`+`
`23`	`26`	`public NsxAnswer(final Command command, final boolean success, final String details) {`
`24`	`27`	`super(command, success, details);`
`25`	`28`	`}`
`@@ -28,4 +31,11 @@ public NsxAnswer(final Command command, final Exception e) {`
`28`	`31`	`super(command, e);`
`29`	`32`	`}`
`30`	`33`
	`34`	`+ public boolean isObjectExistent() {`
	`35`	`+ return objectExists;`
	`36`	`+ }`
	`37`	`+`
	`38`	`+ public void setObjectExists(boolean objectExisted) {`
	`39`	`+ this.objectExists = objectExisted;`
	`40`	`+ }`
`31`	`41`	`}`