Skip to content

Commit 950e3ad

Browse files
Dogface2kDogface2k
committed
NSX: preserve VPN gateway ownership on retry
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>
1 parent f07698c commit 950e3ad

2 files changed

Lines changed: 157 additions & 3 deletions

File tree

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1016,10 +1016,17 @@ public IpAddress acquireVpnGatewayIp(Vpc vpc, IpAddress requestedIp) {
10161016
ip = allocateVpnGatewayIp(vpc);
10171017
autoAcquired = true;
10181018
}
1019+
boolean requestedIpOwnershipMarkerAdded = false;
10191020
if (!autoAcquired) {
10201021
try {
1021-
// Mark ownership first so ambiguous NSX responses remain recoverable.
1022-
userIpAddressDetailsDao.addDetail(ip.getId(), NSX_VPN_GATEWAY_IP_DETAIL, "false", false);
1022+
// Preserve an earlier ownership marker: it may represent an ambiguous create that
1023+
// still requires provider-side cleanup and must not be downgraded on a retry.
1024+
UserIpAddressDetailVO existingOwnershipMarker = userIpAddressDetailsDao.findDetail(
1025+
ip.getId(), NSX_VPN_GATEWAY_IP_DETAIL);
1026+
if (existingOwnershipMarker == null) {
1027+
userIpAddressDetailsDao.addDetail(ip.getId(), NSX_VPN_GATEWAY_IP_DETAIL, "false", false);
1028+
requestedIpOwnershipMarkerAdded = true;
1029+
}
10231030
} catch (Exception e) {
10241031
throw new CloudRuntimeException(String.format(
10251032
"Failed to record NSX VPN ownership for requested IP %s of VPC %s",
@@ -1045,7 +1052,7 @@ public IpAddress acquireVpnGatewayIp(Vpc vpc, IpAddress requestedIp) {
10451052
} else if (autoAcquired) {
10461053
logger.warn("Retaining auto-acquired VPN gateway IP {} for VPC {} because the NSX endpoint may still be using it",
10471054
ip.getAddress(), vpc.getName());
1048-
} else if (!endpointMayBeInUse) {
1055+
} else if (!endpointMayBeInUse && requestedIpOwnershipMarkerAdded) {
10491056
try {
10501057
userIpAddressDetailsDao.removeDetail(ip.getId(), NSX_VPN_GATEWAY_IP_DETAIL);
10511058
} catch (Exception cleanupException) {

‎plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxVpnGatewayOwnershipTest.java‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package org.apache.cloudstack.service;
18+
19+
import com.cloud.network.IpAddress;
20+
import com.cloud.network.Network;
21+
import com.cloud.network.dao.FirewallRulesDao;
22+
import com.cloud.network.dao.IPAddressDao;
23+
import com.cloud.network.dao.IPAddressVO;
24+
import com.cloud.network.dao.LoadBalancerDao;
25+
import com.cloud.network.nsx.NsxVpnGatewayResult;
26+
import com.cloud.network.rules.dao.PortForwardingRulesDao;
27+
import com.cloud.network.vpc.VpcManager;
28+
import com.cloud.network.vpc.VpcVO;
29+
import com.cloud.utils.exception.CloudRuntimeException;
30+
import com.cloud.utils.net.Ip;
31+
import org.apache.cloudstack.resourcedetail.UserIpAddressDetailVO;
32+
import org.apache.cloudstack.resourcedetail.dao.UserIpAddressDetailsDao;
33+
import org.junit.Assert;
34+
import org.junit.Before;
35+
import org.junit.Test;
36+
import org.junit.runner.RunWith;
37+
import org.mockito.Mock;
38+
import org.mockito.junit.MockitoJUnitRunner;
39+
40+
import java.util.List;
41+
42+
import static org.mockito.ArgumentMatchers.anyBoolean;
43+
import static org.mockito.ArgumentMatchers.anyLong;
44+
import static org.mockito.ArgumentMatchers.anyString;
45+
import static org.mockito.Mockito.never;
46+
import static org.mockito.Mockito.verify;
47+
import static org.mockito.Mockito.when;
48+
49+
@RunWith(MockitoJUnitRunner.class)
50+
public class NsxVpnGatewayOwnershipTest {
51+
52+
private static final long VPC_ID = 9L;
53+
private static final long IP_ADDRESS_ID = 20L;
54+
private static final String IP_ADDRESS = "203.0.113.20";
55+
56+
@Mock
57+
private NsxServiceImpl nsxService;
58+
@Mock
59+
private VpcManager vpcManager;
60+
@Mock
61+
private IPAddressDao ipAddressDao;
62+
@Mock
63+
private UserIpAddressDetailsDao userIpAddressDetailsDao;
64+
@Mock
65+
private FirewallRulesDao firewallRulesDao;
66+
@Mock
67+
private PortForwardingRulesDao portForwardingRulesDao;
68+
@Mock
69+
private LoadBalancerDao loadBalancerDao;
70+
@Mock
71+
private VpcVO vpc;
72+
@Mock
73+
private IpAddress requestedIp;
74+
@Mock
75+
private IPAddressVO ipAddress;
76+
77+
private NsxElement element;
78+
79+
@Before
80+
public void setUp() {
81+
element = new NsxElement();
82+
element.nsxService = nsxService;
83+
element.vpcManager = vpcManager;
84+
element.ipAddressDao = ipAddressDao;
85+
element.userIpAddressDetailsDao = userIpAddressDetailsDao;
86+
element.firewallRulesDao = firewallRulesDao;
87+
element.portForwardingRulesDao = portForwardingRulesDao;
88+
element.loadBalancerDao = loadBalancerDao;
89+
90+
when(vpc.getId()).thenReturn(VPC_ID);
91+
when(vpc.getName()).thenReturn("vpc");
92+
when(vpcManager.isProviderSupportServiceInVpc(VPC_ID, Network.Service.Vpn, Network.Provider.Nsx))
93+
.thenReturn(true);
94+
when(requestedIp.getId()).thenReturn(IP_ADDRESS_ID);
95+
when(ipAddressDao.findById(IP_ADDRESS_ID)).thenReturn(ipAddress);
96+
when(ipAddress.getId()).thenReturn(IP_ADDRESS_ID);
97+
when(ipAddress.getVpcId()).thenReturn(VPC_ID);
98+
when(ipAddress.readyToUse()).thenReturn(true);
99+
when(ipAddress.getAddress()).thenReturn(new Ip(IP_ADDRESS));
100+
when(firewallRulesDao.listByIpAndNotRevoked(IP_ADDRESS_ID)).thenReturn(List.of());
101+
when(portForwardingRulesDao.listByIpAndNotRevoked(IP_ADDRESS_ID)).thenReturn(List.of());
102+
when(loadBalancerDao.listByIpAddress(IP_ADDRESS_ID)).thenReturn(List.of());
103+
}
104+
105+
@Test
106+
public void testExistingOwnershipMarkerIsNotOverwrittenOrRemovedOnFailure() {
107+
UserIpAddressDetailVO existingDetail = new UserIpAddressDetailVO(
108+
IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL, "true", false);
109+
when(userIpAddressDetailsDao.findDetail(IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL))
110+
.thenReturn(existingDetail);
111+
when(nsxService.createVpnGateway(vpc, IP_ADDRESS)).thenReturn(new NsxVpnGatewayResult(false, false));
112+
113+
Assert.assertThrows(CloudRuntimeException.class,
114+
() -> element.acquireVpnGatewayIp(vpc, requestedIp));
115+
116+
verify(userIpAddressDetailsDao, never()).addDetail(anyLong(), anyString(), anyString(), anyBoolean());
117+
verify(userIpAddressDetailsDao, never()).removeDetail(IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL);
118+
}
119+
120+
@Test
121+
public void testTemporaryOwnershipMarkerIsRemovedAfterUnambiguousFailure() {
122+
when(userIpAddressDetailsDao.findDetail(IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL))
123+
.thenReturn(null);
124+
when(nsxService.createVpnGateway(vpc, IP_ADDRESS)).thenReturn(new NsxVpnGatewayResult(false, false));
125+
126+
Assert.assertThrows(CloudRuntimeException.class,
127+
() -> element.acquireVpnGatewayIp(vpc, requestedIp));
128+
129+
verify(userIpAddressDetailsDao).addDetail(
130+
IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL, "false", false);
131+
verify(userIpAddressDetailsDao).removeDetail(IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL);
132+
}
133+
134+
@Test
135+
public void testTemporaryOwnershipMarkerIsRetainedAfterAmbiguousFailure() {
136+
when(userIpAddressDetailsDao.findDetail(IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL))
137+
.thenReturn(null);
138+
when(nsxService.createVpnGateway(vpc, IP_ADDRESS)).thenReturn(new NsxVpnGatewayResult(false, true));
139+
140+
Assert.assertThrows(CloudRuntimeException.class,
141+
() -> element.acquireVpnGatewayIp(vpc, requestedIp));
142+
143+
verify(userIpAddressDetailsDao).addDetail(
144+
IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL, "false", false);
145+
verify(userIpAddressDetailsDao, never()).removeDetail(IP_ADDRESS_ID, NsxElement.NSX_VPN_GATEWAY_IP_DETAIL);
146+
}
147+
}

0 commit comments

Comments
 (0)