From 368ec74b57dcfd79300151a132826f46b7a78a20 Mon Sep 17 00:00:00 2001 From: Allan Burdajewicz Date: Fri, 20 Dec 2019 15:01:45 +1000 Subject: [PATCH 1/2] [JENKINS-60551] Copy constructor test should check all fields * Default jelly values from java constants * Apply min values for readTimeout (15 sec) and connectTimeout (5 sec) * Add missing serverCertificate, readTimeout and capOnlyOnAlivePods to copy constructor --- .../plugins/kubernetes/KubernetesCloud.java | 44 +++++++++++++++--- .../kubernetes/KubernetesCloud/config.jelly | 8 ++-- .../kubernetes/KubernetesCloudTest.java | 45 ++++++++++++------- 3 files changed, 73 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java index 16dd08fa16..950c40ec8d 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java @@ -96,6 +96,10 @@ public class KubernetesCloud extends Cloud { /** Default timeout for idle workers that don't correctly indicate exit. */ public static final int DEFAULT_RETENTION_TIMEOUT_MINUTES = 5; + public static final int DEFAULT_READ_TIMEOUT_SECONDS = 15; + + public static final int DEFAULT_CONNECT_TIMEOUT_SECONDS = 5; + private String defaultsProviderTemplate; @Nonnull @@ -118,8 +122,8 @@ public class KubernetesCloud extends Cloud { private String credentialsId; private int containerCap = Integer.MAX_VALUE; private int retentionTimeout = DEFAULT_RETENTION_TIMEOUT_MINUTES; - private int connectTimeout; - private int readTimeout; + private int connectTimeout = DEFAULT_CONNECT_TIMEOUT_SECONDS; + private int readTimeout = DEFAULT_READ_TIMEOUT_SECONDS; /** @deprecated Stored as a list of PodLabels */ @Deprecated private transient Map labels; @@ -152,6 +156,7 @@ public KubernetesCloud(@NonNull String name, @NonNull KubernetesCloud source) { this.defaultsProviderTemplate = source.defaultsProviderTemplate; this.templates.addAll(source.templates); this.serverUrl = source.serverUrl; + this.serverCertificate = source.serverCertificate; this.skipTlsVerify = source.skipTlsVerify; this.addMasterProxyEnvVars = source.addMasterProxyEnvVars; this.namespace = source.namespace; @@ -162,10 +167,12 @@ public KubernetesCloud(@NonNull String name, @NonNull KubernetesCloud source) { this.containerCap = source.containerCap; this.retentionTimeout = source.retentionTimeout; this.connectTimeout = source.connectTimeout; + this.readTimeout = source.readTimeout; this.usageRestricted = source.usageRestricted; this.maxRequestsPerHost = source.maxRequestsPerHost; this.podRetention = source.podRetention; this.waitForPodSec = source.waitForPodSec; + this.capOnlyOnAlivePods = source.capOnlyOnAlivePods; setPodLabels(source.podLabels); } @@ -412,7 +419,7 @@ public int getReadTimeout() { @DataBoundSetter public void setReadTimeout(int readTimeout) { - this.readTimeout = readTimeout; + this.readTimeout = Math.max(DEFAULT_READ_TIMEOUT_SECONDS, readTimeout); } public int getConnectTimeout() { @@ -486,7 +493,7 @@ public int getMaxRequestsPerHost() { @DataBoundSetter public void setConnectTimeout(int connectTimeout) { - this.connectTimeout = connectTimeout; + this.connectTimeout = Math.max(DEFAULT_CONNECT_TIMEOUT_SECONDS, connectTimeout); } /** @@ -761,6 +768,7 @@ public static void addAliases() { } @RequirePOST + @SuppressWarnings("unused") // used by jelly public FormValidation doTestConnection(@QueryParameter String name, @QueryParameter String serverUrl, @QueryParameter String credentialsId, @QueryParameter String serverCertificate, @QueryParameter boolean skipTlsVerify, @@ -790,6 +798,7 @@ public FormValidation doTestConnection(@QueryParameter String name, @QueryParame } @RequirePOST + @SuppressWarnings("unused") // used by jelly public ListBoxModel doFillCredentialsIdItems(@QueryParameter String serverUrl) { Jenkins.get().checkPermission(Jenkins.ADMINISTER); return new StandardListBoxModel().withEmptySelection() // @@ -813,6 +822,7 @@ public ListBoxModel doFillCredentialsIdItems(@QueryParameter String serverUrl) { } @RequirePOST + @SuppressWarnings("unused") // used by jelly public FormValidation doCheckMaxRequestsPerHostStr(@QueryParameter String value) throws IOException, ServletException { try { Integer.parseInt(value); @@ -822,6 +832,7 @@ public FormValidation doCheckMaxRequestsPerHostStr(@QueryParameter String value) } } + @SuppressWarnings("unused") // used by jelly public FormValidation doCheckDirectConnection(@QueryParameter boolean value, @QueryParameter String jenkinsUrl) throws IOException, ServletException { int slaveAgentPort = Jenkins.get().getSlaveAgentPort(); if(slaveAgentPort == -1) return FormValidation.warning( @@ -845,6 +856,7 @@ public FormValidation doCheckDirectConnection(@QueryParameter boolean value, @Qu return FormValidation.ok(); } + @SuppressWarnings("unused") // used by jelly public FormValidation doCheckJenkinsUrl(@QueryParameter String value, @QueryParameter boolean directConnection) throws IOException, ServletException { try { if(!isEmpty(value)) new URL(value); @@ -854,6 +866,7 @@ public FormValidation doCheckJenkinsUrl(@QueryParameter String value, @QueryPara return FormValidation.ok(); } + @SuppressWarnings("unused") // used by jelly public List> getAllowedPodRetentions() { Jenkins jenkins = Jenkins.getInstanceOrNull(); if (jenkins == null) { @@ -862,7 +875,7 @@ public List> getAllowedPodRetentions() { return DescriptorVisibilityFilter.apply(this, jenkins.getDescriptorList(PodRetention.class)); } - @SuppressWarnings("rawtypes") + @SuppressWarnings({"rawtypes", "unused"}) // used by jelly public Descriptor getDefaultPodRetention() { Jenkins jenkins = Jenkins.getInstanceOrNull(); if (jenkins == null) { @@ -871,6 +884,25 @@ public Descriptor getDefaultPodRetention() { return jenkins.getDescriptor(PodRetention.getKubernetesCloudDefault().getClass()); } + @SuppressWarnings("unused") // used by jelly + public int getDefaultReadTimeout() { + return DEFAULT_READ_TIMEOUT_SECONDS; + } + + @SuppressWarnings("unused") // used by jelly + public int getDefaultConnectTimeout() { + return DEFAULT_CONNECT_TIMEOUT_SECONDS; + } + + @SuppressWarnings("unused") // used by jelly + public int getDefaultRetentionTimeout() { + return DEFAULT_RETENTION_TIMEOUT_MINUTES; + } + + public int getDefaultWaitForPod() { + return DEFAULT_WAIT_FOR_POD_SEC; + } + } @Override @@ -913,6 +945,8 @@ private Object readResolve() { if (podRetention == null) { podRetention = PodRetention.getKubernetesCloudDefault(); } + setConnectTimeout(connectTimeout); + setReadTimeout(readTimeout); setRetentionTimeout(retentionTimeout); if (waitForPodSec == null) { waitForPodSec = DEFAULT_WAIT_FOR_POD_SEC; diff --git a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly index 935e8cf58a..f68460b7bf 100644 --- a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly +++ b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly @@ -40,11 +40,11 @@ - + - + @@ -64,12 +64,12 @@ - + - + diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java index c51a3970c1..aa6b127300 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java @@ -3,6 +3,7 @@ import static org.junit.Assert.*; import java.io.IOException; +import java.lang.reflect.InvocationTargetException; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; import java.security.UnrecoverableKeyException; @@ -26,6 +27,10 @@ import com.gargoylesoftware.htmlunit.html.HtmlInput; import com.gargoylesoftware.htmlunit.html.HtmlPage; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; +import org.apache.commons.beanutils.PropertyUtils; +import org.apache.commons.lang3.RandomStringUtils; +import org.apache.commons.lang3.RandomUtils; import org.csanchez.jenkins.plugins.kubernetes.pod.retention.Always; import org.csanchez.jenkins.plugins.kubernetes.pod.retention.PodRetention; import org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume; @@ -274,28 +279,38 @@ public void testLabels() { } @Test - public void copyConstructor() { + public void copyConstructor() throws Exception { PodTemplate pt = new PodTemplate(); pt.setName("podTemplate"); KubernetesCloud cloud = new KubernetesCloud("name"); - cloud.setDefaultsProviderTemplate("default"); + ArrayList objectProperties = Lists.newArrayList("templates", "podRetention", "podLabels", "labels"); + for (String property: PropertyUtils.describe(cloud).keySet()) { + if (PropertyUtils.isWriteable(cloud, property)) { + Class propertyType = PropertyUtils.getPropertyType(cloud, property); + if (propertyType == String.class) { + if (property.endsWith("Str")) { + // setContainerCapStr + // setMaxRequestsPerHostStr + PropertyUtils.setProperty(cloud, property, RandomStringUtils.randomNumeric(3)); + } else { + PropertyUtils.setProperty(cloud, property, RandomStringUtils.randomAlphabetic(10)); + } + } else if (propertyType == int.class) { + PropertyUtils.setProperty(cloud, property, RandomUtils.nextInt()); + } else if (propertyType == Integer.class) { + PropertyUtils.setProperty(cloud, property, Integer.valueOf(RandomUtils.nextInt())); + } else if (propertyType == boolean.class) { + PropertyUtils.setProperty(cloud, property, RandomUtils.nextBoolean()); + } else if (!objectProperties.contains(property)) { + fail("Unhandled field in copy constructor: " + property); + } + } + } cloud.setTemplates(Collections.singletonList(pt)); - cloud.setServerUrl("serverUrl"); - cloud.setSkipTlsVerify(true); - cloud.setAddMasterProxyEnvVars(true); - cloud.setNamespace("namespace"); - cloud.setJenkinsUrl("jenkinsUrl"); - cloud.setJenkinsTunnel("tunnel"); - cloud.setCredentialsId("abcd"); - cloud.setContainerCapStr("100"); - cloud.setRetentionTimeout(1000); - cloud.setConnectTimeout(123); - cloud.setUsageRestricted(true); - cloud.setMaxRequestsPerHostStr("42"); cloud.setPodRetention(new Always()); - cloud.setWaitForPodSec(245); cloud.setPodLabels(PodLabel.listOf("foo", "bar", "cat", "dog")); + cloud.setLabels(ImmutableMap.of("foo", "bar")); KubernetesCloud copy = new KubernetesCloud("copy", cloud); From 0a4ed30c749824d6d3d8e388f74f9b5d0f0cb87a Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 20 Dec 2019 15:44:23 +0100 Subject: [PATCH 2/2] Reviews from Jesse * Use XStream for copy constructor * Convert fields with numeric input to f:number with minimum * Validate integer are in expected ranges --- .../plugins/kubernetes/KubernetesCloud.java | 51 +++++++++---------- .../kubernetes/KubernetesCloud/config.jelly | 10 ++-- .../KubernetesCloud/help-connectTimeout.html | 4 +- .../KubernetesCloud/help-readTimeout.html | 4 +- .../kubernetes/KubernetesCloudTest.java | 5 +- 5 files changed, 37 insertions(+), 37 deletions(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java index 950c40ec8d..2e3bbe03a9 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java @@ -3,6 +3,7 @@ import static org.apache.commons.lang.StringUtils.isEmpty; import java.io.IOException; +import java.io.StringReader; import java.net.ConnectException; import java.net.MalformedURLException; import java.net.SocketTimeoutException; @@ -28,6 +29,7 @@ import javax.annotation.Nonnull; import javax.servlet.ServletException; +import hudson.util.XStream2; import io.fabric8.openshift.client.OpenShiftClient; import org.apache.commons.codec.binary.Base64; import org.apache.commons.lang.StringUtils; @@ -153,27 +155,11 @@ public KubernetesCloud(String name) { */ public KubernetesCloud(@NonNull String name, @NonNull KubernetesCloud source) { super(name); - this.defaultsProviderTemplate = source.defaultsProviderTemplate; + XStream2 xs = new XStream2(); + xs.omitField(Cloud.class, "name"); + xs.omitField(KubernetesCloud.class, "templates"); // TODO PodTemplate and fields needs to implement equals + xs.unmarshal(XStream2.getDefaultDriver().createReader(new StringReader(xs.toXML(source))), this); this.templates.addAll(source.templates); - this.serverUrl = source.serverUrl; - this.serverCertificate = source.serverCertificate; - this.skipTlsVerify = source.skipTlsVerify; - this.addMasterProxyEnvVars = source.addMasterProxyEnvVars; - this.namespace = source.namespace; - this.directConnection = source.directConnection; - this.jenkinsUrl = source.jenkinsUrl; - this.jenkinsTunnel = source.jenkinsTunnel; - this.credentialsId = source.credentialsId; - this.containerCap = source.containerCap; - this.retentionTimeout = source.retentionTimeout; - this.connectTimeout = source.connectTimeout; - this.readTimeout = source.readTimeout; - this.usageRestricted = source.usageRestricted; - this.maxRequestsPerHost = source.maxRequestsPerHost; - this.podRetention = source.podRetention; - this.waitForPodSec = source.waitForPodSec; - this.capOnlyOnAlivePods = source.capOnlyOnAlivePods; - setPodLabels(source.podLabels); } @Deprecated @@ -824,12 +810,25 @@ public ListBoxModel doFillCredentialsIdItems(@QueryParameter String serverUrl) { @RequirePOST @SuppressWarnings("unused") // used by jelly public FormValidation doCheckMaxRequestsPerHostStr(@QueryParameter String value) throws IOException, ServletException { - try { - Integer.parseInt(value); - return FormValidation.ok(); - } catch (NumberFormatException e) { - return FormValidation.error("Please supply an integer"); - } + return FormValidation.validatePositiveInteger(value); + } + + @RequirePOST + @SuppressWarnings("unused") // used by jelly + public FormValidation doCheckConnectTimeout(@QueryParameter String value) { + return FormValidation.validateIntegerInRange(value, DEFAULT_CONNECT_TIMEOUT_SECONDS, Integer.MAX_VALUE); + } + + @RequirePOST + @SuppressWarnings("unused") // used by jelly + public FormValidation doCheckReadTimeout(@QueryParameter String value) { + return FormValidation.validateIntegerInRange(value, DEFAULT_READ_TIMEOUT_SECONDS, Integer.MAX_VALUE); + } + + @RequirePOST + @SuppressWarnings("unused") // used by jelly + public FormValidation doCheckRetentionTimeout(@QueryParameter String value) { + return FormValidation.validateIntegerInRange(value, DEFAULT_RETENTION_TIMEOUT_MINUTES, Integer.MAX_VALUE); } @SuppressWarnings("unused") // used by jelly diff --git a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly index f68460b7bf..5aec1ad612 100644 --- a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly +++ b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/config.jelly @@ -40,15 +40,15 @@ - + - + - + @@ -60,7 +60,7 @@ descriptors="${descriptor.allowedPodRetentions}" default="${descriptor.defaultPodRetention}" /> - + @@ -69,7 +69,7 @@ - + diff --git a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/help-connectTimeout.html b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/help-connectTimeout.html index a423943bf2..2a2d183eaa 100644 --- a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/help-connectTimeout.html +++ b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/help-connectTimeout.html @@ -1,3 +1,3 @@
- The connection timeout in seconds for connections to Kubernetes API. A value of 0 means no timeout. -
\ No newline at end of file + The connection timeout in seconds for connections to Kubernetes API. Minimum value is 5. + diff --git a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/help-readTimeout.html b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/help-readTimeout.html index 8d94da79f7..4869882592 100644 --- a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/help-readTimeout.html +++ b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud/help-readTimeout.html @@ -1,3 +1,3 @@
- The read timeout in seconds for connections to Kubernetes API. A value of 0 means no timeout. -
\ No newline at end of file + The read timeout in seconds for connections to Kubernetes API. Minimum value is 15. + diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java index aa6b127300..6e3a90a94e 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloudTest.java @@ -284,7 +284,7 @@ public void copyConstructor() throws Exception { pt.setName("podTemplate"); KubernetesCloud cloud = new KubernetesCloud("name"); - ArrayList objectProperties = Lists.newArrayList("templates", "podRetention", "podLabels", "labels"); + ArrayList objectProperties = Lists.newArrayList("templates", "podRetention", "podLabels", "labels", "serverCertificate"); for (String property: PropertyUtils.describe(cloud).keySet()) { if (PropertyUtils.isWriteable(cloud, property)) { Class propertyType = PropertyUtils.getPropertyType(cloud, property); @@ -307,13 +307,14 @@ public void copyConstructor() throws Exception { } } } + cloud.setServerCertificate("-----BEGIN CERTIFICATE-----"); cloud.setTemplates(Collections.singletonList(pt)); cloud.setPodRetention(new Always()); cloud.setPodLabels(PodLabel.listOf("foo", "bar", "cat", "dog")); cloud.setLabels(ImmutableMap.of("foo", "bar")); KubernetesCloud copy = new KubernetesCloud("copy", cloud); - + assertEquals("copy", copy.name); assertEquals("Expected cloud from copy constructor to be equal to the source except for name", cloud, copy); }