Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplifying JNLPLauncher #8762

Merged
merged 9 commits into from
Dec 13, 2023
3 changes: 1 addition & 2 deletions core/src/main/java/hudson/model/Slave.java
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,7 @@ public ComputerLauncher getLauncher() {
LOGGER.log(Level.WARNING, "could not update historical agentCommand setting to CommandLauncher", x);
}
}
// Default launcher does not use Work Directory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should only affect loading of ancient node definitions from disk, or I guess someone defining nodes via XML produced from template who for some reason declined to specify a launcher.

return launcher == null ? new JNLPLauncher(false) : launcher;
return launcher == null ? new JNLPLauncher() : launcher;
}

public void setLauncher(ComputerLauncher launcher) {
Expand Down
100 changes: 39 additions & 61 deletions core/src/main/java/hudson/slaves/JNLPLauncher.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
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;
Expand All @@ -43,10 +42,10 @@
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.
Expand All @@ -56,28 +55,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;

/**
Expand All @@ -88,14 +82,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) {
Expand All @@ -105,36 +92,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).
Comment on lines -109 to -110
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would only affect

  • Java callers
  • which use the no-arg ctor that had been deprecated for years
  • which use the now-deprecated -jnlpUrl mode
  • which even care about the location of the work dir

@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
jglick marked this conversation as resolved.
Show resolved Hide resolved
public JNLPLauncher(boolean enableWorkDir) {
this(null, null, enableWorkDir
? RemotingWorkDirSettings.getEnabledDefaults()
Expand All @@ -150,16 +131,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;
Expand All @@ -170,15 +149,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) {
Expand Down Expand Up @@ -210,6 +187,11 @@
return getRemotingOptions(escapeWindows(computer.getName()));
}

@Restricted(DoNotUse.class)
public boolean isConfigured() {
return webSocket || tunnel != null || workDirSettings.isConfigured();

Check warning on line 192 in core/src/main/java/hudson/slaves/JNLPLauncher.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 192 is only partially covered, 2 branches are missing
}

private String getRemotingOptions(String computerName) {
StringBuilder sb = new StringBuilder();
sb.append("-name ");
Expand Down Expand Up @@ -296,23 +278,19 @@
return DescriptorImpl.class.equals(getClass());
}

public FormValidation doCheckWebSocket(@QueryParameter boolean webSocket, @QueryParameter String tunnel) {
jglick marked this conversation as resolved.
Show resolved Hide resolved
if (webSocket) {
if (!WebSockets.isSupported()) {
return FormValidation.error(Messages.JNLPLauncher_WebsocketNotEnabled());
}
if (Util.fixEmptyAndTrim(tunnel) != null) {
return FormValidation.error(Messages.JNLPLauncher_TunnelingNotSupported());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other improper combinations now handled by args4j in the launcher itself.

}
} 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();
@Restricted(DoNotUse.class)
public boolean isTcpSupported() {
return Jenkins.get().getTcpSlaveAgentListener() != null;

Check warning on line 283 in core/src/main/java/hudson/slaves/JNLPLauncher.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 283 is only partially covered, one branch is missing
}

@Restricted(DoNotUse.class)
public boolean isInstanceIdentityInstalled() {
return InstanceIdentityProvider.RSA.getCertificate() != null && InstanceIdentityProvider.RSA.getPrivateKey() != null;

Check warning on line 288 in core/src/main/java/hudson/slaves/JNLPLauncher.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 288 is only partially covered, 2 branches are missing
}

@Restricted(DoNotUse.class)
public boolean isWebSocketSupported() {
return WebSockets.isSupported();
}

}
Expand Down
15 changes: 8 additions & 7 deletions core/src/main/java/jenkins/slaves/RemotingWorkDirSettings.java
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 warning on line 80 in core/src/main/java/jenkins/slaves/RemotingWorkDirSettings.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 80 is only partially covered, 4 branches are missing
}

/**
* Check if workdir is disabled.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand All @@ -36,4 +40,5 @@ THE SOFTWARE.
<f:textbox field="tunnel"/>
</f:entry>
</f:advanced>
</j:jelly>
</j:if>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -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.
52 changes: 22 additions & 30 deletions core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -25,41 +25,13 @@ 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" xmlns:i="jelly:fmt">
<j:choose>
<j:when test="${!it.launcher.webSocket and app.slaveAgentPort == -1}">
<div class="error">
${%slaveAgentPort.disabled}
<l:isAdmin><a href="${rootURL}/configureSecurity">${%configure.link.text}</a>.</l:isAdmin>
</div>
</j:when>
<j:when test="${it.offline and !it.temporarilyOffline}">
<j:set var="jenkinsURL" value="${h.inferHudsonURL(request)}"/>
<j:set var="copy_agent_jar_unix" value="curl -sO ${jenkinsURL}jnlpJars/agent.jar" />
<j:set var="copy_agent_jar_windows" value="curl.exe -sO ${jenkinsURL}jnlpJars/agent.jar" />
<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)}">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clause tried to omit the -secret when Jenkins is run without security, which did not actually work (perhaps as of jenkinsci/remoting#677, or perhaps due to changes in #8639). I do not see any compelling reason not to just always offer -secret.

<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>
Expand Down Expand Up @@ -104,8 +76,28 @@ ${copy_secret_to_file}
${copy_agent_jar_windows}
${copy_java_cmd_secret2_windows}
</pre>
</j:otherwise>
</j:choose>
<j:if test="${!it.launcher.configured}">
<p>
${%commonOptions}
</p>
</j:if>
<j:if test="${!it.launcher.descriptor.tcpSupported}">
<p>
${%tcp-port-disabled}
<l:isAdmin><a href="${rootURL}/manage/configureSecurity/">${%configure.link.text}</a>.</l:isAdmin>
</p>
Comment on lines +85 to +88
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to older message, but not marked as an error since it may be perfectly normal in a WebSockets-only installation.

</j:if>
<j:if test="${!it.launcher.descriptor.instanceIdentityInstalled}">
<p>
${%instance-identity-missing}
<l:isAdmin><a href="${rootURL}/manage/pluginManager/available">${%install-instance-identity}</a>.</l:isAdmin>
</p>
Comment on lines +91 to +94
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inbound-instance-identity

</j:if>
<j:if test="${!it.launcher.descriptor.webSocketSupported}">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(untested)

<p>
${%web-socket-unsupported}
</p>
</j:if>
<p>
${%powerShell.cli.curl}
</p>
Expand Down
19 changes: 16 additions & 3 deletions core/src/main/resources/hudson/slaves/JNLPLauncher/main.properties
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,22 @@
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

configure.link.text=Go to security configuration screen and change it
slaveAgentPort.disabled=JNLP agent port is disabled and agents cannot connect this way.
configure.link.text=Go to the security configuration screen and change it
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=\
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt -tunnel and the various workdir-related options are relevant to enough people to be worth enumerating here.

The most commonly used option is <code>-webSocket</code>. \
Run <code>java -jar agent.jar -help</code> for more.
tcp-port-disabled=\
The TCP port is disabled so TCP agents may not be connected. \
You may still use WebSocket agents.
instance-identity-missing=\
The <code>instance-identity</code> plugin appears to be missing. \
This would prevent you from connecting a TCP agent. \
You may still use WebSocket agents.
install-instance-identity=Install it now
web-socket-unsupported=\
WebSockets are not supported in this Jenkins installation. \
You may still use TCP agents.
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ Run\ from\ agent\ command\ line\:=\
Стартиране на агента от командния ред
launch\ agent=\
стартиране на агента
slaveAgentPort.disabled=\
Портът на агента е изключен
Launch\ agent\ from\ browser=\
Стартиране на агент от браузър
Connected\ via\ JNLP\ agent.=\
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
configure.link.text=Zur globalen Sicherheitskonfiguration wechseln und ändern
launch\ agent=Agent starten
Connected\ via\ JNLP\ agent.=Verbunden über JNLP-Agent.
slaveAgentPort.disabled=JNLP=Agenten können nicht verbinden, da der JNLP-Port deaktiviert ist.
Launch\ agent\ from\ browser=JNLP-Agent via Webbrowser starten
Connect\ agent\ to\ Jenkins\ one\ of\ these\ ways\:=Möglichkeiten, den Agent mit Jenkins zu verbinden:
Run\ from\ agent\ command\ line\:=Auf der Befehlszeile ausführen:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.

slaveAgentPort.disabled=El puerto TCP para los agentes via JNLP está deshabilitado.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming key and deleting old translations since there is now a significant extra clause about WebSockets.

launch\ agent=Lanzar agente
Or\ if\ the\ agent\ is\ headless\:=O si el agente no tiene pantalla
Run\ from\ agent\ command\ line\:=Ejecutar agente desde la línea de comandos del nodo
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,3 @@ Or\ if\ the\ agent\ is\ headless\:=O se l''agente è senza interfaccia grafica:
Run\ from\ agent\ command\ line\:=Esegui dalla riga di comando dell''agente
Run\ from\ agent\ command\ line,\ with\ the\ secret\ stored\ in\ a\ file\:=\
Esegui dalla riga di comando dell''agente con il segreto salvato in un file
slaveAgentPort.disabled=La porta agente JNLP è disabilitata e gli agenti non \
possono connettersi in questa modalità.
Loading
Loading