Copilot commented on code in PR #12966:
URL: https://github.com/apache/cloudstack/pull/12966#discussion_r3043138158
##########
api/src/main/java/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java:
##########
@@ -153,6 +160,13 @@ private Long getProjectId() {
return projectId;
}
+ public Long getStorageId() {
+ if (snapshotId != null && storageId != null) {
+ throw new IllegalArgumentException("StorageId parameter cannot be
specified with the SnapshotId parameter.");
+ }
+ return storageId;
+ }
Review Comment:
The mutual-exclusion check for snapshotId vs storageId is currently
ineffective: when snapshotId is provided, createVolume() never calls
getStorageId(), so a request with both parameters will silently ignore
storageid and proceed down the snapshot path.
To ensure the API contract is enforced, validate this combination in a code
path that always runs (e.g., in CreateVolumeCmd.create()/execute() or in
VolumeApiServiceImpl.allocVolume/createVolume before branching) and throw an
InvalidParameterValueException/ServerApiException rather than letting the
conflict slip through.
##########
api/src/main/java/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java:
##########
@@ -109,6 +110,12 @@ public class CreateVolumeCmd extends
BaseAsyncCreateCustomIdCmd implements UserC
description = "The ID of the Instance; to be used with snapshot
Id, Instance to which the volume gets attached after creation")
private Long virtualMachineId;
+ @Parameter(name = ApiConstants.STORAGE_ID,
+ type = CommandType.UUID,
+ entityType = StoragePoolResponse.class,
+ description = "Storage pool ID to create the volume in. Exclusive
with SnapshotId parameter.")
Review Comment:
storageid is a user-exposed parameter here, but there’s no indication it’s
restricted (no authorized=Admin) and the backend path in VolumeApiServiceImpl
doesn’t enforce pool-level access/compatibility checks. If this is intended
primarily for admin/migration workflows, consider restricting the parameter to
admins (as done in other user commands that expose storage pool IDs) or ensure
the service layer performs full access checks before acting on the provided
pool ID.
```suggestion
description = "Storage pool ID to create the volume in.
Exclusive with SnapshotId parameter.",
authorized = {RoleType.Admin})
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -1074,6 +1085,8 @@ public VolumeVO createVolume(CreateVolumeCmd cmd) {
throw new CloudRuntimeException(message.toString());
}
}
+ } else if (cmd.getStorageId() != null) {
+ volume = allocateVolumeOnStorage(cmd.getEntityId(),
cmd.getStorageId());
Review Comment:
New storageid-driven creation path in createVolume()/allocateVolumeOnStorage
isn’t covered by existing unit tests (there are tests for VolumeApiServiceImpl
in server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java, but
none exercise this new branch). Adding a unit test that verifies pool
validation + volService.createVolumeAsync invocation and failure handling would
help prevent regressions.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -1043,6 +1043,17 @@ public boolean validateVolumeSizeInBytes(long size) {
return true;
}
+ private VolumeVO allocateVolumeOnStorage(Long volumeId, Long storageId)
throws ExecutionException, InterruptedException {
+ DataStore destStore = dataStoreMgr.getDataStore(storageId,
DataStoreRole.Primary);
+ VolumeInfo destVolume = volFactory.getVolume(volumeId, destStore);
+ AsyncCallFuture<VolumeApiResult> createVolumeFuture =
volService.createVolumeAsync(destVolume, destStore);
+ VolumeApiResult createVolumeResult = createVolumeFuture.get();
+ if (createVolumeResult.isFailed()) {
+ throw new CloudRuntimeException("Creation of a dest volume failed:
" + createVolumeResult.getResult());
+ }
+ return _volsDao.findById(destVolume.getId());
Review Comment:
allocateVolumeOnStorage() doesn’t validate that the provided storageId
resolves to an existing/accessible primary storage pool
(dataStoreMgr.getDataStore may return null) and doesn’t check pool conditions
(e.g., in-maintenance/disabled, zone mismatch with the volume, storage access
groups). As-is, this can lead to NPEs and also allows callers to force volume
creation on arbitrary pools bypassing the normal allocation/compatibility
checks.
Consider resolving StoragePoolVO/PrimaryDataStore first, rejecting null /
in-maintenance, validating the pool is in the expected state (Ready/Up) and in
the same zone as the volume, and running the same compatibility checks used
elsewhere (e.g.,
storageMgr.checkIfReadyVolumeFitsInStoragePoolWithStorageAccessGroups and
doesStoragePoolSupportDiskOffering / capacity checks) before calling
volService.createVolumeAsync.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -1043,6 +1043,17 @@ public boolean validateVolumeSizeInBytes(long size) {
return true;
}
+ private VolumeVO allocateVolumeOnStorage(Long volumeId, Long storageId)
throws ExecutionException, InterruptedException {
+ DataStore destStore = dataStoreMgr.getDataStore(storageId,
DataStoreRole.Primary);
+ VolumeInfo destVolume = volFactory.getVolume(volumeId, destStore);
+ AsyncCallFuture<VolumeApiResult> createVolumeFuture =
volService.createVolumeAsync(destVolume, destStore);
+ VolumeApiResult createVolumeResult = createVolumeFuture.get();
Review Comment:
allocateVolumeOnStorage() is a bit misleadingly named: it actually triggers
backend creation (createVolumeAsync) and blocks waiting for completion.
Renaming it to reflect the side-effect (e.g., createVolumeOnStoragePool) would
make the control flow in createVolume() clearer and reduce confusion with
allocVolume(), which only persists DB state.
--
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]
