Use UUID for clouds

core/src/main/java/hudson/slaves/Cloud.java

@@ -51,12 +51,14 @@
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Objects;
+import java.util.UUID;
 import java.util.concurrent.Future;
 import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.DoNotUse;
 import org.kohsuke.stapler.DataBoundConstructor;
+import org.kohsuke.stapler.DataBoundSetter;
 import org.kohsuke.stapler.HttpRedirect;
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.StaplerRequest;
@@ -66,26 +68,34 @@
 import org.kohsuke.stapler.verb.POST;
 
 /**
- * Creates {@link Node}s to dynamically expand/shrink the agents attached to Hudson.
+ * Creates {@link Node}s to dynamically expand/shrink the agents attached to
+ * Hudson.
  *
  * <p>
  * Put another way, this class encapsulates different communication protocols
  * needed to start a new agent programmatically.
  *
  * <h2>Notes for implementers</h2>
  * <h3>Automatically delete idle agents</h3>
- * Nodes provisioned from a cloud do not automatically get released just because it's created from {@link Cloud}.
- * Doing so requires a use of {@link RetentionStrategy}. Instantiate your {@link Slave} subtype with something
- * like {@link CloudSlaveRetentionStrategy} so that it gets automatically deleted after some idle time.
+ * Nodes provisioned from a cloud do not automatically get released just because
+ * it's created from {@link Cloud}.
+ * Doing so requires a use of {@link RetentionStrategy}. Instantiate your
+ * {@link Slave} subtype with something
+ * like {@link CloudSlaveRetentionStrategy} so that it gets automatically
+ * deleted after some idle time.
  *
  * <h3>Freeing an external resource when an agent is removed</h3>
- * Whether you do auto scale-down or not, you often want to release an external resource tied to a cloud-allocated
+ * Whether you do auto scale-down or not, you often want to release an external
+ * resource tied to a cloud-allocated
  * agent when it is removed.
  *
  * <p>
- * To do this, have your {@link Slave} subtype remember the necessary handle (such as EC2 instance ID)
- * as a field. Such fields need to survive the user-initiated re-configuration of {@link Slave}, so you'll need to
- * expose it in your {@link Slave} {@code configure-entries.jelly} and read it back in through {@link DataBoundConstructor}.
+ * To do this, have your {@link Slave} subtype remember the necessary handle
+ * (such as EC2 instance ID)
+ * as a field. Such fields need to survive the user-initiated re-configuration
+ * of {@link Slave}, so you'll need to
+ * expose it in your {@link Slave} {@code configure-entries.jelly} and read it
+ * back in through {@link DataBoundConstructor}.
  *
  * <p>
  * You then implement your own {@link Computer} subtype, override {@link Slave#createComputer()}, and instantiate
@@ -110,10 +120,15 @@
 public abstract class Cloud extends Actionable implements ExtensionPoint, Describable<Cloud>, AccessControlled {
 
     /**
-     * Uniquely identifies this {@link Cloud} instance among other instances in {@link jenkins.model.Jenkins#clouds}.
-     *
-     * This is expected to be short ID-like string that does not contain any character unsafe as variable name or
-     * URL path token.
+     * Unique identifier for this cloud instance.
+     * Used for stable URL routing when multiple clouds have the same name.
+     */
+    private volatile String uniqueId;
+
+    /**
+     * Display name for this cloud, shown in the UI.
+     * Note: Multiple clouds may share the same name. Use {@link #getUniqueId()} for
+     * stable identification across renames, reordering, or duplicate names.
      */
     public String name;
 
@@ -128,6 +143,49 @@
         return name;
     }
 
+    /**
+     * Called after XStream deserialization to ensure uniqueId exists.
+     * This handles migration of existing configurations that don't have IDs.
+     */
+    @SuppressWarnings("unused")
+    private Object readResolve() {
+        if (uniqueId == null) {
+            uniqueId = UUID.randomUUID().toString();
+        }
+        return this;
+    }
+
+    /**
+     * Gets the unique identifier for this cloud.
+     * Thread-safe with double-checked locking for performance.
+     * @return unique identifier string, never null
+     */
+    @NonNull
+    public String getUniqueId() {
+        String id = uniqueId;
+        if (id == null) {
+            synchronized (this) {
+                id = uniqueId;
+                if (id == null) {
+                    uniqueId = id = UUID.randomUUID().toString();
+                }
+            }
+        }
+        return id;
+    }
+
+    /**
+     * Sets the unique ID from form submission.
+     * Only sets if the current uniqueId is null and the provided id is valid.
+     * @param id the unique identifier from form data
+     */
+    @DataBoundSetter
+    public void setUniqueId(String id) {
+        if (this.uniqueId == null && id != null && !id.trim().isEmpty()) {
+            this.uniqueId = id;
+        }
+    }
+
     @Override
     public String getDisplayName() {
         return name;
@@ -140,7 +198,7 @@
      * @return Jenkins relative URL.
      */
     public @NonNull String getUrl() {
-        return "cloud/" + Util.rawEncode(name) + "/";
+        return "cloud/" + Util.rawEncode(getUniqueId()) + "/";
     }
 
     @Override
@@ -316,6 +374,17 @@
         return new HttpRedirect("..");
     }
 
