NSX: add native route-based Site-to-Site VPN support - #13766
Conversation
Follow-up validation and lifecycle hardeningCommit
Live validation on 4.22.1.0 with NSX 4.2.4 used an intentionally unreachable RFC 5737 test peer. The connection reached The supplied pre-deploy |
Additional live data-plane evidenceOperator-side testing between CloudStack connection 8 remained |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13766 +/- ##
============================================
+ Coverage 19.65% 19.80% +0.15%
- Complexity 19792 20051 +259
============================================
Files 6368 6377 +9
Lines 574881 576796 +1915
Branches 70351 70563 +212
============================================
+ Hits 112970 114247 +1277
- Misses 449639 450155 +516
- Partials 12272 12394 +122
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds native, route-based Site-to-Site VPN support for NSX NAT-mode VPCs by terminating IPsec on the NSX Tier-1 gateway (keeping the CloudStack virtual router out of the data path) and by making the existing Vpn/Nsx service mapping functional via provider-dispatched lifecycle handling.
Changes:
- Extends the Site-to-Site VPN provider SPI to support provider-owned gateway IP acquisition/release and persisted ownership-based provider selection.
- Implements NSX VPN gateway/connection lifecycle (gateway creation, route/NAT exemptions, session state updates, and teardown) plus a scheduled management-server poller mapping NSX session status back to CloudStack state.
- Adds extensive unit tests across
api,server, and NSX plugin modules to cover lifecycle hardening, rollback/ambiguity handling, and sensitive-data logging behavior.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/test/java/com/cloud/network/vpn/Site2SiteVpnManagerImplTest.java | Expands S2S manager tests for provider selection/ownership, locking, and lifecycle error handling. |
| server/src/test/java/com/cloud/network/vpn/RemoteAccessVpnManagerImplTest.java | Adds tests ensuring Remote Access VPN provider mapping is validated before persistence/start. |
| server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java | Routes S2S lifecycle through the owning provider, supports provider-owned gateway IPs, and hardens connection locking/state transitions. |
| server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java | Selects a single mapped Remote Access VPN provider and fails fast when none is available. |
| plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/utils/NsxVpnCryptoUtilsTest.java | Adds unit tests for CloudStack-to-NSX VPN crypto policy mapping/validation. |
| plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/utils/NsxHelperTest.java | Adds tests for deterministic VTI /30 address derivation. |
| plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/utils/NsxControllerUtilsTest.java | Tests failed-answer behavior for “throwing” vs “returning” NSX command helpers. |
| plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxServiceImplTest.java | Adds tests for status polling behavior and VPN gateway failure result propagation. |
| plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxElementTest.java | Adds tests for NSX gateway IP acquisition/release semantics and S2S VPN lifecycle dispatch. |
| plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxApiClientTest.java | Adds extensive tests for NSX Policy object ordering, rollback, retries, and cleanup behavior. |
| plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/resource/NsxResourceTest.java | Adds tests validating agent-side command dispatch, rollback semantics, and PSK log redaction on the wire/loggers. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/utils/NsxVpnCryptoUtils.java | Implements crypto parameter mapping/validation for NSX constraints. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/utils/NsxHelper.java | Adds deterministic VTI address allocation helper for NSX route-based VPN sessions. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/utils/NsxControllerUtils.java | Adds VPN naming helpers and splits “throw on failure” vs “return failed answer” command sending. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxServiceImpl.java | Adds NSX VPN lifecycle command dispatch and a scheduled poller to sync NSX session state to CloudStack. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxElement.java | Implements Site2SiteVpnServiceProvider for NSX, including gateway IP ownership markers and NSX-side lifecycle operations. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java | Adds agent handling for NSX VPN commands and introduces per-Tier1 locking for VPN mutations/teardown ordering. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/NsxAnswer.java | Extends NSX answer payload with structured “endpoint may be in use” failure signaling. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/agent/api/UpdateNsxVpnConnectionStateCommand.java | New agent command for enabling/disabling an NSX VPN session. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/agent/api/GetNsxVpnSessionStatusCommand.java | New agent command for fetching NSX session status. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/agent/api/DeleteNsxVpnGatewayCommand.java | New agent command for deleting an NSX VPN service on Tier-1. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/agent/api/DeleteNsxVpnConnectionCommand.java | New agent command for deleting an NSX VPN session. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/agent/api/CreateNsxVpnGatewayCommand.java | New agent command for creating an NSX VPN service/local endpoint on Tier-1. |
| plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/agent/api/CreateNsxVpnConnectionCommand.java | New agent command for creating a route-based VPN session, VTI, routes, and NAT exemptions (with PSK log redaction). |
| api/src/test/java/org/apache/cloudstack/api/command/user/vpn/VpnConnectionLifecycleCmdTest.java | Adds tests ensuring VPN reset/delete commands synchronize on VPC. |
| api/src/main/java/org/apache/cloudstack/api/command/user/vpn/ResetVpnConnectionCmd.java | Synchronizes reset jobs on VPC to serialize Tier-1/VPN mutations. |
| api/src/main/java/org/apache/cloudstack/api/command/user/vpn/DeleteVpnGatewayCmd.java | Synchronizes gateway deletion jobs on VPC. |
| api/src/main/java/org/apache/cloudstack/api/command/user/vpn/DeleteVpnConnectionCmd.java | Synchronizes connection deletion jobs on VPC. |
| api/src/main/java/com/cloud/network/nsx/NsxVpnGatewayResult.java | New API DTO for structured NSX gateway creation outcomes. |
| api/src/main/java/com/cloud/network/nsx/NsxService.java | Extends NSX service interface with VPN gateway/connection lifecycle operations. |
| api/src/main/java/com/cloud/network/element/Site2SiteVpnServiceProvider.java | Extends provider SPI with gateway IP acquisition/release, ownership, and customer-gateway validation hooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Dogface2k , want to address the co-pilot comments ? |
|
@DaanHoogland Addressed both Copilot findings in 4cea137: the poller now uses a state-filtered DAO query, and its executor is restart-safe across start/stop/start. Added focused DAO, lifecycle, and status-mapping tests; the complete affected suites pass (engine/schema 385/385 and NSX plugin 202/202), with checkstyle clean. Both review threads are now resolved. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java:1
- Connection deletion/reset now depends on getVpnGatewayForConnection(), which throws when the gateway row is missing. That makes delete/cleanup non-idempotent: a partially-deleted environment (missing gateway/VPC) will prevent connection teardown and can strand the DB row/lock-based workflows. Consider making the delete path tolerant to a missing gateway (e.g., in stopVpnConnectionLocked when deleting==true, skip provider-side stop/delete if the gateway is absent, proceed with DB removal; or make getVpnGatewayForConnection optionally return null and handle it in delete/stop flows).
server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java:1 - Connection deletion/reset now depends on getVpnGatewayForConnection(), which throws when the gateway row is missing. That makes delete/cleanup non-idempotent: a partially-deleted environment (missing gateway/VPC) will prevent connection teardown and can strand the DB row/lock-based workflows. Consider making the delete path tolerant to a missing gateway (e.g., in stopVpnConnectionLocked when deleting==true, skip provider-side stop/delete if the gateway is absent, proceed with DB removal; or make getVpnGatewayForConnection optionally return null and handle it in delete/stop flows).
server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java:1 - These validation methods were widened from private to package-visible to support testing. This increases the callable surface area of internal validation logic and makes it easier for other production classes in the package to start depending on them. If possible, keep them private and test via the public entrypoints; alternatively, clearly document they are package-visible only for tests (and keep tests in the same package) to reduce the risk of accidental reuse.
server/src/main/java/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java:1 - These validation methods were widened from private to package-visible to support testing. This increases the callable surface area of internal validation logic and makes it easier for other production classes in the package to start depending on them. If possible, keep them private and test via the public entrypoints; alternatively, clearly document they are package-visible only for tests (and keep tests in the same package) to reduce the risk of accidental reuse.
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/utils/NsxControllerUtils.java:58 - When NSX returns a failed (but non-null) NsxAnswer, the log and exception discard useful troubleshooting context (command type, zone, and the answer details/error). Include cmd.getClass()/cmd.toString(), zoneId, and answer.getDetails() (and any structured flags like endpointMayBeInUse/objectExists) in the log message and/or exception text so operators can diagnose failures without enabling deeper debug logging.
NsxAnswer answer = sendNsxCommandForResult(cmd, zoneId);
if (!answer.getResult()) {
logger.error("NSX API Command failed");
throw new InvalidParameterValueException("Failed API call to NSX controller");
}
| private void releaseAutoAcquiredVpnGatewayIp(IPAddressVO ip) { | ||
| boolean disassociated = ipAddressManager.disassociatePublicIpAddress(ip, CallContext.current().getCallingUserId(), | ||
| CallContext.current().getCallingAccount()); | ||
| if (!disassociated) { | ||
| throw new CloudRuntimeException(String.format("Failed to disassociate auto-acquired VPN gateway IP %s", ip.getAddress())); | ||
| } | ||
| userIpAddressDetailsDao.removeDetail(ip.getId(), NSX_VPN_GATEWAY_IP_DETAIL); | ||
| } |
There was a problem hiding this comment.
@Dogface2k , I’ve seen this feedback from co-pilot before . I don’t have an assessment of its value. But as a generic comment it makes sense. The question you should ask is, “would any reconnect or restart have this call as an asynchronous result. If so, take the comment serious.
There was a problem hiding this comment.
@DaanHoogland Thanks I traced that specific possibility. Reset, reconnect, restart and stop operate only on the VPN connection/session and never call releaseVpnGatewayIp. The release path is reached only during gateway-creation rollback, explicit gateway deletion, or VPC teardown. The background VPC teardown runs as a ManagedContextRunnable using the system user/account, and CallContext.current() already registers a system context when none exists. API-triggered deletion retains its propagated caller context. The ownership marker is also removed only after disassociation succeeds, so a failed release remains recoverable. I therefore don't think an additional fallback or provider-SPI signature change is warranted here. (false positive)
Description
This PR implements native, route-based Site-to-Site VPN for NSX NAT-mode VPCs. The VPN terminates on the VPC's NSX Tier-1 gateway; the CloudStack virtual router remains outside the IPsec data path.
The implementation makes the existing built-in
Vpn/Nsxservice mapping functional and supersedes the capability-removal approach in the closed #13765. It does not change seeded offerings or the database schema.The main lifecycle is provider-dispatched through
Site2SiteVpnServiceProviderinstead of adding another provider-specific branch toSite2SiteVpnManagerImpl:IPSecVpnServiceand local endpoint;RouteBasedIPSecVpnSession, and a VTI nameddefault-tunnel-interface;NO_SNATrule so traffic is not rewritten by the VPC's catch-all source NAT before entering the tunnel;The public endpoint is intentionally separate from the VPC source-NAT address. NSX requires the IPsec local endpoint to differ from the Tier-1 uplink address. Auto-acquired endpoint addresses are released on gateway deletion; operator-supplied addresses are detached but not released.
Compatibility and failure handling are explicit:
The Tier-0 must redistribute
TIER1_IPSEC_LOCAL_ENDPOINTroutes for the dedicated gateway address to be externally reachable.Related context: #13764 and the superseded closed PR #13765.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
Not applicable.
How Has This Been Tested?
The exact upstream-clean commit was built and tested from current
mainat4f117071c9397b1e4714c8fb61c384883c872842with Java 17:The deployed 4.22.1.0 focused NSX run after the lifecycle hardening reported 206/206 tests passing, including
testAddVpnConnectionRoutesRetriesMarkedForDeletion. This is an additional backport result; the local checkout used for review has no JDK/Maven executable, so repository CI remains the authoritative build check.\n\nThe changedapi,server, and NSX plugin modules also passed:The NSX suite covers all six management-server/agent command paths, including a real CloudStack
Request/Gson round trip proving that the PSK remains on the wire while@LogLevel(Off)excludes it from log serialization.An earlier 4.22.1 backport/prototype was exercised against NSX 4.2.4. Live validation exposed five integration defects that are addressed in this branch: virtual-router command leakage, the virtual-router monitor overwriting NSX state, NSX marked-for-deletion races, idle tunnels being removed by periodic DPD, and catch-all SNAT rewriting VPN traffic.
Post-deploy live control-plane validation was run on 2026-08-02 against an isolated test VPC on 4.22.1.0/NSX 4.2.4: a fresh NSX VPN gateway, customer gateway, and connection were created, then reset immediately; the reset job succeeded. The disposable connection, customer gateway, VPN gateway, and VPC were then deleted successfully. Production connection 8 remained Connected before and after the run. This validates the management-plane lifecycle only: no tunnel-traffic success is claimed from the intentionally unreachable RFC 5737 peer, and no production connection was modified. The earlier marked-for-deletion 431 occurred on a pre-deploy build and is not presented as a current-build failure. Full peer traffic and Tier-0 endpoint-route redistribution remain environment-dependent validation prerequisites.
How did you try to break this feature and the system with this change?
The negative and compatibility coverage exercises the failure boundaries rather than only the happy path:
NO_SNATrules during deletion;UP,DOWN, degraded, and not-found results map to the intended CloudStack states;No generic CloudStack command proxy, broad exception suppression, schema migration, retry of an ambiguous CloudStack mutation, or unrelated UI/CKS/network-offering change is included.