Enhance attach connection method and try to fix unit tests

CHANGELOG.md

@@ -4,7 +4,8 @@
 A pre-release can be downloaded from https://ci.jenkins.io/job/Plugins/job/docker-plugin/job/master/
 * Enhancement: container stop timeout now configurable [#732](https://github.com/jenkinsci/docker-plugin/issues/732)
 * Fix possible resource leak [#786](https://github.com/jenkinsci/docker-plugin/issues/786)
-* Enhancement: can now add/drop capabilites [#696](https://github.com/jenkinsci/docker-plugin/issues/696)
+* Enhancement: can now add/drop docker capabilites [#696](https://github.com/jenkinsci/docker-plugin/issues/696)
+* Enhancement: can now customise "attach" connections [#790](https://github.com/jenkinsci/docker-plugin/issues/790)
 
 ## 1.2.0
 _2020-04-02_

src/main/java/com/nirima/jenkins/plugins/docker/strategy/DockerOnceRetentionStrategy.java

@@ -22,6 +22,8 @@
 
 import static java.util.concurrent.TimeUnit.MINUTES;
 
+import java.util.Objects;
+
 /**
  * Mix of {@link org.jenkinsci.plugins.durabletask.executors.OnceRetentionStrategy} (1.3) and {@link CloudRetentionStrategy}
  * that allows configure it parameters and has Descriptor.
@@ -115,13 +117,16 @@ private synchronized void done(final DockerComputer c) {
         });
     }
 
+    @Override
+    public int hashCode() {
+        return Objects.hash(idleMinutes);
+    }
+
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
-
         DockerOnceRetentionStrategy that = (DockerOnceRetentionStrategy) o;
-
         return idleMinutes == that.idleMinutes;
     }
 

src/main/java/io/jenkins/docker/FastNodeProvisionerStrategy.java

@@ -80,10 +80,9 @@ private StrategyDecision applyFoCloud(@Nonnull NodeProvisioner.StrategyState sta
         if (availableCapacity >= currentDemand) {
             LOGGER.log(FINE, "Provisioning completed");
             return PROVISIONING_COMPLETED;
-        } else {
-            LOGGER.log(FINE, "Provisioning not complete, consulting remaining strategies");
-            return CONSULT_REMAINING_STRATEGIES;
         }
+        LOGGER.log(FINE, "Provisioning not complete, consulting remaining strategies");
+        return CONSULT_REMAINING_STRATEGIES;
     }
 
     /**

src/main/java/io/jenkins/docker/connector/DockerComputerAttachConnector.java

@@ -1,20 +1,30 @@
 package io.jenkins.docker.connector;
 
+import static com.nirima.jenkins.plugins.docker.DockerTemplateBase.splitAndFilterEmpty;
+
 import com.github.dockerjava.api.DockerClient;
 import com.github.dockerjava.api.command.CreateContainerCmd;
 import com.github.dockerjava.api.command.ExecCreateCmd;
 import com.github.dockerjava.api.command.ExecCreateCmdResponse;
 import com.github.dockerjava.api.command.InspectContainerResponse;
+import com.google.common.base.Joiner;
+
+import hudson.EnvVars;
 import hudson.Extension;
+import hudson.Util;
 import hudson.model.Descriptor;
 import hudson.model.TaskListener;
 import hudson.remoting.Channel;
 import hudson.slaves.ComputerLauncher;
 import hudson.slaves.SlaveComputer;
 import io.jenkins.docker.client.DockerAPI;
 import io.jenkins.docker.client.DockerMultiplexedInputStream;
+import jenkins.model.Jenkins;
+
 import org.apache.commons.lang.StringUtils;
 import org.jenkinsci.Symbol;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.DataBoundSetter;
 
@@ -25,13 +35,26 @@
 import java.io.PrintWriter;
 import java.io.Serializable;
 import java.net.Socket;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Objects;
+
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 
 /**
  * @author <a href="mailto:nicolas.deloof@gmail.com">Nicolas De Loof</a>
  */
 public class DockerComputerAttachConnector extends DockerComputerConnector implements Serializable {
 
+    @CheckForNull
     private String user;
+    @CheckForNull
+    private String javaExe;
+    @CheckForNull
+    private String[] jvmArgs;
+    @CheckForNull
+    private String[] entryPointCmd;
 
     @DataBoundConstructor
     public DockerComputerAttachConnector() {
@@ -41,15 +64,104 @@ public DockerComputerAttachConnector(String user) {
         this.user = user;
     }
 
+    @Nonnull
     public String getUser() {
-        return user;
+        return user==null ? "" : user;
     }
 
     @DataBoundSetter
     public void setUser(String user) {
-        this.user = user;
+        if ( user==null || user.trim().isEmpty()) {
+            this.user = null;
+        } else {
+            this.user = user;
+        }
+    }
+
+    @Nonnull
+    public String getJavaExe() {
+        return javaExe==null ? "" : javaExe;
+    }
+
+    @DataBoundSetter
+    public void setJavaExe(String javaExe) {
+        if ( javaExe==null || javaExe.trim().isEmpty()) {
+            this.javaExe = null;
+        } else {
+            this.javaExe = javaExe;
+        }
+    }
+
+    @CheckForNull
+    public String[] getEntryPointCmd(){
+        return entryPointCmd;
+    }
+
+    @Nonnull
+    public String getEntryPointCmdString() {
+        if (entryPointCmd == null) return "";
+        return Joiner.on("\n").join(entryPointCmd);
     }
 
+    @DataBoundSetter
+    public void setEntryPointCmdString(String entryPointCmdString) {
+        setEntryPointCmd(splitAndFilterEmpty(entryPointCmdString, "\n"));
+    }
+
+    public void setEntryPointCmd(String[] entryPointCmd) {
+        if (entryPointCmd == null || entryPointCmd.length == 0) {
+            this.entryPointCmd = null;
+        } else {
+            this.entryPointCmd = entryPointCmd;
+        }
+    }
+
+    @CheckForNull
+    public String[] getJvmArgs(){
+        return jvmArgs;
+    }
+
+    @Nonnull
+    public String getJvmArgsString() {
+        if (jvmArgs == null) return "";
+        return Joiner.on("\n").join(jvmArgs);
+    }
+
+    @DataBoundSetter
+    public void setJvmArgsString(String jvmArgsString) {
+        setJvmArgs(splitAndFilterEmpty(jvmArgsString, "\n"));
+    }
+
+    public void setJvmArgs(String[] jvmArgs) {
+        if (jvmArgs == null || jvmArgs.length == 0) {
+            this.jvmArgs = null;
+        } else {
+            this.jvmArgs = jvmArgs;
+        }
+    }
+
+    @Override
+    public int hashCode() {
+        final int prime = 31;
+        int result = super.hashCode();
+        result = prime * result + Arrays.hashCode(entryPointCmd);
+        result = prime * result + Arrays.hashCode(jvmArgs);
+        result = prime * result + Objects.hash(javaExe, user);
+        return result;
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+        if (this == obj) {
+            return true;
+        }
+        if (!super.equals(obj)) {
+            return false;
+        }
+        DockerComputerAttachConnector other = (DockerComputerAttachConnector) obj;
+        return Arrays.equals(entryPointCmd, other.entryPointCmd) && Objects.equals(javaExe, other.javaExe)
+                && Arrays.equals(jvmArgs, other.jvmArgs) && Objects.equals(user, other.user);
+    }
 
     @Override
     public void beforeContainerCreated(DockerAPI api, String workdir, CreateContainerCmd cmd) throws IOException, InterruptedException {
@@ -63,45 +175,106 @@ public void afterContainerStarted(DockerAPI api, String workdir, String containe
         }
     }
 
-    @Override
-    protected ComputerLauncher createLauncher(DockerAPI api, String workdir, InspectContainerResponse inspect, TaskListener listener) throws IOException, InterruptedException {
-        return new DockerAttachLauncher(api, inspect.getId(), user, workdir);
+    @Restricted(NoExternalUse.class)
+    public enum ArgumentVariables {
+        JavaExe("JAVA", "The java binary, e.g. java, /usr/bin/java etc."), //
+        JvmArgs("JVM_ARGS", "Any arguments for the JVM itself, e.g. -Xmx250m."), //
+        JarName("JAR_NAME", "The name of the jar file the node must run, e.g. slave.jar."), //
+        RemoteFs("FS_DIR",
+                "The filesystem folder in which the slave process is to be run."), //
+        JenkinsUrl("JENKINS_URL", "The Jenkins root URL.");
+        private final String name;
+        private final String description;
+
+        ArgumentVariables(String name, String description) {
+            this.name = name;
+            this.description = description;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public String getDescription() {
+            return description;
+        }
     }
 
+    private static final String DEFAULT_ENTRY_POINT_CMD_STRING = "${" + ArgumentVariables.JavaExe.getName() + "}\n"
+            + "${" + ArgumentVariables.JvmArgs.getName() + "}\n"
+            + "-jar\n"
+            + "${" + ArgumentVariables.RemoteFs.getName() + "}/${" + ArgumentVariables.JarName.getName() + "}\n"
+            + "-noReconnect\n"
+            + "-noKeepAlive\n"
+            + "-slaveLog\n"
+            + "${" + ArgumentVariables.RemoteFs.getName() + "}/agent.log";
+
     @Override
-    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-        DockerComputerAttachConnector that = (DockerComputerAttachConnector) o;
-        return user != null ? user.equals(that.user) : that.user == null;
+    protected ComputerLauncher createLauncher(DockerAPI api, String workdir, InspectContainerResponse inspect, TaskListener listener) throws IOException, InterruptedException {
+        return new DockerAttachLauncher(api, inspect.getId(), getUser(), workdir, getJavaExe(), getJvmArgsString(), getEntryPointCmdString());
     }
 
+
     @Extension(ordinal = 100) @Symbol("attach")
     public static class DescriptorImpl extends Descriptor<DockerComputerConnector> {
+
+        public String getDefaultJavaExe() {
+            return "java";
+        }
+
+        public String getJavaExeVariableName() {
+            return ArgumentVariables.JavaExe.name;
+        }
+
+        public String getJvmArgsVariableName() {
+            return ArgumentVariables.JvmArgs.name;
+        }
+
+        public Collection<ArgumentVariables> getEntryPointCmdVariables() {
+            return Arrays.asList(ArgumentVariables.values());
+        }
+
+        public Collection<String> getDefaultEntryPointCmd() {
+            final String[] args = splitAndFilterEmpty(DEFAULT_ENTRY_POINT_CMD_STRING, "\n");
+            return Arrays.asList(args);
+        }
+
         @Override
         public String getDisplayName() {
             return "Attach Docker container";
         }
     }
 
     private static class DockerAttachLauncher extends ComputerLauncher {
-
         private final DockerAPI api;
         private final String containerId;
         private final String user;
         private final String remoteFs;
+        private final String javaExe;
+        private final String jvmArgs;
+        private final String entryPointCmd;
 
-        private DockerAttachLauncher(DockerAPI api, String containerId, String user, String remoteFs) {
+        private DockerAttachLauncher(DockerAPI api, String containerId, String user, String remoteFs, String javaExe, String jvmArgs, String entryPointCmd) {
             this.api = api;
             this.containerId = containerId;
             this.user = user;
             this.remoteFs = remoteFs;
+            this.javaExe = javaExe;
+            this.jvmArgs = jvmArgs;
+            this.entryPointCmd = entryPointCmd;
         }
 
         @Override
         public void launch(final SlaveComputer computer, TaskListener listener) throws IOException, InterruptedException {
             final PrintStream logger = computer.getListener().getLogger();
-            logger.println("Connecting to docker container "+containerId);
+            final String jenkinsUrl = Jenkins.getInstance().getRootUrl();
+            final String effectiveJavaExe = javaExe.isEmpty() ? "java" : javaExe;
+            final String effectiveJvmArgs = jvmArgs.isEmpty() ? "" : jvmArgs;
+            final EnvVars knownVariables = calculateVariablesForVariableSubstitution(effectiveJavaExe, effectiveJvmArgs, remoting.getName(), remoteFs, jenkinsUrl);
+            final String effectiveEntryPointCmdString = StringUtils.isNotBlank(entryPointCmd) ? entryPointCmd : DEFAULT_ENTRY_POINT_CMD_STRING;
+            final String resolvedEntryPointCmdString = Util.replaceMacro(effectiveEntryPointCmdString, knownVariables);
+            final String[] resolvedEntryPointCmd = splitAndFilterEmpty(resolvedEntryPointCmdString, "\n");
+            logger.println("Connecting to docker container " + containerId + ", running command " + Joiner.on(" ").join(resolvedEntryPointCmd));
 
             final String execId;
             try(final DockerClient client = api.getClient()) {
@@ -110,7 +283,7 @@ public void launch(final SlaveComputer computer, TaskListener listener) throws I
                         .withAttachStdout(true)
                         .withAttachStderr(true)
                         .withTty(false)
-                        .withCmd("java", "-jar", remoteFs + '/' + remoting.getName(), "-noReconnect", "-noKeepAlive", "-slaveLog", remoteFs + "/agent.log");
+                        .withCmd(resolvedEntryPointCmd);
                 if (StringUtils.isNotBlank(user)) {
                     cmd.withUser(user);
                 }
@@ -160,6 +333,46 @@ public void onClosed(Channel channel, IOException cause) {
 
         }
 
+        private static EnvVars calculateVariablesForVariableSubstitution(final String javaExe, final String jvmArgs, final String jarName, final String remoteFs,
+                final String jenkinsUrl) throws IOException, InterruptedException {
+            final EnvVars knownVariables = new EnvVars();
+            final Jenkins j = Jenkins.getInstance();
+            addEnvVars(knownVariables, j.getGlobalNodeProperties());
+            for (final ArgumentVariables v : ArgumentVariables.values()) {
+                // This switch statement MUST handle all possible
+                // values of v.
+                final String argValue;
+                switch (v) {
+                    case JavaExe :
+                        argValue = javaExe;
+                        break;
+                    case JvmArgs :
+                        argValue = jvmArgs;
+                        break;
+                    case JarName :
+                        argValue = jarName;
+                        break;
+                    case RemoteFs :
+                        argValue = remoteFs;
+                        break;
+                    case JenkinsUrl :
+                        argValue = jenkinsUrl;
+                        break;
+                    default :
+                        final String msg = "Internal code error: Switch statement is missing \"case " + v.name()
+                                + " : argValue = ... ; break;\" code.";
+                        // If this line throws an exception then it's because
+                        // someone has added a new variable to the enum without
+                        // adding code above to handle it.
+                        // The two have to be kept in step in order to
+                        // ensure that the help text stays in step.
+                        throw new RuntimeException(msg);
+                }
+                addEnvVar(knownVariables, v.getName(), argValue);
+            }
+            return knownVariables;
+        }
+
         private String readLine(InputStream in) throws IOException {
             StringBuilder s = new StringBuilder();
             int c;