+    /*
+     * Accepts the update to the node configuration.
+     */
+    /**
+     * Generates a new unique ID for this cloud instance.
+     * Useful when copying a cloud to ensure the copy has a distinct identity.
+     */
+    public synchronized void provisionNewId() {
+        uniqueId = UUID.randomUUID().toString();
+    }
+
     /**
      * Accepts the update to the node configuration.
      */
@@ -324,21 +393,29 @@
         checkPermission(Jenkins.ADMINISTER);
 
         Jenkins j = Jenkins.get();
-        Cloud cloud = j.getCloud(this.name);
-        if (cloud == null) {
-            throw new ServletException("No such cloud " + this.name);
+
+        Cloud reconfigured = this.reconfigure(req, req.getSubmittedForm());
+
+        if (reconfigured == null) {
+            j.clouds.remove(this);
+            j.save();
+            return FormApply.success("../");
+        }
+
+        // The uniqueId should be set via @DataBoundSetter from the hidden form field
+        if (!this.getUniqueId().equals(reconfigured.getUniqueId())) {
+            throw new Descriptor.FormException("Cloud identity mismatch. The cloud may have been modified by another user.", "uniqueId");
         }
-        Cloud result = cloud.reconfigure(req, req.getSubmittedForm());
-        String proposedName = result.name;
+
+        String proposedName = reconfigured.name;
         if (!proposedName.equals(this.name)
                 && j.getCloud(proposedName) != null) {
             throw new Descriptor.FormException(jenkins.agents.Messages.CloudSet_CloudAlreadyExists(proposedName), "name");
         }
-        j.clouds.replace(this, result);
-        j.save();
-        // take the user back to the cloud top page.
-        return FormApply.success("../" + result.name + '/');
 
+        j.clouds.replace(this, reconfigured);
+        j.save();
+        return FormApply.success("../" + Util.rawEncode(reconfigured.getUniqueId()) + '/');
     }
 
     /**

core/src/main/java/jenkins/agents/CloudSet.java

@@ -24,6 +24,7 @@
 
 package jenkins.agents;
 
+import edu.umd.cs.findbugs.annotations.CheckForNull;
 import hudson.Extension;
 import hudson.ExtensionList;
 import hudson.Functions;
@@ -73,7 +74,7 @@
     }
 
     public Cloud getDynamic(String token) {
-        return Jenkins.get().getCloud(token);
+        return getById(token);
     }
 
     @Override
@@ -113,11 +114,8 @@
     @Restricted(DoNotUse.class) // stapler
     public String getCloudUrl(StaplerRequest2 request, Jenkins jenkins, Cloud cloud) {
         String context = Functions.getNearestAncestorUrl(request, jenkins);
-        if (Jenkins.get().getCloud(cloud.name) != cloud) { // this cloud is not the first occurrence with this name
-            return context + "/cloud/cloudByIndex/" + getClouds().indexOf(cloud) + "/";
-        } else {
-            return context + "/" + cloud.getUrl();
-        }
+        // Always use UUID-based URLs for stability across renames, reordering, and duplicates
+        return context + "/" + cloud.getUrl();
     }
 
     /**
@@ -130,140 +128,152 @@
         return getCloudUrl(StaplerRequest.toStaplerRequest2(request), jenkins, cloud);
     }
 
-    @SuppressWarnings("unused") // stapler
-    @Restricted(DoNotUse.class) // stapler
-    public Cloud getCloudByIndex(int index) {
-        return Jenkins.get().clouds.get(index);
+    /**
+     * Gets a cloud by its unique ID.
+     */
+    @CheckForNull
+    public Cloud getById(String id) {
+        if (id == null || id.trim().isEmpty()) {
+            return null;
+        }
+        for (Cloud c : Jenkins.get().clouds) {
+            if (id.equals(c.getUniqueId())) {
+                return c;
+            }
+        }
+        return null;
     }
 
     @SuppressWarnings("unused") // stapler
     public boolean isCloudAvailable() {
         return !Cloud.all().isEmpty();
     }
 
     @SuppressWarnings("unused") // stapler
     public String getCloudUpdateCenterCategoryLabel() {
         return URLEncoder.encode(UpdateCenter.getCategoryDisplayName("cloud"), StandardCharsets.UTF_8);
     }
 
     @Override
     public ModelObjectWithContextMenu.ContextMenu doChildrenContextMenu(StaplerRequest2 request, StaplerResponse2 response) throws Exception {
         ModelObjectWithContextMenu.ContextMenu m = new ModelObjectWithContextMenu.ContextMenu();
         Jenkins.get().clouds.forEach(m::add);
         return m;
     }
 
