Simplifying JNLPLauncher

core/src/main/java/hudson/slaves/JNLPLauncher.java

@@ -34,19 +34,16 @@
 import hudson.model.Computer;
 import hudson.model.Descriptor;
 import hudson.model.TaskListener;
-import hudson.util.FormValidation;
 import jenkins.model.Jenkins;
-import jenkins.model.identity.InstanceIdentityProvider;
 import jenkins.slaves.RemotingWorkDirSettings;
 import jenkins.util.SystemProperties;
-import jenkins.websocket.WebSockets;
 import org.apache.commons.lang.StringUtils;
 import org.jenkinsci.Symbol;
 import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.DoNotUse;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.DataBoundSetter;
-import org.kohsuke.stapler.QueryParameter;
 
 /**
  * {@link ComputerLauncher} via inbound connections.
@@ -56,28 +53,23 @@
 */
 public class JNLPLauncher extends ComputerLauncher {
     /**
-     * If the agent needs to tunnel the connection to the controller,
-     * specify the "host:port" here. This can include the special
-     * syntax "host:" and ":port" to indicate the default host/port
-     * shall be used.
-     *
-     * <p>
-     * Null if no tunneling is necessary.
-     *
-     * @since 1.250
+     * Deprecated (only used with deprecated {@code -jnlpUrl} mode), but cannot mark it as such without breaking CasC.
      */
+    @DataBoundSetter
     @CheckForNull
-    public final String tunnel;
+    public String tunnel;
 
     /**
      * @deprecated No longer used.
      */
     @Deprecated
     public final transient String vmargs = null;
 
+    @Deprecated
     @NonNull
     private RemotingWorkDirSettings workDirSettings = RemotingWorkDirSettings.getEnabledDefaults();
 
+    @Deprecated
     private boolean webSocket;
 
     /**
@@ -88,14 +80,7 @@
     public static final String CUSTOM_INBOUND_URL_PROPERTY = "jenkins.agent.inboundUrl";
 
     /**
-     * Constructor.
-     * @param tunnel Tunnel settings
-     * @param vmargs JVM arguments
-     * @param workDirSettings Settings for Work Directory management in Remoting.
-     *                        If {@code null}, {@link RemotingWorkDirSettings#getEnabledDefaults()}
-     *                        will be used to enable work directories by default in new agents.
-     * @since 2.68
-     * @deprecated use {@link #JNLPLauncher(String, String)} and {@link #setWorkDirSettings(RemotingWorkDirSettings)}
+     * @deprecated no useful properties, use {@link #JNLPLauncher()}
      */
     @Deprecated
     public JNLPLauncher(@CheckForNull String tunnel, @CheckForNull String vmargs, @CheckForNull RemotingWorkDirSettings workDirSettings) {
@@ -105,36 +90,30 @@
         }
     }
 
-    // TODO cannot easily make tunnel into a @DataBoundSetter because then the @DataBoundConstructor would be on a no-arg constructor
-    // which is already defined and deprecated. Could retroactively let no-arg constructor use default for workDirSettings,
-    // which would be a behavioral change only for callers of the Java constructor (unlikely).
-    @DataBoundConstructor
+    /**
+     * @deprecated no useful properties, use {@link #JNLPLauncher()}
+     */
+    @Deprecated
     public JNLPLauncher(@CheckForNull String tunnel) {
         this.tunnel = Util.fixEmptyAndTrim(tunnel);
     }
 
     /**
-     * @deprecated use {@link JNLPLauncher#JNLPLauncher(String)}
+     * @deprecated no useful properties, use {@link #JNLPLauncher()}
      */
     @Deprecated
     public JNLPLauncher(@CheckForNull String tunnel, @CheckForNull String vmargs) {
         this.tunnel = Util.fixEmptyAndTrim(tunnel);
     }
 
-    /**
-     * @deprecated This Launcher does not enable the work directory.
-     *             It is recommended to use {@link #JNLPLauncher(boolean)}
-     */
-    @Deprecated
+    @DataBoundConstructor
     public JNLPLauncher() {
-        this(false);
     }
 
     /**
-     * Constructor with default options.
-     *
-     * @param enableWorkDir If {@code true}, the work directory will be enabled with default settings.
+     * @deprecated no useful properties, use {@link #JNLPLauncher()}
      */
