From 1fc91cab10d972fd0957dd609b3645be02e0cd0f Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 3 Sep 2023 08:07:50 -0600 Subject: [PATCH 1/4] [JENKINS-70729] Stop supporting an empty cloud name https://github.com/jenkinsci/jenkins/pull/7658 reworks cloud management to use multiple pages instead of a single large page. That rework also requires that a cloud may not be named with an empty string. The docker plugin specifically tested that a cloud was allowed to have an empty string as its name, but performed no other operations with a cloud that is named with an empty string. Cloud named with an empty string may have worked prior to Jenkins 2.403, but they would have been complicated to manage, especially in a controller with multiple clouds defined. A cloud named with an empty string is much more difficult to manage. It is better to mention in the changelog that clouds are no longer allowed to have an empty name rather than wrestling with all the ways that an empty name will break cloud administration. https://issues.jenkins.io/browse/JENKINS-70729 is the enhancement request that implements the change. https://issues.jenkins.io/browse/JENKINS-71825 is the bug report for the automated test failure that started this investigation. --- .../jenkins/plugins/docker/utils/JenkinsUtilsTest.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/test/java/com/nirima/jenkins/plugins/docker/utils/JenkinsUtilsTest.java b/src/test/java/com/nirima/jenkins/plugins/docker/utils/JenkinsUtilsTest.java index dc58defed..979a025dd 100644 --- a/src/test/java/com/nirima/jenkins/plugins/docker/utils/JenkinsUtilsTest.java +++ b/src/test/java/com/nirima/jenkins/plugins/docker/utils/JenkinsUtilsTest.java @@ -28,12 +28,11 @@ public class JenkinsUtilsTest { public void getCloudByNameOrThrowGivenNameThenReturnsCloud() throws Exception { // Given final DockerAPI dockerApi = new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")); - final DockerCloud cloudEmpty = new DockerCloud("", dockerApi, Collections.emptyList()); final String expectedCloudName = "expectedCloudName"; final DockerCloud cloudExpected = new DockerCloud(expectedCloudName, dockerApi, Collections.emptyList()); final DockerCloud cloudOther = new DockerCloud("otherCloudName", dockerApi, Collections.emptyList()); final OtherTypeOfCloud cloudForeign = new OtherTypeOfCloud("foreign"); - final List clouds = List.of(cloudEmpty, cloudOther, cloudExpected, cloudForeign); + final List clouds = List.of(cloudOther, cloudExpected, cloudForeign); jenkins.getInstance().clouds.replaceBy(clouds); // When @@ -47,14 +46,13 @@ public void getCloudByNameOrThrowGivenNameThenReturnsCloud() throws Exception { public void getCloudByNameOrThrowGivenUnknownNameThenThrows() throws Exception { // Given final DockerAPI dockerApi = new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")); - final DockerCloud cloudEmpty = new DockerCloud("", dockerApi, Collections.emptyList()); final String requestedCloudName = "anUnknownCloudName"; final String cloudName1 = "expectedCloudName"; final DockerCloud cloud1 = new DockerCloud(cloudName1, dockerApi, Collections.emptyList()); final String cloudName2 = "otherCloudName"; final DockerCloud cloud2 = new DockerCloud(cloudName2, dockerApi, Collections.emptyList()); final OtherTypeOfCloud cloudForeign = new OtherTypeOfCloud("foreign"); - final List clouds = List.of(cloudEmpty, cloud2, cloud1, cloudForeign); + final List clouds = List.of(cloud2, cloud1, cloudForeign); jenkins.getInstance().clouds.replaceBy(clouds); try { @@ -75,14 +73,13 @@ public void getCloudByNameOrThrowGivenUnknownNameThenThrows() throws Exception { public void getCloudByNameOrThrowGivenForeignCloudNameThenThrows() throws Exception { // Given final DockerAPI dockerApi = new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")); - final DockerCloud cloudEmpty = new DockerCloud("", dockerApi, Collections.emptyList()); final String requestedCloudName = "foreign"; final String cloudName1 = "DockerCloud1Name"; final DockerCloud cloud1 = new DockerCloud(cloudName1, dockerApi, Collections.emptyList()); final String cloudName2 = "DockerCloud2Name"; final DockerCloud cloud2 = new DockerCloud(cloudName2, dockerApi, Collections.emptyList()); final OtherTypeOfCloud cloudForeign = new OtherTypeOfCloud(requestedCloudName); - final List clouds = List.of(cloudEmpty, cloud2, cloud1, cloudForeign); + final List clouds = List.of(cloud2, cloud1, cloudForeign); jenkins.getInstance().clouds.replaceBy(clouds); try { From 1416f898ce72c97b6c109de8ef066d8d6be5662c Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 3 Sep 2023 08:41:48 -0600 Subject: [PATCH 2/4] Log a warning if a DockerCloud name is blank Do not break existing usage of blank names, but warn in the log file that blank names are no longer supported after Jenkins 2.402. The Jenkins 2.421 cloud creation UI will not allow a blank name, but earlier versions may have allowed it. Assert the log message for blank or name cloud name --- .../jenkins/plugins/docker/DockerCloud.java | 3 ++ .../plugins/docker/DockerCloudTest.java | 30 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java b/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java index 73f2d4a85..ec90b3d31 100644 --- a/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java +++ b/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java @@ -138,6 +138,9 @@ public DockerCloud(String name, DockerAPI dockerApi, List templa super(name); this.dockerApi = dockerApi; this.templates = templates; + if (name == null || name.isBlank()) { + LOGGER.warn("Docker cloud requires a non-blank name after Jenkins 2.402"); + } } @Deprecated diff --git a/src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java b/src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java index 06267977d..64a7a761d 100644 --- a/src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java +++ b/src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java @@ -18,12 +18,17 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.logging.Level; +import org.hamcrest.MatcherAssert; +import org.hamcrest.core.IsIterableContaining; import org.jenkinsci.plugins.docker.commons.credentials.DockerServerCredentials; import org.jenkinsci.plugins.docker.commons.credentials.DockerServerEndpoint; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.LoggerRule; /** * @author Kanstantsin Shautsou @@ -33,6 +38,9 @@ public class DockerCloudTest { @Rule public JenkinsRule jenkins = new JenkinsRule(); + @Rule + public LoggerRule lr = new LoggerRule(); + @SuppressWarnings("unused") @Test public void testConstructor_0_10_2() { @@ -48,6 +56,28 @@ public void testConstructor_0_10_2() { null); // dockerHostname } + private static final String LOG_MESSAGE = "Docker cloud requires a non-blank name after Jenkins 2.402"; + + @Issue("JENKINS-70729") // Warn if cloud name is empty + @Test + public void testConstructorWithEmptyName() { + lr.record(DockerCloud.class.getName(), Level.ALL).capture(16); + DockerCloud cloud = + new DockerCloud("", new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")), List.of()); + Assert.assertEquals(cloud.getDisplayName(), ""); + MatcherAssert.assertThat(lr.getMessages(), IsIterableContaining.hasItem(LOG_MESSAGE)); + } + + @Issue("JENKINS-70729") // Warn if cloud name is null + @Test + public void testConstructorWithNullName() { + lr.record(DockerCloud.class.getName(), Level.ALL).capture(16); + DockerCloud cloud = + new DockerCloud(null, new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")), List.of()); + Assert.assertEquals(cloud.getDisplayName(), null); + MatcherAssert.assertThat(lr.getMessages(), IsIterableContaining.hasItem(LOG_MESSAGE)); + } + @Test public void globalConfigRoundtrip() throws Exception { From 80825c378997c127c58d6b054ff94630c4729dd8 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 3 Sep 2023 20:09:46 -0600 Subject: [PATCH 3/4] Log blank or null cloud name in readResolve --- .../java/com/nirima/jenkins/plugins/docker/DockerCloud.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java b/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java index ec90b3d31..ef479b8a6 100644 --- a/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java +++ b/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java @@ -730,6 +730,10 @@ protected Object readResolve() { dockerApi = new DockerAPI(dockerHost, connectTimeout, readTimeout, version, dockerHostname); } + if (name == null || name.isBlank()) { + LOGGER.warn("Docker cloud requires a non-blank name after Jenkins 2.402"); + } + return this; } From 57d301000787e1bc834c9b8683d68db0ce3276cd Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Mon, 4 Sep 2023 04:59:44 -0600 Subject: [PATCH 4/4] Convert empty or blank name to non-blank in readResolve Jenkins 2.403 improves the cloud management user interface. Each cloud is now managed from a separate page. That separate page requires a non-blank name for the cloud. Jenkins 2.402 and later allowed a blank name. When a blank name is detected, convert it to a non-blank name based on the hashCode of the DockerAPI and templates. --- .../jenkins/plugins/docker/DockerCloud.java | 11 ++++++++++- .../jenkins/plugins/docker/DockerCloudTest.java | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java b/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java index ef479b8a6..38805850a 100644 --- a/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java +++ b/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java @@ -143,6 +143,13 @@ public DockerCloud(String name, DockerAPI dockerApi, List templa } } + /* Constructor to create a new DockerCloud based on an existing DockerCloud */ + public DockerCloud(@NonNull String name, @NonNull DockerCloud source) { + super(name); + this.dockerApi = source.dockerApi; + this.templates = source.templates; + } + @Deprecated public DockerCloud( String name, @@ -731,7 +738,9 @@ protected Object readResolve() { } if (name == null || name.isBlank()) { - LOGGER.warn("Docker cloud requires a non-blank name after Jenkins 2.402"); + String newName = "docker-cloud-" + Integer.toHexString(hashCode()); + LOGGER.warn("Docker cloud requires a non-blank name after Jenkins 2.402, using '" + newName + "'"); + return new DockerCloud(newName, this); } return this; diff --git a/src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java b/src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java index 64a7a761d..b1db16be3 100644 --- a/src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java +++ b/src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java @@ -78,6 +78,21 @@ public void testConstructorWithNullName() { MatcherAssert.assertThat(lr.getMessages(), IsIterableContaining.hasItem(LOG_MESSAGE)); } + @Issue("JENKINS-70729") // Handle null or empty cloud name + @Test + public void testCopyConstructor() { + lr.record(DockerCloud.class.getName(), Level.ALL).capture(16); + DockerCloud cloud = + new DockerCloud(null, new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")), List.of()); + Assert.assertEquals(cloud.getDisplayName(), null); + MatcherAssert.assertThat(lr.getMessages(), IsIterableContaining.hasItem(LOG_MESSAGE)); + String newName = "docker-cloud-" + Integer.toHexString(cloud.hashCode()); + DockerCloud copy = new DockerCloud(newName, cloud); + Assert.assertEquals(cloud.getDockerApi(), copy.getDockerApi()); + Assert.assertEquals(cloud.getTemplates().hashCode(), copy.getTemplates().hashCode()); + Assert.assertEquals(newName, copy.getDisplayName()); + } + @Test public void globalConfigRoundtrip() throws Exception {