Fix native VPN lifecycle ownership and endpoint reservation

Keep gateway provider identity through partial cleanup and recheck the IP allocation before release, connection operations and polling. Preserve the prior lifecycle and reserved-endpoint fixes.

Strengthen validation, provider-selection and route/NAT payload tests; exercise customer-gateway updates through the public API and control retry waits in tests.

Author: Dogface2k

api/src/main/java/com/cloud/network/NetworkModel.java

@@ -219,6 +219,8 @@ public interface NetworkModel {
 
     boolean checkIpForService(IpAddress ip, Service service, Long networkId);
 
+    boolean isIpAddressReserved(IpAddress ip);
+
     boolean providerSupportsCapability(Set<Provider> providers, Service service, Capability cap);
 
     void checkCapabilityForProvider(Set<Provider> providers, Service service, Capability cap, String capValue);

api/src/main/java/com/cloud/network/element/NetworkElement.java

@@ -96,6 +96,15 @@ default boolean releaseIp(IpAddress ipAddress) {
         return true;
     }
 
+    /**
+     * Whether this provider reserves an address for its own endpoint. Such an address must not
+     * be released or reassigned by ordinary public-IP operations, including while creation or
+     * cleanup is being retried. The owning provider remains responsible for releasing it.
+     */
+    default boolean isIpAddressReserved(IpAddress ipAddress) {
+        return false;
+    }
+
     /**
      * The network is being shutdown.
      * @param network

api/src/main/java/com/cloud/network/element/Site2SiteVpnServiceProvider.java

@@ -26,6 +26,9 @@
 import com.cloud.utils.component.Adapter;
 
 public interface Site2SiteVpnServiceProvider extends Adapter {
+    // Gateway-scoped identity survives release and reuse of its public IP allocation.
+    String VPN_GATEWAY_PROVIDER_DETAIL = "vpnProvider";
+
     default void validateSite2SiteVpnCustomerGateway(Site2SiteCustomerGateway customerGateway) {
     }
 
@@ -64,9 +67,9 @@ default void releaseVpnGatewayIp(Site2SiteVpnGateway gateway) {
     }
 
     /**
-     * Identifies a gateway previously owned by this provider. This is used during teardown when
-     * an offering has been edited since the gateway was created and the current service map no
-     * longer advertises the provider.
+     * Identifies the gateway resources currently owned by this provider, independent of the
+     * current offering. A recorded gateway provider retains teardown ownership after its public
+     * IP has been released; this live claim must not adopt a later allocation of the same IP.
      */
     default boolean ownsVpnGateway(Site2SiteVpnGateway gateway) {
         return false;

plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java

@@ -274,12 +274,14 @@ private Answer executeRequest(CreateOrUpdateNsxTier1NatRuleCommand cmd) {
         String translatedIpAddress = cmd.getTranslatedIpAddress();
         String natRuleId = cmd.getNatRuleId();
         String natId = "USER";
-        try {
-            nsxApiClient.createTier1NatRule(tier1GatewayName, natId, natRuleId, action, translatedIpAddress);
-        } catch (CloudRuntimeException e) {
-            String msg = String.format("Error creating the NAT rule with ID %s on Tier1 Gateway %s: %s", natRuleId, tier1GatewayName, e.getMessage());
-            logger.error(msg, e);
-            return new NsxAnswer(cmd, e);
+        synchronized (getVpnTier1Lock(tier1GatewayName)) {
+            try {
+                nsxApiClient.createTier1NatRule(tier1GatewayName, natId, natRuleId, action, translatedIpAddress);
+            } catch (CloudRuntimeException e) {
+                String msg = String.format("Error creating the NAT rule with ID %s on Tier1 Gateway %s: %s", natRuleId, tier1GatewayName, e.getMessage());
+                logger.error(msg, e);
+                return new NsxAnswer(cmd, e);
+            }
         }
         return new NsxAnswer(cmd, true, "");
     }
@@ -532,7 +534,11 @@ private NsxAnswer executeRequest(CreateNsxVpnGatewayCommand cmd) {
             } catch (Exception e) {
                 logger.error(String.format("Failed to check the existing NSX VPN service on tier-1 gateway %s before creation: %s",
                         tier1GatewayName, e.getMessage()));
-                return new NsxAnswer(cmd, new CloudRuntimeException(e.getMessage()));
+                NsxAnswer answer = new NsxAnswer(cmd, new CloudRuntimeException(e.getMessage()));
+                // A retry may own an endpoint left by an earlier ambiguous failure. A failed
+                // read cannot prove that endpoint is gone. Fresh attempts have not mutated NSX.
+                answer.setEndpointMayBeInUse(cmd.isReconcileExistingService());
+                return answer;
             }
             if (vpnServiceExisted && !cmd.isReconcileExistingService()) {
                 NsxAnswer answer = new NsxAnswer(cmd, new CloudRuntimeException(String.format(

plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java

@@ -300,10 +300,16 @@ public boolean isNsxControllerActive() {
     public void createTier1NatRule(String tier1GatewayName, String natId, String natRuleId,
                                    String action, String translatedIp) {
         NatRules natRulesService = (NatRules) nsxService.apply(NatRules.class);
-        PolicyNatRule natPolicy = new PolicyNatRule.Builder()
-                .setAction(action)
-                .setTranslatedNetwork(translatedIp)
-                .build();
+        PolicyNatRule natPolicy;
+        try {
+            natPolicy = natRulesService.get(tier1GatewayName, natId, natRuleId);
+        } catch (NotFound e) {
+            natPolicy = new PolicyNatRule.Builder().build();
+        }
+        // Policy PATCH replaces omitted fields unless partial PATCH is explicitly enabled.
+        // Preserve VPN ordering, ownership tags and the rest of an existing rule on IP updates.
+        natPolicy.setAction(action);
+        natPolicy.setTranslatedNetwork(translatedIp);
         natRulesService.patch(tier1GatewayName, natId, natRuleId, natPolicy);
     }
 
@@ -353,8 +359,9 @@ private void demoteCatchAllSourceNatRule(String tier1GatewayName) {
             }
             logger.debug("Moving CloudStack source NAT rule {} on tier-1 gateway {} behind VPN no-SNAT rules",
                     ruleId, tier1GatewayName);
-            natRulesService.patch(tier1GatewayName, NatId.USER.name(), ruleId,
-                    copyNatRule(natRule, CATCH_ALL_NAT_SEQUENCE_NUMBER, tags));
+            natRule.setSequenceNumber(CATCH_ALL_NAT_SEQUENCE_NUMBER);
+            natRule.setTags(tags);
+            natRulesService.patch(tier1GatewayName, NatId.USER.name(), ruleId, natRule);
         } catch (NotFound e) {
             logger.debug("CloudStack source NAT rule is absent on tier-1 gateway {}; no VPN NAT ordering change is required",
                     tier1GatewayName);
@@ -385,8 +392,9 @@ void restoreSourceNatRuleSequence(String tier1GatewayName) {
                         originalSequenceTag.getTag(), tier1GatewayName), e);
             }
             tags.remove(originalSequenceTag);
-            natRulesService.patch(tier1GatewayName, NatId.USER.name(), ruleId,
-                    copyNatRule(natRule, originalSequence, tags));
+            natRule.setSequenceNumber(originalSequence);
+            natRule.setTags(tags);
+            natRulesService.patch(tier1GatewayName, NatId.USER.name(), ruleId, natRule);
         } catch (NotFound e) {
             logger.debug("CloudStack source NAT rule is absent on tier-1 gateway {}; no sequence restoration is required",
                     tier1GatewayName);
@@ -413,27 +421,6 @@ private Tag findOriginalSnatSequenceTag(List<Tag> tags) {
                 .orElse(null);
     }
 
-    private PolicyNatRule copyNatRule(PolicyNatRule natRule, long sequenceNumber, List<Tag> tags) {
-        return new PolicyNatRule.Builder()
-                .setId(natRule.getId())
-                .setDisplayName(natRule.getDisplayName())
-                .setDescription(natRule.getDescription())
-                .setAction(natRule.getAction())
-                .setTranslatedNetwork(natRule.getTranslatedNetwork())
-                .setTranslatedPorts(natRule.getTranslatedPorts())
-                .setSourceNetwork(natRule.getSourceNetwork())
-                .setDestinationNetwork(natRule.getDestinationNetwork())
-                .setService(natRule.getService())
-                .setScope(natRule.getScope())
-                .setFirewallMatch(natRule.getFirewallMatch())
-                .setPolicyBasedVpnMode(natRule.getPolicyBasedVpnMode())
-                .setLogging(natRule.getLogging())
-                .setEnabled(natRule.getEnabled())
-                .setTags(tags)
-                .setSequenceNumber(sequenceNumber)
-                .build();
-    }
-
     private boolean isCatchAllSourceNatRule(PolicyNatRule natRule) {
         return NatAction.SNAT.name().equals(natRule.getAction())
                 && isAnyNatMatch(natRule.getSourceNetwork())
@@ -1515,10 +1502,9 @@ private void ensureTier1IpsecLocalEndpointAdvertisement(String tier1GatewayName)
         }
         updatedTypes.add(RouteAdvertisementType.TIER1_IPSEC_LOCAL_ENDPOINT.name());
         Tier1s tier1service = (Tier1s) nsxService.apply(Tier1s.class);
-        Tier1 tier1Update = new Tier1.Builder()
-                .setRouteAdvertisementTypes(updatedTypes)
-                .build();
-        tier1service.patch(tier1GatewayName, tier1Update);
+        // Retain the Tier-0 attachment and all other router settings under full PATCH semantics.
+        tier1.setRouteAdvertisementTypes(updatedTypes);
+        tier1service.patch(tier1GatewayName, tier1);
     }
 
     public void deleteVpnService(String tier1GatewayName) {
@@ -1560,7 +1546,7 @@ private <T> T retryWhileMarkedForDeletion(long connectionId, Supplier<T> operati
                 logger.info("A VPN object for connection {} is still being purged by NSX, retrying in {}s (attempt {}/{})",
                         connectionId, VPN_MARKED_FOR_DELETION_RETRY_INTERVAL_SECS, attempt, VPN_MARKED_FOR_DELETION_RETRIES);
                 try {
-                    Thread.sleep(VPN_MARKED_FOR_DELETION_RETRY_INTERVAL_SECS * 1000L);
+                    waitBeforeVpnRetry(VPN_MARKED_FOR_DELETION_RETRY_INTERVAL_SECS * 1000L);
                 } catch (InterruptedException ie) {
                     Thread.currentThread().interrupt();
                     throw e;
@@ -1570,6 +1556,10 @@ private <T> T retryWhileMarkedForDeletion(long connectionId, Supplier<T> operati
         throw lastFailure;
     }
 
+    void waitBeforeVpnRetry(long milliseconds) throws InterruptedException {
+        Thread.sleep(milliseconds);
+    }
+
     private boolean isMarkedForDeletionError(CloudRuntimeException e) {
         return e.getMessage() != null && e.getMessage().contains("marked for deletion");
     }