+    @Deprecated
     public JNLPLauncher(boolean enableWorkDir) {
         this(null, null, enableWorkDir
                 ? RemotingWorkDirSettings.getEnabledDefaults()
@@ -150,16 +129,14 @@
         return this;
     }
 
-    /**
-     * Returns work directory settings.
-     *
-     * @since 2.72
-     */
-    @NonNull
+    @Deprecated
     public RemotingWorkDirSettings getWorkDirSettings() {
         return workDirSettings;
     }
 
+    /**
+     * Deprecated (only used with deprecated {@code -jnlpUrl} mode), but cannot mark it as such without breaking CasC.
+     */
     @DataBoundSetter
     public final void setWorkDirSettings(@NonNull RemotingWorkDirSettings workDirSettings) {
         this.workDirSettings = workDirSettings;
@@ -170,15 +147,13 @@
         return false;
     }
 
-    /**
-     * @since 2.216
-     */
+    @Deprecated
     public boolean isWebSocket() {
         return webSocket;
     }
 
     /**
-     * @since 2.216
+     * Deprecated (only used with deprecated {@code -jnlpUrl} mode), but cannot mark it as such without breaking CasC.
      */
     @DataBoundSetter
     public void setWebSocket(boolean webSocket) {
@@ -210,6 +185,11 @@
         return getRemotingOptions(escapeWindows(computer.getName()));
     }
 
+    @Restricted(DoNotUse.class)
+    public boolean isConfigured() {
+        return webSocket || tunnel != null || workDirSettings.isConfigured();
+    }
+
     private String getRemotingOptions(String computerName) {
         StringBuilder sb = new StringBuilder();
         sb.append("-name ");
@@ -296,25 +276,6 @@
             return DescriptorImpl.class.equals(getClass());
         }
 
-        public FormValidation doCheckWebSocket(@QueryParameter boolean webSocket, @QueryParameter String tunnel) {
-            if (webSocket) {
-                if (!WebSockets.isSupported()) {
-                    return FormValidation.error(Messages.JNLPLauncher_WebsocketNotEnabled());
-                }
-                if (Util.fixEmptyAndTrim(tunnel) != null) {
-                    return FormValidation.error(Messages.JNLPLauncher_TunnelingNotSupported());
-                }
-            } else {
-                if (Jenkins.get().getTcpSlaveAgentListener() == null) {
-                    return FormValidation.error(Messages.JNLPLauncher_TCPPortDisabled());
-                }
-                if (InstanceIdentityProvider.RSA.getCertificate() == null || InstanceIdentityProvider.RSA.getPrivateKey() == null) {
-                    return FormValidation.error(Messages.JNLPLauncher_InstanceIdentityRequired());
-                }
-            }
-            return FormValidation.ok();
-        }
-
     }
 
     /**

core/src/main/java/jenkins/slaves/RemotingWorkDirSettings.java

@@ -41,14 +41,9 @@
 import org.kohsuke.stapler.DataBoundConstructor;
 
 /**
- * Defines settings of the Remoting work directory.
- *
- * This class contains Remoting Work Directory settings, which can be used when starting Jenkins agents.
- * See <a href="https://github.com/jenkinsci/remoting/blob/master/docs/workDir.md">Remoting Work Dir Documentation</a>.
- *
- * @author Oleg Nenashev
- * @since 2.72
+ * @deprecated only used with deprecated {@code -jnlpUrl} mode
  */
+@Deprecated
 public class RemotingWorkDirSettings implements Describable<RemotingWorkDirSettings> {
 
     private static final String DEFAULT_INTERNAL_DIR = "remoting";
@@ -79,6 +74,12 @@
         this(false, null, DEFAULT_INTERNAL_DIR, false);
     }
 
