Remove JNLP terminology from GUI when referring to inbound agents #3998

jglick · 2019-04-22T15:00:04Z

Even Jenkins contributors tend to get confused about how JNLP is used for Jenkins agents. For agents running Java 11 (at least without IcedTea), there is no actual Java Web Start involved, and anyway most users do not actually use javaws to launch the agent; typically the *.jnlp file is merely used as a downloadable description of some arguments to pass to the internal launcher.

Where it was necessary to use a term at all to differentiate this agent launcher from others, I used “inbound” or “inbound TCP”, since the other commonly used launchers all involve the master initiating an “outbound” connection. Of course some other inbound launch method using e.g. WebSocket (rather than TcpSlaveAgentListener / AgentProtocol) could be developed in the future. JENKINS-44100 would allow the current implementation to be hosted in a plugin, making the choice of implementations clearer.

If accepted, could be matched by some updates to jenkins.io.

Proposed changelog entries

Removed misleading references to Java Web Start and JNLP from GUI surrounding inbound Jenkins agents.

Desired reviewers

@jenkinsci/code-reviewers

jglick

jnlpSecurity.html appears to have been unused. Perhaps left over from some long-passed security refactoring?

jglick · 2019-04-22T15:00:54Z

core/src/main/java/hudson/TcpSlaveAgentListener.java

