[Dogface2k]

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/Nsx service 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 Site2SiteVpnServiceProvider instead of adding another provider-specific branch to Site2SiteVpnManagerImpl:

• gateway creation validates or acquires a dedicated public IP, records the original provider with the gateway and whether the endpoint IP was auto-acquired, and creates a deterministic Tier-1 IPSecVpnService and local endpoint;
• connection start validates CloudStack crypto policy, creates NSX IKE/tunnel/DPD profiles, a RouteBasedIPSecVpnSession, and a VTI named default-tunnel-interface;
• each remote CIDR receives a Tier-1 static route and a NO_SNAT rule so traffic is not rewritten by the VPC's catch-all source NAT before entering the tunnel;
• stop disables the session and removes connection routes/NAT exemptions; restart recreates them; permanent deletion also removes the session and profiles;
• permanent gateway and VPC teardown remove VPN objects before Tier-1 deletion; cleanup restarts preserve existing native VPN resources;
• a scheduled status poll maps NSX detailed session status back to CloudStack connection state without allowing a transient query failure to corrupt the last known state.

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. Gateway deletion releases an auto-acquired address only while it still belongs to that gateway's VPC/account. For an operator-supplied address, it removes the VPN ownership marker and retains the allocation. Release reloads and checks the allocation under the existing IP lock.

Compatibility and failure handling are explicit:

• the new provider SPI methods have defaults, preserving source compatibility for the existing virtual-router implementation and test/extension implementors; NSX opts in explicitly;
• older gateway rows without a saved provider retain the existing positive provider-ownership lookup and virtual-router fallback; normal operations still validate the current VPC/account allocation;
• the provider detail is written in the same transaction as a new gateway; positively owned older native gateways are backfilled before cleanup can release their endpoint. Saved identity prevents an offering change or partial cleanup from redirecting the gateway, and an unknown or unavailable saved provider fails closed;
• only gateway teardown can use saved provider identity after an endpoint is released or reused. Normal connection operations require a live endpoint claim; the poller repeats allocation and ownership checks after taking the connection lock;
• fresh gateway creation refuses to adopt an unowned pre-existing deterministic NSX service; an ownership-marked retry may reconcile its service, while ambiguous rollback retains the endpoint;
• Tier-1 VPN mutations share a per-gateway lock with Tier-1 teardown;
• PSKs are excluded from CloudStack command logging while remaining present on the agent wire payload;
• unsupported algorithms, invalid lifetimes, oversized PSKs and deterministic VTI collisions fail with specific errors. Start/stop reject missing VPC or gateway state; provider cleanup tolerates a missing VPC and backend cleanup tolerates appropriate NotFound responses. The manager still rejects a missing gateway database row.

The Tier-0 must redistribute TIER1_IPSEC_LOCAL_ENDPOINT routes for the dedicated gateway address to be externally reachable.

Related context: #13764 and the superseded closed PR #13765.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

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?

Current head: 6eee828

This consolidates the previous two commits into one replacement, with parent 8f1028aef0916b10681575fffbc7211ee2bd8c25 and exact tested tree f080cb8ec6eaf54b0520a09cdd14dba20b3eac57. The old head is preserved by a verified backup ref. The PR remains on main, milestone 24.0, with 11 commits and 60 changed paths. Upstream main was verified at a7be64af7ad8fdefe1948efa175e87d4852d3453; the true merge base remains d3f0700e978390f65805a488a96ef9ca8e9e8070.

The correction addresses a concrete partial-cleanup failure: endpoint release may complete while the old gateway row remains. A later allocation must not become that gateway's endpoint or lose its IP/marker during a retry. The final code preserves the original provider on the gateway, validates live allocation/ownership for normal operations, and reloads allocation ownership under the IP lock before release. The poller also repeats its checks after acquiring the connection lock.

Both previous commits' protections are retained, including the VPC firewall reservation guard. Fifty published PR blobs are unchanged; ten contain the reviewed corrections and stronger tests. No file is deleted. All 53 removed/replaced blocks (119 old lines) relative to the previous head were independently accounted for. Two annotation-only tests and two stack-location assertions were removed; production annotations and behavioral lock/exclusion checks remain.

