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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-59094] Jenkins connection failure with tunneling #345

Merged
merged 9 commits into from
Sep 6, 2019
Merged

[JENKINS-59094] Jenkins connection failure with tunneling #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
af69f80
[JENKINS-59094] Jenkins connection failure with tunneling
jeffret-b Aug 28, 2019
809d446
Add back in spaces.
jeffret-b Aug 28, 2019
6ffbc0a
Also allow the host to be empty.
jeffret-b Aug 28, 2019
9bfbd0c
Add checks for valid port values.
jeffret-b Aug 28, 2019
3bdbe44
Enable at least some of the magic.
jeffret-b Aug 29, 2019
4a163e2
Improve readability in equation.
jeffret-b Aug 30, 2019
c0aa4aa
Give more information in log message.
jeffret-b Aug 30, 2019
1e39f22
Add back in the single parameter constructor.
jeffret-b Sep 3, 2019
3f5b0ab
Add a test for NumberFormatException.
jeffret-b Sep 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions src/main/java/org/jenkinsci/remoting/engine/HostPort.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,42 @@
package org.jenkinsci.remoting.engine;

class HostPort {

private static final int PORT_MIN = 0;
private static final int PORT_MAX = (1 << 16) - 1;

private String host;
private int port;

public HostPort(String value) {
splitHostPort(value);
splitHostPort(value, null, 0);
}

public HostPort(String value, String defaultHost, int defaultPort) {
oleg-nenashev marked this conversation as resolved.
Show resolved Hide resolved
splitHostPort(value, defaultHost, defaultPort);
}

private void splitHostPort(String value) {
private void splitHostPort(String value, String defaultHost, int defaultPort) {
String hostPortValue = value.trim();
if (hostPortValue.charAt(0) == '[') {
extractIPv6(hostPortValue);
return;
}
int portSeparator = hostPortValue.lastIndexOf(':');
if (portSeparator <= 0) {
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);
String hostValue = hostPortValue.substring(0, portSeparator).trim();
host = hostValue.length() > 0 ? hostValue : defaultHost;
String portString = hostPortValue.substring(portSeparator + 1).trim();
if (portString.length() > 0) {
port = Integer.parseInt(portString);
jeffret-b marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Unhandled NumberFormatException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

if (port <= PORT_MIN || port > PORT_MAX) {
throw new IllegalArgumentException("Port " + value + " out of valid range [" + PORT_MIN + ", " + PORT_MAX + ")");
}
} else {
port = defaultPort;
}
port = Integer.parseInt(hostPortValue.substring(portSeparator + 1).trim());
}

private void extractIPv6(String hostPortValue) {
Expand All @@ -45,4 +59,5 @@ public String getHost() {
public int getPort() {
return port;
}

}
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ public int compare(String o1, String o2) {
}
});
if (tunnel != null) {
HostPort hostPort = new HostPort(tunnel);
HostPort hostPort = new HostPort(tunnel, host, port);
host = hostPort.getHost();
port = hostPort.getPort();
}
Expand Down
48 changes: 39 additions & 9 deletions src/test/java/org/jenkinsci/remoting/engine/HostPortTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,28 +72,58 @@ public void testInvalidPort() {
}

@Test(expected = IllegalArgumentException.class)
public void testNoPort() {
public void testNoSeparator() {
new HostPort("hostname");
}

@Test(expected = IllegalArgumentException.class)
@Test
public void testNoHost() {
new HostPort(":5555");
HostPort hostPort = new HostPort(":5555", "hostname", -1);
assertThat(hostPort.getHost(), is("hostname"));
assertThat(hostPort.getPort(), is(5555));
}

@Test(expected = IllegalArgumentException.class)
@Test
public void testEmptyHost() {
new HostPort(" :5555");
HostPort hostPort = new HostPort(" :5555", "hostname", -1);
assertThat(hostPort.getHost(), is("hostname"));
assertThat(hostPort.getPort(), is(5555));
}

@Test(expected = IllegalArgumentException.class)
@Test
public void testEmptyPort() {
new HostPort("hostname: ");
HostPort hostPort = new HostPort("hostname: ", null, 7777);
assertThat(hostPort.getHost(), is("hostname"));
assertThat(hostPort.getPort(), is(7777));
}

@Test(expected = IllegalArgumentException.class)
@Test
public void testSeparatorNoPort() {
new HostPort("hostname:");
HostPort hostPort = new HostPort("hostname:", null, 7777);
assertThat(hostPort.getHost(), is("hostname"));
assertThat(hostPort.getPort(), is( 7777));
}

@Test(expected = IllegalArgumentException.class)
public void testNegativePort() {
new HostPort("hostname:-4");
}

@Test(expected = IllegalArgumentException.class)
public void testPortTooHigh() {
new HostPort("hostname:100000");
}

@Test
public void testOnlySeparator() {
HostPort hostPort = new HostPort(":", "hostname", 7777);
assertThat(hostPort.getHost(), is("hostname"));
assertThat(hostPort.getPort(), is( 7777));
}

@Test(expected = NumberFormatException.class)
public void testPortNotANumber() {
new HostPort("hostname:notAPort");
}

}