@@ -70,7 +70,7 @@
 import org.kohsuke.accmod.restrictions.NoExternalUse;

 /**
- * Listens to incoming TCP connections from JNLP agents and deprecated Remoting-based CLI.
+ * Listens to incoming TCP connections, for example from agents.


As in #3993 this was forgotten.

jglick · 2019-04-22T15:01:15Z

core/src/main/java/hudson/TcpSlaveAgentListener.java

@@ -550,37 +550,3 @@ public String toString() {
    @Restricted(NoExternalUse.class)
    public static Integer CLI_PORT = SystemProperties.getInteger(TcpSlaveAgentListener.class.getName()+".port");
 }
-
-/*
-Pasted from http://today.java.net/pub/a/today/2005/09/01/webstart.html


I think this design decision has long since been taken!

jglick · 2019-04-22T15:01:26Z

core/src/main/java/hudson/lifecycle/WindowsInstallerLink.java

-            // in certain situations where we know the user is just trying Jenkins (like when Jenkins is launched
-            // from JNLP), also put this link on the navigation bar to increase
-            // visibility
+            // TODO possibly now unused (JNLP installation mode is long gone):


Probably dead code here, unknown.

jglick · 2019-04-22T15:01:46Z

core/src/main/java/jenkins/model/Jenkins.java

@@ -4746,7 +4746,7 @@ public boolean isSubjectToMandatoryReadPermissionCheck(String restOfPath) {
     */
    public Collection<String> getUnprotectedRootActions() {
        Set<String> names = new TreeSet<>();
-        names.add("jnlpJars"); // TODO cleaner to refactor doJnlpJars into a URA
+        names.add("jnlpJars"); // TODO cleaner to refactor doJnlpJars into a URA (see also JENKINS-44100)


See PR description for comment.

jglick · 2019-04-22T15:02:03Z

core/src/main/resources/hudson/TcpSlaveAgentListener/index.jelly

@@ -26,7 +26,7 @@ THE SOFTWARE.
 <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" trim="true">
  <st:contentType value="text/plain;charset=UTF-8"/>
  <!--
-    Publicize the TCP port number for TCP agents and Remoting CLI so that they know where to connect.
+    Publicize the TCP port number for inbound agents so that they know where to connect.


Again as in #3993.

jglick · 2019-04-22T15:02:19Z

core/src/main/resources/hudson/model/Computer/custom-jnlp.jelly

@@ -29,8 +29,7 @@ THE SOFTWARE.
    <l:main-panel>
      <h1>Tweak Launch Parameters</h1>
      <p>
-        This page allows you to tweak the JVM parameters when launching JNLP agents.
-        <!-- TODO: work from home for the rest -->


LOL I guess @kohsuke just went to sleep instead!

jglick · 2019-04-22T15:03:42Z

core/src/main/resources/hudson/security/GlobalSecurityConfiguration/index.groovy

@@ -43,7 +43,7 @@ l.layout(norefresh:true, permission:app.ADMINISTER, title:my.displayName, csscla
            }

            f.section(title: _("Agents")) {
-                f.entry(title: _("TCP port for JNLP agents"), field: "slaveAgentPort") {
+                f.entry(title: _("TCP port for inbound agents"), field: "slaveAgentPort") {


One of the more prominent GUI changes.

Note that this is technically inaccurate since the TCP port can be used for other purposes:

Remoting-based CLI, since removed.

CloudBees Jenkins Operations Center.

In practice it seems sufficient to label it according to what is by far its most important use.

jglick · 2019-04-22T15:03:59Z

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

+  In one mode, <a href="https://en.wikipedia.org/wiki/Java_Web_Start" target="_blank">Java Web Start</a> is used. \
+  In this case, a JNLP file must be opened on the agent machine, \
+  which will establish a TCP connection to the Jenkins master. \
+  (Other launch methods use a JNLP file but not Java Web Start, or do not use a JNLP file at all.)<br>\


For the last clause, see https://github.com/jenkinsci/docker-jnlp-slave/issues/87.

jglick · 2019-04-22T15:04:24Z

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

+JnlpSlaveAgentProtocol.displayName=Inbound TCP Agent Protocol/1 (deprecated, unencrypted)
+JnlpSlaveAgentProtocol2.displayName=Inbound TCP Agent Protocol/2 (deprecated, unencrypted)
+JnlpSlaveAgentProtocol3.displayName=Inbound TCP Agent Protocol/3 (deprecated, basic encryption)
+JnlpSlaveAgentProtocol4.displayName=Inbound TCP Agent Protocol/4 (TLS encryption)


Usually seen only when you open an advanced-mode button.

This seems to be the only place where it is referred to as "Inbound TCP" rather than just "Inbound". This difference seems a little confusing. I haven't quite figured out why the "TCP" part is important or what it needs to say. Would it be just as well to call it "Inbound Agent Protocol/x"?

See second paragraph of #3998 (comment). It is true that in this context the TCP could probably be dropped—any of these protocols are running on the same TCP port.

jglick · 2019-04-22T15:04:35Z

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

@@ -23,7 +23,7 @@
 RetentionStrategy.Always.displayName=Keep this agent online as much as possible
 RetentionStrategy.Demand.displayName=Bring this agent online when in demand, and take offline when idle
 RetentionStrategy.Demand.OfflineIdle=Offline because computer was idle; it will be relaunched when needed.
-JNLPLauncher.displayName=Launch agent via Java Web Start
+JNLPLauncher.displayName=Launch agent by connecting it to the master


The other prominent GUI rename.

daniel-beck · 2019-04-22T15:02:43Z

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

@@ -23,7 +23,7 @@
 RetentionStrategy.Always.displayName=Keep this agent online as much as possible
 RetentionStrategy.Demand.displayName=Bring this agent online when in demand, and take offline when idle
 RetentionStrategy.Demand.OfflineIdle=Offline because computer was idle; it will be relaunched when needed.
-JNLPLauncher.displayName=Launch agent via Java Web Start
+JNLPLauncher.displayName=Launch agent by connecting it to the master


Suggested change

JNLPLauncher.displayName=Launch agent by connecting it to the master

JNLPLauncher.displayName=Launch agent by connecting it to the master TCP agent port

I guess -0. Seems excessively technical. From the user PoV, either I am asking Jenkins to start an agent, typically with SSH; or I am connecting an agent to Jenkins, and the fact that there is a “TCP port” involved in that is usually uninteresting, unless you ran into some firewall issues forcing you to set a fixed port. Anyone else feel strongly one way or the other?

FWIW this is trying to tie it to the UI option to enable the TCP agent listener port.

Right. But note that the launcher option does not even appear until you enable the port.

I don't have much opinion one way or the other. I tend to think that adding "TCP agent port" doesn't help much, but it's also not totally clear what "Launch agent by connecting it to the master" means either. I've tried to come up with better wording but haven't had success yet.

war/src/main/webapp/help/system-config/master-slave/jnlpSecurity.html

jeffret-b

This is a very nice improvement to clarify behaviors for users and developers.

jeffret-b · 2019-04-22T17:38:48Z

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

@@ -23,7 +23,7 @@
 RetentionStrategy.Always.displayName=Keep this agent online as much as possible
 RetentionStrategy.Demand.displayName=Bring this agent online when in demand, and take offline when idle
 RetentionStrategy.Demand.OfflineIdle=Offline because computer was idle; it will be relaunched when needed.
-JNLPLauncher.displayName=Launch agent via Java Web Start
+JNLPLauncher.displayName=Launch agent by connecting it to the master


I don't have much opinion one way or the other. I tend to think that adding "TCP agent port" doesn't help much, but it's also not totally clear what "Launch agent by connecting it to the master" means either. I've tried to come up with better wording but haven't had success yet.

jeffret-b · 2019-04-22T17:42:47Z

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

+JnlpSlaveAgentProtocol.displayName=Inbound TCP Agent Protocol/1 (deprecated, unencrypted)
+JnlpSlaveAgentProtocol2.displayName=Inbound TCP Agent Protocol/2 (deprecated, unencrypted)
+JnlpSlaveAgentProtocol3.displayName=Inbound TCP Agent Protocol/3 (deprecated, basic encryption)
+JnlpSlaveAgentProtocol4.displayName=Inbound TCP Agent Protocol/4 (TLS encryption)


This seems to be the only place where it is referred to as "Inbound TCP" rather than just "Inbound". This difference seems a little confusing. I haven't quite figured out why the "TCP" part is important or what it needs to say. Would it be just as well to call it "Inbound Agent Protocol/x"?

daniel-beck · 2019-04-24T11:47:47Z

@jglick Do you want to keep this open to wait for feedback from docs?

MarkEWaite

This looks great to me. I'm sure that I'm one of the long-time users that has made mistakes describing the different connection options.

@daniel-beck I recommend we merge this now. If someone determines there are specific improvements to make, we can make them as additional pull requests.

I don't know if I'm "docs", but I'll be proposing the creation of the "docs" special interest group and am a mentor and administrative owner for our submission to Google Season of Docs. I think that means I'm as close to docs as we're likely to get on this pull request.

jglick · 2019-04-24T12:28:18Z

Do you want to keep this open to wait for feedback from docs?

No particular opinion. Obviously there is no urgency here. There is also little harm in merging it—we can always adjust verbiage later if there is consensus among documenters, as @MarkEWaite points out.

daniel-beck · 2019-04-24T12:34:43Z

Agree, no bad API we have to live with forever, instead a big step forward and easily amended.

Remove JNLP terminology from GUI when referring to inbound agents.

c5c8037

jglick requested a review from jeffret-b April 22, 2019 15:00

jglick commented Apr 22, 2019

View reviewed changes

daniel-beck approved these changes Apr 22, 2019

View reviewed changes

jeffret-b approved these changes Apr 22, 2019

View reviewed changes

MarkEWaite approved these changes Apr 24, 2019

View reviewed changes

daniel-beck merged commit a5a92af into jenkinsci:master Apr 24, 2019

jglick deleted the jnlp-to-inbound branch April 24, 2019 12:35

jglick mentioned this pull request Sep 27, 2022

Prefer inbound as symbol for JNLPLauncher #7171

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove JNLP terminology from GUI when referring to inbound agents #3998

Remove JNLP terminology from GUI when referring to inbound agents #3998

jglick commented Apr 22, 2019

jglick left a comment

jglick Apr 22, 2019

jglick Apr 22, 2019

jglick Apr 22, 2019

jglick Apr 22, 2019

jglick Apr 22, 2019

jglick Apr 22, 2019

jglick Apr 22, 2019

jglick Apr 22, 2019

jglick Apr 22, 2019

jeffret-b Apr 22, 2019

jglick Apr 22, 2019

jglick Apr 22, 2019

daniel-beck Apr 22, 2019

jglick Apr 22, 2019

daniel-beck Apr 22, 2019

jglick Apr 22, 2019

jeffret-b Apr 22, 2019

jeffret-b left a comment

jeffret-b Apr 22, 2019

jeffret-b Apr 22, 2019

daniel-beck commented Apr 24, 2019

MarkEWaite left a comment •

edited

Loading

jglick commented Apr 24, 2019

daniel-beck commented Apr 24, 2019

	JNLPLauncher.displayName=Launch agent by connecting it to the master
	JNLPLauncher.displayName=Launch agent by connecting it to the master TCP agent port

Remove JNLP terminology from GUI when referring to inbound agents #3998

Remove JNLP terminology from GUI when referring to inbound agents #3998

Conversation

jglick commented Apr 22, 2019

Proposed changelog entries

Desired reviewers

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffret-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Apr 24, 2019

MarkEWaite left a comment • edited Loading

Choose a reason for hiding this comment

jglick commented Apr 24, 2019

daniel-beck commented Apr 24, 2019

MarkEWaite left a comment •

edited

Loading