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.84% +0.19%
- Complexity 19792 20087 +295
============================================
Files 6368 6378 +10
Lines 574881 576954 +2073
Branches 70351 70578 +227
============================================
+ Hits 112970 114500 +1530
- Misses 449639 450030 +391
- Partials 12272 12424 +152
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");
}
|
Live end-to-end validation completed against the deployed 4.23.0.0-SNAPSHOT build with NSX 4.2.4, using disposable account-scoped resources.\n\n- Created an NSX NAT-mode VPC, tier, dedicated VPN gateway endpoint and route-based connection.\n- Reset the connection immediately after creation twice. Both reset jobs completed successfully in about 16 seconds, directly exercising the marked-for-deletion retry and route-based session update paths.\n- Deleted the connection, customer gateway and VPN gateway; the dedicated endpoint address was released.\n- Repeated teardown with an active VPN connection and deleted the VPC. The Tier-1 VPN service, session, gateway record, connection record and endpoint address were all absent afterward.\n- Confirmed the NSX Remote Access VPN capability guard rejects the unsupported provider before persisting a remote-access VPN record.\n- Existing bidirectional routed-tunnel traffic evidence remains unchanged; no production VPN connection was modified during this disposable lifecycle run.\n\nThis completes the live control-plane lifecycle validation for the scope of this PR. Connection/tunnel already has E2E |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18766 |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18773 |
|
@Dogface2k fyi (the github action output can be inconvenient) |
The added system-VM source-NAT test asserted behavior that the production legacy fallback does not implement, and the test also failed compilation due to a missing ArgumentCaptor import. Restore the previously validated test tree without changing production code.
|
Reviewing one final time. |
Do not overwrite an existing nsxVpnGatewayIp detail when a requested endpoint is retried. Remove the marker on failure only when the current attempt created it, preserving ownership from an earlier ambiguous operation. Add focused tests for existing, temporary and ambiguous ownership-marker handling. Signed-off-by: Brad <100990646+Dogface2k@users.noreply.github.com>
Refine native route-based site-to-site VPN across the provider SPI, management and agent boundary, Tier-1 lifecycle, status polling, API output, and UI details. Preserve provider ownership and database state when NSX lookup or teardown fails, treat only SDK NotFound as absence, fail closed on ambiguous operations, and keep legacy virtual-router dispatch isolated. Consolidate boundary-focused coverage for rollback, deletion, redaction, and provider failure behavior while removing duplicate and implementation-coupled tests. Signed-off-by: Dogface2k <100990646+Dogface2k@users.noreply.github.com>
|
@blueorangutan package |
|
@Dogface2k a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18806 |
| @SerializedName(ApiConstants.LOCAL_VTI_IP) | ||
| @Param(description = "The provider-side virtual tunnel interface IP address", since = "4.23.0.0") | ||
| private String localVtiIp; | ||
|
|
||
| @SerializedName(ApiConstants.PEER_VTI_IP) | ||
| @Param(description = "The peer-side virtual tunnel interface IP address", since = "4.23.0.0") | ||
| private String peerVtiIp; | ||
|
|
||
| @SerializedName(ApiConstants.VTI_PREFIX_LENGTH) | ||
| @Param(description = "The virtual tunnel interface network prefix length", since = "4.23.0.0") | ||
| private Integer vtiPrefixLength; | ||
|
|
There was a problem hiding this comment.
i don’t think this will make the version claimed in the ‘since’ attributes here (i.e. “4.23.0.0”)
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: the UI change adds three optional provider fields to the existing canonical VPN-connection details renderer and localization path; it adds no component, style, layout, control, action, or client-side behavior.
How Has This Been Tested?
All current local validation is bound to exact PR head
a70061fef3dc040b31e72cdbf23b22c44f994a16, targetingmain, with OpenJDK 17.0.19 and Maven 3.9.12. The local branch, fork branch, and GitHub PR head are identical and the worktree/index are clean.The PR merge base is
4f117071c9397b1e4714c8fb61c384883c872842. The refreshed target used for the final conflict audit isdd3427d91401f820ba5746403696a6146ab42328; its three changed paths do not overlap any of this PR's 43 paths, andgit merge-tree --write-tree HEAD origin/maincompletes without conflict.Local results on the exact head:
The full Maven build used the repository's
developer,systemvmprofile with-Dsimulator -Dnoredist; the quality run added the repository'squalityprofile and was followed byscripts/coverage-grade.shagainst the generated aggregate XML. The UI install's mechanical lockfile-format rewrite was discarded after verification; no generated or dependency artifact is part of the PR.Test sensitivity was checked rather than inferred:
NsxApiClientTest.testDeleteVpnServicePropagatesTier1TransportFailureBeforeCleanupfail for the intended missing exception; restoring the fix made it pass;RemoteAccessVpnManagerImplTest.createRemoteAccessVpnRejectsVpcWithoutMappedProviderBeforePersistencefail before persistence; restoring the guard made it pass;NotFoundTier-1 transport failure propagates through the NSX client/element/manager path and preserves the ownership marker, public IP, and gateway row.Two low-value tests were removed: one asserted mocked DAO self-interactions without executing its SQL/result contract, and one duplicated stronger NSX ownership cases already exercised through
NsxElementTest.The NSX suite covers the management-server/agent command paths, including normal and logger Gson serialization plus deserialization of the real
Command[]payload, proving that the PSK remains in the command wire JSON while@LogLevel(Off)excludes it from logger serialization.Live validation already recorded on this PR remains applicable to this implementation:
192.168.46.5and10.35.0.1succeeded in both directions over the FortiGate/NSX route, with the CloudStack connection remainingConnected;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.
The focused manager tests cover the established single-source-NAT virtual-router fallback and the NSX provider-owned endpoint path through
testCreateVpnGatewayAcceptsValidProviderOwnedIp. The focused ownership tests do not separately assert system-address, static-NAT, firewall, load-balancer, legacy system-VM source-NAT filtering, or multiple-candidate selection branches; those are not claimed as focused regression proof.