+    /** if this is not {@link #ENABLED_DEFAULT} */
+    @Restricted(NoExternalUse.class)
+    public boolean isConfigured() {
+        return disabled || workDirPath != null || !DEFAULT_INTERNAL_DIR.equals(internalDir) || failIfWorkDirIsMissing;
+    }
+
     /**
      * Check if workdir is disabled.
      *

core/src/main/resources/hudson/slaves/JNLPLauncher/config.jelly

@@ -24,6 +24,10 @@ THE SOFTWARE.
 
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+ <j:if test="${it.launcher.configured}">
+  <f:block>
+    ${%configured}
+  </f:block>
   <!-- This property is included only if used directly. Causes JENKINS-45895 in the case of includes otherwise -->
   <j:if test="${descriptor.workDirSupported}"> 
     <f:property title="${%Enable work directory}" field="workDirSettings" />
@@ -36,4 +40,5 @@ THE SOFTWARE.
       <f:textbox field="tunnel"/>
     </f:entry>
   </f:advanced>
-</j:jelly>
+ </j:if>
+</j:jelly>

core/src/main/resources/hudson/slaves/JNLPLauncher/config.properties

@@ -0,0 +1,4 @@
+configured=\
+  Configuring any properties on the inbound agent launcher is deprecated \
+  as it would only have an effect in deprecated <code>-jnlpUrl</code> mode. \
+  Instead pass any desired options to the agent yourself.

core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly

@@ -38,28 +38,6 @@ THE SOFTWARE.
       <j:set var="copy_java_cmd_unix" value="java -jar agent.jar -url ${jenkinsURL} ${it.launcher.getRemotingOptionsUnix(it)}${it.launcher.getWorkDirOptions(it)}" />
       <j:set var="copy_java_cmd_windows" value="java -jar agent.jar -url ${jenkinsURL} ${it.launcher.getRemotingOptionsWindows(it)}${it.launcher.getWorkDirOptions(it)}" />
       <j:if test="${h.hasPermission(it, it.CONNECT)}">
-          <j:choose>
-            <j:when test="${it.ACL.hasPermission(app.ANONYMOUS, it.CONNECT)}">
-                <h3>
-                  ${%slaveAgent.cli.run} (Unix)
-                  <l:copyButton text="${copy_agent_jar_unix};${copy_java_cmd_unix}"/>
-                </h3>
-<pre>
-${copy_agent_jar_unix}
-${copy_java_cmd_unix}
-</pre>
-
-                <h3>
-                  ${%slaveAgent.cli.run} (Windows)
-                  <l:copyButton text="${copy_agent_jar_windows} &amp; ${copy_java_cmd_windows}"/>
-                </h3>
-<pre>
-${copy_agent_jar_windows}
-${copy_java_cmd_windows}
-</pre>
-
-            </j:when>
-            <j:otherwise>
                 <j:set var="copy_java_cmd_secret_unix" value="java -jar agent.jar -url ${jenkinsURL} -secret ${it.jnlpMac} ${it.launcher.getRemotingOptionsUnix(it)}${it.launcher.getWorkDirOptions(it)}" />
                 <j:set var="copy_java_cmd_secret_windows" value="java -jar agent.jar -url ${jenkinsURL} -secret ${it.jnlpMac} ${it.launcher.getRemotingOptionsWindows(it)}${it.launcher.getWorkDirOptions(it)}" />
                 <h3>
@@ -104,8 +82,11 @@ ${copy_secret_to_file}
 ${copy_agent_jar_windows}
 ${copy_java_cmd_secret2_windows}
 </pre>
-            </j:otherwise>
-          </j:choose>
+          <j:if test="${!it.configured}">
+              <p>
+                  ${%commonOptions}
+              </p>
+          </j:if>
           <p>
             ${%powerShell.cli.curl}
           </p>

core/src/main/resources/hudson/slaves/JNLPLauncher/main.properties

@@ -25,4 +25,7 @@ slaveAgentPort.disabled=JNLP agent port is disabled and agents cannot connect th
 slaveAgent.connected=Agent is connected.
 slaveAgent.cli.run=Run from agent command line:
 slaveAgent.cli.run.secret=Or run from agent command line, with the secret stored in a file:
-powerShell.cli.curl=Note: PowerShell users must use curl.exe instead of curl because curl is a default PowerShell cmdlet alias for Invoke-WebRequest.
+powerShell.cli.curl=Note: PowerShell users must use curl.exe instead of curl because curl is a default PowerShell cmdlet alias for Invoke-WebRequest.
+commonOptions=\
+  The most commonly used option is <code>-webSocket</code>. \
+  Run <code>java -jar agent.jar -help</code> for more.

core/src/main/resources/hudson/slaves/Messages.properties

@@ -42,7 +42,3 @@ ComputerLauncher.JavaVersionResult={0} -version returned {1}.
 ComputerLauncher.UnknownJavaVersion=Couldn’t figure out the Java version of {0}
 Cloud.ProvisionPermission.Description=Provision new nodes
 Cloud.RequiredName=Cloud must have a unique non-empty name.
-JNLPLauncher.TCPPortDisabled=Either WebSocket mode is selected, or the TCP port for inbound agents must be enabled
-JNLPLauncher.InstanceIdentityRequired=You must install the instance-identity plugin to use inbound agents in TCP mode
-JNLPLauncher.WebsocketNotEnabled=WebSocket support is not enabled in this Jenkins installation
-JNLPLauncher.TunnelingNotSupported=Tunneling is not supported in WebSocket mode

core/src/main/resources/hudson/slaves/Messages_pt_BR.properties

@@ -40,9 +40,3 @@ DumbSlave.displayName=Agente permanente
 RetentionStrategy.Always.displayName=Mantenha este agente em serviço o máximo possível
 JNLPLauncher.displayName=Lançar um agente conectando-o ao controlador
 Cloud.ProvisionPermission.Description=Provisionar novos nós
-JNLPLauncher.InstanceIdentityRequired=É necessário instalar a extensão instance-identify para usar agentes de entrada \
-  no modo TCP
-JNLPLauncher.TunnelingNotSupported=Tunelamento não é suportado no modo WebSocket
-JNLPLauncher.WebsocketNotEnabled=O suporte a WebSocket não está habilitado para esta instalação do Jenkins
-JNLPLauncher.TCPPortDisabled=Ou o modo WebSocket é selecionado ou a porta TCP para agentes de entrada precisa ser \
-  habilitada

core/src/main/resources/hudson/slaves/Messages_ru.properties

@@ -40,9 +40,3 @@ ComputerLauncher.NoJavaFound=Обнаружена версия Java {0}, но т
 ComputerLauncher.JavaVersionResult=Команда {0} -version вернула {1}.
 ComputerLauncher.UnknownJavaVersion=Невозможно определить версию Java {0}
 Cloud.ProvisionPermission.Description=Выделить новые узлы
-JNLPLauncher.TCPPortDisabled=Или включён режим WebSocket, или необходимо включить TCP-порт для агентов по \
-  входящему подключению
-JNLPLauncher.InstanceIdentityRequired=Необходимо установить плагин instance-identity, чтобы использовать агенты по \
-  входящему подключению в режиме TCP
-JNLPLauncher.WebsocketNotEnabled=В этой установке Jenkins не включена поддержка WebSocket
-JNLPLauncher.TunnelingNotSupported=Туннелирование не поддерживается в режиме WebSocket

test/src/test/java/hudson/slaves/JNLPLauncherRealTest.java

@@ -27,12 +27,10 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 
-import hudson.ExtensionList;
 import hudson.PluginWrapper;
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
 import hudson.model.Slave;
-import hudson.util.FormValidation;
 import java.io.File;
 import jenkins.agents.WebSocketAgentsTest;
 import jenkins.slaves.JnlpSlaveAgentProtocol4;
@@ -98,7 +96,6 @@ public void run(JenkinsRule r) throws Throwable {
             for (PluginWrapper plugin : r.jenkins.pluginManager.getPlugins()) {
                 System.err.println(plugin + " active=" + plugin.isActive() + " enabled=" + plugin.isEnabled());
             }
-            assertThat(ExtensionList.lookupSingleton(JNLPLauncher.DescriptorImpl.class).doCheckWebSocket(webSocket, null).kind, is(FormValidation.Kind.OK));
             Slave agent = (Slave) r.jenkins.getNode(agentName);
             FreeStyleProject p = r.createFreeStyleProject();
             p.setAssignedNode(agent);

test/src/test/java/hudson/slaves/JNLPLauncherTest.java

@@ -28,6 +28,7 @@
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -112,10 +113,8 @@ public void testNoWorkDirMigration() {
     }
 
     @Test
-    @Issue("JENKINS-44112")
-    @SuppressWarnings("deprecation")
     public void testDefaults() {
-        assertTrue("Work directory should be disabled for agents created via old API", new JNLPLauncher().getWorkDirSettings().isDisabled());
+        assertFalse("Work directory enabled by default", new JNLPLauncher().getWorkDirSettings().isDisabled());
     }
 
     @Test
@@ -158,6 +157,7 @@ private ArgumentListBuilder buildJnlpArgs(Computer c) throws Exception {
         ArgumentListBuilder args = new ArgumentListBuilder();
         args.add(new File(new File(System.getProperty("java.home")), "bin/java").getPath(), "-jar");
         args.add(Which.jarFile(Launcher.class).getAbsolutePath());
+        // TODO deprecated mode
         args.add("-jnlpUrl", j.getURL() + "computer/" + c.getName() + "/jenkins-agent.jnlp");
 
         if (c instanceof SlaveComputer) {