Skip to content

Commit

Permalink
Limit pod name to 63 characters, and change the randomly generated st…
Browse files Browse the repository at this point in the history
…ring
  • Loading branch information
austinmoore- committed Feb 28, 2017
1 parent 6471a8e commit f03113d
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.logging.Level;
import java.util.logging.Logger;

import org.apache.commons.lang.RandomStringUtils;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.plugins.durabletask.executors.OnceRetentionStrategy;
import org.jvnet.localizer.Localizable;
Expand Down Expand Up @@ -83,16 +84,16 @@ public KubernetesSlave(PodTemplate template, String nodeDescription, String clou
}

static String getSlaveName(PodTemplate template) {
String hex = Long.toHexString(System.nanoTime());
String randString = RandomStringUtils.random(5, "bcdfghjklmnpqrstvwxz0123456789");
String name = template.getName();
if (StringUtils.isEmpty(name)) {
return hex;
return String.format("kubernetes-%s", randString);
}
// no spaces
name = template.getName().replace(" ", "-").toLowerCase();
// keep it under 256 chars
name = name.substring(0, Math.min(name.length(), 256 - hex.length()));
return String.format("%s-%s", name, hex);
// keep it under 63 chars (62 is used to account for the '-')
name = name.substring(0, Math.min(name.length(), 62 - randString.length()));
return String.format("%s-%s", name, randString);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,11 @@ public boolean start() throws Exception {
cloud.getClass().getName()));
}
KubernetesCloud kubernetesCloud = (KubernetesCloud) cloud;
String uuid = UUID.randomUUID().toString().replaceAll("-", "");
String name = String.format(NAME_FORMAT, step.getName(), uuid);

This comment has been minimized.

Copy link
@iocanel

iocanel Mar 17, 2017

Contributor

@austinmoore-: I think that here lies an issue that I missed in my original review. The PodTemplate that we generate using the pipeline is added to the cloud configuration. This has the following side effects:

  • Two builds using podTemplates with the same name can get in each other way
  • It's possible to mess up with preconfigured podTemplate (found in config.xml)

So I am considering reverting just that part of the commit to how it used to be.

This comment has been minimized.

Copy link
@iocanel

iocanel Mar 17, 2017

Contributor

@carlossg: wdyt?


PodTemplateAction action = new PodTemplateAction(getContext().get(Run.class));

PodTemplate newTemplate = new PodTemplate();
newTemplate.setName(name);
newTemplate.setName(step.getName());
newTemplate.setInheritFrom(!Strings.isNullOrEmpty( action.getParentTemplates()) ? action.getParentTemplates() : step.getInheritFrom());
newTemplate.setInstanceCap(step.getInstanceCap());
newTemplate.setLabel(step.getLabel());
Expand All @@ -66,7 +64,7 @@ public boolean start() throws Exception {
getContext().newBodyInvoker().withContext(step).withCallback(new PodTemplateCallback(newTemplate)).start();


action.push(name);
action.push(step.getName());
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ public void testGetSlaveName() {
List<? extends PodVolume> volumes = Collections.emptyList();
List<ContainerTemplate> containers = Collections.emptyList();

assertRegex(KubernetesSlave.getSlaveName(new PodTemplate("image", volumes)), "^[0-9a-f]{10,}$");
assertRegex(KubernetesSlave.getSlaveName(new PodTemplate("", volumes, containers)), "^[0-9a-f]{10,}$");
assertRegex(KubernetesSlave.getSlaveName(new PodTemplate("image", volumes)), "^kubernetes-[0-9a-z]{5}$");
assertRegex(KubernetesSlave.getSlaveName(new PodTemplate("", volumes, containers)), "^kubernetes-[0-9a-z]{5}$");
assertRegex(KubernetesSlave.getSlaveName(new PodTemplate("a name", volumes, containers)),
("^a-name-[0-9a-f]{10,}$"));
("^a-name-[0-9a-z]{5}$"));
}

private void assertRegex(String name, String regex) {
Expand Down

0 comments on commit f03113d

Please sign in to comment.