Merge pull request #343 from jenkinsci/label-validation

[JENKINS-51248] Fix label validation regex
jenkinsci · Jun 13, 2018 · c5f8fe2 · c5f8fe2
2 parents 4dca515 + 2c05b81
commit c5f8fe2
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,10 @@ Known issues
 
 See the full list of issues at [JIRA](https://issues.jenkins-ci.org/issues/?filter=15575)
 
+1.8.0
+-----
+* Validate label and container names with regex [#332](https://github.com/jenkinsci/kubernetes-plugin/pull/332) [#343](https://github.com/jenkinsci/kubernetes-plugin/pull/343) [JENKINS-51248](https://issues.jenkins-ci.org/browse/JENKINS-51248)
+
 1.7.1
 -----
 * Do not print credentials in build output or logs. Only affects certain pipeline steps like `withDockerRegistry`. `sh` step is not affected [SECURITY-883](https://issues.jenkins-ci.org/browse/SECURITY-883)

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java
@@ -61,6 +61,8 @@ public class PodTemplateUtils {
 
     private static final Logger LOGGER = Logger.getLogger(PodTemplateUtils.class.getName());
 
+    private static final Pattern LABEL_VALIDATION = Pattern.compile("[a-zA-Z0-9]([_\\.\\-a-zA-Z0-9]*[a-zA-Z0-9])?");
+
     /**
      * Combines a {@link ContainerTemplate} with its parent.
      * @param parent        The parent container template (nullable).
@@ -522,13 +524,7 @@ public static boolean validateContainerName(String name) {
      * Pulled from https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set
      */
     public static boolean validateLabel(String label) {
-        if (label != null && !label.isEmpty()) {
-            Pattern labelValidation = Pattern.compile("[a-zA-Z0-9]([_\\.-A-Za-z0-9]*[A-Za-z0-9])?");
-            if (label.length() > 63 || !labelValidation.matcher(label).matches()) {
-                return false;
-            }
-        }
-        return true;
+        return StringUtils.isBlank(label) ? true : label.length() <= 63 && LABEL_VALIDATION.matcher(label).matches();
     }
 
     private static List<EnvVar> combineEnvVars(Container parent, Container template) {

diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java
@@ -30,7 +30,6 @@
 import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -70,7 +69,6 @@ public class PodTemplateUtilsTest {
     private static final ContainerTemplate MAVEN_1 = new ContainerTemplate("maven", "maven:1", "sh -c", "cat");
     private static final ContainerTemplate MAVEN_2 = new ContainerTemplate("maven", "maven:2");
 
-
     @Test
     public void shouldReturnContainerTemplateWhenParentIsNull() {
         ContainerTemplate result = combine(null, JNLP_2);
@@ -84,7 +82,6 @@ public void shouldOverrideTheImageAndInheritTheRest() {
         assertEquals("cat", result.getArgs());
     }
 
-
     @Test
     public void shouldReturnPodTemplateWhenParentIsNull() {
         PodTemplate template = new PodTemplate();
@@ -94,7 +91,6 @@ public void shouldReturnPodTemplateWhenParentIsNull() {
         assertEquals(result, template);
     }
 
-
     @Test
     public void shouldOverrideServiceAccountIfSpecified() {
         PodTemplate parent = new PodTemplate();
@@ -135,8 +131,6 @@ public void shouldOverrideNodeSelectorIfSpecified() {
         assertEquals("key:value", result.getNodeSelector());
     }
 
-
-
     @Test
     public void shouldCombineAllImagePullSecrets() {
         PodTemplate parent = new PodTemplate();
@@ -165,7 +159,6 @@ public void shouldCombineAllImagePullSecrets() {
         assertEquals(1, result.getImagePullSecrets().size());
     }
 
-
     @Test
     public void shouldCombineAllAnnotations() {
         PodTemplate parent = new PodTemplate();
@@ -212,7 +205,6 @@ public void shouldUnwrapParent() {
         template1.setServiceAccount("sa1");
         template1.setImagePullSecrets(asList(SECRET_2, SECRET_3));
 
-
         PodTemplate result = unwrap(template1, asList(parent, template1));
         assertEquals(3, result.getImagePullSecrets().size());
         assertEquals("sa1", result.getServiceAccount());
@@ -230,14 +222,14 @@ public void shouldDropNoDataWhenIdentical() {
         podTemplate.setNodeUsageMode(Node.Mode.EXCLUSIVE);
         podTemplate.setImagePullSecrets(asList(SECRET_1));
         podTemplate.setInheritFrom("Inherit");
-        podTemplate.setInstanceCap(99);        
+        podTemplate.setInstanceCap(99);
         podTemplate.setSlaveConnectTimeout(99);
         podTemplate.setIdleMinutes(99);
         podTemplate.setActiveDeadlineSeconds(99);
         podTemplate.setServiceAccount("ServiceAccount");
         podTemplate.setCustomWorkspaceVolumeEnabled(true);
         podTemplate.setYaml("Yaml");
-        
+
         PodTemplate selfCombined = combine(podTemplate, podTemplate);
 
         assertEquals("Name", podTemplate.getName());
@@ -248,7 +240,7 @@ public void shouldDropNoDataWhenIdentical() {
         assertEquals(Node.Mode.EXCLUSIVE, podTemplate.getNodeUsageMode());
         assertEquals(asList(SECRET_1), podTemplate.getImagePullSecrets());
         assertEquals("Inherit", podTemplate.getInheritFrom());
-        assertEquals(99, podTemplate.getInstanceCap());        
+        assertEquals(99, podTemplate.getInstanceCap());
         assertEquals(99, podTemplate.getSlaveConnectTimeout());
         assertEquals(99, podTemplate.getIdleMinutes());
         assertEquals(99, podTemplate.getActiveDeadlineSeconds());
@@ -285,14 +277,14 @@ public void shouldUnwrapMultipleParents() {
         toUnwrap.setName("toUnwrap");
         toUnwrap.setInheritFrom("template1 template2");
 
-
         PodTemplate result = unwrap(toUnwrap, asList(parent, template1, template2));
         assertEquals(3, result.getImagePullSecrets().size());
         assertEquals("sa1", result.getServiceAccount());
         assertEquals("key:value", result.getNodeSelector());
         assertEquals(2, result.getContainers().size());
 
-        ContainerTemplate mavenTemplate = result.getContainers().stream().filter(c -> c.getName().equals("maven")).findFirst().orElse(null);
+        ContainerTemplate mavenTemplate = result.getContainers().stream().filter(c -> c.getName().equals("maven"))
+                .findFirst().orElse(null);
         assertNotNull(mavenTemplate);
         assertEquals("maven:2", mavenTemplate.getImage());
     }
@@ -403,7 +395,8 @@ public void shouldCombineAllSecretEnvVars() {
 
         ContainerTemplate result = combine(template1, template2);
 
-        assertThat(result.getEnvVars(), contains(containerSecretEnvVar1, containerSecretEnvVar2, containerSecretEnvVar3));
+        assertThat(result.getEnvVars(),
+                contains(containerSecretEnvVar1, containerSecretEnvVar2, containerSecretEnvVar3));
     }
 
     @Test
@@ -452,15 +445,15 @@ public void shouldCombineAllPodMounts() {
 
     @Test
     public void shouldCombineAllTolerations() {
-    	PodSpec podSpec1 = new PodSpec();
+        PodSpec podSpec1 = new PodSpec();
         Pod pod1 = new Pod();
         Toleration toleration1 = new Toleration("effect1", "key1", "oper1", Long.parseLong("1"), "val1");
         Toleration toleration2 = new Toleration("effect2", "key2", "oper2", Long.parseLong("2"), "val2");
         podSpec1.setTolerations(asList(toleration1, toleration2));
         pod1.setSpec(podSpec1);
         pod1.setMetadata(new ObjectMeta());
 
-    	PodSpec podSpec2 = new PodSpec();
+        PodSpec podSpec2 = new PodSpec();
         Pod pod2 = new Pod();
         Toleration toleration3 = new Toleration("effect3", "key3", "oper3", Long.parseLong("3"), "val3");
         Toleration toleration4 = new Toleration("effect4", "key4", "oper4", Long.parseLong("4"), "val4");
@@ -524,6 +517,63 @@ public void shouldSubstituteMultipleEnvVarsAndNotUseDefaultsForMissing() {
         Map<String, String> properties = new HashMap<>();
         properties.put("key1", "value1");
         properties.put("key2", "value2");
-        assertEquals("value1 or value2 or ${key3}", substitute("${key1} or ${key2} or ${key3}", properties, "defaultValue"));
+        assertEquals("value1 or value2 or ${key3}",
+                substitute("${key1} or ${key2} or ${key3}", properties, "defaultValue"));
+    }
+
+    @Test
+    public void testValidateLabelA() {
+        assertTrue(validateLabel("1"));
+        assertTrue(validateLabel("a"));
+    }
+
+    @Test
+    public void testValidateLabelAb() {
+        assertTrue(validateLabel("12"));
+        assertTrue(validateLabel("ab"));
+    }
+
+    @Test
+    public void testValidateLabelAbc() {
+        assertTrue(validateLabel("123"));
+        assertTrue(validateLabel("abc"));
+    }
+
+    @Test
+    public void testValidateLabelAbcd() {
+        assertTrue(validateLabel("1234"));
+        assertTrue(validateLabel("abcd"));
+    }
+
+    @Test
+    public void testValidateLabelMypod() {
+        assertTrue(validateLabel("mypod"));
+    }
+
+    @Test
+    public void testValidateLabelMyPodNested() {
+        assertTrue(validateLabel("mypodNested"));
+    }
+
+    @Test
+    public void testValidateLabelSpecialChars() {
+        assertTrue(validateLabel("x-_.z"));
+    }
+
+    @Test
+    public void testValidateLabelStartWithSpecialChars() {
+        assertFalse(validateLabel("-x"));
+    }
+
+    @Test
+    public void testValidateLabelLong() {
+        assertTrue(validateLabel("123456789012345678901234567890123456789012345678901234567890123"));
+        assertTrue(validateLabel("abcdefghijklmnopqrstuwxyzabcdefghijklmnopqrstuwxyzabcdefghijklm"));
+    }
+
+    @Test
+    public void testValidateLabelTooLong() {
+        assertFalse(validateLabel("1234567890123456789012345678901234567890123456789012345678901234"));
+        assertFalse(validateLabel("abcdefghijklmnopqrstuwxyzabcdefghijklmnopqrstuwxyzabcdefghijklmn"));
     }
 }