Copilot commented on code in PR #13585:
URL: https://github.com/apache/cloudstack/pull/13585#discussion_r3558595194
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java:
##########
@@ -36,41 +42,201 @@
public class OntapStorageUtils {
private static final Logger logger =
LogManager.getLogger(OntapStorageUtils.class);
-
private static final String BASIC = "Basic";
private static final String AUTH_HEADER_COLON = ":";
+ /**
+ * Method generates authentication headers using storage backend
credentials passed as normal string
+ *
+ * @param username -->> username of the storage backend
+ * @param password -->> normal decoded password of the storage backend
+ * @return
+ */
public static String generateAuthHeader (String username, String password)
{
byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON
+ password).getBytes(StandardCharsets.UTF_8));
return BASIC + StringUtils.SPACE + new String(encodedBytes);
}
+ public static CloudStackVolume
createCloudStackVolumeRequestByProtocol(StoragePoolVO storagePool, Map<String,
String> details, DataObject volumeObject) {
+ CloudStackVolume cloudStackVolumeRequest = null;
+
+ String protocol = details.get(OntapStorageConstants.PROTOCOL);
+ ProtocolType protocolType = ProtocolType.valueOf(protocol);
+ switch (protocolType) {
+ case NFS3:
+ cloudStackVolumeRequest = new CloudStackVolume();
+
cloudStackVolumeRequest.setDatastoreId(String.valueOf(storagePool.getId()));
+ cloudStackVolumeRequest.setVolumeInfo(volumeObject);
+ break;
+ case ISCSI:
+ Svm svm = new Svm();
+ svm.setName(details.get(OntapStorageConstants.SVM_NAME));
+ cloudStackVolumeRequest = new CloudStackVolume();
+ Lun lunRequest = new Lun();
+ lunRequest.setSvm(svm);
+
+ LunSpace lunSpace = new LunSpace();
+ lunSpace.setSize(volumeObject.getSize());
+ lunRequest.setSpace(lunSpace);
+ //Lun name is full path like in unified
"/vol/VolumeName/LunName"
+ String lunName =
volumeObject.getName().replace(OntapStorageConstants.HYPHEN,
OntapStorageConstants.UNDERSCORE);
+ if(!isValidName(lunName)) {
+ String errMsg = "createAsync: Invalid dataObject name [" +
lunName + "]. It must start with a letter and can only contain letters, digits,
and underscores, and be up to 200 characters long.";
+ throw new InvalidParameterValueException(errMsg);
+ }
+ String lunFullName = getLunName(storagePool.getName(),
lunName);
+ lunRequest.setName(lunFullName);
+
+ String osType =
getOSTypeFromHypervisor(storagePool.getHypervisor().name());
+ lunRequest.setOsType(Lun.OsTypeEnum.valueOf(osType));
+
+ cloudStackVolumeRequest.setLun(lunRequest);
+ break;
+ default:
+ throw new CloudRuntimeException("Unsupported protocol " +
protocol);
+
+ }
+ return cloudStackVolumeRequest;
+ }
+
+ public static boolean isValidName(String name) {
+ // Check for null and length constraint first
+ if (name == null || name.length() > 200) {
+ return false;
+ }
+ // Regex: Starts with a letter, followed by letters, digits, or
underscores
+ return name.matches(OntapStorageConstants.ONTAP_NAME_REGEX);
+ }
+
+ public static String getOSTypeFromHypervisor(String hypervisorType) {
+ switch (hypervisorType) {
+ case OntapStorageConstants.KVM:
+ return Lun.OsTypeEnum.LINUX.name();
+ default:
+ String errMsg = "getOSTypeFromHypervisor : Unsupported
hypervisor type " + hypervisorType + " for ONTAP storage";
+ logger.error(errMsg);
+ throw new CloudRuntimeException(errMsg);
+ }
+ }
+
+ /**
+ * Returns a connected {@link StorageStrategy} for operations on an
existing pool (snapshots,
+ * delete, revert, grant/revoke). Does not require aggregate free space
for the full pool size.
+ */
public static StorageStrategy getStrategyByStoragePoolDetails(Map<String,
String> details) {
+ return getStrategyByStoragePoolDetails(details, false);
+ }
+
+ public static StorageStrategy getStrategyByStoragePoolDetails(Map<String,
String> details,
+ boolean validateAggregatesForVolumeCreation) {
if (details == null || details.isEmpty()) {
logger.error("getStrategyByStoragePoolDetails: Storage pool
details are null or empty");
- throw new CloudRuntimeException("getStrategyByStoragePoolDetails:
Storage pool details are null or empty");
+ throw new CloudRuntimeException("Storage pool details are null or
empty");
}
String protocol = details.get(OntapStorageConstants.PROTOCOL);
OntapStorage ontapStorage = new
OntapStorage(details.get(OntapStorageConstants.USERNAME),
details.get(OntapStorageConstants.PASSWORD),
- details.get(OntapStorageConstants.MANAGEMENT_LIF),
details.get(OntapStorageConstants.SVM_NAME),
Long.parseLong(details.get(OntapStorageConstants.SIZE)),
- ProtocolType.valueOf(protocol),
-
Boolean.parseBoolean(details.get(OntapStorageConstants.IS_DISAGGREGATED)));
+ details.get(OntapStorageConstants.STORAGE_IP),
details.get(OntapStorageConstants.SVM_NAME),
Long.parseLong(details.get(OntapStorageConstants.SIZE)),
+ ProtocolType.valueOf(protocol));
StorageStrategy storageStrategy =
StorageProviderFactory.getStrategy(ontapStorage);
- boolean isValid = storageStrategy.connect();
+ boolean isValid =
storageStrategy.connect(validateAggregatesForVolumeCreation);
if (isValid) {
logger.info("Connection to Ontap SVM [{}] successful",
details.get(OntapStorageConstants.SVM_NAME));
return storageStrategy;
} else {
logger.error("getStrategyByStoragePoolDetails: Connection to Ontap
SVM [" + details.get(OntapStorageConstants.SVM_NAME) + "] failed");
- throw new CloudRuntimeException("getStrategyByStoragePoolDetails:
Connection to Ontap SVM [" + details.get(OntapStorageConstants.SVM_NAME) + "]
failed");
+ throw new CloudRuntimeException("Connection to Ontap SVM [" +
details.get(OntapStorageConstants.SVM_NAME) + "] failed");
}
}
- public static String getIgroupName(String svmName, ScopeType scopeType,
Long scopeId) {
- return OntapStorageConstants.CS + OntapStorageConstants.UNDERSCORE +
svmName + OntapStorageConstants.UNDERSCORE + scopeType.toString().toLowerCase()
+ OntapStorageConstants.UNDERSCORE + scopeId;
+ public static String getIgroupName(String svmName, String hostUuid) {
+ //Igroup name format: cs_hostUuid_svmName
+ String sanitizedHostUuid = hostUuid.replaceAll("[^a-zA-Z0-9_-]", "_");
+ String igroupName = OntapStorageConstants.CS +
OntapStorageConstants.UNDERSCORE + sanitizedHostUuid +
OntapStorageConstants.UNDERSCORE + svmName;
+ // ONTAP igroup names are limited to 96 characters; truncate if longer.
+ if (igroupName.length() >
OntapStorageConstants.IGROUP_NAME_MAX_LENGTH) {
+ igroupName = igroupName.substring(0,
OntapStorageConstants.IGROUP_NAME_MAX_LENGTH);
+ }
+ return igroupName;
}
Review Comment:
getIgroupName() still throws NullPointerException when hostUuid is null
(hostUuid.replaceAll(...)). The PR description calls out this NPE; this method
should validate inputs (and ideally fail fast with a clear error) before
sanitizing/truncating.
##########
ui/src/views/infra/zone/ZoneWizardLaunchZone.vue:
##########
@@ -1604,6 +1604,18 @@ export default {
params.url = this.powerflexURL(this.prefillContent.powerflexGateway,
this.prefillContent.powerflexGatewayUsername,
this.prefillContent.powerflexGatewayPassword,
this.prefillContent.powerflexStoragePool)
}
+ if (this.prefillContent.provider === 'NetApp ONTAP') {
+ params['details[0].storageIP'] = this.prefillContent.ontapIP
+ params['details[0].username'] = this.prefillContent.ontapUsername
+ params['details[0].password'] = btoa(this.prefillContent.ontapPassword)
+ params['details[0].svmName'] = this.prefillContent.ontapSvmName
+ params['details[0].protocol'] =
this.prefillContent.primaryStorageProtocol
Review Comment:
Using btoa() directly for the ONTAP password is fragile: it throws for
non-Latin1 input and bypasses the UI’s existing base64+UTF-8 helper used
elsewhere. Consider using $toBase64AndURIEncoded (ui/src/utils/plugins.js) for
consistent encoding.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/provider/StorageProviderFactory.java:
##########
@@ -36,23 +39,23 @@ public class StorageProviderFactory {
public static StorageStrategy getStrategy(OntapStorage ontapStorage) {
ProtocolType protocol = ontapStorage.getProtocol();
logger.info("Initializing StorageProviderFactory with protocol: " +
protocol);
+ String decodedPassword = new
String(java.util.Base64.getDecoder().decode(ontapStorage.getPassword()),
StandardCharsets.UTF_8);
+ ontapStorage = new OntapStorage(
+ ontapStorage.getUsername(),
+ decodedPassword,
+ ontapStorage.getStorageIP(),
+ ontapStorage.getSvmName(),
+ ontapStorage.getSize(),
+ protocol);
Review Comment:
StorageProviderFactory decodes ontapStorage.getPassword() as Base64
unconditionally. If the stored value is not valid Base64 (e.g. created by older
code paths / API clients), this will throw IllegalArgumentException and break
all ONTAP operations at runtime.
##########
ui/src/views/infra/AddPrimaryStorage.vue:
##########
@@ -890,6 +935,14 @@ export default {
params['details[0].api_username'] = values.flashArrayUsername
params['details[0].api_password'] = values.flashArrayPassword
url = values.flashArrayURL
+ } else if (values.provider === 'NetApp ONTAP') {
+ params['details[0].storageIP'] = values.ontapIP
+ params['details[0].username'] = values.ontapUsername
+ params['details[0].password'] = btoa(values.ontapPassword)
+ params['details[0].svmName'] = values.ontapSvmName
+ params['details[0].protocol'] = values.protocol
+ values.managed = true
+ url = this.ontapURL(values.ontapIP)
}
Review Comment:
ONTAP password is encoded with btoa(), which is not UTF-8 safe and can throw
for non-Latin1 characters. The UI already provides $toBase64AndURIEncoded for
consistent base64 encoding across forms.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -207,67 +217,54 @@ private long validateInitializeInputs(Long capacityBytes,
Long podId, Long clust
capacityBytes = ONTAP_MIN_VOLUME_SIZE_IN_BYTES;
}
- // Scope (pod/cluster/zone) validation
+ // Validate scope
if (podId == null ^ clusterId == null) {
throw new CloudRuntimeException("Cluster Id or Pod Id is null,
cannot create primary storage");
}
- if (podId == null && clusterId == null) {
- if (zoneId != null) {
- logger.info("Both Pod Id and Cluster Id are null, Primary
storage pool will be associated with a Zone");
- } else {
- throw new CloudRuntimeException("Pod Id, Cluster Id and Zone
Id are all null, cannot create primary storage");
- }
+
+ if (podId == null && zoneId == null) {
+ throw new CloudRuntimeException("Pod Id, Cluster Id and Zone Id
are all null, cannot create primary storage");
+ }
+
+ if (podId == null) {
+ logger.info("Both Pod Id and Cluster Id are null, Primary storage
pool will be associated with a Zone");
}
- // Basic parameter validation
if (StringUtils.isBlank(storagePoolName)) {
throw new CloudRuntimeException("Storage pool name is null or
empty, cannot create primary storage");
}
+
if (StringUtils.isBlank(providerName)) {
throw new CloudRuntimeException("Provider name is null or empty,
cannot create primary storage");
}
+
logger.debug("ONTAP primary storage will be created as " + (managed ?
"managed" : "unmanaged"));
if (!managed) {
throw new CloudRuntimeException("ONTAP primary storage must be
managed");
}
- // Details key validation
+ //Required ONTAP detail keys
Set<String> requiredKeys = Set.of(
OntapStorageConstants.USERNAME,
OntapStorageConstants.PASSWORD,
OntapStorageConstants.SVM_NAME,
OntapStorageConstants.PROTOCOL,
- OntapStorageConstants.MANAGEMENT_LIF
- );
- Set<String> optionalKeys = Set.of(
- OntapStorageConstants.IS_DISAGGREGATED
+ OntapStorageConstants.STORAGE_IP
);
- Set<String> allowedKeys = new java.util.HashSet<>(requiredKeys);
- allowedKeys.addAll(optionalKeys);
-
- if (StringUtils.isNotBlank(url)) {
- for (String segment : url.split(OntapStorageConstants.SEMICOLON)) {
- if (segment.isEmpty()) {
- continue;
- }
- String[] kv = segment.split(OntapStorageConstants.EQUALS, 2);
- if (kv.length == 2) {
- details.put(kv[0].trim(), kv[1].trim());
- }
- }
- }
+ // Validate existing entries (reject unexpected keys, empty values)
for (Map.Entry<String, String> e : details.entrySet()) {
String key = e.getKey();
String val = e.getValue();
- if (!allowedKeys.contains(key)) {
+ if (!requiredKeys.contains(key)) {
throw new CloudRuntimeException("Unexpected ONTAP detail key
in URL: " + key);
}
if (StringUtils.isBlank(val)) {
throw new CloudRuntimeException("ONTAP primary storage
creation failed, empty detail: " + key);
}
Review Comment:
validateInitializeInputs() throws "Unexpected ONTAP detail key in URL" but
the code no longer parses ONTAP details from the URL; it validates the provided
details map. This error message is misleading, and details can also be null
(causing NPE) since it is not validated before iterating.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -87,6 +97,19 @@ CloudStackVolume updateCloudStackVolume(CloudStackVolume
cloudstackVolume) {
@Override
public void deleteCloudStackVolume(CloudStackVolume cloudstackVolume) {
+ logger.info("deleteCloudStackVolume: Delete cloudstack volume " +
cloudstackVolume);
+ try {
+ // Step 1: Send command to KVM host to delete qcow2 file using
qemu-img
+ Answer answer =
deleteVolumeOnKVMHost(cloudstackVolume.getVolumeInfo());
+ if (answer == null || !answer.getResult()) {
+ String errMsg = answer != null ? answer.getDetails() : "Failed
to delete qcow2 on KVM host";
+ logger.error("deleteCloudStackVolume: " + errMsg);
+ throw new CloudRuntimeException(errMsg);
+ }
+ } catch (Exception e) {
+ logger.error("deleteCloudStackVolume: error occured " + e);
+ throw new CloudRuntimeException(e);
+ }
Review Comment:
Same logging issue as createCloudStackVolume(): the catch block logs "error
occured" (typo) and drops stack trace by string concatenation. Use
logger.error(..., e).
##########
engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreProvider.java:
##########
@@ -29,6 +29,11 @@ public interface DataStoreProvider {
String SAMPLE_IMAGE = "Sample";
String SMB = "NFS";
String DEFAULT_PRIMARY = "DefaultPrimary";
+ /**
+ * Primary storage provider name for NetApp ONTAP ({@code
OntapPrimaryDatastoreProvider#getName}).
+ * Single canonical value; use this instead of duplicating the string
across modules.
+ */
+ String ONTAP_PLUGIN_NAME = "NetApp ONTAP";
Review Comment:
This introduces a new canonical provider name value ("NetApp ONTAP"). If any
existing installations have storage_pool.storage_provider_name persisted as the
previous ONTAP plugin name (the ONTAP plugin previously used "ONTAP" in
OntapStorageConstants), equality checks against
DataStoreProvider.ONTAP_PLUGIN_NAME will stop matching and can break
snapshot/managed-storage logic after upgrade. Consider supporting both legacy
and new names (or adding a DB migration) to avoid upgrade regressions.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -77,7 +72,22 @@ public void setOntapStorage(OntapStorage ontapStorage) {
@Override
public CloudStackVolume createCloudStackVolume(CloudStackVolume
cloudstackVolume) {
- return null;
+ logger.info("createCloudStackVolume: Create cloudstack volume " +
cloudstackVolume);
+ try {
+ // Step 1: set cloudstack volume metadata
+ updateCloudStackVolumeMetadata(cloudstackVolume.getDatastoreId(),
cloudstackVolume.getVolumeInfo());
+ // Step 2: Send command to KVM host to create qcow2 file using
qemu-img
+ Answer answer =
createVolumeOnKVMHost(cloudstackVolume.getVolumeInfo());
+ if (answer == null || !answer.getResult()) {
+ String errMsg = answer != null ? answer.getDetails() : "Failed
to create qcow2 on KVM host";
+ logger.error("createCloudStackVolume: " + errMsg);
+ throw new CloudRuntimeException(errMsg);
+ }
+ return cloudstackVolume;
+ }catch (Exception e) {
+ logger.error("createCloudStackVolume: error occured " + e);
+ throw new CloudRuntimeException(e);
+ }
Review Comment:
The catch block logs "error occured" (typo) and drops the stack trace by
concatenating the exception into the message. Prefer structured logging with
the throwable so the stack trace is preserved.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -158,6 +159,56 @@ public boolean connectPhysicalDisk(String volumeUuid,
KVMStoragePool pool, Map<S
return true;
}
+ /**
+ * Checks the result of an iscsiadm node-create command.
+ * Returns true if the node was created or already exists, false on
failure.
+ */
+ boolean handleNodeCreateResult(String result, String volumeUuid) {
+ if (result == null) {
+ logger.debug("Successfully added iSCSI node for target {}",
volumeUuid);
+ return true;
+ }
+ String msg = result.toLowerCase();
+ if (msg.contains("already exists") || msg.contains("database exists")
|| msg.contains("exists")) {
+ logger.debug("iSCSI node already exists for target {},
proceeding", volumeUuid);
+ return true;
+ }
Review Comment:
handleNodeCreateResult() treats any iscsiadm output containing "exists" as a
success. This can incorrectly mark failures as success for messages like "does
not exist" (contains "exist"). Restrict the check to the known benign
node-exists strings.
--
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]
