Copilot commented on code in PR #13766:
URL: https://github.com/apache/cloudstack/pull/13766#discussion_r4515782566
##########
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxElement.java:
##########
@@ -941,4 +988,341 @@ public List<Class<?>> getCommands() {
public boolean updateVpcSourceNatIp(Vpc vpc, IpAddress address) {
return nsxService.updateVpcSourceNatIp(vpc, address);
}
+
+ protected boolean isVpnProvidedByNsx(Vpc vpc) {
+ if (Objects.isNull(vpc)) {
+ return false;
+ }
+ return vpcManager.isProviderSupportServiceInVpc(vpc.getId(),
Network.Service.Vpn, Network.Provider.Nsx);
+ }
+
+ protected boolean isVpnProvidedByNsx(Vpc vpc, Site2SiteVpnGateway gateway)
{
+ return vpc != null && ownsVpnGateway(gateway);
+ }
+
+ private void validateVpnGatewayOwnedByNsx(Vpc vpc, Site2SiteVpnGateway
gateway) {
+ if (!isVpnProvidedByNsx(vpc, gateway)) {
+ throw new CloudRuntimeException(String.format(
+ "NSX does not own Site-to-Site VPN gateway %s of VPC %s",
gateway.getId(), vpc.getName()));
+ }
+ }
+
+ @Override
+ public IpAddress acquireVpnGatewayIp(Vpc vpc, IpAddress requestedIp) {
+ if (!isVpnProvidedByNsx(vpc)) {
+ return null;
+ }
+ IPAddressVO ip;
+ boolean autoAcquired = false;
+ boolean reconcileExistingService = false;
+ UserIpAddressDetailVO ownershipMarker = null;
+ if (Objects.nonNull(requestedIp)) {
+ ip = validateRequestedVpnGatewayIp(vpc, requestedIp);
+ ownershipMarker = userIpAddressDetailsDao.findDetail(ip.getId(),
NSX_VPN_GATEWAY_IP_DETAIL);
+ if (ownershipMarker != null) {
+ autoAcquired = isAutoAcquiredVpnGatewayIp(ownershipMarker, ip,
vpc);
+ reconcileExistingService = true;
+ }
+ } else {
+ Pair<IPAddressVO, UserIpAddressDetailVO> retainedIp =
findRetainedVpnGatewayIp(vpc);
+ if (retainedIp != null) {
+ ip = retainedIp.first();
+ ownershipMarker = retainedIp.second();
+ autoAcquired = isAutoAcquiredVpnGatewayIp(ownershipMarker, ip,
vpc);
+ reconcileExistingService = true;
+ } else {
+ ip = allocateVpnGatewayIp(vpc);
+ autoAcquired = true;
+ }
+ }
+ boolean requestedIpOwnershipMarkerAdded = false;
+ if (!autoAcquired && ownershipMarker == null) {
+ try {
+ userIpAddressDetailsDao.addDetail(ip.getId(),
NSX_VPN_GATEWAY_IP_DETAIL, "false", false);
+ requestedIpOwnershipMarkerAdded = true;
+ } catch (Exception e) {
+ throw new CloudRuntimeException(String.format(
+ "Failed to record NSX VPN ownership for requested IP
%s of VPC %s",
+ ip.getAddress(), vpc.getName()), e);
+ }
+ }
+ boolean endpointMayBeInUse = true;
+ try {
+ NsxVpnGatewayResult result = nsxService.createVpnGateway(vpc,
ip.getAddress().addr(), reconcileExistingService);
+ endpointMayBeInUse = result.isEndpointMayBeInUse();
+ if (!result.isSuccessful()) {
+ throw new CloudRuntimeException(String.format("The NSX VPN
gateway service for VPC %s was not created: the provider returned an
unsuccessful answer",
+ vpc.getName()));
+ }
+ } catch (Exception e) {
+ if (autoAcquired && !endpointMayBeInUse) {
+ try {
+ releaseAutoAcquiredVpnGatewayIp(ip);
+ } catch (Exception cleanupException) {
+ logger.warn("Failed to release the auto-acquired VPN
gateway IP {} of VPC {} after creation failed: {}",
+ ip.getAddress(), vpc.getName(),
cleanupException.getMessage());
+ }
+ } else if (autoAcquired) {
+ logger.warn("Retaining auto-acquired VPN gateway IP {} for VPC
{} because the NSX endpoint may still be using it",
+ ip.getAddress(), vpc.getName());
+ } else if (!endpointMayBeInUse && requestedIpOwnershipMarkerAdded)
{
+ try {
+ userIpAddressDetailsDao.removeDetail(ip.getId(),
NSX_VPN_GATEWAY_IP_DETAIL);
+ } catch (Exception cleanupException) {
+ logger.warn("Failed to remove the NSX VPN ownership marker
from requested IP {} of VPC {} after gateway creation failed: {}",
+ ip.getAddress(), vpc.getName(),
cleanupException.getMessage());
+ }
+ }
+ throw new CloudRuntimeException(String.format("Failed to create
the NSX VPN gateway for VPC %s: %s",
+ vpc.getName(), e.getMessage()), e);
+ }
+ return ip;
+ }
+
+ private Pair<IPAddressVO, UserIpAddressDetailVO>
findRetainedVpnGatewayIp(Vpc vpc) {
+ Pair<IPAddressVO, UserIpAddressDetailVO> retainedIp = null;
+ List<IPAddressVO> nonSourceNatIps =
ipAddressDao.listByAssociatedVpc(vpc.getId(), false);
+ if (CollectionUtils.isNullOrEmpty(nonSourceNatIps)) {
+ return null;
+ }
+ for (IPAddressVO candidate : nonSourceNatIps) {
+ UserIpAddressDetailVO ownershipMarker =
userIpAddressDetailsDao.findDetail(
+ candidate.getId(), NSX_VPN_GATEWAY_IP_DETAIL);
+ if (ownershipMarker == null) {
+ continue;
+ }
+ if (retainedIp != null) {
+ throw new CloudRuntimeException(String.format(
+ "VPC %s has more than one IP retained for an NSX VPN
gateway; refusing ambiguous recovery",
+ vpc.getName()));
+ }
+ retainedIp = new Pair<>(validateRequestedVpnGatewayIp(vpc,
candidate), ownershipMarker);
+ }
+ return retainedIp;
+ }
+
+ private boolean isAutoAcquiredVpnGatewayIp(UserIpAddressDetailVO
ownershipMarker, IPAddressVO ip, Vpc vpc) {
+ if ("true".equalsIgnoreCase(ownershipMarker.getValue())) {
+ return true;
+ }
+ if ("false".equalsIgnoreCase(ownershipMarker.getValue())) {
+ return false;
+ }
+ throw new CloudRuntimeException(String.format(
+ "IP %s retained for the NSX VPN gateway of VPC %s has invalid
ownership state '%s'",
+ ip.getAddress(), vpc == null ? ip.getVpcId() : vpc.getName(),
ownershipMarker.getValue()));
+ }
+
+ private IPAddressVO validateRequestedVpnGatewayIp(Vpc vpc, IpAddress
requestedIp) {
+ IPAddressVO ip = ipAddressDao.findById(requestedIp.getId());
+ if (Objects.isNull(ip) || !Objects.equals(ip.getVpcId(), vpc.getId())
+ || !ip.readyToUse() || ip.getRemoved() != null ||
ip.getAddress() == null) {
+ throw new InvalidParameterValueException(String.format(
+ "The requested IP id %s is not an allocated, active IP
associated to the VPC %s",
+ requestedIp.getId(), vpc.getName()));
+ }
+ if (ip.isSourceNat() || ip.isForSystemVms()) {
+ throw new InvalidParameterValueException(String.format(
+ "The requested IP %s cannot be used as the VPN gateway IP
as it is a source NAT or system IP", ip.getAddress().addr()));
+ }
+ if (ip.isOneToOneNat() ||
!firewallRulesDao.listByIpAndNotRevoked(ip.getId()).isEmpty()
+ ||
!portForwardingRulesDao.listByIpAndNotRevoked(ip.getId()).isEmpty()
+ || !loadBalancerDao.listByIpAddress(ip.getId()).isEmpty()) {
+ throw new InvalidParameterValueException(String.format(
+ "The requested IP %s cannot be used as the VPN gateway IP
as it is already in use by static NAT or network rules",
ip.getAddress().addr()));
+ }
+ return ip;
+ }
+
+ private IPAddressVO allocateVpnGatewayIp(Vpc vpc) {
+ Account owner = accountMgr.getAccount(vpc.getAccountId());
+ DataCenterVO zone = dataCenterDao.findById(vpc.getZoneId());
+ IpAddress allocatedIp = null;
+ try {
+ allocatedIp = ipAddressManager.allocateIp(owner, false,
CallContext.current().getCallingAccount(),
+ CallContext.current().getCallingUser(), zone, null, null);
+ vpcService.associateIPToVpc(allocatedIp.getId(), vpc.getId());
+ userIpAddressDetailsDao.addDetail(allocatedIp.getId(),
NSX_VPN_GATEWAY_IP_DETAIL, "true", false);
+ IPAddressVO ip = ipAddressDao.findById(allocatedIp.getId());
+ if (ip == null) {
+ throw new CloudRuntimeException(String.format("The allocated
VPN gateway IP %s could not be loaded after association",
+ allocatedIp.getId()));
+ }
+ if (ip.isSourceNat()) {
+ throw new CloudRuntimeException(String.format(
+ "The allocated IP %s became a source NAT IP when it
was associated to VPC %s; it cannot be used as a dedicated VPN endpoint",
+ ip.getAddress(), vpc.getName()));
+ }
+ return ip;
+ } catch (Exception e) {
+ // do not leak the IP when associating or tagging it fails after
allocation succeeded
+ if (Objects.nonNull(allocatedIp)) {
+ IPAddressVO ipToRelease =
ipAddressDao.findById(allocatedIp.getId());
+ if (Objects.nonNull(ipToRelease)) {
+ try {
+ releaseAutoAcquiredVpnGatewayIp(ipToRelease);
+ } catch (Exception releaseException) {
+ logger.warn("Failed to release the IP {} allocated for
the VPN gateway of VPC {}: {}",
+ ipToRelease.getAddress().addr(),
vpc.getName(), releaseException.getMessage());
+ }
+ } else {
+ try {
+
ipAddressManager.disassociatePublicIpAddress(allocatedIp,
CallContext.current().getCallingUserId(),
+ CallContext.current().getCallingAccount());
+ } catch (Exception releaseException) {
+ logger.warn("Failed to release allocated VPN gateway
IP {} of VPC {} after its database row disappeared: {}",
+ allocatedIp.getId(), vpc.getName(),
releaseException.getMessage());
+ }
+ }
+ }
+ throw new CloudRuntimeException(String.format("Failed to acquire
an IP for the VPN gateway of VPC %s: %s",
+ vpc.getName(), e.getMessage()), e);
+ }
+ }
+
+ 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);
Review Comment:
`disassociatePublicIpAddress` and `removeDetail` are executed sequentially
without handling failures of `removeDetail`. If `removeDetail` throws after a
successful disassociation, the ownership marker can remain stale and may cause
future ownership checks/poller gating to incorrectly treat the IP as still
NSX-owned. Consider wrapping marker cleanup in a try/catch (logging +
compensating action if appropriate) or otherwise ensuring the marker state
cannot remain inconsistent with the IP’s actual association state.
##########
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxServiceImpl.java:
##########
@@ -196,6 +298,192 @@ public boolean deleteFirewallRules(Network network,
List<NsxNetworkRule> netRule
return result.getResult();
}
+ @Override
+ public NsxVpnGatewayResult createVpnGateway(Vpc vpc, String
localEndpointIp) {
+ return createVpnGateway(vpc, localEndpointIp, false);
+ }
+
+ @Override
+ public NsxVpnGatewayResult createVpnGateway(Vpc vpc, String
localEndpointIp, boolean reconcileExistingService) {
+ CreateNsxVpnGatewayCommand createNsxVpnGatewayCommand = new
CreateNsxVpnGatewayCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName(), localEndpointIp,
+ reconcileExistingService);
+ NsxAnswer result =
nsxControllerUtils.sendNsxCommandForResult(createNsxVpnGatewayCommand,
vpc.getZoneId());
+ return new NsxVpnGatewayResult(result.getResult(),
result.isEndpointMayBeInUse());
+ }
+
+ public boolean deleteVpnGateway(Vpc vpc) {
+ DeleteNsxVpnGatewayCommand deleteNsxVpnGatewayCommand = new
DeleteNsxVpnGatewayCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName());
+ NsxAnswer result =
nsxControllerUtils.sendNsxCommand(deleteNsxVpnGatewayCommand, vpc.getZoneId());
+ return result.getResult();
+ }
+
+ public boolean createVpnConnection(Vpc vpc, long connectionId, String
peerAddress, String psk,
+ String ikePolicy, String espPolicy,
Long ikeLifetime, Long espLifetime,
+ boolean dpdEnabled, String ikeVersion,
boolean passive, List<String> peerCidrs,
+ String vtiLocalIp, String vtiPeerIp,
int vtiPrefixLength, String localEndpointIp) {
+ CreateNsxVpnConnectionCommand createNsxVpnConnectionCommand = new
CreateNsxVpnConnectionCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName(), connectionId, peerAddress, psk,
+ ikePolicy, espPolicy, ikeLifetime, espLifetime, dpdEnabled,
ikeVersion, passive, peerCidrs,
+ vtiLocalIp, vtiPeerIp, vtiPrefixLength, vpc.getCidr(),
localEndpointIp);
+ NsxAnswer result =
nsxControllerUtils.sendNsxCommand(createNsxVpnConnectionCommand,
vpc.getZoneId());
+ return result.getResult();
+ }
+
+ public boolean deleteVpnConnection(Vpc vpc, long connectionId) {
+ DeleteNsxVpnConnectionCommand deleteNsxVpnConnectionCommand = new
DeleteNsxVpnConnectionCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName(), connectionId);
+ NsxAnswer result =
nsxControllerUtils.sendNsxCommand(deleteNsxVpnConnectionCommand,
vpc.getZoneId());
+ return result.getResult();
+ }
+
+ public boolean updateVpnConnectionState(Vpc vpc, long connectionId,
boolean enabled) {
+ UpdateNsxVpnConnectionStateCommand command = new
UpdateNsxVpnConnectionStateCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName(), connectionId, enabled);
+ NsxAnswer result = nsxControllerUtils.sendNsxCommand(command,
vpc.getZoneId());
+ return result.getResult();
+ }
+
+ public String getVpnConnectionStatus(Vpc vpc, long connectionId) {
+ GetNsxVpnSessionStatusCommand getNsxVpnSessionStatusCommand = new
GetNsxVpnSessionStatusCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName(), connectionId);
+ NsxAnswer result =
nsxControllerUtils.sendNsxCommand(getNsxVpnSessionStatusCommand,
vpc.getZoneId());
+ return result.getDetails();
+ }
+
+ /**
+ * The global lock makes the scan a cluster-wide singleton. The connection
row lock in
+ * transitionVpnConnectionState separately protects each state transition
from a concurrent
+ * user operation that starts, stops, resets, or deletes the connection.
+ */
+ protected class VpnStatusPollTask extends ManagedContextRunnable {
+ @Override
+ protected void runInContext() {
+ GlobalLock scanLock =
GlobalLock.getInternLock(VPN_STATUS_POLL_LOCK_NAME);
+ try {
+ if (scanLock.lock(VPN_STATUS_POLL_LOCK_TIMEOUT)) {
+ try {
+ pollVpnConnectionStatuses();
+ } finally {
+ scanLock.unlock();
+ }
+ }
+ } catch (Exception e) {
+ logger.warn("Failed to poll the status of the NSX Site-to-Site
VPN connections: {}", e.getMessage(), e);
+ } finally {
+ scanLock.releaseRef();
+ }
+ }
+ }
+
+ private void pollVpnConnectionStatuses() {
+ Set<Long> polledConnectionIds = new HashSet<>();
+ List<Site2SiteVpnConnectionVO> connections =
site2SiteVpnConnectionDao.listByStates(
+ VPN_POLLED_STATES.toArray(new
Site2SiteVpnConnection.State[0]));
+ for (Site2SiteVpnConnectionVO connection : connections) {
+ Site2SiteVpnGatewayVO vpnGateway =
site2SiteVpnGatewayDao.findById(connection.getVpnGatewayId());
+ if (vpnGateway == null) {
+ continue;
+ }
+ VpcVO vpc = vpcDao.findById(vpnGateway.getVpcId());
+ if (vpc == null || !isVpnProvidedByNsx(vpc, vpnGateway)) {
+ continue;
+ }
+ polledConnectionIds.add(connection.getId());
+ pollVpnConnectionStatus(connection, vpc);
+ }
+ // Drop the failure counters of connections that were deleted or left
the polled states.
+ vpnStatusPollFailures.keySet().retainAll(polledConnectionIds);
+ }
+
+ private boolean isVpnProvidedByNsx(Vpc vpc, Site2SiteVpnGatewayVO
vpnGateway) {
+ return vpc != null
+ && userIpAddressDetailsDao != null
+ && vpnGateway != null
+ && userIpAddressDetailsDao.findDetail(vpnGateway.getAddrId(),
NsxElement.NSX_VPN_GATEWAY_IP_DETAIL) != null;
Review Comment:
`NsxServiceImpl` depends on `NsxElement.NSX_VPN_GATEWAY_IP_DETAIL`, which is
currently a `protected static final` implementation detail of another class.
This creates brittle coupling across components in the same package; consider
moving this key to a dedicated shared constants holder (or making it a clearly
documented public constant in an appropriate shared module) to prevent
accidental breakage during refactors.
##########
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/utils/NsxControllerUtils.java:
##########
@@ -51,15 +51,30 @@ public static String
getNsxDistributedFirewallPolicyRuleId(String segmentName, l
}
public NsxAnswer sendNsxCommand(NsxCommand cmd, long zoneId) throws
IllegalArgumentException {
+ NsxAnswer answer = sendNsxCommandForResult(cmd, zoneId);
+ if (!answer.getResult()) {
+ logger.error("NSX command {} failed in zone {}",
cmd.getClass().getSimpleName(), zoneId);
+ throw new InvalidParameterValueException("Failed API call to NSX
controller");
+ }
+ return answer;
+ }
Review Comment:
The thrown error message is generic and discards potentially actionable
context already available (e.g., `answer.getDetails()`, command name). Consider
including sanitized details (and/or the command name/zone) in the exception
message so operators can troubleshoot without needing to reproduce with debug
logs.
##########
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxServiceImpl.java:
##########
@@ -196,6 +298,192 @@ public boolean deleteFirewallRules(Network network,
List<NsxNetworkRule> netRule
return result.getResult();
}
+ @Override
+ public NsxVpnGatewayResult createVpnGateway(Vpc vpc, String
localEndpointIp) {
+ return createVpnGateway(vpc, localEndpointIp, false);
+ }
+
+ @Override
+ public NsxVpnGatewayResult createVpnGateway(Vpc vpc, String
localEndpointIp, boolean reconcileExistingService) {
+ CreateNsxVpnGatewayCommand createNsxVpnGatewayCommand = new
CreateNsxVpnGatewayCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName(), localEndpointIp,
+ reconcileExistingService);
+ NsxAnswer result =
nsxControllerUtils.sendNsxCommandForResult(createNsxVpnGatewayCommand,
vpc.getZoneId());
+ return new NsxVpnGatewayResult(result.getResult(),
result.isEndpointMayBeInUse());
+ }
+
+ public boolean deleteVpnGateway(Vpc vpc) {
+ DeleteNsxVpnGatewayCommand deleteNsxVpnGatewayCommand = new
DeleteNsxVpnGatewayCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName());
+ NsxAnswer result =
nsxControllerUtils.sendNsxCommand(deleteNsxVpnGatewayCommand, vpc.getZoneId());
+ return result.getResult();
+ }
+
+ public boolean createVpnConnection(Vpc vpc, long connectionId, String
peerAddress, String psk,
+ String ikePolicy, String espPolicy,
Long ikeLifetime, Long espLifetime,
+ boolean dpdEnabled, String ikeVersion,
boolean passive, List<String> peerCidrs,
+ String vtiLocalIp, String vtiPeerIp,
int vtiPrefixLength, String localEndpointIp) {
+ CreateNsxVpnConnectionCommand createNsxVpnConnectionCommand = new
CreateNsxVpnConnectionCommand(vpc.getDomainId(),
+ vpc.getAccountId(), vpc.getZoneId(), vpc.getId(),
vpc.getName(), connectionId, peerAddress, psk,
+ ikePolicy, espPolicy, ikeLifetime, espLifetime, dpdEnabled,
ikeVersion, passive, peerCidrs,
+ vtiLocalIp, vtiPeerIp, vtiPrefixLength, vpc.getCidr(),
localEndpointIp);
+ NsxAnswer result =
nsxControllerUtils.sendNsxCommand(createNsxVpnConnectionCommand,
vpc.getZoneId());
+ return result.getResult();
+ }
Review Comment:
These methods implement the `NsxService` interface but are missing
`@Override` annotations (same applies to the other newly added VPN methods in
this class). Adding `@Override` helps the compiler catch signature drift during
future refactors and improves readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
