[JENKINS-59094] Jenkins connection failure with tunneling

[jeffret-b]

See JENKINS-59094

Under some conditions, agents using tunneling have failed to connect after changes in the 3.34 Remoting release. I haven't been able to reproduce it or isolate it yet, but this change restores the host:port parsing logic to previous behavior for handling an empty port. At a minimum, this should at least get us past the exception. It may well solve the problem.

[bbara]

Hi!

I have a pretty similar problem but in my case the HOST is empty.

This line suggests that the tunnel string, on some point in code, is auto-completed:

remoting/src/main/java/hudson/remoting/jnlp/Main.java

Lines 70 to 74 in 58588ea

	@Option(name="-tunnel",metaVar="HOST:PORT",
	usage="Connect to the specified host and port, instead of connecting directly to Jenkins. " +
	"Useful when connection to Jenkins needs to be tunneled. Can be also HOST: or :PORT, " +
	"in which case the missing portion will be auto-configured like the default behavior")
	public String tunnel;

However, this is not done up to this point, where the failure happens:

remoting/src/main/java/org/jenkinsci/remoting/engine/HostPort.java

Lines 17 to 24 in bdfcda0

	int portSeparator = hostPortValue.lastIndexOf(':');
	if (portSeparator <= 0) {
	throw new IllegalArgumentException("Invalid HOST:PORT value: " + value);
	}
	host = hostPortValue.substring(0, portSeparator).trim();
	if (host.isEmpty()) {
	throw new IllegalArgumentException("Invalid HOST:PORT value: " + value);
	}

I currently don't have time to investigate into it further, but at some point later in code, there must have happened some kind of auto-completion.

Unfortunately, I also haven't figured out where the tunnel is set at all, because I'm not sure where this jelly is located in the UI.

BR,
BB

Edit: phrasing

[jvz]

Changes LGTM. No manual testing done by myself.

[jeffret-b]

This should get the parsing back to the state it was before, which should resolve [JENKINS-59094] and the related issue reported above by @bbara. I haven't been able to reproduce or test these other scenarios. It would be great if someone else with those setups could give it a try.

[bbara]

I currently have limited access to my Jenkins instance but I will provide feedback asap.

Some more findings:
Help for the JNLP Tunnel
Setting up the tunnel is an advanced configuration
Function to get the ServerName that is actually never called

[bbara]

Does not work for me:

Aug 29, 2019 9:12:16 AM hudson.remoting.jnlp.Main$CuiListener status
INFO: Handshaking
Aug 29, 2019 9:12:16 AM hudson.remoting.jnlp.Main$CuiListener status
INFO: Connecting to :8000
Aug 29, 2019 9:12:27 AM hudson.remoting.jnlp.Main$CuiListener status
INFO: Connecting to :8000 (retrying:2)
java.io.IOException: Failed to connect to :8000
        at org.jenkinsci.remoting.engine.JnlpAgentEndpoint.open(JnlpAgentEndpoint.java:243)
        at hudson.remoting.Engine.connectTcp(Engine.java:680)
        at hudson.remoting.Engine.innerRun(Engine.java:558)
        at hudson.remoting.Engine.run(Engine.java:490)

So for my case, it seems like the server name/HOST got lost somewhere.

However, I found the advanced setting to configure the tunnel of a node which is, in my setup, not needed anymore anyways.

For Reproduction:
As this setup worked in the last version, simply setting the tunnel of a node to the TCP agent port, which is set in the security settings of Jenkins, should be sufficient. The same should be valid for the HOST part, so simply setting the HOST to the actual server name and omit the actual port number should auto complete it to the one mentioned above.

[jeffret-b]

Thanks to @bbara for the suggestion on how to configure this for testing I was able to make some progress and figure out at least some of the Jenkins magic going on here. I tweaked the PR to handle those cases. I've tested it out with no tunnel set and with it set to values like "hostname:port", "hostname:", ":port", and even ":". (The latter doesn't do anything useful that I know of, but kind of exists to support the others.)

I feel better after being able to reproduce at least some of the problems, at least in a simplified case.

Any other reviews and testing would be welcome. I'm hopeful this is ready to go.

[jvz]

Updated changes look good. Since last time, I did discover that Jetty has their own class fairly similar to this if you want inspiration: https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java

[oleg-nenashev]

If you break the APi anyway, would be nice to ensure proper error propagation without runtime exceptions so that we do not hit other regressions later

[oleg-nenashev]

Unhandled NumberFormatException

[jeffret-b]

NumberFormatException was unhandled in prior versions as well. I checked it by running 3.33 (before any of these changes) with a port value that is not a number. The agent fails on startup with the NumberFormatException.

Do you think we should change that behavior here now? Should it throw a different exception? Or log and ignore the error?

[jeffret-b]

@oleg-nenashev, could you please take a look again?