diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java index 087e9aa681b4..15374811bf74 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java @@ -19,10 +19,13 @@ package org.apache.cloudstack.storage.feign.model; +import java.util.List; + +import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; -import java.util.List; +import com.fasterxml.jackson.annotation.JsonValue; /** * ExportRule @@ -54,6 +57,7 @@ public enum ProtocolsEnum { this.value = value; } + @JsonValue public String getValue() { return value; } @@ -63,9 +67,13 @@ public String toString() { return String.valueOf(value); } + @JsonCreator public static ProtocolsEnum fromValue(String text) { + if (text == null) { + return null; + } for (ProtocolsEnum b : ProtocolsEnum.values()) { - if (String.valueOf(b.value).equals(text)) { + if (String.valueOf(b.value).equalsIgnoreCase(text)) { return b; } } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java index ecdd3efd2c5c..993e2d182804 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java @@ -19,30 +19,38 @@ package org.apache.cloudstack.storage.listener; +import java.util.List; +import java.util.Map; + import javax.inject.Inject; -import com.cloud.agent.api.ModifyStoragePoolCommand; +import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.cloudstack.storage.service.StorageStrategy; +import org.apache.cloudstack.storage.service.model.AccessGroup; +import org.apache.cloudstack.storage.service.model.ProtocolType; +import org.apache.cloudstack.storage.utils.OntapStorageConstants; +import org.apache.cloudstack.storage.utils.OntapStorageUtils; +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; import com.cloud.agent.api.ModifyStoragePoolAnswer; +import com.cloud.agent.api.ModifyStoragePoolCommand; import com.cloud.agent.api.StoragePoolInfo; import com.cloud.alert.AlertManager; +import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.StoragePool; import com.cloud.storage.StoragePoolHostVO; import com.cloud.storage.dao.StoragePoolHostDao; -import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.LogManager; -import com.cloud.agent.AgentManager; -import com.cloud.agent.api.Answer; -import com.cloud.agent.api.DeleteStoragePoolCommand; -import com.cloud.host.Host; -import com.cloud.storage.StoragePool; import com.cloud.utils.exception.CloudRuntimeException; -import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; -import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; -import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; -import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener; -import com.cloud.host.dao.HostDao; - -import java.util.Map; public class OntapHostListener implements HypervisorHostListener { protected Logger logger = LogManager.getLogger(getClass()); @@ -63,26 +71,39 @@ public class OntapHostListener implements HypervisorHostListener { @Override public boolean hostConnect(long hostId, long poolId) { - logger.info("Connect to host " + hostId + " from pool " + poolId); + logger.info("hostConnect: Connecting host {} to pool {}", hostId, poolId); Host host = _hostDao.findById(hostId); if (host == null) { - logger.error("host was not found with id : {}", hostId); + logger.error("hostConnect: Host was not found with id: {}", hostId); return false; } if (!host.getHypervisorType().equals(Hypervisor.HypervisorType.KVM)) { - logger.error("ONTAP plugin does not support {} type host currently ", host.getHypervisorType()); + logger.error("hostConnect: ONTAP plugin does not support {} type host currently", host.getHypervisorType()); return false; } StoragePool pool = _storagePoolDao.findById(poolId); if (pool == null) { - logger.error("Failed to connect host - storage pool not found with id: {}", poolId); + logger.error("hostConnect: Failed to connect host - storage pool not found with id: {}", poolId); return false; } - logger.info("Connecting host {} to ONTAP storage pool {}", host.getName(), pool.getName()); + logger.info("hostConnect: Connecting host {} to ONTAP storage pool {}", host.getName(), pool.getName()); try { // Load storage pool details from database to pass mount options and other config to agent Map detailsMap = _storagePoolDetailsDao.listDetailsKeyPairs(poolId); + if (detailsMap == null || detailsMap.isEmpty()) { + logger.error("hostConnect: Failed to load storage pool details for pool id: {}", poolId); + return false; + } + + if (detailsMap.get(OntapStorageConstants.PROTOCOL) == null) { + logger.error("hostConnect: Storage pool details missing required protocol type for pool id: {}", poolId); + return false; + } + + // Update NFS export policy for this connected host when the pool protocol is NFS3. + updateNfsExportPolicyForConnectedHostIfNeeded(poolId, hostId, host, detailsMap); + // Create the ModifyStoragePoolCommand to send to the agent // Note: Always send command even if database entry exists, because agent may have restarted // and lost in-memory pool registration. The command handler is idempotent. @@ -118,7 +139,7 @@ public boolean hostConnect(long hostId, long poolId) { } String localPath = poolInfo.getLocalPath(); - logger.info("Storage pool {} successfully mounted at: {}", pool.getName(), localPath); + logger.info("hostConnect: Storage pool {} successfully mounted at: {}", pool.getName(), localPath); // Update or create the storage_pool_host_ref entry with the correct local_path StoragePoolHostVO storagePoolHost = storagePoolHostDao.findByPoolHost(poolId, hostId); @@ -126,11 +147,11 @@ public boolean hostConnect(long hostId, long poolId) { if (storagePoolHost == null) { storagePoolHost = new StoragePoolHostVO(poolId, hostId, localPath); storagePoolHostDao.persist(storagePoolHost); - logger.info("Created storage_pool_host_ref entry for pool {} and host {}", pool.getName(), host.getName()); + logger.info("hostConnect: Created storage_pool_host_ref entry for pool {} and host {}", pool.getName(), host.getName()); } else { storagePoolHost.setLocalPath(localPath); storagePoolHostDao.update(storagePoolHost.getId(), storagePoolHost); - logger.info("Updated storage_pool_host_ref entry with local_path: {}", localPath); + logger.info("hostConnect: Updated storage_pool_host_ref entry with local_path: {}", localPath); } // Update pool capacity/usage information @@ -139,11 +160,11 @@ public boolean hostConnect(long hostId, long poolId) { poolVO.setCapacityBytes(poolInfo.getCapacityBytes()); poolVO.setUsedBytes(poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes()); _storagePoolDao.update(poolVO.getId(), poolVO); - logger.info("Updated storage pool capacity: {} GB, used: {} GB", poolInfo.getCapacityBytes() / (1024 * 1024 * 1024), (poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes()) / (1024 * 1024 * 1024)); + logger.info("hostConnect: Updated storage pool capacity: {} GB, used: {} GB", poolInfo.getCapacityBytes() / (1024 * 1024 * 1024), (poolInfo.getCapacityBytes() - poolInfo.getAvailableBytes()) / (1024 * 1024 * 1024)); } } catch (Exception e) { - logger.error("Exception while connecting host {} to storage pool {}", host.getName(), pool.getName(), e); + logger.error("hostConnect: Exception while connecting host {} to storage pool {}", host.getName(), pool.getName(), e); // CRITICAL: Don't throw exception - it crashes the agent and causes restart loops // Return false to indicate failure without crashing return false; @@ -151,50 +172,115 @@ public boolean hostConnect(long hostId, long poolId) { return true; } - @Override - public boolean hostDisconnected(long hostId, long poolId) { - logger.info("Disconnect from host " + hostId + " from pool " + poolId); + private void updateNfsExportPolicyForConnectedHostIfNeeded(long poolId, long hostId, Host host, Map detailsMap) { + if (!ProtocolType.NFS3.name().equalsIgnoreCase(detailsMap.get(OntapStorageConstants.PROTOCOL))) { + return; + } - Host hostToremove = _hostDao.findById(hostId); - if (hostToremove == null) { - logger.error("Failed to add host by HostListener as host was not found with id : {}", hostId); - return false; + if (host == null) { + throw new CloudRuntimeException("Host was not found with id: " + hostId); } - StoragePool pool = _storagePoolDao.findById(poolId); - if (pool == null) { - logger.error("Failed to disconnect host - storage pool not found with id: {}", poolId); + if (!isNfs3EnabledOnHost(host)) { + throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); + } + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(poolId); + accessGroup.setHostsToConnect(List.of((HostVO) host)); + + StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(detailsMap); + strategy.updateAccessGroup(accessGroup); + logger.info("hostConnect: updateNfsExportPolicyForConnectedHostIfNeeded: Updated NFS export policy rules for host {} on storage pool {}", host.getName(), poolId); + } + + private boolean isNfs3EnabledOnHost(Host host) { + if (host == null) { return false; } - logger.info("Disconnecting host {} from ONTAP storage pool {}", hostToremove.getName(), pool.getName()); - try { - DeleteStoragePoolCommand cmd = new DeleteStoragePoolCommand(pool); - Answer answer = _agentMgr.easySend(hostId, cmd); - if (answer != null && answer.getResult()) { - logger.info("Successfully disconnected host {} from ONTAP storage pool {}", hostToremove.getName(), pool.getName()); - return true; - } else { - String errMsg = (answer != null) ? answer.getDetails() : "Unknown error"; - logger.warn("Failed to disconnect host {} from storage pool {}. Error: {}", hostToremove.getName(), pool.getName(), errMsg); - return false; - } - } catch (Exception e) { - logger.error("Exception while disconnecting host {} from storage pool {}", hostToremove.getName(), pool.getName(), e); + String storageIp = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : ""; + if (storageIp.isEmpty() && StringUtils.isBlank(host.getPrivateIpAddress())) { + logger.warn("isNfs3EnabledOnHost: Host {} is not eligible for NFS3 protocol: both storage IP and private IP are empty", + host.getId()); return false; } + + return true; } @Override - public boolean hostAboutToBeRemoved(long hostId) { + public boolean hostDisconnected(long hostId, long poolId) { + logger.info("hostDisconnected: Disconnecting host {} from pool {}", hostId, poolId); + // Note: This is not currently being called for NetApp ONTAP storage plugin. return false; } + @Override + public boolean hostAboutToBeRemoved(long hostId) { + logger.info("hostAboutToBeRemoved: Host {} is about to be removed", hostId); + + Host host = _hostDao.findById(hostId); + if (host == null) { + logger.warn("hostAboutToBeRemoved: Host not found with id: {}, considering it as no-op", hostId); + return true; + } + + List poolHostRefs = storagePoolHostDao.listByHostId(hostId); + if (poolHostRefs == null || poolHostRefs.isEmpty()) { + logger.debug("hostAboutToBeRemoved: No storage pool associations found for host {}", hostId); + return true; + } + + for (StoragePoolHostVO ref : poolHostRefs) { + StoragePoolVO pool = _storagePoolDao.findById(ref.getPoolId()); + if (pool != null) { + removeHostFromOntapPoolIfNeeded(pool, host); + } + } + + logger.info("hostAboutToBeRemoved: Cleaned up ONTAP export policies for host {} about to be removed", hostId); + return true; + } + @Override public boolean hostRemoved(long hostId, long clusterId) { return false; } + private void removeHostFromOntapPoolIfNeeded(StoragePoolVO pool, Host host) { + try { + Map detailsMap = _storagePoolDetailsDao.listDetailsKeyPairs(pool.getId()); + if (detailsMap == null || detailsMap.isEmpty()) { + logger.debug("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: No pool details found for pool id: {}", pool.getId()); + return; + } + + // Skip non-NFS3 pools; Currently, for iSCSI type, iGroup rules are being handled as part of revokeAccess in OntapPrimaryDataStoreDriver, so no need to handle here. + if (!ProtocolType.NFS3.name().equalsIgnoreCase(detailsMap.get(OntapStorageConstants.PROTOCOL))) { + return; + } + + logger.info("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Removing export policy rule for host {} from storage pool {}", host.getName(), pool.getName()); + if (!isNfs3EnabledOnHost(host)) { + logger.warn("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Skipping NFS export policy removal for host {} on pool {} as host is not NFS-enabled", + host.getId(), pool.getId()); + return; + } + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(pool.getId()); + accessGroup.setHostsToConnect(List.of((HostVO) host)); + accessGroup.setHostRuleAction(AccessGroup.HostRuleAction.REMOVE); + + StorageStrategy strategy = OntapStorageUtils.getStrategyByStoragePoolDetails(detailsMap); + strategy.updateAccessGroup(accessGroup); + logger.info("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Removed NFS export policy rules for removed host {} from storage pool {}", host.getName(), pool.getName()); + } catch (Exception e) { + logger.warn("hostAboutToBeRemoved: removeHostFromOntapPoolIfNeeded: Failed to remove NFS export policy rule for host {} from pool {}: {}", host.getId(), pool.getName(), e.getMessage()); + // Continue processing other pools even if one fails + } + } + @Override public boolean hostEnabled(long hostId) { return false; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index d349305e5e43..69c17f1289cd 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -19,13 +19,16 @@ package org.apache.cloudstack.storage.service; -import com.cloud.utils.exception.CloudRuntimeException; -import feign.FeignException; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; + import org.apache.cloudstack.storage.feign.FeignClientFactory; import org.apache.cloudstack.storage.feign.client.AggregateFeignClient; import org.apache.cloudstack.storage.feign.client.JobFeignClient; -import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; import org.apache.cloudstack.storage.feign.client.NASFeignClient; +import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; import org.apache.cloudstack.storage.feign.client.SANFeignClient; import org.apache.cloudstack.storage.feign.client.SnapshotFeignClient; import org.apache.cloudstack.storage.feign.client.SvmFeignClient; @@ -48,10 +51,9 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Objects; +import com.cloud.utils.exception.CloudRuntimeException; + +import feign.FeignException; /** * Storage Strategy represents the communication path for all the ONTAP storage options @@ -615,7 +617,7 @@ public abstract JobResponse revertSnapshotForCloudStackVolume(String snapshotNam * @param accessGroup the access group to update * @return the updated AccessGroup object */ - abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); + public abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); /** * Method encapsulates the behavior based on the opted protocol in subclasses diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java index 198957ca5db8..0a257a29527b 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java @@ -19,19 +19,21 @@ package org.apache.cloudstack.storage.service; -import com.cloud.agent.api.Answer; -import com.cloud.host.HostVO; -import com.cloud.storage.Storage; -import com.cloud.storage.VolumeVO; -import com.cloud.storage.dao.VolumeDao; -import com.cloud.utils.exception.CloudRuntimeException; -import feign.FeignException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import javax.inject.Inject; + import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; import org.apache.cloudstack.engine.subsystem.api.storage.EndPoint; import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector; import org.apache.cloudstack.storage.command.CreateObjectCommand; import org.apache.cloudstack.storage.command.DeleteCommand; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; +import org.apache.cloudstack.storage.feign.model.CliSnapshotRestoreRequest; import org.apache.cloudstack.storage.feign.model.ExportPolicy; import org.apache.cloudstack.storage.feign.model.ExportRule; import org.apache.cloudstack.storage.feign.model.FileInfo; @@ -42,19 +44,22 @@ import org.apache.cloudstack.storage.feign.model.Volume; import org.apache.cloudstack.storage.feign.model.response.JobResponse; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; -import org.apache.cloudstack.storage.feign.model.CliSnapshotRestoreRequest; import org.apache.cloudstack.storage.service.model.AccessGroup; import org.apache.cloudstack.storage.service.model.CloudStackVolume; -import org.apache.cloudstack.storage.volume.VolumeObject; import org.apache.cloudstack.storage.utils.OntapStorageConstants; import org.apache.cloudstack.storage.utils.OntapStorageUtils; +import org.apache.cloudstack.storage.volume.VolumeObject; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import javax.inject.Inject; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; +import com.cloud.agent.api.Answer; +import com.cloud.host.HostVO; +import com.cloud.storage.Storage; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.utils.exception.CloudRuntimeException; + +import feign.FeignException; public class UnifiedNASStrategy extends NASStrategy { private static final Logger logger = LogManager.getLogger(UnifiedNASStrategy.class); @@ -191,7 +196,134 @@ public void deleteAccessGroup(AccessGroup accessGroup) { @Override public AccessGroup updateAccessGroup(AccessGroup accessGroup) { - return null; + if (accessGroup == null) { + throw new CloudRuntimeException("Invalid accessGroup object - accessGroup is null"); + } + // Check if an AccessGroup was constructed without associating it to a storage pool. + if (accessGroup.getStoragePoolId() == null) { + throw new CloudRuntimeException("Invalid accessGroup object - storagePoolId is null"); + } + // At least one host is required regardless of ADD or REMOVE action. + // An empty list means there is nothing to add to or remove from the export policy client list. + if (accessGroup.getHostsToConnect() == null || accessGroup.getHostsToConnect().isEmpty()) { + throw new CloudRuntimeException("Invalid accessGroup object - hostsToConnect is null or empty"); + } + + Map details = storagePoolDetailsDao.listDetailsKeyPairs(accessGroup.getStoragePoolId()); + if (details == null || details.isEmpty()) { + throw new CloudRuntimeException("No storage pool details found for storagePoolId: " + accessGroup.getStoragePoolId()); + } + String exportPolicyId = details.get(OntapStorageConstants.EXPORT_POLICY_ID); + if (exportPolicyId == null || exportPolicyId.isEmpty()) { + throw new CloudRuntimeException("No export policy found for storagePoolId: " + accessGroup.getStoragePoolId()); + } + + + try { + String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); + ExportPolicy existingPolicy = nasFeignClient.getExportPolicyById(authHeader, exportPolicyId); + // Check if the export policy was deleted externally on ONTAP or the stored ID is stale. + if (existingPolicy == null) { + throw new CloudRuntimeException("Failed to fetch existing export policy with id: " + exportPolicyId); + } + + List rules = existingPolicy.getRules(); + if (rules == null || rules.isEmpty()) { + throw new CloudRuntimeException("Export policy " + existingPolicy.getName() + + " has no rules — unexpected state, the plugin always creates a rule at pool registration"); + } + + ExportRule targetRule = rules.get(0); + + Set hostMatches = new HashSet<>(); + for (HostVO host : accessGroup.getHostsToConnect()) { + String hostStorageIp = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : null; + String ip = (hostStorageIp != null && !hostStorageIp.isEmpty()) ? hostStorageIp + : (host.getPrivateIpAddress() != null ? host.getPrivateIpAddress().trim() : null); + // Occurs when a CloudStack host has neither a storage IP nor a private IP configured + // (misconfigured or partially registered host). Skip it to avoid inserting a broken + // or empty match entry into the ONTAP export rule. + if (ip == null || ip.isEmpty()) { + logger.warn("updateAccessGroup: Host {} has no storage/private IP, skipping export rule update", host.getName()); + continue; + } + hostMatches.add(ip + "/32"); + } + + // Occurs when every host in hostsToConnect had no valid IP (all were skipped above). + // There is nothing to add or remove, so skip the ONTAP API call and return early. + if (hostMatches.isEmpty()) { + accessGroup.setPolicy(existingPolicy); + return accessGroup; + } + + boolean updated = false; + // Differentiates between removing hosts (e.g., host decommissioned or removed from the cluster) + // and the default ADD path (e.g., new host being connected to the storage pool). + List exportClients = targetRule.getClients(); + // Existing rules can legitimately have no clients yet; treat that as an empty list. + if (exportClients == null) { + exportClients = new ArrayList<>(); + targetRule.setClients(exportClients); + } + + if (AccessGroup.HostRuleAction.REMOVE.equals(accessGroup.getHostRuleAction())) { + updated = exportClients.removeIf(c -> c != null && c.getMatch() != null && hostMatches.contains(c.getMatch())); + // None of the requested host IPs were present in the policy — log for diagnostics + // so operators can investigate whether the policy state is already correct or stale. + if (!updated) { + logger.info("updateAccessGroup: No matching host IPs found in export policy {} for removal", existingPolicy.getName()); + } + } else { + Set existingMatches = new HashSet<>(); + for (ExportRule.ExportClient exportClient : exportClients) { + // Skips null client entries or entries with a null match field that may have been + // inserted externally on ONTAP. Avoids polluting the dedup set with null values + // which would cause subsequent hosts to be incorrectly treated as duplicates. + if (exportClient != null && exportClient.getMatch() != null) { + existingMatches.add(exportClient.getMatch()); + } + } + + for (String match : hostMatches) { + // Set.add() returns false when the element was already present, acting as a dedup check. + // Prevents inserting a duplicate client match entry for a host that is already allowed + // in the export policy — ONTAP may reject or behave unpredictably with duplicate matches. + if (existingMatches.add(match)) { + ExportRule.ExportClient exportClient = new ExportRule.ExportClient(); + exportClient.setMatch(match); + exportClients.add(exportClient); + updated = true; + } + } + } + + // Occurs when the export policy is already in the desired state: + // ADD path — all provided host IPs were already present (all were duplicates). + // REMOVE path — none of the provided host IPs matched any existing entry. + // In both cases, skip the ONTAP PATCH call to avoid an unnecessary round-trip. + if (!updated) { + // Only log the "nothing to add" message for the ADD path; the REMOVE no-op + // is already logged above in its own branch to avoid double-logging. + if (!AccessGroup.HostRuleAction.REMOVE.equals(accessGroup.getHostRuleAction())) { + logger.info("updateAccessGroup: No new host IPs to add to export policy {}", existingPolicy.getName()); + } + accessGroup.setPolicy(existingPolicy); + return accessGroup; + } + + ExportPolicy updateRequest = new ExportPolicy(); + updateRequest.setRules(rules); + nasFeignClient.updateExportPolicy(authHeader, exportPolicyId, updateRequest); + + existingPolicy.setRules(rules); + accessGroup.setPolicy(existingPolicy); + logger.info("updateAccessGroup: Successfully updated export policy {} with new host client rules", existingPolicy.getName()); + return accessGroup; + } catch (Exception e) { + logger.error("updateAccessGroup: Failed to update export policy for pool {}", accessGroup.getStoragePoolId(), e); + throw new CloudRuntimeException("Failed to update export policy for NFS host connection: " + e.getMessage(), e); + } } @Override @@ -307,10 +439,10 @@ private ExportPolicy createExportPolicyRequest(AccessGroup accessGroup,String sv List exportClients = new ArrayList<>(); List hosts = accessGroup.getHostsToConnect(); for (HostVO host : hosts) { - String hostStorageIp = host.getStorageIpAddress(); + String hostStorageIp = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : null; String ip = (hostStorageIp != null && !hostStorageIp.isEmpty()) ? hostStorageIp - : host.getPrivateIpAddress(); + : (host.getPrivateIpAddress() != null ? host.getPrivateIpAddress().trim() : null); String ipToUse = ip + "/32"; ExportRule.ExportClient exportClient = new ExportRule.ExportClient(); exportClient.setMatch(ipToUse); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java index 9815724fc1aa..8b1aa24fe5d9 100755 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java @@ -19,21 +19,28 @@ package org.apache.cloudstack.storage.service.model; -import com.cloud.host.HostVO; +import java.util.List; + import org.apache.cloudstack.engine.subsystem.api.storage.Scope; import org.apache.cloudstack.storage.feign.model.ExportPolicy; import org.apache.cloudstack.storage.feign.model.Igroup; -import java.util.List; +import com.cloud.host.HostVO; public class AccessGroup { + public enum HostRuleAction { + ADD, + REMOVE + } + private Igroup igroup; private ExportPolicy exportPolicy; private List hostsToConnect; private Long storagePoolId; private Scope scope; + private HostRuleAction hostRuleAction = HostRuleAction.ADD; public Igroup getIgroup() { @@ -74,4 +81,12 @@ public Scope getScope() { public void setScope(Scope scope) { this.scope = scope; } + + public HostRuleAction getHostRuleAction() { + return hostRuleAction; + } + + public void setHostRuleAction(HostRuleAction hostRuleAction) { + this.hostRuleAction = hostRuleAction; + } } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java index 6f09c3649757..a563dbca33de 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java @@ -18,8 +18,11 @@ */ package org.apache.cloudstack.storage.service; -import com.cloud.utils.exception.CloudRuntimeException; -import feign.FeignException; +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + import org.apache.cloudstack.storage.feign.client.AggregateFeignClient; import org.apache.cloudstack.storage.feign.client.JobFeignClient; import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; @@ -40,33 +43,31 @@ import org.apache.cloudstack.storage.service.model.CloudStackVolume; import org.apache.cloudstack.storage.service.model.ProtocolType; import org.apache.cloudstack.storage.utils.OntapStorageConstants; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; -import org.mockito.junit.jupiter.MockitoSettings; -import org.mockito.quality.Strictness; - -import java.lang.reflect.Field; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; - import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import org.mockito.Mock; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import com.cloud.utils.exception.CloudRuntimeException; + +import feign.FeignException; @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -165,7 +166,7 @@ public void deleteAccessGroup(AccessGroup accessGroup) { } @Override - AccessGroup updateAccessGroup(AccessGroup accessGroup) { + public AccessGroup updateAccessGroup(AccessGroup accessGroup) { return null; } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java index c4d5ddf6878c..b04e9c0b1b26 100755 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java @@ -37,6 +37,7 @@ import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; import org.apache.cloudstack.storage.feign.client.SANFeignClient; import org.apache.cloudstack.storage.feign.model.ExportPolicy; +import org.apache.cloudstack.storage.feign.model.ExportRule; import org.apache.cloudstack.storage.feign.model.Job; import org.apache.cloudstack.storage.feign.model.OntapStorage; import org.apache.cloudstack.storage.feign.model.response.JobResponse; @@ -63,6 +64,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; @@ -72,6 +74,7 @@ import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -582,4 +585,350 @@ public void testDeleteCloudStackVolume_AnswerNull() throws Exception { strategy.deleteCloudStackVolume(cloudStackVolume); }); } -} + + // ------------------------------------------------------------------------- + // updateAccessGroup tests + // ------------------------------------------------------------------------- + + private Map detailsWithExportPolicyId() { + Map details = new HashMap<>(); + details.put(OntapStorageConstants.EXPORT_POLICY_ID, "policy-42"); + return details; + } + + private ExportPolicy existingPolicyWithClients(String... matchIps) { + ExportRule rule = new ExportRule(); + List clients = new ArrayList<>(); + for (String ip : matchIps) { + ExportRule.ExportClient client = new ExportRule.ExportClient(); + client.setMatch(ip); + clients.add(client); + } + rule.setClients(clients); + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(new ArrayList<>(List.of(rule))); + return policy; + } + + // updateAccessGroup - null accessGroup + @Test + public void testUpdateAccessGroup_NullAccessGroup() { + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(null)); + } + + // updateAccessGroup - null storagePoolId + @Test + public void testUpdateAccessGroup_NullStoragePoolId() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + // storagePoolId is null by default + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - null hostsToConnect + @Test + public void testUpdateAccessGroup_NullHostsToConnect() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + // hostsToConnect is null by default + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - empty hostsToConnect + @Test + public void testUpdateAccessGroup_EmptyHostsToConnect() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(new ArrayList<>()); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - storagePoolDetailsDao returns null + @Test + public void testUpdateAccessGroup_NoStoragePoolDetails() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(null); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - details missing EXPORT_POLICY_ID key + @Test + public void testUpdateAccessGroup_MissingExportPolicyId() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + Map details = new HashMap<>(); + details.put("someOtherKey", "someValue"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(details); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - getExportPolicyById returns null + @Test + public void testUpdateAccessGroup_ExportPolicyNotFound() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(null); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - existing policy has null rules + @Test + public void testUpdateAccessGroup_NullRules() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(null); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(policy); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - existing policy has empty rules list + @Test + public void testUpdateAccessGroup_EmptyRules() { + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(mock(HostVO.class))); + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(new ArrayList<>()); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(policy); + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + + // updateAccessGroup - all hosts have no IP: returns early without ONTAP patch + @Test + public void testUpdateAccessGroup_AllHostsHaveNoIp_ReturnsEarly() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn(null); + when(host.getPrivateIpAddress()).thenReturn(null); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + verify(nasFeignClient, never()).updateExportPolicy(anyString(), anyString(), any()); + } + + // updateAccessGroup - ADD: new host IP added to policy + @Test + public void testUpdateAccessGroup_Add_NewHost_Success() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.2"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + // default action is ADD + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + // Existing client + new client = 2 + assertEquals(2, existingPolicy.getRules().get(0).getClients().size()); + verify(nasFeignClient).updateExportPolicy(anyString(), eq("policy-42"), any(ExportPolicy.class)); + } + + // updateAccessGroup - ADD: host uses private IP when storage IP is absent + @Test + public void testUpdateAccessGroup_Add_UsesPrivateIpWhenStorageIpAbsent() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn(null); + when(host.getPrivateIpAddress()).thenReturn("192.168.1.50"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients(); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + List clients = existingPolicy.getRules().get(0).getClients(); + assertEquals(1, clients.size()); + assertEquals("192.168.1.50/32", clients.get(0).getMatch()); + verify(nasFeignClient).updateExportPolicy(anyString(), eq("policy-42"), any(ExportPolicy.class)); + } + + // updateAccessGroup - ADD: host IP already present in policy (no-op) + @Test + public void testUpdateAccessGroup_Add_DuplicateHost_NoUpdate() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.1"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + // Client count must remain 1 (no duplicate inserted) + assertEquals(1, existingPolicy.getRules().get(0).getClients().size()); + verify(nasFeignClient, never()).updateExportPolicy(anyString(), anyString(), any()); + } + + // updateAccessGroup - ADD: existing rule has null clients list + @Test + public void testUpdateAccessGroup_Add_NullClientsInRule() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.5"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportRule rule = new ExportRule(); + rule.setClients(null); // null clients list + ExportPolicy policy = new ExportPolicy(); + policy.setName("test-policy"); + policy.setRules(new ArrayList<>(List.of(rule))); + + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(policy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertEquals(1, rule.getClients().size()); + assertEquals("10.0.0.5/32", rule.getClients().get(0).getMatch()); + verify(nasFeignClient).updateExportPolicy(anyString(), eq("policy-42"), any(ExportPolicy.class)); + } + + // updateAccessGroup - REMOVE: matching host IP removed from policy + @Test + public void testUpdateAccessGroup_Remove_MatchingHost_Success() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.1"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + accessGroup.setHostRuleAction(AccessGroup.HostRuleAction.REMOVE); + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32", "10.0.0.2/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + // Only 10.0.0.2/32 should remain + List clients = existingPolicy.getRules().get(0).getClients(); + assertEquals(1, clients.size()); + assertEquals("10.0.0.2/32", clients.get(0).getMatch()); + verify(nasFeignClient).updateExportPolicy(anyString(), eq("policy-42"), any(ExportPolicy.class)); + } + + // updateAccessGroup - REMOVE: IP not in policy (no-op) + @Test + public void testUpdateAccessGroup_Remove_IpNotPresent_NoUpdate() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.99"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + accessGroup.setHostRuleAction(AccessGroup.HostRuleAction.REMOVE); + + ExportPolicy existingPolicy = existingPolicyWithClients("10.0.0.1/32"); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + AccessGroup result = strategy.updateAccessGroup(accessGroup); + + assertNotNull(result); + assertSame(existingPolicy, result.getPolicy()); + assertEquals(1, existingPolicy.getRules().get(0).getClients().size()); + verify(nasFeignClient, never()).updateExportPolicy(anyString(), anyString(), any()); + } + + // updateAccessGroup - FeignException from ONTAP wrapped in CloudRuntimeException + @Test + public void testUpdateAccessGroup_FeignExceptionWrapped() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn("10.0.0.1"); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))) + .thenThrow(new RuntimeException("ONTAP unreachable")); + + assertThrows(CloudRuntimeException.class, () -> strategy.updateAccessGroup(accessGroup)); + } + // updateAccessGroup - whitespace in storage IP is trimmed before building match + @Test + public void testUpdateAccessGroup_TrimsWhitespaceFromStorageIp() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn(" 10.0.0.2 "); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients(); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + strategy.updateAccessGroup(accessGroup); + + List clients = existingPolicy.getRules().get(0).getClients(); + assertEquals(1, clients.size()); + assertEquals("10.0.0.2/32", clients.get(0).getMatch()); + } + + // updateAccessGroup - whitespace in private IP is trimmed when storage IP absent + @Test + public void testUpdateAccessGroup_TrimsWhitespaceFromPrivateIp() { + HostVO host = mock(HostVO.class); + when(host.getStorageIpAddress()).thenReturn(null); + when(host.getPrivateIpAddress()).thenReturn(" 192.168.1.10 "); + + AccessGroup accessGroup = new AccessGroup(); + accessGroup.setStoragePoolId(1L); + accessGroup.setHostsToConnect(List.of(host)); + + ExportPolicy existingPolicy = existingPolicyWithClients(); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(detailsWithExportPolicyId()); + when(nasFeignClient.getExportPolicyById(anyString(), eq("policy-42"))).thenReturn(existingPolicy); + + strategy.updateAccessGroup(accessGroup); + + List clients = existingPolicy.getRules().get(0).getClients(); + assertEquals(1, clients.size()); + assertEquals("192.168.1.10/32", clients.get(0).getMatch()); + }}