Two independent reviewers in fresh contexts read the complete final PR, followed by council reconciliation, before commit creation. Both passed the exact final tree. Post-publication reads confirm that tree, the intended parent, all 60 file blobs, and the unchanged first ten commits.

Validation on the final source	Result
Java/Maven	Temurin 17.0.20.1; Maven 3.9.16; Windows
Selected affected reactor	34 modules, BUILD SUCCESS; 24 fresh suites; 749 reported cases: 747 passed, 0 failures, 0 errors, 2 existing database-dependent skips
NSX suites within that total	272 tests passed
Checkstyle	27 fresh reports, zero violations
Deliberate fault controls	16 passing baselines; all 16 compiled mutants failed at the intended behavioral assertion
Source identity	All 11,912 runtime paths match the authenticated Git tree, with explicit Windows text line-ending normalization; all ten final overlays match raw bytes
UI evidence	Earlier Node 16 lint, production build and 193 tests / 10 suites passed; its entire UI subtree is identical to this head. This is reused evidence, not a fresh UI run

The tests now use a valid crypto fixture and exact rejection reason, register the replacement offering provider, capture actual route/NO_SNAT payload values, exercise customer-gateway updates through the public API, save independent expected tags, and distinguish absent and "false" ownership markers. Retry tests control the wait while asserting the unchanged 15,000ms production delay and interruption handling. The concurrency test retains its positive contention witness and backend exclusion/order assertions; removing either lock or changing its key makes it fail.

Five additional fault controls specifically reject ignored saved-provider identity, release of a reallocated IP, omitted post-lock poller revalidation, omitted provider-detail persistence and omitted legacy ownership backfill. Fault controls are repeat executions of selected methods, not additional unique tests.

Actual selected reactor command:

mvn -B -ntp -Dnoredist -pl plugins/network-elements/nsx -am -Dtest=ApiResponseHelperTest,ConfigurationManagerTest,NetworkServiceImplTest,NetworkModelImplTest,NetworkModelTest,IpAddressManagerTest,RemoteAccessVpnManagerImplTest,Site2SiteVpnManagerImplTest,VpcManagerImplTest,VpcVirtualRouterElementTest,VpnConnectionLifecycleCmdTest,Nsx*Test,PagedFetcherTest,FirewallManagerTest,FirewallManagerImplTest -Dsurefire.failIfNoSpecifiedTests=false test

The invocation used -Dexec.skip=true through MAVEN_OPTS for unavailable Bash/WSL tasks. This was an incremental selected-test reactor, not a clean or full-repository run. The two existing skips are FirewallManagerTest.testApplyRules and testApplyFWRules, both requiring a configured database. Mocked backend/DAO tests establish decisions, payloads and ordering; no current-head live NSX realization/traffic, physical database rollback/crash injection or cross-management-server fault injection is claimed.

Automatic CI

Automatic CI for 6eee828 has started and is not yet complete. Current checks are the source for new-head results; no workflow was manually operated.

The preceding head 8e982497 finished with 25 successful checks and two simulator failures. The failing methods were test_mm_project_limits.TestProjectsMemoryLimits.test_03_project_vmlifecycle_delete_instance and test_memory_limits.TestDomainMemoryLimitsConfiguration.test_03_delete_instance, each reporting RAM 2048 instead of 0 after VM deletion. Their cause remains unresolved; they are not labelled flaky, unrelated or fixed, and those historical results are not substituted for new-head CI.

Latest review comments

Review point	Independently checked disposition
Ownership-marker cleanup	No second marker removal after successful disassociation. Existing IP unassignment clears details; the final locked allocation recheck also protects later allocations. Gateway provider identity survives partial cleanup.
Shared ownership key	Existing NSX IP-marker key remains a single definition shared within the package. The separate generic gateway-provider key belongs in the shared provider SPI because both server and NSX use it.
Controller exception context	Existing ERROR logs retain operation context; raw backend details have no sanitization contract for API exposure. Richer sanitized public wording is optional.
Missing annotations	All seven VPN methods declared by NsxService have @Override in NsxServiceImpl.