-    public Cloud getDynamic(String name, StaplerRequest2 req, StaplerResponse2 rsp) throws IOException, ServletException {
-        return Jenkins.get().clouds.getByName(name);
+    public Cloud getDynamic(String token, StaplerRequest2 req, StaplerResponse2 rsp) throws IOException, ServletException {
+        return getById(token);
     }
 
     @SuppressWarnings("unused") // stapler
     @Restricted(DoNotUse.class) // stapler
     public Jenkins.CloudList getClouds() {
         return Jenkins.get().clouds;
     }
 
     @SuppressWarnings("unused") // stapler
     @Restricted(DoNotUse.class) // stapler
     public boolean hasClouds() {
         return !Jenkins.get().clouds.isEmpty();
     }
 
     /**
      * Makes sure that the given name is good as an agent name.
      * @return trimmed name if valid; throws ParseException if not
      */
     public String checkName(String name) throws Failure {
         if (name == null)
             throw new Failure("Query parameter 'name' is required");
 
         name = name.trim();
         Jenkins.checkGoodName(name);
 
         if (Jenkins.get().getCloud(name) != null)
             throw new Failure(Messages.CloudSet_CloudAlreadyExists(name));
 
         // looks good
         return name;
     }
 
     @SuppressWarnings("unused") // stapler
     public FormValidation doCheckName(@QueryParameter String value) {
         Jenkins.get().checkPermission(Jenkins.ADMINISTER);
         if (Util.fixEmpty(value) == null) {
             return FormValidation.ok();
         }
         try {
             checkName(value);
             return FormValidation.ok();
         } catch (Failure e) {
             return FormValidation.error(e.getMessage());
         }
     }
 
     /**
      * First check point in creating a new cloud.
      */
     @RequirePOST
     public synchronized void doCreate(StaplerRequest2 req, StaplerResponse2 rsp,
-                                          @QueryParameter String name, @QueryParameter String mode,
-                                          @QueryParameter String from) throws IOException, ServletException, Descriptor.FormException {
+            @QueryParameter String name, @QueryParameter String mode,
+            @QueryParameter String from) throws IOException, ServletException, Descriptor.FormException {
         final Jenkins jenkins = Jenkins.get();
         jenkins.checkPermission(Jenkins.ADMINISTER);
 
         if (mode != null && mode.equals("copy")) {
             name = checkName(name);
 
             Cloud src = jenkins.getCloud(from);
             if (src == null) {
                 if (Util.fixEmpty(from) == null) {
                     throw new Failure(Messages.CloudSet_SpecifyCloudToCopy());
                 } else {
                     throw new Failure(Messages.CloudSet_NoSuchCloud(from));
                 }
             }
 
             // copy through XStream
             String xml = Jenkins.XSTREAM.toXML(src);
             Cloud result = (Cloud) Jenkins.XSTREAM.fromXML(xml);
             result.name = name;
+            // Provision a new unique ID for the copied cloud to ensure distinct identity
+            result.provisionNewId();
             jenkins.clouds.add(result);
             // send the browser to the config page
             rsp.sendRedirect2(Functions.getNearestAncestorUrl(req, jenkins) + "/" + result.getUrl() + "configure");
         } else {
             // proceed to step 2
             if (mode == null) {
                 throw new Failure("No mode given");
             }
 
             Descriptor<Cloud> d = Cloud.all().findByName(mode);
             if (d == null) {
                 throw new Failure("No node type ‘" + mode + "’ is known");
             }
             handleNewCloudPage(d, name, req, rsp);
         }
     }
 
     private void handleNewCloudPage(Descriptor<Cloud> descriptor, String name, StaplerRequest2 req, StaplerResponse2 rsp) throws IOException, ServletException, Descriptor.FormException {
         checkName(name);
         JSONObject formData = req.getSubmittedForm();
         formData.put("name", name);
         formData.remove("mode"); // Cloud descriptors won't have this field.
         req.setAttribute("instance", formData);
         req.setAttribute("descriptor", descriptor);
         req.getView(this, "_new.jelly").forward(req, rsp);
     }
 
     /**
      * Really creates a new agent.
      */
     @POST
     public synchronized void doDoCreate(StaplerRequest2 req, StaplerResponse2 rsp,
-                                            @QueryParameter String cloudDescriptorName) throws IOException, ServletException, Descriptor.FormException {
+            @QueryParameter String cloudDescriptorName) throws IOException, ServletException, Descriptor.FormException {
         Jenkins.get().checkPermission(Jenkins.ADMINISTER);
         Descriptor<Cloud> cloudDescriptor = Cloud.all().findByName(cloudDescriptorName);
         if (cloudDescriptor == null) {
-            throw new Failure(String.format("No cloud type ‘%s’ is known", cloudDescriptorName));
+            throw new Failure(String.format("No cloud type '%s' is known", cloudDescriptorName));
         }
         Cloud cloud = cloudDescriptor.newInstance(req, req.getSubmittedForm());
         if (!Jenkins.get().clouds.add(cloud)) {