Copilot commented on code in PR #13053:
URL: https://github.com/apache/cloudstack/pull/13053#discussion_r3497116667
##########
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(
Review Comment:
StorageProviderFactory always Base64-decodes the password. If the password
is already plain-text (e.g., API/user input not base64, or older DB entries),
Base64.getDecoder().decode(...) will throw IllegalArgumentException and prevent
the ONTAP strategy from being created.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -207,67 +214,64 @@ 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");
}
+
+ PrimaryDataStoreParameters parameters = new
PrimaryDataStoreParameters();
+ if (clusterId != null) {
+ ClusterVO clusterVO = _clusterDao.findById(clusterId);
+ Preconditions.checkNotNull(clusterVO, "Unable to locate the
specified cluster");
+ if (clusterVO.getHypervisorType() !=
Hypervisor.HypervisorType.KVM) {
+ throw new CloudRuntimeException("ONTAP primary storage is
supported only for KVM hypervisor");
+ }
+ parameters.setHypervisorType(clusterVO.getHypervisorType());
+ }
Review Comment:
validateInitializeInputs() creates a PrimaryDataStoreParameters instance
only to set the hypervisor type, but that local variable is never used (the
method just returns capacityBytes). This is dead code and can be removed while
keeping the KVM validation.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -411,23 +396,49 @@ private boolean
validateProtocolSupportAndFetchHostsIdentifier(List<HostVO> host
for (HostVO host : hosts) {
if (host != null) {
ip = host.getStorageIpAddress() != null ?
host.getStorageIpAddress().trim() : "";
- if (ip.isEmpty()) {
- if (host.getPrivateIpAddress() == null ||
host.getPrivateIpAddress().trim().isEmpty()) {
- return false;
- }
- ip = host.getPrivateIpAddress().trim();
+ if (ip.isEmpty() &&
StringUtils.isBlank(host.getPrivateIpAddress() )) {
+ // TODO we will inform customer through alert for
excluded host because of protocol enabled on host
+ continue;
+ } else {
+ ip = ip.isEmpty() ?
host.getPrivateIpAddress().trim() : ip;
}
}
hostIdentifiers.add(ip);
}
break;
default:
- throw new
CloudRuntimeException("validateProtocolSupportAndFetchHostsIdentifier :
Unsupported protocol: " + protocolType.name());
+ throw new CloudRuntimeException("Unsupported protocol: " +
protocolType.name());
}
logger.info("validateProtocolSupportAndFetchHostsIdentifier: All hosts
support the protocol: " + protocolType.name());
return true;
}
+ /**
+ * Creates an NFS export policy (access group) on the ONTAP storage if the
protocol is NFS3
+ * and there are eligible hosts. Skipped for iSCSI (igroups are created
per-host in grantAccess).
+ */
+ private void createNfsAccessGroupIfNeeded(Map<String, String> details,
List<String> hostsIdentifier,
+ List<HostVO> hostsToConnect,
Scope scope,
+ StoragePoolVO storagePool,
StorageStrategy strategy) {
+ if
(!ProtocolType.NFS3.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL)))
{
+ return;
+ }
+ if (hostsIdentifier.isEmpty()) {
+ // No eligible hosts — export policy will be created later via
HostListener when hosts come up
+ return;
+ }
+ try {
+ AccessGroup accessGroupRequest = new AccessGroup();
+ accessGroupRequest.setHostsToConnect(hostsToConnect);
+ accessGroupRequest.setScope(scope);
+ accessGroupRequest.setStoragePoolId(storagePool.getId());
+ strategy.createAccessGroup(accessGroupRequest);
+ } catch (Exception e) {
+ logger.error("Failed to create NFS access group on storage for
pool {}: {}", storagePool.getName(), e.getMessage());
+ throw new CloudRuntimeException("Failed to create NFS access group
on storage for pool " + storagePool.getName() + ": " + e.getMessage());
+ }
Review Comment:
createNfsAccessGroupIfNeeded() passes the full hostsToConnect list into the
AccessGroup, even though validateProtocolSupportAndFetchHostsIdentifier() can
skip hosts (e.g., missing storage/private IP). That can lead to export-policy
rules being generated with null/blank client matches (e.g., "null/32").
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:
##########
@@ -411,23 +396,49 @@ private boolean
validateProtocolSupportAndFetchHostsIdentifier(List<HostVO> host
for (HostVO host : hosts) {
if (host != null) {
ip = host.getStorageIpAddress() != null ?
host.getStorageIpAddress().trim() : "";
- if (ip.isEmpty()) {
- if (host.getPrivateIpAddress() == null ||
host.getPrivateIpAddress().trim().isEmpty()) {
- return false;
- }
- ip = host.getPrivateIpAddress().trim();
+ if (ip.isEmpty() &&
StringUtils.isBlank(host.getPrivateIpAddress() )) {
+ // TODO we will inform customer through alert for
excluded host because of protocol enabled on host
+ continue;
+ } else {
+ ip = ip.isEmpty() ?
host.getPrivateIpAddress().trim() : ip;
}
}
hostIdentifiers.add(ip);
}
Review Comment:
validateProtocolSupportAndFetchHostsIdentifier() (NFS3 case) adds an IP to
hostIdentifiers even when host is null, and it can also append the previous
host's IP when a null host is encountered because `ip` is defined outside the
loop. This can result in incorrect/duplicate host identifiers and later invalid
export-policy rules.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -96,24 +119,40 @@ public void copyCloudStackVolume(CloudStackVolume
cloudstackVolume) {
@Override
public CloudStackVolume getCloudStackVolume(Map<String, String>
cloudStackVolumeMap) {
- return null;
+ logger.info("getCloudStackVolume: Get cloudstack volume " +
cloudStackVolumeMap);
+ CloudStackVolume cloudStackVolume = null;
+ FileInfo fileInfo =
getFile(cloudStackVolumeMap.get(OntapStorageConstants.VOLUME_UUID),cloudStackVolumeMap.get(OntapStorageConstants.FILE_PATH));
+
+ if (fileInfo != null) {
+ cloudStackVolume = new CloudStackVolume();
+
cloudStackVolume.setFlexVolumeUuid(cloudStackVolumeMap.get(OntapStorageConstants.VOLUME_UUID));
+ cloudStackVolume.setFile(fileInfo);
+ } else {
+ logger.warn("getCloudStackVolume: File not found for volume UUID:
{} and file path: {}",
cloudStackVolumeMap.get(OntapStorageConstants.VOLUME_UUID),
cloudStackVolumeMap.get(OntapStorageConstants.FILE_PATH));
+ }
+
Review Comment:
getCloudStackVolume() checks `if (fileInfo != null)` and logs a "File not
found" warning in the else branch, but getFile() always throws when the file is
missing (it never returns null). This makes the else branch unreachable and the
null-check redundant.
##########
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
+ params.managed = true
+ params.url = this.ontapURL(this.prefillContent.ontapIP)
+ if (this.prefillContent.capacityBytes &&
this.prefillContent.capacityBytes.length > 0) {
+ params.capacityBytes =
this.prefillContent.capacityBytes.split(',').join('')
+ }
+ }
Review Comment:
ONTAP password is Base64-encoded with btoa(). btoa only supports Latin1 and
can throw for non-ASCII passwords; additionally Base64 output may need URI-safe
encoding. Consider using the existing $toBase64AndURIEncoded() helper (used
elsewhere in the UI utils) for consistent UTF-8-safe encoding.
##########
plugins/storage/volume/ontap/pom.xml:
##########
@@ -151,6 +164,7 @@
<version>${maven-surefire-plugin.version}</version>
<configuration>
<skipTests>false</skipTests>
+
<argLine>-javaagent:${settings.localRepository}/net/bytebuddy/byte-buddy-agent/${byte-buddy-agent.version}/byte-buddy-agent-${byte-buddy-agent.version}.jar</argLine>
<includes>
Review Comment:
The surefire <argLine> hard-codes the Byte Buddy agent path using
${settings.localRepository}. That property is often unset, which can produce an
invalid -javaagent path and fail test JVM startup. Also, this overwrites any
existing argLine (e.g., from parent configs).
##########
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 Base64-encoded with btoa(). btoa only supports Latin1 and
can throw for non-ASCII passwords, and raw Base64 may include '+', '/', '='
which can be problematic in query/form encoding. The UI already provides
$toBase64AndURIEncoded() to handle UTF-8 + URI-safe encoding.
--
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]