These are technical dispositions. Replies and thread resolution are separate GitHub actions.

Historical validation recorded at a70061 — not rerun on the current head

The following results were recorded for earlier head a70061fef3dc040b31e72cdbf23b22c44f994a16, targeting main, with OpenJDK 17.0.19 and Maven 3.9.12. They are retained as historical records, not current-head execution. The branch/ref and clean-worktree statements applied at that earlier validation.

That earlier validation used merge base 4f117071c9397b1e4714c8fb61c384883c872842. The refreshed target used for that conflict audit was dd3427d91401f820ba5746403696a6146ab42328; its three changed paths do not overlap any of this PR's 43 paths, and git merge-tree --write-tree HEAD origin/main completes without conflict.

Recorded local results on a70061:

Focused VPN/NSX tests:
Site2SiteVpnManagerImplTest  65 passed
NsxElementTest               56 passed
NsxApiClientTest             39 passed

Affected 34-module clean reactor:
8,486 tests; 0 failures; 0 errors; 11 skipped
NSX plugin: 215 tests; 0 failures; 0 errors; 0 skipped
Server: 3,643 tests; 0 failures; 0 errors; 5 skipped

Full developer/systemvm/simulator/noredist reactor:
165 modules: BUILD SUCCESS
13,909 tests; 0 failures; 0 errors; 13 skipped

Full quality/JaCoCo reactor:
165 modules: BUILD SUCCESS
Normal reports: 13,909 tests; 0 failures; 0 errors; 13 skipped
Instrumented reports: 13,909 tests; 0 failures; 0 errors; 13 skipped
Aggregate line coverage: 24.73% (repository grade D)

UI (Node 16.20.2 / npm 8.19.4):
production build passed
lint passed with zero errors
4 suites / 175 unit tests passed

Apache RAT:
11,111 resources; 0 unapproved/unknown licenses

Pre-commit:
standard all-files hooks passed
manual-stage hooks passed

Packaging:
el8, el9, el10, Debian and SUSE 15 passed (Blue Orangutan SL-JID 18806)

The full Maven build used the repository's developer,systemvm profile with -Dsimulator -Dnoredist; the quality run added the repository's quality profile and was followed by scripts/coverage-grade.sh against 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:

• temporarily restoring the unsafe broad Tier-1 exception-to-null behavior made NsxApiClientTest.testDeleteVpnServicePropagatesTier1TransportFailureBeforeCleanup fail for the intended missing exception; restoring the fix made it pass;
• temporarily removing the Remote Access VPN provider guard made RemoteAccessVpnManagerImplTest.createRemoteAccessVpnRejectsVpcWithoutMappedProviderBeforePersistence fail before persistence; restoring the guard made it pass;
• end-to-end failure tests verify that a non-NotFound Tier-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.

Historical live validation was reported earlier on this PR; it has not been repeated for the current head:

• on 4.23.0.0-SNAPSHOT with NSX 4.2.4, disposable NAT-mode VPC resources exercised gateway/connection creation, two immediate resets, ordered connection/customer-gateway/VPN-gateway cleanup, endpoint release, active-connection VPC teardown, and the unsupported Remote Access VPN guard;
• separate end-to-end traffic validation between 192.168.46.5 and 10.35.0.1 succeeded in both directions over the FortiGate/NSX route, with the CloudStack connection remaining Connected;
• no production VPN connection was modified during the disposable lifecycle run.

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:

