Copilot commented on code in PR #12650:
URL: https://github.com/apache/cloudstack/pull/12650#discussion_r2813695605
##########
server/src/main/java/com/cloud/template/TemplateManagerImpl.java:
##########
@@ -476,7 +476,8 @@ public DataStore getImageStore(String storeUuid, Long
zoneId, VolumeVO volume) {
}
if (imageStore == null) {
- throw new CloudRuntimeException(String.format("Cannot find an
image store for zone [%s].", zoneId));
+ logger.error("Cannot find an image store for zone [{}].", zoneId);
+ throw new CloudRuntimeException("Failed to create template. Please
contact the cloud administrator.");
Review Comment:
`getImageStore` is used for both template creation and volume upload (e.g.,
`VolumeApiServiceImpl.uploadVolume` calls `_tmpltMgr.getImageStore(...)`).
Throwing "Failed to create template..." here will surface an incorrect error
message to volume-upload callers. Consider using an operation-neutral message
(e.g., "Failed to find an image store. Please contact the cloud
administrator."), or passing a caller-specific context string into
`getImageStore` so each API reports the right high-level failure without
exposing the zone ID.
```suggestion
throw new CloudRuntimeException("Failed to find an image store.
Please contact the cloud administrator.");
```
##########
server/src/main/java/com/cloud/template/TemplateManagerImpl.java:
##########
@@ -476,7 +476,8 @@ public DataStore getImageStore(String storeUuid, Long
zoneId, VolumeVO volume) {
}
if (imageStore == null) {
- throw new CloudRuntimeException(String.format("Cannot find an
image store for zone [%s].", zoneId));
+ logger.error("Cannot find an image store for zone [{}].", zoneId);
+ throw new CloudRuntimeException("Failed to create template. Please
contact the cloud administrator.");
Review Comment:
There are existing unit tests for `getImageStore` in
`TemplateManagerImplTest`, but they currently only assert that an exception is
thrown. Since this change is specifically about preventing infrastructure/zone
ID leakage via exception messages, please add assertions that the thrown
`CloudRuntimeException` message does not include the zone ID (and matches the
new sanitized message).
##########
server/src/main/java/com/cloud/template/TemplateManagerImpl.java:
##########
@@ -1702,7 +1703,8 @@ public VirtualMachineTemplate
createPrivateTemplate(CreateTemplateCmd command) t
}
DataStore store =
_dataStoreMgr.getImageStoreWithFreeCapacity(zoneId);
if (store == null) {
- throw new CloudRuntimeException("cannot find an image store
for zone " + zoneId);
+ logger.error("Cannot find an image store for zone [{}].",
zoneId);
+ throw new CloudRuntimeException("Failed to create template.
Please contact the cloud administrator.");
Review Comment:
The same log+throw pattern and user-facing message is now duplicated here
and in `getImageStore`. To avoid the two sites drifting (especially if you
later tweak the sanitized message), consider factoring this into a small helper
(e.g., `throwImageStoreNotFound(zoneId, operation)`), which logs the detailed
message and throws the sanitized exception.
--
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]
