-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Use UUID for clouds #26233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Use UUID for clouds #26233
Conversation
- Add unique ID field to Cloud class for stable identification - Implement cloud/byId URL routing to prevent URL breakage - Add CloudByIdDispatcher in CloudSet and Jenkins.CloudList - Preserve cloud identity across reconfigurations - Add comprehensive tests for unique ID behavior
… URLs instead of name-based encoded URLs
|
|
||
| /** | ||
| * Stapler dispatcher that routes cloud requests by unique ID. | ||
| * Handles URL patterns like /cloud/cloudById/{uuid}/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we either do:
/cloud/id/{uuid}
or just:
/cloud/{uuid} and we can route it by checking if the name is a UUID / it matches a cloud that we have a uuid for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i will change it to cloud/{uuud}
-Update getDynamic methods in CloudSet to consistently use getById() instead of mixing name-based and UUID-based lookups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should include the uuid in the configure.jelly as a hidden field.
In the doConfigsubmit method we check if the uuids of this and the new cloud are identical. If not we throw an error. Don't know if that works though. Would require a DataBoundSetter setUniqueId
Might avoid all the logic we have in the doConfigSubmit at the beginning
I think we need a test for following scenario:
- Jenkins starts with a cloud that has no uuid
- the cloud should now have an id
- restart Jenkins
- the uuid you get in step 2 shouldn't change.
The problem right now might be that after the call to readResolve during startup there is no save of the Jenkins configuration happening which could lead to a loss of the uuid.
There is work in one of my plugins jenkinsci/agent-maintenance-plugin#320 that would greatly benefit from that uuid
| } | ||
|
|
||
| @Test | ||
| void testReadResolveMigrationAssignsId() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not testing the readResolve method. You test here that adding a cloud without id to Jenkins assigns a new id.
| private volatile String uniqueId; | ||
|
|
||
| /** | ||
| * Uniquely identifies this {@link Cloud} instance among other instances in {@link jenkins.model.Jenkins#clouds}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comment is not true. Should be adjusted accordingly.
| // Use identity comparison to find the correct cloud to replace | ||
| // This avoids issues where equals() (often based on name) matches multiple | ||
| // clouds | ||
| List<Cloud> newClouds = new ArrayList<>(j.clouds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that threadsafe? Assume one user changes a cloud and another user deletes a cloud at the same time. Unlikely but possible. One of the changes might then be lost when you replace the complete cloud list below
| } | ||
| } | ||
|
|
||
| Cloud reconfigured = cloud.reconfigure(req, req.getSubmittedForm()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just use this.reconfigure here?
| 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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already here the new uuid should be set I think
Fixes #26187
Cloud configuration URLs break when clouds are deleted or reordered. If you have three clouds and delete the first one, the URLs for the remaining two shift, causing 404 errors. This is especially problematic when multiple clouds share the same name since there's no way to distinguish them by URL alone.
The fix assigns each cloud a persistent unique ID (UUID) and routes URLs through that ID instead of the cloud name or index. This way, deleting or reordering clouds doesn't affect the URLs of other clouds.
Testing done
Wrote 14 unit tests in CloudUniqueIdTest covering:
UUID generation and uniqueness across 200 clouds
Thread safety of UUID generation (50 concurrent threads)
Duplicate name clouds can be individually accessed by their unique IDs
Deleting a cloud doesn't affect the IDs of remaining clouds
Copied clouds automatically get a new UUID via the overridden add() in CloudList
Input validation on getCloudById (null, empty, blank, missing)
Backward compatibility with cloudByIndex
Also manually tested on localhost:
Screenshots (UI changes only)
N/A
Proposed changelog entries
Proposed changelog category
/label bug
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@timja
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.