• focused endpoint-conflict tests prove rejection of inactive, source-NAT, and port-forwarding addresses before NSX mutation;
• auto-acquired and operator-supplied endpoint addresses have different, verified release behavior;
• partial and ambiguous gateway creation failures preserve enough ownership state for safe operator recovery and never adopt an unrelated deterministic NSX service;
• connection creation rolls back partial sessions, routes, and NAT exemptions;
• changed peer CIDR lists do not strand old routes or NO_SNAT rules during deletion;
• deterministic VTI address collisions are detected against live Tier-1 sessions and fail closed;
• invalid/unsupported cipher, hash, DH group, IKE version, lifetime, and PSK values are rejected before an NSX mutation;
• stop, restart, reset, permanent connection deletion, gateway deletion, provider/backend missing-resource cleanup, and full VPC teardown were separately covered;
• source-NAT rule demotion preserves the complete original rule and sequence, and the exact sequence is restored when the final VPN exemption is removed;
• operator-created VPN services on the Tier-1 are not swept during CloudStack teardown;
• provider ownership remains stable if an offering mapping changes after gateway creation;
• legacy virtual-router VPN selection and command generation remain covered, while NSX-owned connections are excluded from virtual-router commands and monitoring;
• repeated status-query failures alert without forcing a false state transition, and successful UP, DOWN, degraded, and not-found results map to the intended CloudStack states;
• missing VPC or gateway state fails start/stop explicitly. Provider/backend missing-resource cleanup is narrower than manager-level deletion: the manager still rejects a missing gateway database row.

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.

[Dogface2k]

Follow-up validation and lifecycle hardening

Commit 98cd4ed6f4 adds the lifecycle hardening identified by live NSX validation.

• The existing marked-for-deletion retry is now shared by route-based session creation and the complete static-route/NO_SNAT programming operation. It remains bounded at 24 attempts with a 15-second interval.
• NsxApiClientTest.testAddVpnConnectionRoutesRetriesMarkedForDeletion forces the NSX marked-for-deletion response during route creation and verifies the deterministic route is retried before NO_SNAT programming continues.
• State changes use a read-modify-write of the existing route-based session, preserving the mandatory tunnel_interfaces field implicated by the pre-deploy failure.
• VPN connection, gateway, customer-gateway, and VPC lifecycle operations use VPC-scoped synchronization and deterministic cleanup.

Live validation on 4.22.1.0 with NSX 4.2.4 used an intentionally unreachable RFC 5737 test peer. The connection reached Disconnected; the first reset reproduced the marked-for-deletion 431, the same reset succeeded after the purge window, and connection/customer-gateway/VPN-gateway/VPC deletion completed with the dedicated test IP released. No production connection was modified, and no tunnel-traffic success is claimed from this test peer.

The supplied pre-deploy tunnel_interfaces error is a distinct historical failure; the current read-modify-write path passed the stop half of the live reset test. The local checkout has no JDK/Maven executable, so the new focused test must be run by repository CI; git diff --check is clean.

[Dogface2k]

Additional live data-plane evidence

Operator-side testing between 192.168.46.5 and 10.35.0.1 succeeded in both directions over the documented FortiGate/NSX path. The original direction returned 2/2 ICMP replies with 0% loss and 17.45 ms average latency; traceroute showed 6x.xxx.xxx.xxx (FortiGate) followed by 10.35.0.1.

CloudStack connection 8 remained Connected before and after. This is positive bidirectional endpoint-reachability evidence through the VPN path.

[codecov]

Codecov Report

❌ Patch coverage is 71.63814% with 696 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.09%. Comparing base (d3f0700) to head (6eee828).
⚠️ Report is 96 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/org/apache/cloudstack/service/NsxApiClient.java	72.58%	184 Missing and 40 partials ⚠️
...java/org/apache/cloudstack/service/NsxElement.java	74.85%	52 Missing and 33 partials ⚠️
...com/cloud/network/vpn/Site2SiteVpnManagerImpl.java	75.00%	44 Missing and 30 partials ⚠️
.../org/apache/cloudstack/service/NsxServiceImpl.java	64.40%	48 Missing and 15 partials ⚠️
...va/org/apache/cloudstack/resource/NsxResource.java	84.26%	26 Missing and 2 partials ⚠️
...stack/agent/api/CreateNsxVpnConnectionCommand.java	77.08%	21 Missing and 1 partial ⚠️
...n/java/com/cloud/network/IpAddressManagerImpl.java	37.93%	18 Missing ⚠️
...stack/agent/api/DeleteNsxVpnConnectionCommand.java	40.74%	15 Missing and 1 partial ⚠️
...stack/agent/api/GetNsxVpnSessionStatusCommand.java	40.74%	15 Missing and 1 partial ⚠️
.../agent/api/UpdateNsxVpnConnectionStateCommand.java	48.38%	15 Missing and 1 partial ⚠️
... and 14 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13766      +/-   ##
============================================
+ Coverage     20.39%   21.09%   +0.69%     
- Complexity    21283    22384    +1101     
============================================
  Files          6481     6506      +25     
  Lines        588040   594158    +6118     
  Branches      72021    72914     +893     
