Skip to content

Ownership selection in VPC tiers and VPC public IPs #9692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
gp-santos wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Ownership selection in VPC tiers and VPC public IPs #9692

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c3d62ec
pr
gp-santos Sep 5, 2024
ffa6af3
add tier's account to listing
gp-santos Sep 6, 2024
f498429
add acl response check
gp-santos Sep 9, 2024
b9d656b
cleanup random change
gp-santos Sep 16, 2024
147a534
add tests
gp-santos Sep 18, 2024
68a3bfe
Apply suggestions from code review
gp-santos Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,7 @@ public class ApiConstants {
public static final String VPC_OFF_ID = "vpcofferingid";
public static final String VPC_OFF_NAME = "vpcofferingname";
public static final String NETWORK = "network";
public static final String VPC_ACCESS = "vpcaccess";
public static final String VPC_ID = "vpcid";
public static final String VPC_NAME = "vpcname";
public static final String GATEWAY_ID = "gatewayid";
Expand Down
2 changes: 1 addition & 1 deletion api/src/main/java/org/apache/cloudstack/api/command/user/network/ListNetworksCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ public void execute() {
private void updateNetworkResponse(List<NetworkResponse> response) {
for (NetworkResponse networkResponse : response) {
ResourceIcon resourceIcon = resourceIconManager.getByResourceTypeAndUuid(ResourceTag.ResourceObjectType.Network, networkResponse.getId());
if (resourceIcon == null && networkResponse.getVpcId() != null) {
if (resourceIcon == null && networkResponse.getVpcId() != null && networkResponse.getVpcAccess()) {
resourceIcon = resourceIconManager.getByResourceTypeAndUuid(ResourceTag.ResourceObjectType.Vpc, networkResponse.getVpcId());
}
if (resourceIcon == null) {
Expand Down
8 changes: 8 additions & 0 deletions api/src/main/java/org/apache/cloudstack/api/response/IPAddressResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ public class IPAddressResponse extends BaseResponseWithAnnotations implements Co
@Param(description = "purpose of the IP address. In Acton this value is not null for Ips with isSystem=true, and can have either StaticNat or LB value")
private String purpose;

@SerializedName(ApiConstants.VPC_ACCESS)
@Param(description = "Whether the calling account has access to this network's VPC", since = "4.20")
private boolean vpcAccess;

@SerializedName(ApiConstants.VPC_ID)
@Param(description = "VPC id the ip belongs to")
private String vpcId;
Expand Down Expand Up @@ -297,6 +301,10 @@ public void setPurpose(String purpose) {
this.purpose = purpose;
}

public void setVpcAccess(boolean vpcAccess) {
this.vpcAccess = vpcAccess;
}

public void setVpcId(String vpcId) {
this.vpcId = vpcId;
}
Expand Down
12 changes: 12 additions & 0 deletions api/src/main/java/org/apache/cloudstack/api/response/NetworkResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,10 @@ public class NetworkResponse extends BaseResponseWithAssociatedNetwork implement
@Param(description = "true if network supports specifying ip ranges, false otherwise")
private Boolean specifyIpRanges;

@SerializedName(ApiConstants.VPC_ACCESS)
@Param(description = "Whether the calling account has access to this network's VPC", since = "4.20")
private Boolean vpcAccess;

@SerializedName(ApiConstants.VPC_ID)
@Param(description = "VPC the network belongs to")
private String vpcId;
Expand Down Expand Up @@ -491,6 +495,14 @@ public void setSpecifyIpRanges(Boolean specifyIpRanges) {
this.specifyIpRanges = specifyIpRanges;
}

public void setVpcAccess(boolean vpcAccess) {
this.vpcAccess = vpcAccess;
}

public Boolean getVpcAccess() {
return vpcAccess;
}

public void setVpcId(String vpcId) {
this.vpcId = vpcId;
}
Expand Down
60 changes: 38 additions & 22 deletions server/src/main/java/com/cloud/api/ApiResponseHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ public IPAddressResponse createIPAddressResponse(ResponseView view, IpAddress ip
}


setVpcIdInResponse(ipAddr.getVpcId(), ipResponse::setVpcId, ipResponse::setVpcName);
setVpcIdInResponse(ipAddr.getVpcId(), ipResponse::setVpcId, ipResponse::setVpcName, ipResponse::setVpcAccess);


// Network id the ip is associated with (if associated networkId is
Expand Down Expand Up @@ -1144,20 +1144,43 @@ public IPAddressResponse createIPAddressResponse(ResponseView view, IpAddress ip
return ipResponse;
}

protected void setVpcIdInResponse(Long vpcId, Consumer<String> vpcUuidSetter, Consumer<String> vpcNameSetter, Consumer<Boolean> vpcAccessSetter) {
if (vpcId == null) {
return;
}
Vpc vpc = ApiDBUtils.findVpcById(vpcId);
if (vpc == null) {
return;
}

private void setVpcIdInResponse(Long vpcId, Consumer<String> vpcUuidSetter, Consumer<String> vpcNameSetter) {
if (vpcId != null) {
Vpc vpc = ApiDBUtils.findVpcById(vpcId);
if (vpc != null) {
try {
_accountMgr.checkAccess(CallContext.current().getCallingAccount(), null, false, vpc);
vpcUuidSetter.accept(vpc.getUuid());
} catch (PermissionDeniedException e) {
logger.debug("Not setting the vpcId to the response because the caller does not have access to the VPC");
}
vpcNameSetter.accept(vpc.getName());
}
try {
_accountMgr.checkAccess(CallContext.current().getCallingAccount(), null, false, vpc);
vpcAccessSetter.accept(true);
} catch (PermissionDeniedException e) {
vpcAccessSetter.accept(false);
logger.debug("Setting [%s] as false because the caller does not have access to the VPC [%s].", ApiConstants.VPC_ACCESS, vpc);
}
vpcNameSetter.accept(vpc.getName());
vpcUuidSetter.accept(vpc.getUuid());
}

protected void setAclIdInResponse(Network network, NetworkResponse response) {
if (network.getNetworkACLId() == null) {
return;
}

NetworkACL acl = ApiDBUtils.findByNetworkACLId(network.getNetworkACLId());
if (acl == null) {
return;
}

if (Boolean.FALSE.equals(response.getVpcAccess()) && acl.getVpcId() != 0) {
logger.debug("[{}] not set in response, since caller does not have access to it.", acl);
return;
}

response.setAclId(acl.getUuid());
response.setAclName(acl.getName());
}

private void showVmInfoForSharedNetworks(boolean forVirtualNetworks, IpAddress ipAddr, IPAddressResponse ipResponse) {
Expand Down Expand Up @@ -2657,7 +2680,8 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
response.setSpecifyIpRanges(network.getSpecifyIpRanges());


setVpcIdInResponse(network.getVpcId(), response::setVpcId, response::setVpcName);
setVpcIdInResponse(network.getVpcId(), response::setVpcId, response::setVpcName, response::setVpcAccess);
setAclIdInResponse(network, response);

setResponseAssociatedNetworkInformation(response, network.getId());

Expand All @@ -2674,14 +2698,6 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
response.setHasAnnotation(annotationDao.hasAnnotations(network.getUuid(), AnnotationService.EntityType.NETWORK.name(),
_accountMgr.isRootAdmin(CallContext.current().getCallingAccount().getId())));

if (network.getNetworkACLId() != null) {
NetworkACL acl = ApiDBUtils.findByNetworkACLId(network.getNetworkACLId());
if (acl != null) {
response.setAclId(acl.getUuid());
response.setAclName(acl.getName());
}
}

response.setStrechedL2Subnet(network.isStrechedL2Network());
if (network.isStrechedL2Network()) {
Set<String> networkSpannedZones = new HashSet<String>();
Expand Down
147 changes: 147 additions & 0 deletions server/src/test/java/com/cloud/api/ApiResponseHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import com.cloud.capacity.Capacity;
import com.cloud.configuration.Resource;
import com.cloud.domain.DomainVO;
import com.cloud.exception.PermissionDeniedException;
import com.cloud.network.Network;
import com.cloud.network.PublicIpQuarantine;
import com.cloud.network.as.AutoScaleVmGroup;
import com.cloud.network.as.AutoScaleVmGroupVO;
Expand All @@ -29,6 +31,8 @@
import com.cloud.network.dao.LoadBalancerVO;
import com.cloud.network.dao.NetworkServiceMapDao;
import com.cloud.network.dao.NetworkVO;
import com.cloud.network.vpc.NetworkACL;
import com.cloud.network.vpc.VpcVO;
import com.cloud.storage.VMTemplateVO;
import com.cloud.usage.UsageVO;
import com.cloud.user.Account;
Expand All @@ -46,6 +50,7 @@
import org.apache.cloudstack.api.response.AutoScaleVmProfileResponse;
import org.apache.cloudstack.api.response.DirectDownloadCertificateResponse;
import org.apache.cloudstack.api.response.IpQuarantineResponse;
import org.apache.cloudstack.api.response.NetworkResponse;
import org.apache.cloudstack.api.response.NicSecondaryIpResponse;
import org.apache.cloudstack.api.response.UnmanagedInstanceResponse;
import org.apache.cloudstack.api.response.UsageRecordResponse;
Expand Down Expand Up @@ -105,6 +110,13 @@ public class ApiResponseHelperTest {
@Mock
IPAddressDao ipAddressDaoMock;

@Mock
VpcVO vpcVOMock;

@Mock
NetworkACL networkACLMock;


@Spy
@InjectMocks
ApiResponseHelper apiResponseHelper = new ApiResponseHelper();
Expand All @@ -123,6 +135,9 @@ public class ApiResponseHelperTest {

static long autoScaleUserId = 7L;

static final String A_NAME = "name";
static final String A_UUID = "021f94d4-73f9-4a9a-b003-1df9dd968a09";

@Before
public void injectMocks() throws SecurityException, NoSuchFieldException,
IllegalArgumentException, IllegalAccessException {
Expand Down Expand Up @@ -481,4 +496,136 @@ public void testCapacityListingForSingleNonGpuType() {
Assert.assertTrue(apiResponseHelper.capacityListingForSingleNonGpuType(List.of(c1, c2)));
Assert.assertFalse(apiResponseHelper.capacityListingForSingleNonGpuType(List.of(c1, c2, c3)));
}

@Test
public void setVpcIdInResponseTestNullVpcIdReturnNull() {
NetworkResponse networkResponse = new NetworkResponse();

apiResponseHelper.setVpcIdInResponse(null, networkResponse::setVpcId, networkResponse::setVpcName, networkResponse::setVpcAccess);
Assert.assertNull(networkResponse.getVpcId());
Assert.assertNull(networkResponse.getVpcName());
Assert.assertNull(networkResponse.getVpcAccess());
}

@Test
public void setVpcIdInResponseTestNullVpcReturnNull() {
NetworkResponse networkResponse = new NetworkResponse();

try (MockedStatic<ApiDBUtils> utils = Mockito.mockStatic(ApiDBUtils.class)) {
utils.when(() -> ApiDBUtils.findVpcById(1L)).thenReturn(null);
apiResponseHelper.setVpcIdInResponse(1L, networkResponse::setVpcId, networkResponse::setVpcName, networkResponse::setVpcAccess);
}
Assert.assertNull(networkResponse.getVpcId());
Assert.assertNull(networkResponse.getVpcName());
Assert.assertNull(networkResponse.getVpcAccess());
}

@Test
public void setVpcIdInResponseCallerHasAccessReturnVpcAccessTrueAndVpcIdAndVpcName() {
NetworkResponse networkResponse = new NetworkResponse();
Mockito.when(vpcVOMock.getName()).thenReturn(A_NAME);
Mockito.when(vpcVOMock.getUuid()).thenReturn(A_UUID);

try (MockedStatic<ApiDBUtils> utils = Mockito.mockStatic(ApiDBUtils.class)) {
utils.when(() -> ApiDBUtils.findVpcById(1L)).thenReturn(vpcVOMock);
apiResponseHelper.setVpcIdInResponse(1L, networkResponse::setVpcId, networkResponse::setVpcName, networkResponse::setVpcAccess);
};
Assert.assertEquals(A_UUID, networkResponse.getVpcId());
Assert.assertEquals(A_NAME, networkResponse.getVpcName());
Assert.assertTrue(networkResponse.getVpcAccess());
}

@Test
public void setVpcIdInResponseCallerDoesNotHaveAccessReturnVpcAccessFalseAndVpcIdAndVpcName() {
NetworkResponse networkResponse = new NetworkResponse();
Mockito.when(vpcVOMock.getName()).thenReturn(A_NAME);
Mockito.when(vpcVOMock.getUuid()).thenReturn(A_UUID);

try (MockedStatic<ApiDBUtils> utils = Mockito.mockStatic(ApiDBUtils.class)) {
utils.when(() -> ApiDBUtils.findVpcById(1L)).thenReturn(vpcVOMock);
Mockito.doThrow(PermissionDeniedException.class).when(accountManagerMock).checkAccess(Mockito.any(), Mockito.any(), Mockito.anyBoolean(), Mockito.any());
apiResponseHelper.setVpcIdInResponse(1L, networkResponse::setVpcId, networkResponse::setVpcName, networkResponse::setVpcAccess);
};
Assert.assertEquals(A_UUID, networkResponse.getVpcId());
Assert.assertEquals(A_NAME, networkResponse.getVpcName());
Assert.assertFalse(networkResponse.getVpcAccess());
}

@Test
public void setAclIdInResponseTestNullNetworkAclIdReturnNull() {
NetworkResponse networkResponse = new NetworkResponse();
Network networkMock = Mockito.mock(Network.class);
Mockito.when(networkMock.getNetworkACLId()).thenReturn(null);

apiResponseHelper.setAclIdInResponse(networkMock, networkResponse);
Assert.assertNull(networkResponse.getAclId());
Assert.assertNull(networkResponse.getAclName());
}

@Test
public void setAclIdInResponseTestNullAclReturnNull() {
NetworkResponse networkResponse = new NetworkResponse();
Network networkMock = Mockito.mock(Network.class);
Mockito.when(networkMock.getNetworkACLId()).thenReturn(1L);

try (MockedStatic<ApiDBUtils> utils = Mockito.mockStatic(ApiDBUtils.class)) {
utils.when(() -> ApiDBUtils.findByNetworkACLId(1L)).thenReturn(null);
apiResponseHelper.setAclIdInResponse(networkMock, networkResponse);
}
Assert.assertNull(networkResponse.getAclId());
Assert.assertNull(networkResponse.getAclName());
}

@Test
public void setAclIdInResponseTestCallerDoesNotHaveAccessReturnNull() {
NetworkResponse networkResponse = new NetworkResponse();
networkResponse.setVpcAccess(false);
Network networkMock = Mockito.mock(Network.class);
Mockito.when(networkMock.getNetworkACLId()).thenReturn(1L);
Mockito.when(networkACLMock.getVpcId()).thenReturn(2L);

try (MockedStatic<ApiDBUtils> utils = Mockito.mockStatic(ApiDBUtils.class)) {
utils.when(() -> ApiDBUtils.findByNetworkACLId(1L)).thenReturn(networkACLMock);
apiResponseHelper.setAclIdInResponse(networkMock, networkResponse);
}
Assert.assertNull(networkResponse.getAclId());
Assert.assertNull(networkResponse.getAclName());
}

@Test
public void setAclIdInResponseTestCallerDoesNotHaveAccessButAclIsGlobalReturnAclIdAndAclName() {
NetworkResponse networkResponse = new NetworkResponse();
networkResponse.setVpcAccess(false);
Network networkMock = Mockito.mock(Network.class);
Mockito.when(networkMock.getNetworkACLId()).thenReturn(1L);
Mockito.when(networkACLMock.getVpcId()).thenReturn(0L);
Mockito.when(networkACLMock.getName()).thenReturn(A_NAME);
Mockito.when(networkACLMock.getUuid()).thenReturn(A_UUID);

try (MockedStatic<ApiDBUtils> utils = Mockito.mockStatic(ApiDBUtils.class)) {
utils.when(() -> ApiDBUtils.findByNetworkACLId(1L)).thenReturn(networkACLMock);
apiResponseHelper.setAclIdInResponse(networkMock, networkResponse);
}
Assert.assertEquals(A_UUID, networkResponse.getAclId());
Assert.assertEquals(A_NAME, networkResponse.getAclName());
}

@Test
public void setAclIdInResponseTestCallerHasAccessReturnAclIdAndAclName() {
NetworkResponse networkResponse = new NetworkResponse();
networkResponse.setVpcAccess(true);
Network networkMock = Mockito.mock(Network.class);
Mockito.when(networkMock.getNetworkACLId()).thenReturn(1L);
Mockito.lenient().when(networkACLMock.getVpcId()).thenReturn(2L);
Mockito.when(networkACLMock.getName()).thenReturn(A_NAME);
Mockito.when(networkACLMock.getUuid()).thenReturn(A_UUID);

try (MockedStatic<ApiDBUtils> utils = Mockito.mockStatic(ApiDBUtils.class)) {
utils.when(() -> ApiDBUtils.findByNetworkACLId(1L)).thenReturn(networkACLMock);
apiResponseHelper.setAclIdInResponse(networkMock, networkResponse);
}
Assert.assertEquals(A_UUID, networkResponse.getAclId());
Assert.assertEquals(A_NAME, networkResponse.getAclName());
}

}
3 changes: 2 additions & 1 deletion ui/src/components/view/InfoCard.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,8 @@
<resource-icon :image="getImage(images.vpc)" size="1x" style="margin-right: 5px"/>
</span>
<deployment-unit-outlined v-else />
<router-link :to="{ path: '/vpc/' + resource.vpcid }">{{ resource.vpcname || resource.vpcid }}</router-link>
<router-link v-if="resource.vpcaccess" :to="{ path: '/vpc/' + resource.vpcid }">{{ resource.vpcname || resource.vpcid }}</router-link>
<span v-else>{{ resource.vpcname || resource.vpcid }}</span>
</div>
</div>

Expand Down
2 changes: 1 addition & 1 deletion ui/src/components/view/ListView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@
<router-link :to="{ path: '/guestnetwork/' + record.associatednetworkid }">{{ text }}</router-link>
</template>
<template v-if="column.key === 'vpcname'">
<a v-if="record.vpcid">
<a v-if="record.vpcid && record.vpcaccess">
<router-link :to="{ path: '/vpc/' + record.vpcid }">{{ text || record.vpcid }}</router-link>
</a>
<span v-else>{{ text }}</span>
Expand Down
Loading