@@ -201,79 +201,105 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) {
201201 if (accessGroup == null ) {
202202 throw new CloudRuntimeException ("Invalid accessGroup object - accessGroup is null" );
203203 }
204+ // Check if an AccessGroup was constructed without associating it to a storage pool.
204205 if (accessGroup .getStoragePoolId () == null ) {
205206 throw new CloudRuntimeException ("Invalid accessGroup object - storagePoolId is null" );
206207 }
208+ // At least one host is required regardless of ADD or REMOVE action.
209+ // An empty list means there is nothing to add to or remove from the export policy client list.
207210 if (accessGroup .getHostsToConnect () == null || accessGroup .getHostsToConnect ().isEmpty ()) {
208211 throw new CloudRuntimeException ("Invalid accessGroup object - hostsToConnect is null or empty" );
209212 }
210213
211214 Map <String , String > details = storagePoolDetailsDao .listDetailsKeyPairs (accessGroup .getStoragePoolId ());
215+ if (details == null || details .isEmpty ()) {
216+ throw new CloudRuntimeException ("No storage pool details found for storagePoolId: " + accessGroup .getStoragePoolId ());
217+ }
212218 String exportPolicyId = details .get (OntapStorageConstants .EXPORT_POLICY_ID );
213-
214- // No policy exists yet (e.g. pool was created before any eligible hosts came up), create it now.
215219 if (exportPolicyId == null || exportPolicyId .isEmpty ()) {
216- logger .info ("updateAccessGroup: No export policy found for pool {}. Creating one now." , accessGroup .getStoragePoolId ());
217- return createAccessGroup (accessGroup );
220+ throw new CloudRuntimeException ("No export policy found for storagePoolId: " + accessGroup .getStoragePoolId ());
218221 }
219222
223+
220224 try {
221225 String authHeader = OntapStorageUtils .generateAuthHeader (storage .getUsername (), storage .getPassword ());
222226 ExportPolicy existingPolicy = nasFeignClient .getExportPolicyById (authHeader , exportPolicyId );
227+ // Check if the export policy was deleted externally on ONTAP or the stored ID is stale.
223228 if (existingPolicy == null ) {
224229 throw new CloudRuntimeException ("Failed to fetch existing export policy with id: " + exportPolicyId );
225230 }
226231
227232 List <ExportRule > rules = existingPolicy .getRules ();
233+ // An existing export policy should always have at least one rule. A null or empty rules list
234+ // indicates the policy was corrupted or modified externally on ONTAP and is in an unexpected
235+ // state. updateAccessGroup only modifies an existing policy — it does not create rules from scratch.
228236 if (rules == null || rules .isEmpty ()) {
229- rules = new ArrayList <>();
230- ExportRule newRule = new ExportRule ();
231- newRule .setProtocols (List .of (ExportRule .ProtocolsEnum .NFS3 ));
232- newRule .setRoRule (List .of ("sys" ));
233- newRule .setRwRule (List .of ("sys" ));
234- newRule .setSuperuser (List .of ("sys" ));
235- newRule .setClients (new ArrayList <>());
236- rules .add (newRule );
237+ throw new CloudRuntimeException ("Export policy " + existingPolicy .getName () + " has no rules. " +
238+ "Cannot update an export policy with no existing rules." );
237239 }
238240
239- ExportRule exportRule = rules . get ( 0 );
240- List < ExportRule . ExportClient > exportClients = exportRule . getClients ();
241- if (exportClients == null ) {
242- exportClients = new ArrayList <>();
243- exportRule . setClients ( exportClients );
241+ // This plugin creates a single NFS export rule per policy. More than one rule means the
242+ // policy was changed outside the plugin and we no longer know which rule should be mutated.
243+ if (rules . size () != 1 ) {
244+ throw new CloudRuntimeException ( "Export policy " + existingPolicy . getName () +
245+ " is expected to have exactly one rule but found: " + rules . size () );
244246 }
245247
248+ ExportRule targetRule = rules .get (0 );
249+
246250 Set <String > hostMatches = new HashSet <>();
247251 for (HostVO host : accessGroup .getHostsToConnect ()) {
248252 String hostStorageIp = host .getStorageIpAddress ();
249253 String ip = (hostStorageIp != null && !hostStorageIp .isEmpty ()) ? hostStorageIp : host .getPrivateIpAddress ();
254+ // Occurs when a CloudStack host has neither a storage IP nor a private IP configured
255+ // (misconfigured or partially registered host). Skip it to avoid inserting a broken
256+ // or empty match entry into the ONTAP export rule.
250257 if (ip == null || ip .isEmpty ()) {
251258 logger .warn ("updateAccessGroup: Host {} has no storage/private IP, skipping export rule update" , host .getName ());
252259 continue ;
253260 }
254261 hostMatches .add (ip + "/32" );
255262 }
256263
264+ // Occurs when every host in hostsToConnect had no valid IP (all were skipped above).
265+ // There is nothing to add or remove, so skip the ONTAP API call and return early.
257266 if (hostMatches .isEmpty ()) {
258267 accessGroup .setPolicy (existingPolicy );
259268 return accessGroup ;
260269 }
261270
262271 boolean updated = false ;
272+ // Differentiates between removing hosts (e.g., host decommissioned or removed from the cluster)
273+ // and the default ADD path (e.g., new host being connected to the storage pool).
274+ List <ExportRule .ExportClient > exportClients = targetRule .getClients ();
275+ // Existing rules can legitimately have no clients yet; treat that as an empty list.
276+ if (exportClients == null ) {
277+ exportClients = new ArrayList <>();
278+ targetRule .setClients (exportClients );
279+ }
280+
263281 if (AccessGroup .HostRuleAction .REMOVE .equals (accessGroup .getHostRuleAction ())) {
264282 updated = exportClients .removeIf (c -> c != null && c .getMatch () != null && hostMatches .contains (c .getMatch ()));
283+ // None of the requested host IPs were present in the policy — log for diagnostics
284+ // so operators can investigate whether the policy state is already correct or stale.
265285 if (!updated ) {
266286 logger .info ("updateAccessGroup: No matching host IPs found in export policy {} for removal" , existingPolicy .getName ());
267287 }
268288 } else {
269289 Set <String > existingMatches = new HashSet <>();
270290 for (ExportRule .ExportClient exportClient : exportClients ) {
291+ // Skips null client entries or entries with a null match field that may have been
292+ // inserted externally on ONTAP. Avoids polluting the dedup set with null values
293+ // which would cause subsequent hosts to be incorrectly treated as duplicates.
271294 if (exportClient != null && exportClient .getMatch () != null ) {
272295 existingMatches .add (exportClient .getMatch ());
273296 }
274297 }
275298
276299 for (String match : hostMatches ) {
300+ // Set.add() returns false when the element was already present, acting as a dedup check.
301+ // Prevents inserting a duplicate client match entry for a host that is already allowed
302+ // in the export policy — ONTAP may reject or behave unpredictably with duplicate matches.
277303 if (existingMatches .add (match )) {
278304 ExportRule .ExportClient exportClient = new ExportRule .ExportClient ();
279305 exportClient .setMatch (match );
@@ -283,13 +309,16 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) {
283309 }
284310 }
285311
312+ // Occurs when the export policy is already in the desired state:
313+ // ADD path — all provided host IPs were already present (all were duplicates).
314+ // REMOVE path — none of the provided host IPs matched any existing entry.
315+ // In both cases, skip the ONTAP PATCH call to avoid an unnecessary round-trip.
286316 if (!updated ) {
317+ // Only log the "nothing to add" message for the ADD path; the REMOVE no-op
318+ // is already logged above in its own branch to avoid double-logging.
287319 if (!AccessGroup .HostRuleAction .REMOVE .equals (accessGroup .getHostRuleAction ())) {
288320 logger .info ("updateAccessGroup: No new host IPs to add to export policy {}" , existingPolicy .getName ());
289321 }
290- }
291-
292- if (!updated ) {
293322 accessGroup .setPolicy (existingPolicy );
294323 return accessGroup ;
295324 }
0 commit comments