============================================
+ Hits         119957   125321    +5364     
- Misses       455052   455217     +165     
- Partials      13031    13620     +589     

Flag	Coverage Δ
uitests	3.88% <ø> (+0.17%)	⬆️
unittests	22.42% <71.63%> (+0.71%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

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.

[DaanHoogland]

@Dogface2k , want to address the co-pilot comments ?

[Dogface2k]

@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.

[Copilot]

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");
        }

[Dogface2k]

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

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@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.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18766

[blueorangutan]

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18773

[DaanHoogland]

@Dogface2k fyi

09:45:43 [INFO] -------------------------------------------------------------
09:45:52 [ERROR] COMPILATION ERROR : 
09:45:52 [INFO] -------------------------------------------------------------
09:45:52 [ERROR] /jenkins/workspace/acs-deb-pkg-builder/server/src/test/java/com/cloud/network/vpn/Site2SiteVpnManagerImplTest.java:[362,8] error: cannot find symbol
09:45:52   symbol:   class ArgumentCaptor
09:45:52   location: class Site2SiteVpnManagerImplTest
09:45:52 [ERROR] /jenkins/workspace/acs-deb-pkg-builder/server/src/test/java/com/cloud/network/vpn/Site2SiteVpnManagerImplTest.java:[362,62] error: cannot find symbol
09:45:52   symbol:   variable ArgumentCaptor
09:45:52   location: class Site2SiteVpnManagerImplTest
09:45:52 [INFO] 2 errors 

(the github action output can be inconvenient)

[Dogface2k]

Reviewing one final time.

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@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.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18888

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[vishesh92]

@blueorangutan test

[blueorangutan]

@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-16918)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 59242 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13766-t16918-kvm-ol8.zip
Smoke tests completed. 156 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[DaanHoogland]

@Pearl1594 @nvazquez , can you have a look please?

I am not sure if we can test this reliably , given the infra requirements for this.

[nvazquez]

Code LGTM - thanks @Dogface2k. Unfortunately, I do not have the infra anymore to test it, however changes look good to me

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open issues

• disassociatePublicIpAddress and removeDetail are executed sequentially without handling… · New
• NsxServiceImpl depends on NsxElement.NSX_VPN_GATEWAY_IP_DETAIL, which is currently a `protected… · New
• The thrown error message is generic and discards potentially actionable context already available… · New
• These methods implement the NsxService interface but are missing @Override annotations (same… · New

1 resolved since last review

• Releasing an auto-acquired VPN endpoint IP relies on CallContext.current(). If this cleanup is…

_{Review effort: Lite}

[Dogface2k]

Consolidated the latest two commits into 6eee828, retaining the lifecycle, endpoint-reservation and firewall fixes.

The gateway now retains its original provider through partial cleanup; release and polling recheck the current IP allocation. The misleading validation fixtures and missing route/NAT payload assertions are corrected.

Two fresh independent reviews passed the complete final tree and accounted for every removal. On that exact source: 747 tests passed, 0 failures/errors, 2 existing DB skips; all 16 deliberate fault controls failed as intended.

The four review points were independently rechecked. New-head CI is running; the earlier two simulator failures remain unexplained. Live NSX testing was not rerun.