From a5f4f7ac19f07239a90b872f24fe0441137764c1 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 12 Nov 2019 22:35:07 -0500 Subject: [PATCH 01/33] Add a WebSocket agent client. --- pom.xml | 16 +++++ src/main/java/hudson/remoting/Engine.java | 62 +++++++++++++++++++- src/main/java/hudson/remoting/Launcher.java | 1 + src/main/java/hudson/remoting/jnlp/Main.java | 2 + 4 files changed, 80 insertions(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index bd5bb25ef..cdc166e56 100644 --- a/pom.xml +++ b/pom.xml @@ -94,6 +94,11 @@ THE SOFTWARE. animal-sniffer-annotations provided + + org.glassfish.tyrus.bundles + tyrus-standalone-client-jdk + 1.12 + junit @@ -343,6 +348,17 @@ THE SOFTWARE. org.jenkins-ci + + bundle-tyrus + process-classes + + unpack-dependencies + + + ${project.build.outputDirectory} + tyrus-standalone-client-jdk + + diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 01352414b..75a46d04b 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -29,7 +29,9 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.net.Socket; +import java.net.URI; import java.net.URL; +import java.nio.ByteBuffer; import java.nio.file.Path; import java.security.AccessController; import java.security.KeyManagementException; @@ -51,6 +53,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.CheckForNull; @@ -61,6 +64,12 @@ import javax.net.ssl.SSLSocketFactory; import javax.net.ssl.TrustManager; import javax.net.ssl.TrustManagerFactory; +import javax.websocket.ClientEndpointConfig; +import javax.websocket.ContainerProvider; +import javax.websocket.Endpoint; +import javax.websocket.EndpointConfig; +import javax.websocket.MessageHandler; +import javax.websocket.Session; import org.jenkinsci.remoting.engine.JnlpEndpointResolver; import org.jenkinsci.remoting.engine.Jnlp4ConnectionState; @@ -441,6 +450,57 @@ public void removeListener(EngineListener el) { public void run() { // Create the engine try { + if (true) { // TODO conditionally enable WS support + AtomicReference ch = new AtomicReference<>(); + ContainerProvider.getWebSocketContainer().connectToServer(new Endpoint() { + @Override + public void onOpen(Session session, EndpointConfig config) { + events.status("WebSocket connection open"); + AtomicReference receiver = new AtomicReference<>(); + session.addMessageHandler(byte[].class, message -> { + LOGGER.finest(() -> "received message of length " + message.length); + receiver.get().handle(message); + }); + try { + ch.set(new ChannelBuilder(slaveName, Executors.newCachedThreadPool()).build(new AbstractByteArrayCommandTransport() { + @Override + public void setup(AbstractByteArrayCommandTransport.ByteArrayReceiver _receiver) { + events.status("Setting up channel"); + receiver.set(_receiver); + } + @Override + public void writeBlock(Channel channel, byte[] payload) throws IOException { + LOGGER.finest(() -> "sending message of length " + payload.length); + session.getBasicRemote().sendBinary(ByteBuffer.wrap(payload)); + } + @Override + public Capability getRemoteCapability() throws IOException { + return new Capability(); + // TODO negotiate + } + @Override + public void closeWrite() throws IOException { + events.status("Write side closed"); + // TODO + } + @Override + public void closeRead() throws IOException { + events.status("Read side closed"); + // TODO + } + })); + } catch (IOException x) { + events.error(x); + } + } + }, ClientEndpointConfig.Builder.create().build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/" + slaveName + "?secret=" + secretKey)); + while (ch.get() == null) { + Thread.sleep(100); + } + LOGGER.info(() -> "Waiting for channel"); + ch.get().join(); + return; + } IOHub hub = IOHub.create(executor); try { SSLContext context; @@ -489,7 +549,7 @@ public void run() { } finally { hub.close(); } - } catch (IOException e) { + } catch (Exception e) { events.error(e); } } diff --git a/src/main/java/hudson/remoting/Launcher.java b/src/main/java/hudson/remoting/Launcher.java index bbaf7ced1..2d7d1652f 100644 --- a/src/main/java/hudson/remoting/Launcher.java +++ b/src/main/java/hudson/remoting/Launcher.java @@ -383,6 +383,7 @@ public void run() throws Exception { // But we set it up just in case there are overrides somewhere in the logic jnlpArgs.add("-disableHttpsCertValidation"); } + // TODO ws support try { hudson.remoting.jnlp.Main._main(jnlpArgs.toArray(new String[jnlpArgs.size()])); } catch (CmdLineException e) { diff --git a/src/main/java/hudson/remoting/jnlp/Main.java b/src/main/java/hudson/remoting/jnlp/Main.java index b65ec8dd3..597437a46 100644 --- a/src/main/java/hudson/remoting/jnlp/Main.java +++ b/src/main/java/hudson/remoting/jnlp/Main.java @@ -83,6 +83,8 @@ public class Main { usage="Specify the Jenkins root URLs to connect to.") public List urls = new ArrayList<>(); + // TODO option to enable WS + @Option(name="-credentials",metaVar="USER:PASSWORD", usage="HTTP BASIC AUTH header to pass in for making HTTP requests.") public String credentials; From 944b358bff934f9166e3a4090007944bef7a6513 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 20 Nov 2019 19:11:38 -0500 Subject: [PATCH 02/33] Reworked protocol to negotiate remote capabilities. --- src/main/java/hudson/remoting/Capability.java | 11 +- src/main/java/hudson/remoting/Engine.java | 110 ++++++++++++------ 2 files changed, 83 insertions(+), 38 deletions(-) diff --git a/src/main/java/hudson/remoting/Capability.java b/src/main/java/hudson/remoting/Capability.java index b673ac6c1..c963ef1b7 100644 --- a/src/main/java/hudson/remoting/Capability.java +++ b/src/main/java/hudson/remoting/Capability.java @@ -120,10 +120,17 @@ public boolean supportsProxyExceptionFallback() { //TODO: ideally preamble handling needs to be reworked in order to avoid FB suppression /** - * Writes out the capacity preamble. + * Writes {@link #PREAMBLE} then uses {@link #write}. */ void writePreamble(OutputStream os) throws IOException { os.write(PREAMBLE); + write(os); + } + + /** + * Writes this capability to a stream. + */ + public void write(OutputStream os) throws IOException { try (ObjectOutputStream oos = new ObjectOutputStream(Mode.TEXT.wrap(os)) { @Override public void close() throws IOException { @@ -143,7 +150,7 @@ protected void annotateClass(Class c) throws IOException { } /** - * The opposite operation of {@link #writePreamble(OutputStream)}. + * The opposite operation of {@link #write}. */ public static Capability read(InputStream is) throws IOException { try (ObjectInputStream ois = new ObjectInputStream(Mode.TEXT.wrap(is)) { diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 75a46d04b..cfbe8ca4f 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -24,6 +24,8 @@ package hudson.remoting; import hudson.remoting.Channel.Mode; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -65,10 +67,10 @@ import javax.net.ssl.TrustManager; import javax.net.ssl.TrustManagerFactory; import javax.websocket.ClientEndpointConfig; +import javax.websocket.CloseReason; import javax.websocket.ContainerProvider; import javax.websocket.Endpoint; import javax.websocket.EndpointConfig; -import javax.websocket.MessageHandler; import javax.websocket.Session; import org.jenkinsci.remoting.engine.JnlpEndpointResolver; @@ -453,47 +455,83 @@ public void run() { if (true) { // TODO conditionally enable WS support AtomicReference ch = new AtomicReference<>(); ContainerProvider.getWebSocketContainer().connectToServer(new Endpoint() { + Session session; + AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; + Capability remoteCapability; @Override public void onOpen(Session session, EndpointConfig config) { events.status("WebSocket connection open"); - AtomicReference receiver = new AtomicReference<>(); - session.addMessageHandler(byte[].class, message -> { - LOGGER.finest(() -> "received message of length " + message.length); - receiver.get().handle(message); - }); - try { - ch.set(new ChannelBuilder(slaveName, Executors.newCachedThreadPool()).build(new AbstractByteArrayCommandTransport() { - @Override - public void setup(AbstractByteArrayCommandTransport.ByteArrayReceiver _receiver) { - events.status("Setting up channel"); - receiver.set(_receiver); - } - @Override - public void writeBlock(Channel channel, byte[] payload) throws IOException { - LOGGER.finest(() -> "sending message of length " + payload.length); - session.getBasicRemote().sendBinary(ByteBuffer.wrap(payload)); - } - @Override - public Capability getRemoteCapability() throws IOException { - return new Capability(); - // TODO negotiate - } - @Override - public void closeWrite() throws IOException { - events.status("Write side closed"); - // TODO + this.session = session; + session.addMessageHandler(byte[].class, this::onMessage); + executor.submit(() -> { + try { + session.getBasicRemote().sendText(slaveName); + session.getBasicRemote().sendText(secretKey); + try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { + new Capability().write(baos); + session.getBasicRemote().sendBinary(ByteBuffer.wrap(baos.toByteArray())); } - @Override - public void closeRead() throws IOException { - events.status("Read side closed"); - // TODO - } - })); - } catch (IOException x) { - events.error(x); + events.status("Sent agent name, secret, and local capabilities"); + } catch (IOException x) { + events.error(x); + } + }); + } + private void onMessage(byte[] message) { + if (remoteCapability == null) { + try (ByteArrayInputStream bais = new ByteArrayInputStream(message)) { + remoteCapability = Capability.read(bais); + LOGGER.fine(() -> "received " + remoteCapability); + } catch (IOException x) { + events.error(x); + return; + } + events.status("Negotiated remote capabilities; starting channel"); + try { + ch.set(new ChannelBuilder(slaveName, executor).build(new Transport())); + } catch (IOException x) { + events.error(x); + } + } else { + LOGGER.finest(() -> "received message of length " + message.length); + receiver.handle(message); + } + } + @Override + public void onClose(Session session, CloseReason closeReason) { + // TODO + } + @Override + public void onError(Session session, Throwable thr) { + // TODO + } + class Transport extends AbstractByteArrayCommandTransport { + @Override + public void setup(AbstractByteArrayCommandTransport.ByteArrayReceiver _receiver) { + events.status("Setting up channel"); + receiver = _receiver; + } + @Override + public void writeBlock(Channel channel, byte[] payload) throws IOException { + LOGGER.finest(() -> "sending message of length " + payload.length); + session.getBasicRemote().sendBinary(ByteBuffer.wrap(payload)); + } + @Override + public Capability getRemoteCapability() throws IOException { + return remoteCapability; + } + @Override + public void closeWrite() throws IOException { + events.status("Write side closed"); + // TODO + } + @Override + public void closeRead() throws IOException { + events.status("Read side closed"); + // TODO } } - }, ClientEndpointConfig.Builder.create().build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/" + slaveName + "?secret=" + secretKey)); + }, ClientEndpointConfig.Builder.create().build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); while (ch.get() == null) { Thread.sleep(100); } From 43c69782cf1ddd5affc2605bd00a039e57e9b7c6 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 20 Nov 2019 19:42:51 -0500 Subject: [PATCH 03/33] Comments. --- src/main/java/hudson/remoting/Engine.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index cfbe8ca4f..92ce63870 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -537,6 +537,8 @@ public void closeRead() throws IOException { } LOGGER.info(() -> "Waiting for channel"); ch.get().join(); + // TODO handle multiple candidate URLs + // TODO handle reconnection return; } IOHub hub = IOHub.create(executor); From e4fe758e69eed37f8b2475ad9b5de32909546b27 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 21 Nov 2019 14:46:28 -0500 Subject: [PATCH 04/33] Simplifying handshake to use HTTP headers. --- src/main/java/hudson/remoting/Engine.java | 73 ++++++++++++----------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 92ce63870..03973f5dd 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -34,6 +34,7 @@ import java.net.URI; import java.net.URL; import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.security.AccessController; import java.security.KeyManagementException; @@ -48,6 +49,7 @@ import java.security.cert.X509Certificate; import java.security.interfaces.RSAPublicKey; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -71,6 +73,7 @@ import javax.websocket.ContainerProvider; import javax.websocket.Endpoint; import javax.websocket.EndpointConfig; +import javax.websocket.HandshakeResponse; import javax.websocket.Session; import org.jenkinsci.remoting.engine.JnlpEndpointResolver; @@ -454,48 +457,50 @@ public void run() { try { if (true) { // TODO conditionally enable WS support AtomicReference ch = new AtomicReference<>(); + String localCap; + try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { + new Capability().write(baos); + localCap = baos.toString("US-ASCII"); // Base-64 + } + class HeaderHandler extends ClientEndpointConfig.Configurator { + Capability remoteCapability = new Capability(); + @Override + public void beforeRequest(Map> headers) { + headers.put(JnlpConnectionState.CLIENT_NAME_KEY, Collections.singletonList(slaveName)); + headers.put(JnlpConnectionState.SECRET_KEY, Collections.singletonList(secretKey)); + headers.put(Capability.class.getName(), Collections.singletonList(localCap)); + // TODO use JnlpConnectionState.COOKIE_KEY somehow + LOGGER.fine(() -> "Sending: " + headers); + } + @Override + public void afterResponse(HandshakeResponse hr) { + LOGGER.fine(() -> "Receiving: " + hr.getHeaders()); + try (ByteArrayInputStream bais = new ByteArrayInputStream(hr.getHeaders().get(Capability.class.getName()).get(0).getBytes(StandardCharsets.US_ASCII))) { + remoteCapability = Capability.read(bais); + LOGGER.fine(() -> "received " + remoteCapability); + } catch (IOException x) { + events.error(x); + } + } + } + HeaderHandler headerHandler = new HeaderHandler(); ContainerProvider.getWebSocketContainer().connectToServer(new Endpoint() { Session session; AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; - Capability remoteCapability; @Override public void onOpen(Session session, EndpointConfig config) { events.status("WebSocket connection open"); this.session = session; session.addMessageHandler(byte[].class, this::onMessage); - executor.submit(() -> { - try { - session.getBasicRemote().sendText(slaveName); - session.getBasicRemote().sendText(secretKey); - try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { - new Capability().write(baos); - session.getBasicRemote().sendBinary(ByteBuffer.wrap(baos.toByteArray())); - } - events.status("Sent agent name, secret, and local capabilities"); - } catch (IOException x) { - events.error(x); - } - }); + try { + ch.set(new ChannelBuilder(slaveName, executor).build(new Transport())); + } catch (IOException x) { + events.error(x); + } } private void onMessage(byte[] message) { - if (remoteCapability == null) { - try (ByteArrayInputStream bais = new ByteArrayInputStream(message)) { - remoteCapability = Capability.read(bais); - LOGGER.fine(() -> "received " + remoteCapability); - } catch (IOException x) { - events.error(x); - return; - } - events.status("Negotiated remote capabilities; starting channel"); - try { - ch.set(new ChannelBuilder(slaveName, executor).build(new Transport())); - } catch (IOException x) { - events.error(x); - } - } else { - LOGGER.finest(() -> "received message of length " + message.length); - receiver.handle(message); - } + LOGGER.finest(() -> "received message of length " + message.length); + receiver.handle(message); } @Override public void onClose(Session session, CloseReason closeReason) { @@ -518,7 +523,7 @@ public void writeBlock(Channel channel, byte[] payload) throws IOException { } @Override public Capability getRemoteCapability() throws IOException { - return remoteCapability; + return headerHandler.remoteCapability; } @Override public void closeWrite() throws IOException { @@ -531,7 +536,7 @@ public void closeRead() throws IOException { // TODO } } - }, ClientEndpointConfig.Builder.create().build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); + }, ClientEndpointConfig.Builder.create().configurator(headerHandler).build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); while (ch.get() == null) { Thread.sleep(100); } From 12afe79a4329dfc308c8d5e005730a9f475b96ce Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 22 Nov 2019 16:31:33 -0500 Subject: [PATCH 05/33] SpotBugs --- src/findbugs/excludeFilter.xml | 6 ++++++ src/main/java/hudson/remoting/Engine.java | 12 +++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/findbugs/excludeFilter.xml b/src/findbugs/excludeFilter.xml index 4deb5f2bf..f9806a4e2 100644 --- a/src/findbugs/excludeFilter.xml +++ b/src/findbugs/excludeFilter.xml @@ -6,6 +6,12 @@ + + + + + + diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 03973f5dd..8a5cc29e0 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -23,6 +23,7 @@ */ package hudson.remoting; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.remoting.Channel.Mode; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -451,6 +452,7 @@ public void removeListener(EngineListener el) { events.remove(el); } + @SuppressFBWarnings(value = "REC_CATCH_EXCEPTION", justification = "checked exceptions were a mistake to begin with") @Override public void run() { // Create the engine @@ -485,19 +487,18 @@ public void afterResponse(HandshakeResponse hr) { } HeaderHandler headerHandler = new HeaderHandler(); ContainerProvider.getWebSocketContainer().connectToServer(new Endpoint() { - Session session; AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; @Override public void onOpen(Session session, EndpointConfig config) { events.status("WebSocket connection open"); - this.session = session; session.addMessageHandler(byte[].class, this::onMessage); try { - ch.set(new ChannelBuilder(slaveName, executor).build(new Transport())); + ch.set(new ChannelBuilder(slaveName, executor).build(new Transport(session))); } catch (IOException x) { events.error(x); } } + @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") private void onMessage(byte[] message) { LOGGER.finest(() -> "received message of length " + message.length); receiver.handle(message); @@ -511,6 +512,10 @@ public void onError(Session session, Throwable thr) { // TODO } class Transport extends AbstractByteArrayCommandTransport { + final Session session; + Transport(Session session) { + this.session = session; + } @Override public void setup(AbstractByteArrayCommandTransport.ByteArrayReceiver _receiver) { events.status("Setting up channel"); @@ -599,6 +604,7 @@ public void closeRead() throws IOException { } } + @SuppressFBWarnings(value = "UPM_UNCALLED_PRIVATE_METHOD", justification = "TODO pending WS/TCP switch") @SuppressWarnings({"ThrowableInstanceNeverThrown"}) private void innerRun(IOHub hub, SSLContext context, ExecutorService service) { // Create the protocols that will be attempted to connect to the master. From 561713bf7cf67d319014c1696022614d1f14c75e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 9 Dec 2019 21:00:28 -0500 Subject: [PATCH 06/33] Add a -webSocket launch option. --- src/main/java/hudson/remoting/Capability.java | 29 ++- src/main/java/hudson/remoting/Engine.java | 204 +++++++++--------- src/main/java/hudson/remoting/Launcher.java | 1 - src/main/java/hudson/remoting/jnlp/Main.java | 5 +- 4 files changed, 137 insertions(+), 102 deletions(-) diff --git a/src/main/java/hudson/remoting/Capability.java b/src/main/java/hudson/remoting/Capability.java index c963ef1b7..0842cde2a 100644 --- a/src/main/java/hudson/remoting/Capability.java +++ b/src/main/java/hudson/remoting/Capability.java @@ -1,6 +1,8 @@ package hudson.remoting; import hudson.remoting.Channel.Mode; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.ObjectInputStream; @@ -24,6 +26,12 @@ * @see Channel#remoteCapability */ public final class Capability implements Serializable { + + /** + * Key usable as a WebSocket HTTP header to negotiate capabilities. + */ + public static final String KEY = /* Capability.class.getName() */"hudson.remoting.Capability"; + /** * Bit mask of optional capabilities. */ @@ -130,7 +138,7 @@ void writePreamble(OutputStream os) throws IOException { /** * Writes this capability to a stream. */ - public void write(OutputStream os) throws IOException { + private void write(OutputStream os) throws IOException { try (ObjectOutputStream oos = new ObjectOutputStream(Mode.TEXT.wrap(os)) { @Override public void close() throws IOException { @@ -178,6 +186,25 @@ public void close() throws IOException { } } + /** + * Uses {@link #write} to serialize this object to a Base64-encoded ASCII stream. + */ + public String toASCII() throws IOException { + try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { + write(baos); + return baos.toString("US-ASCII"); + } + } + + /** + * The inverse of {@link #toASCII}. + */ + public static Capability fromASCII(String ascii) throws IOException { + try (ByteArrayInputStream bais = new ByteArrayInputStream(ascii.getBytes(StandardCharsets.US_ASCII))) { + return Capability.read(bais); + } + } + private static final long serialVersionUID = 1L; /** diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 8a5cc29e0..80444f1fc 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -25,8 +25,6 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.remoting.Channel.Mode; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -35,7 +33,6 @@ import java.net.URI; import java.net.URL; import java.nio.ByteBuffer; -import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.security.AccessController; import java.security.KeyManagementException; @@ -149,6 +146,7 @@ public Thread newThread(final Runnable r) { private URL hudsonUrl; private final String secretKey; public final String slaveName; + private boolean webSocket; private String credentials; private String proxyCredentials = System.getProperty("proxyCredentials"); @@ -337,6 +335,10 @@ public URL getHudsonUrl() { return hudsonUrl; } + public void setWebSocket(boolean webSocket) { + this.webSocket = webSocket; + } + /** * If set, connect to the specified host and port instead of connecting directly to Jenkins. * @param tunnel Value. {@code null} to disable tunneling @@ -452,105 +454,15 @@ public void removeListener(EngineListener el) { events.remove(el); } - @SuppressFBWarnings(value = "REC_CATCH_EXCEPTION", justification = "checked exceptions were a mistake to begin with") @Override public void run() { + if (webSocket) { + // TODO check for inappropriate options like tunnel, disableHttpsCertValidation, etc. + runWebSocket(); + return; + } // Create the engine try { - if (true) { // TODO conditionally enable WS support - AtomicReference ch = new AtomicReference<>(); - String localCap; - try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { - new Capability().write(baos); - localCap = baos.toString("US-ASCII"); // Base-64 - } - class HeaderHandler extends ClientEndpointConfig.Configurator { - Capability remoteCapability = new Capability(); - @Override - public void beforeRequest(Map> headers) { - headers.put(JnlpConnectionState.CLIENT_NAME_KEY, Collections.singletonList(slaveName)); - headers.put(JnlpConnectionState.SECRET_KEY, Collections.singletonList(secretKey)); - headers.put(Capability.class.getName(), Collections.singletonList(localCap)); - // TODO use JnlpConnectionState.COOKIE_KEY somehow - LOGGER.fine(() -> "Sending: " + headers); - } - @Override - public void afterResponse(HandshakeResponse hr) { - LOGGER.fine(() -> "Receiving: " + hr.getHeaders()); - try (ByteArrayInputStream bais = new ByteArrayInputStream(hr.getHeaders().get(Capability.class.getName()).get(0).getBytes(StandardCharsets.US_ASCII))) { - remoteCapability = Capability.read(bais); - LOGGER.fine(() -> "received " + remoteCapability); - } catch (IOException x) { - events.error(x); - } - } - } - HeaderHandler headerHandler = new HeaderHandler(); - ContainerProvider.getWebSocketContainer().connectToServer(new Endpoint() { - AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; - @Override - public void onOpen(Session session, EndpointConfig config) { - events.status("WebSocket connection open"); - session.addMessageHandler(byte[].class, this::onMessage); - try { - ch.set(new ChannelBuilder(slaveName, executor).build(new Transport(session))); - } catch (IOException x) { - events.error(x); - } - } - @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") - private void onMessage(byte[] message) { - LOGGER.finest(() -> "received message of length " + message.length); - receiver.handle(message); - } - @Override - public void onClose(Session session, CloseReason closeReason) { - // TODO - } - @Override - public void onError(Session session, Throwable thr) { - // TODO - } - class Transport extends AbstractByteArrayCommandTransport { - final Session session; - Transport(Session session) { - this.session = session; - } - @Override - public void setup(AbstractByteArrayCommandTransport.ByteArrayReceiver _receiver) { - events.status("Setting up channel"); - receiver = _receiver; - } - @Override - public void writeBlock(Channel channel, byte[] payload) throws IOException { - LOGGER.finest(() -> "sending message of length " + payload.length); - session.getBasicRemote().sendBinary(ByteBuffer.wrap(payload)); - } - @Override - public Capability getRemoteCapability() throws IOException { - return headerHandler.remoteCapability; - } - @Override - public void closeWrite() throws IOException { - events.status("Write side closed"); - // TODO - } - @Override - public void closeRead() throws IOException { - events.status("Read side closed"); - // TODO - } - } - }, ClientEndpointConfig.Builder.create().configurator(headerHandler).build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); - while (ch.get() == null) { - Thread.sleep(100); - } - LOGGER.info(() -> "Waiting for channel"); - ch.get().join(); - // TODO handle multiple candidate URLs - // TODO handle reconnection - return; - } IOHub hub = IOHub.create(executor); try { SSLContext context; @@ -599,12 +511,106 @@ public void closeRead() throws IOException { } finally { hub.close(); } + } catch (IOException e) { + events.error(e); + } + } + + @SuppressFBWarnings(value = "REC_CATCH_EXCEPTION", justification = "checked exceptions were a mistake to begin with") + private void runWebSocket() { + try { + AtomicReference ch = new AtomicReference<>(); + String localCap = new Capability().toASCII(); + class HeaderHandler extends ClientEndpointConfig.Configurator { + Capability remoteCapability = new Capability(); + @Override + public void beforeRequest(Map> headers) { + headers.put(JnlpConnectionState.CLIENT_NAME_KEY, Collections.singletonList(slaveName)); + headers.put(JnlpConnectionState.SECRET_KEY, Collections.singletonList(secretKey)); + headers.put(Capability.KEY, Collections.singletonList(localCap)); + // TODO use JnlpConnectionState.COOKIE_KEY somehow + LOGGER.fine(() -> "Sending: " + headers); + } + @Override + public void afterResponse(HandshakeResponse hr) { + LOGGER.fine(() -> "Receiving: " + hr.getHeaders()); + try { + remoteCapability = Capability.fromASCII(hr.getHeaders().get(Capability.KEY).get(0)); + LOGGER.fine(() -> "received " + remoteCapability); + } catch (IOException x) { + events.error(x); + } + } + } + HeaderHandler headerHandler = new HeaderHandler(); + ContainerProvider.getWebSocketContainer().connectToServer(new Endpoint() { + AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; + @Override + public void onOpen(Session session, EndpointConfig config) { + events.status("WebSocket connection open"); + session.addMessageHandler(byte[].class, this::onMessage); + try { + ch.set(new ChannelBuilder(slaveName, executor).build(new Transport(session))); + } catch (IOException x) { + events.error(x); + } + } + @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") + private void onMessage(byte[] message) { + LOGGER.finest(() -> "received message of length " + message.length); + receiver.handle(message); + } + @Override + public void onClose(Session session, CloseReason closeReason) { + // TODO + } + @Override + public void onError(Session session, Throwable thr) { + // TODO + } + class Transport extends AbstractByteArrayCommandTransport { + final Session session; + Transport(Session session) { + this.session = session; + } + @Override + public void setup(AbstractByteArrayCommandTransport.ByteArrayReceiver _receiver) { + events.status("Setting up channel"); + receiver = _receiver; + } + @Override + public void writeBlock(Channel channel, byte[] payload) throws IOException { + LOGGER.finest(() -> "sending message of length " + payload.length); + session.getBasicRemote().sendBinary(ByteBuffer.wrap(payload)); + } + @Override + public Capability getRemoteCapability() throws IOException { + return headerHandler.remoteCapability; + } + @Override + public void closeWrite() throws IOException { + events.status("Write side closed"); + // TODO + } + @Override + public void closeRead() throws IOException { + events.status("Read side closed"); + // TODO + } + } + }, ClientEndpointConfig.Builder.create().configurator(headerHandler).build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); + while (ch.get() == null) { + Thread.sleep(100); + } + LOGGER.info(() -> "Waiting for channel"); + ch.get().join(); + // TODO handle multiple candidate URLs + // TODO handle reconnection } catch (Exception e) { events.error(e); } } - @SuppressFBWarnings(value = "UPM_UNCALLED_PRIVATE_METHOD", justification = "TODO pending WS/TCP switch") @SuppressWarnings({"ThrowableInstanceNeverThrown"}) private void innerRun(IOHub hub, SSLContext context, ExecutorService service) { // Create the protocols that will be attempted to connect to the master. diff --git a/src/main/java/hudson/remoting/Launcher.java b/src/main/java/hudson/remoting/Launcher.java index 2d7d1652f..bbaf7ced1 100644 --- a/src/main/java/hudson/remoting/Launcher.java +++ b/src/main/java/hudson/remoting/Launcher.java @@ -383,7 +383,6 @@ public void run() throws Exception { // But we set it up just in case there are overrides somewhere in the logic jnlpArgs.add("-disableHttpsCertValidation"); } - // TODO ws support try { hudson.remoting.jnlp.Main._main(jnlpArgs.toArray(new String[jnlpArgs.size()])); } catch (CmdLineException e) { diff --git a/src/main/java/hudson/remoting/jnlp/Main.java b/src/main/java/hudson/remoting/jnlp/Main.java index 597437a46..8f8013b4a 100644 --- a/src/main/java/hudson/remoting/jnlp/Main.java +++ b/src/main/java/hudson/remoting/jnlp/Main.java @@ -83,7 +83,9 @@ public class Main { usage="Specify the Jenkins root URLs to connect to.") public List urls = new ArrayList<>(); - // TODO option to enable WS + @Option(name="-webSocket", + usage="Make a WebSocket connection to Jenkins rather than using the TCP port.") + public boolean webSocket; @Option(name="-credentials",metaVar="USER:PASSWORD", usage="HTTP BASIC AUTH header to pass in for making HTTP requests.") @@ -292,6 +294,7 @@ public Engine createEngine() { Engine engine = new Engine( headlessMode ? new CuiListener() : new GuiListener(), urls, args.get(0), agentName, directConnection, instanceIdentity, new HashSet<>(protocols)); + engine.setWebSocket(webSocket); if(tunnel!=null) engine.setTunnel(tunnel); if(credentials!=null) From c877936daec0b08e6557309140f3e7cc7c5a2751 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 9 Dec 2019 21:37:48 -0500 Subject: [PATCH 07/33] Better comment. --- src/main/java/hudson/remoting/Capability.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/Capability.java b/src/main/java/hudson/remoting/Capability.java index 0842cde2a..eac09421b 100644 --- a/src/main/java/hudson/remoting/Capability.java +++ b/src/main/java/hudson/remoting/Capability.java @@ -30,7 +30,7 @@ public final class Capability implements Serializable { /** * Key usable as a WebSocket HTTP header to negotiate capabilities. */ - public static final String KEY = /* Capability.class.getName() */"hudson.remoting.Capability"; + public static final String KEY = "hudson.remoting.Capability"; // Capability.class.getName() is not a compile-time constant /** * Bit mask of optional capabilities. From ae63bd915620227f5a5e1c26306934a1dfb2fccd Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 10 Dec 2019 16:13:58 -0500 Subject: [PATCH 08/33] Misleading Javadoc. --- .../hudson/remoting/AbstractByteArrayCommandTransport.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/AbstractByteArrayCommandTransport.java b/src/main/java/hudson/remoting/AbstractByteArrayCommandTransport.java index 7b2a69d2b..4c32417dc 100644 --- a/src/main/java/hudson/remoting/AbstractByteArrayCommandTransport.java +++ b/src/main/java/hudson/remoting/AbstractByteArrayCommandTransport.java @@ -44,11 +44,12 @@ public static interface ByteArrayReceiver { * * As discussed in {@link AbstractByteArrayCommandTransport#writeBlock(Channel, byte[])}, * the block boundary is significant. + * @see CommandReceiver#handle */ void handle(byte[] payload); /** - * See {@link CommandReceiver#handle(Command)} for details. + * @see CommandReceiver#terminate */ void terminate(IOException e); } From 2f77265192ef58e84ee8c0008ac2523cbc48d7de Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 10 Dec 2019 16:32:11 -0500 Subject: [PATCH 09/33] Handling some close and error methods. --- src/main/java/hudson/remoting/Engine.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 80444f1fc..cc5baa081 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -562,11 +562,13 @@ private void onMessage(byte[] message) { } @Override public void onClose(Session session, CloseReason closeReason) { - // TODO + LOGGER.fine(() -> "onClose: " + closeReason); + receiver.terminate(new ChannelClosedException(ch.get(), null)); } @Override - public void onError(Session session, Throwable thr) { - // TODO + public void onError(Session session, Throwable x) { + LOGGER.log(Level.FINE, null, x); + receiver.terminate(new ChannelClosedException(ch.get(), x)); } class Transport extends AbstractByteArrayCommandTransport { final Session session; @@ -590,12 +592,12 @@ public Capability getRemoteCapability() throws IOException { @Override public void closeWrite() throws IOException { events.status("Write side closed"); - // TODO + session.close(); } @Override public void closeRead() throws IOException { events.status("Read side closed"); - // TODO + session.close(); } } }, ClientEndpointConfig.Builder.create().configurator(headerHandler).build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); From aefc7b41c5269b582ceab6a3b3fd54860dbecca6 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 11 Dec 2019 13:54:08 -0500 Subject: [PATCH 10/33] Checking X-Remoting-Minimum-Version if sent. --- src/main/java/hudson/remoting/Engine.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index cc5baa081..799a896af 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -90,6 +90,7 @@ import org.jenkinsci.remoting.protocol.cert.PublicKeyMatchingX509ExtendedTrustManager; import org.jenkinsci.remoting.protocol.impl.ConnectionRefusalException; import org.jenkinsci.remoting.util.KeyUtils; +import org.jenkinsci.remoting.util.VersionNumber; /** * Agent engine that proactively connects to Jenkins master. @@ -534,6 +535,15 @@ public void beforeRequest(Map> headers) { @Override public void afterResponse(HandshakeResponse hr) { LOGGER.fine(() -> "Receiving: " + hr.getHeaders()); + List remotingMinimumVersion = hr.getHeaders().get("X-Remoting-Minimum-Version"); + if (remotingMinimumVersion != null && !remotingMinimumVersion.isEmpty()) { + VersionNumber minimumSupportedVersion = new VersionNumber(remotingMinimumVersion.get(0)); + VersionNumber currentVersion = new VersionNumber(Launcher.VERSION); + if (currentVersion.isOlderThan(minimumSupportedVersion)) { + // TODO these errors should trigger a connection close + events.error(new IOException("Agent version " + minimumSupportedVersion + " or newer is required.")); + } + } try { remoteCapability = Capability.fromASCII(hr.getHeaders().get(Capability.KEY).get(0)); LOGGER.fine(() -> "received " + remoteCapability); From 4c701f36e4dcb29cb04faa89641001b4335ad2ef Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 11 Dec 2019 13:59:19 -0500 Subject: [PATCH 11/33] SpotBugs --- src/main/java/hudson/remoting/Engine.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 799a896af..363cb19b2 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -553,7 +553,8 @@ public void afterResponse(HandshakeResponse hr) { } } HeaderHandler headerHandler = new HeaderHandler(); - ContainerProvider.getWebSocketContainer().connectToServer(new Endpoint() { + @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") + class AgentEndpoint extends Endpoint { AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; @Override public void onOpen(Session session, EndpointConfig config) { @@ -565,7 +566,6 @@ public void onOpen(Session session, EndpointConfig config) { events.error(x); } } - @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") private void onMessage(byte[] message) { LOGGER.finest(() -> "received message of length " + message.length); receiver.handle(message); @@ -610,7 +610,9 @@ public void closeRead() throws IOException { session.close(); } } - }, ClientEndpointConfig.Builder.create().configurator(headerHandler).build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); + } + ContainerProvider.getWebSocketContainer().connectToServer(new AgentEndpoint(), + ClientEndpointConfig.Builder.create().configurator(headerHandler).build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); while (ch.get() == null) { Thread.sleep(100); } From 47ea9457530e475c736cd90a763f1411a109c386 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 11 Dec 2019 14:01:43 -0500 Subject: [PATCH 12/33] More targeted SpotBugs annotation placement. --- src/main/java/hudson/remoting/Engine.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 363cb19b2..ca957488f 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -553,8 +553,8 @@ public void afterResponse(HandshakeResponse hr) { } } HeaderHandler headerHandler = new HeaderHandler(); - @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") class AgentEndpoint extends Endpoint { + @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; @Override public void onOpen(Session session, EndpointConfig config) { From 86cea5b83fd2fc93a9fc10896962eb42c95f80a0 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 11 Dec 2019 16:20:25 -0500 Subject: [PATCH 13/33] Documenting options incompatible with -webSocket. --- src/main/java/hudson/remoting/Engine.java | 1 - src/main/java/hudson/remoting/jnlp/Main.java | 32 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index ca957488f..75046c9da 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -458,7 +458,6 @@ public void removeListener(EngineListener el) { @Override public void run() { if (webSocket) { - // TODO check for inappropriate options like tunnel, disableHttpsCertValidation, etc. runWebSocket(); return; } diff --git a/src/main/java/hudson/remoting/jnlp/Main.java b/src/main/java/hudson/remoting/jnlp/Main.java index 8f8013b4a..ee70b807e 100644 --- a/src/main/java/hudson/remoting/jnlp/Main.java +++ b/src/main/java/hudson/remoting/jnlp/Main.java @@ -271,6 +271,38 @@ public static void _main(String[] args) throws IOException, InterruptedException if(m.urls.isEmpty() && m.directConnection == null) { throw new CmdLineException(p, "At least one -url option is required.", null); } + if (m.webSocket) { + if (m.urls.isEmpty()) { + throw new CmdLineException(p, "-url is required in -webSocket mode", null); + } + if (m.urls.size() > 1) { + throw new CmdLineException(p, "multiple -url is not currently supported in -webSocket mode", null); + } + if (m.directConnection != null) { + throw new CmdLineException(p, "-webSocket and -direct are mutually exclusive", null); + } + if (m.tunnel != null) { + throw new CmdLineException(p, "-tunnel is not currently supported in -webSocket mode", null); + } + if (m.credentials != null) { + throw new CmdLineException(p, "-credentials is not currently supported in -webSocket mode", null); + } + if (m.proxyCredentials != null) { + throw new CmdLineException(p, "-proxyCredentials is not currently supported in -webSocket mode", null); + } + if (m.candidateCertificates != null) { + throw new CmdLineException(p, "-candidateCertificates is not currently supported in -webSocket mode", null); + } + if (m.disableHttpsCertValidation) { + throw new CmdLineException(p, "-disableHttpsCertValidation is not currently supported in -webSocket mode", null); + } + if (m.noKeepAlive) { + throw new CmdLineException(p, "-noKeepAlive is not currently supported in -webSocket mode", null); + } + if (m.noReconnect) { + throw new CmdLineException(p, "-noReconnect is not currently supported in -webSocket mode", null); + } + } m.main(); } From 02db50c6a9e5b5ace44928216fce6f970b12ff9b Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 13 Dec 2019 12:37:44 -0500 Subject: [PATCH 14/33] Comments. --- src/main/java/hudson/remoting/Engine.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 75046c9da..715b82ea9 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -528,7 +528,7 @@ public void beforeRequest(Map> headers) { headers.put(JnlpConnectionState.CLIENT_NAME_KEY, Collections.singletonList(slaveName)); headers.put(JnlpConnectionState.SECRET_KEY, Collections.singletonList(secretKey)); headers.put(Capability.KEY, Collections.singletonList(localCap)); - // TODO use JnlpConnectionState.COOKIE_KEY somehow + // TODO use JnlpConnectionState.COOKIE_KEY somehow (see EngineJnlpConnectionStateListener.afterChannel) LOGGER.fine(() -> "Sending: " + headers); } @Override @@ -560,6 +560,7 @@ public void onOpen(Session session, EndpointConfig config) { events.status("WebSocket connection open"); session.addMessageHandler(byte[].class, this::onMessage); try { + // TODO use jarCache, unless EngineJnlpConnectionStateListener can be used for that purpose ch.set(new ChannelBuilder(slaveName, executor).build(new Transport(session))); } catch (IOException x) { events.error(x); From 6263aae5fb13d06102634bca03604137e6583e7d Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 13 Dec 2019 16:53:21 -0500 Subject: [PATCH 15/33] nginx urgh https://stackoverflow.com/a/49801326/12916 --- src/main/java/hudson/remoting/Capability.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/Capability.java b/src/main/java/hudson/remoting/Capability.java index eac09421b..6abbb48d3 100644 --- a/src/main/java/hudson/remoting/Capability.java +++ b/src/main/java/hudson/remoting/Capability.java @@ -30,7 +30,7 @@ public final class Capability implements Serializable { /** * Key usable as a WebSocket HTTP header to negotiate capabilities. */ - public static final String KEY = "hudson.remoting.Capability"; // Capability.class.getName() is not a compile-time constant + public static final String KEY = "X-Remoting-Capability"; /** * Bit mask of optional capabilities. From 827bfcaeb80af0016e8e44f4d780382decc35123 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 13 Dec 2019 17:20:03 -0500 Subject: [PATCH 16/33] Trying to enable JAR cache. --- src/main/java/hudson/remoting/Engine.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 715b82ea9..8ee108298 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -560,8 +560,9 @@ public void onOpen(Session session, EndpointConfig config) { events.status("WebSocket connection open"); session.addMessageHandler(byte[].class, this::onMessage); try { - // TODO use jarCache, unless EngineJnlpConnectionStateListener can be used for that purpose - ch.set(new ChannelBuilder(slaveName, executor).build(new Transport(session))); + ch.set(new ChannelBuilder(slaveName, executor). + withJarCache(jarCache). // unless EngineJnlpConnectionStateListener can be used for this purpose + build(new Transport(session))); } catch (IOException x) { events.error(x); } From e25ee2c21627924bf177bee5f07c234eebe99a00 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 13 Dec 2019 17:21:16 -0500 Subject: [PATCH 17/33] JnlpConnectionState.fireXXX methods need to be public to support other implementations. --- .../remoting/engine/JnlpConnectionState.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java b/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java index 7d078c2c3..72bb24572 100644 --- a/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java +++ b/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java @@ -327,7 +327,7 @@ private void fire(EventHandler handler) { * * @throws ConnectionRefusalException if the connection has been refused. */ - /*package*/ void fireBeforeProperties() throws ConnectionRefusalException { + public void fireBeforeProperties() throws ConnectionRefusalException { if (lifecycle != State.INITIALIZED) { throw new IllegalStateException("fireBeforeProperties cannot be invoked at lifecycle " + lifecycle); } @@ -356,7 +356,7 @@ public void invoke(JnlpConnectionStateListener listener, JnlpConnectionState eve * * @throws ConnectionRefusalException if the connection has been refused. */ - /*package*/ void fireAfterProperties(@Nonnull Map properties) throws ConnectionRefusalException { + public void fireAfterProperties(@Nonnull Map properties) throws ConnectionRefusalException { if (lifecycle != State.BEFORE_PROPERTIES) { throw new IllegalStateException("fireAfterProperties cannot be invoked at lifecycle " + lifecycle); } @@ -386,7 +386,7 @@ public void invoke(JnlpConnectionStateListener listener, JnlpConnectionState eve * * @param builder the {@link ChannelBuilder} that will be used to create the channel. */ - /*package*/ void fireBeforeChannel(ChannelBuilder builder) { + public void fireBeforeChannel(ChannelBuilder builder) { if (lifecycle != State.APPROVED) { throw new IllegalStateException("fireBeforeChannel cannot be invoked at lifecycle " + lifecycle); } @@ -407,7 +407,7 @@ public void invoke(JnlpConnectionStateListener listener, JnlpConnectionState eve * @param channel the {@link Channel} (may be closed already but should not unless there is a serious race with * the remote). */ - /*package*/ void fireAfterChannel(Channel channel) { + public void fireAfterChannel(Channel channel) { if (lifecycle != State.BEFORE_CHANNEL) { throw new IllegalStateException("fireAfterChannel cannot be invoked at lifecycle " + lifecycle); } @@ -428,7 +428,7 @@ public void invoke(JnlpConnectionStateListener listener, JnlpConnectionState eve * * @param cause the reason why the channel was closed or {@code null} if normally closed */ - /*package*/ void fireChannelClosed(@CheckForNull IOException cause) { + public void fireChannelClosed(@CheckForNull IOException cause) { if (lifecycle.compareTo(State.BEFORE_CHANNEL) < 0) { throw new IllegalStateException("fireChannelClosed cannot be invoked at lifecycle " + lifecycle); } @@ -446,7 +446,7 @@ public void invoke(JnlpConnectionStateListener listener, JnlpConnectionState eve /** * Advances the connection state to indicate that the socket has been closed. */ - /*package*/ void fireAfterDisconnect() { + public void fireAfterDisconnect() { if (lifecycle == State.AFTER_CHANNEL) { fireChannelClosed(null); } From cf08bcfdc65056358f69b5ad89eb244352d29b6f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 13 Dec 2019 17:22:11 -0500 Subject: [PATCH 18/33] JnlpConnectionState.getRemoteEndpointDescription introduced to avoid a hard dependency on Socket. --- .../remoting/engine/JnlpConnectionState.java | 32 ++++++++++++++++--- .../remoting/engine/JnlpProtocol1Handler.java | 2 +- .../remoting/engine/JnlpProtocol2Handler.java | 2 +- .../remoting/engine/JnlpProtocol3Handler.java | 2 +- .../remoting/engine/JnlpProtocol4Handler.java | 4 +-- .../engine/JnlpProtocol4PlainHandler.java | 2 +- .../engine/HandlerLoopbackLoadStress.java | 3 +- 7 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java b/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java index 72bb24572..f015b0860 100644 --- a/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java +++ b/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java @@ -69,8 +69,12 @@ public class JnlpConnectionState { /** * Socket connection to the agent. */ - @Nonnull + @CheckForNull private final Socket socket; + + @Nonnull + private String remoteEndpointDescription = ""; + /** * The {@link JnlpConnectionStateListener} instances that are still interested in this connection event. */ @@ -120,19 +124,37 @@ public class JnlpConnectionState { * @param socket the {@link Socket}. * @param listeners the {@link JnlpConnectionStateListener} instances. */ - protected JnlpConnectionState(@Nonnull Socket socket, List listeners) { + public JnlpConnectionState(@CheckForNull Socket socket, List listeners) { this.socket = socket; this.listeners = new ArrayList(listeners); } /** * Gets the socket that the connection is on. - * - * @return the socket that the connection is on. + *

This should be considered deprecated except in situations where you know an actual socket is involved. + * Use {@link #getRemoteEndpointDescription} for logging purposes. + * @return an actual socket, or just a stub */ @Nonnull public Socket getSocket() { - return socket; + return socket != null ? socket : new Socket(); + } + + /** + * Description of the remote endpoint to which {@link #getSocket} is connected, if using an actual socket and it is actually connected. + * Or may be some other text identifying a remote client. + * @return some text suitable for debug logs + */ + @Nonnull + public String getRemoteEndpointDescription() { + return socket != null ? String.valueOf(socket.getRemoteSocketAddress()) : remoteEndpointDescription; + } + + /** + * Set a specific value for {@link #getRemoteEndpointDescription}. + */ + public void setRemoteEndpointDescription(@Nonnull String description) { + remoteEndpointDescription = description; } /** diff --git a/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol1Handler.java b/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol1Handler.java index 84a294713..9b9871d63 100644 --- a/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol1Handler.java +++ b/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol1Handler.java @@ -146,7 +146,7 @@ void receiveHandshake(@Nonnull LegacyJnlpConnectionState state, @Nonnull Map headers) throws ConnectionRefus case INVALID: LOGGER.log(Level.WARNING, "An attempt was made to connect as {0} from {1} with an invalid client certificate", - new Object[]{clientName, event.getSocket().getRemoteSocketAddress()}); + new Object[]{clientName, event.getRemoteEndpointDescription()}); throw new ConnectionRefusalException("Authentication failure"); default: String secretKey = clientDatabase.getSecretOf(clientName); @@ -312,7 +312,7 @@ public void onReceiveHeaders(Map headers) throws ConnectionRefus if (!secretKey.equals(headers.get(JnlpConnectionState.SECRET_KEY))) { LOGGER.log(Level.WARNING, "An attempt was made to connect as {0} from {1} with an incorrect secret", - new Object[]{clientName, event.getSocket().getRemoteSocketAddress()}); + new Object[]{clientName, event.getRemoteEndpointDescription()}); throw new ConnectionRefusalException("Authorization failure"); } break; diff --git a/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol4PlainHandler.java b/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol4PlainHandler.java index fbb59d9e1..0af9feb16 100644 --- a/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol4PlainHandler.java +++ b/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol4PlainHandler.java @@ -237,7 +237,7 @@ public void onReceiveHeaders(Map headers) throws ConnectionRefus if (!secretKey.equals(headers.get(JnlpConnectionState.SECRET_KEY))) { LOGGER.log(Level.WARNING, "An attempt was made to connect as {0} from {1} with an incorrect secret", - new Object[]{clientName, event.getSocket().getRemoteSocketAddress()}); + new Object[]{clientName, event.getRemoteEndpointDescription()}); throw new ConnectionRefusalException("Authorization failure"); } } diff --git a/src/test/java/org/jenkinsci/remoting/engine/HandlerLoopbackLoadStress.java b/src/test/java/org/jenkinsci/remoting/engine/HandlerLoopbackLoadStress.java index 91b5ee3ce..77896390c 100644 --- a/src/test/java/org/jenkinsci/remoting/engine/HandlerLoopbackLoadStress.java +++ b/src/test/java/org/jenkinsci/remoting/engine/HandlerLoopbackLoadStress.java @@ -554,8 +554,7 @@ public void beforeChannel(@NonNull JnlpConnectionState event) { public void afterChannel(@NonNull JnlpConnectionState event) { String clientName = event.getProperty(JnlpConnectionState.CLIENT_NAME_KEY); if (clientName != null) { - System.out.println("Accepted connection from client " + clientName + " on " + event.getSocket() - .getRemoteSocketAddress()); + System.out.println("Accepted connection from client " + clientName + " on " + event.getRemoteEndpointDescription()); } } } From f2549aa8c5378bf21c3d1ec93e8deacd91377d6e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 16 Dec 2019 16:42:03 -0500 Subject: [PATCH 19/33] Do not advertise tyrus-standalone-client-jdk to Maven dependencies, though its classes & services remain unshaded for now. --- pom.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pom.xml b/pom.xml index edc3a8a78..a54713763 100644 --- a/pom.xml +++ b/pom.xml @@ -98,6 +98,7 @@ THE SOFTWARE. org.glassfish.tyrus.bundles tyrus-standalone-client-jdk 1.12 + provided @@ -356,6 +357,7 @@ THE SOFTWARE. ${project.build.outputDirectory} + provided tyrus-standalone-client-jdk From ee1ecd902678942a5bc8e450eec5c040aa9592ec Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 17 Dec 2019 12:59:37 -0500 Subject: [PATCH 20/33] SpotBugs --- src/main/java/hudson/remoting/Engine.java | 2 +- .../org/jenkinsci/remoting/engine/JnlpConnectionState.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 8ee108298..76c7ff756 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -561,7 +561,7 @@ public void onOpen(Session session, EndpointConfig config) { session.addMessageHandler(byte[].class, this::onMessage); try { ch.set(new ChannelBuilder(slaveName, executor). - withJarCache(jarCache). // unless EngineJnlpConnectionStateListener can be used for this purpose + withJarCacheOrDefault(jarCache). // unless EngineJnlpConnectionStateListener can be used for this purpose build(new Transport(session))); } catch (IOException x) { events.error(x); diff --git a/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java b/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java index f015b0860..d1359b2f4 100644 --- a/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java +++ b/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java @@ -35,6 +35,7 @@ import java.util.Map; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import javax.annotation.concurrent.NotThreadSafe; import org.jenkinsci.remoting.protocol.impl.ConnectionRefusalException; @@ -124,7 +125,7 @@ public class JnlpConnectionState { * @param socket the {@link Socket}. * @param listeners the {@link JnlpConnectionStateListener} instances. */ - public JnlpConnectionState(@CheckForNull Socket socket, List listeners) { + public JnlpConnectionState(@Nullable Socket socket, List listeners) { this.socket = socket; this.listeners = new ArrayList(listeners); } From 21bf70678c346acab45791861f0b3801431ddd60 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 20 Dec 2019 22:44:17 -0500 Subject: [PATCH 21/33] Shade dependencies needed for agent.jar. --- assembly-uberjar-14.xml | 53 ----------------- pom.xml | 126 +++++++++++++++++----------------------- 2 files changed, 54 insertions(+), 125 deletions(-) delete mode 100644 assembly-uberjar-14.xml diff --git a/assembly-uberjar-14.xml b/assembly-uberjar-14.xml deleted file mode 100644 index 118285d6e..000000000 --- a/assembly-uberjar-14.xml +++ /dev/null @@ -1,53 +0,0 @@ - - - - - jdk14-jar-with-dependencies - - jar - - false - - - - - - true - runtime - - - - - target/classes14 - - - - \ No newline at end of file diff --git a/pom.xml b/pom.xml index 867f6055d..e9585d7f9 100644 --- a/pom.xml +++ b/pom.xml @@ -98,9 +98,8 @@ THE SOFTWARE. org.glassfish.tyrus.bundles tyrus-standalone-client-jdk 1.12 - provided + true - junit junit @@ -135,7 +134,7 @@ THE SOFTWARE. args4j args4j 2.33 - provided + true commons-io @@ -154,22 +153,16 @@ THE SOFTWARE. 1.10.19 test - - - org.jvnet - animal-sniffer-annotation - 1.0 - true - org.jenkins-ci constant-pool-scanner 1.2 + true com.github.spotbugs spotbugs-annotations - true + provided org.bouncycastle @@ -187,13 +180,7 @@ THE SOFTWARE. org.kohsuke access-modifier-annotation 1.12 - jar - true - - - org.jenkins-ci - test-annotations - test + provided @@ -225,25 +212,57 @@ THE SOFTWARE. 1.${java.level} + + org.apache.maven.plugins + maven-shade-plugin + 3.2.1 + + + package + + shade + + + false + + + javax.websocket + io.jenkins.remoting.shaded.javax.websocket + + + org.glassfish.tyrus + io.jenkins.remoting.shaded.org.glassfish.tyrus + + + org.kohsuke.args4j + io.jenkins.remoting.shaded.org.kohsuke.args4j + + + org.jenkinsci.constant_pool_scanner + io.jenkins.remoting.shaded.org.jenkinsci.constant_pool_scanner + + + + + hudson.remoting.Launcher + + ${project.version} + + + all-permissions + * + Jenkins Remoting Agent + true + + + + + + + + maven-jar-plugin - - - - - hudson.remoting.Launcher - - - ${project.version} - - - all-permissions - * - Jenkins Remoting Agent - true - - - executable-tests @@ -323,43 +342,6 @@ THE SOFTWARE. - - bundle-arg4j - process-classes - - unpack-dependencies - - - ${project.build.outputDirectory} - provided - args4j - args4j - - - - bundle-cps - process-classes - - unpack-dependencies - - - ${project.build.outputDirectory} - constant-pool-scanner - org.jenkins-ci - - - - bundle-tyrus - process-classes - - unpack-dependencies - - - ${project.build.outputDirectory} - provided - tyrus-standalone-client-jdk - - From 526baab48ef54500b141a7f036095e96233688c1 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 2 Jan 2020 21:40:10 -0500 Subject: [PATCH 22/33] More intuitive log message, as suggested by @jeffret-b. --- src/main/java/hudson/remoting/Engine.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 1763d8dea..67d6bcbf0 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -618,7 +618,7 @@ public void closeRead() throws IOException { while (ch.get() == null) { Thread.sleep(100); } - LOGGER.info(() -> "Waiting for channel"); + LOGGER.info("Connected"); ch.get().join(); // TODO handle multiple candidate URLs // TODO handle reconnection From ce68090f4a24ad8a94e9f55a335427a720a30104 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 2 Jan 2020 21:48:48 -0500 Subject: [PATCH 23/33] FindSecBugs --- .../java/org/jenkinsci/remoting/engine/JnlpConnectionState.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java b/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java index d1359b2f4..9724c88ea 100644 --- a/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java +++ b/src/main/java/org/jenkinsci/remoting/engine/JnlpConnectionState.java @@ -23,6 +23,7 @@ */ package org.jenkinsci.remoting.engine; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.remoting.Channel; import hudson.remoting.ChannelBuilder; import java.io.IOException; @@ -136,6 +137,7 @@ public JnlpConnectionState(@Nullable Socket socket, List Date: Thu, 2 Jan 2020 22:52:35 -0500 Subject: [PATCH 24/33] Some updates to documentation to reflect WebSocket mode. --- README.md | 2 +- docs/{tcpAgent.md => inbound-agent.md} | 18 +++++++++++------- docs/protocols.md | 10 +++++++--- 3 files changed, 19 insertions(+), 11 deletions(-) rename docs/{tcpAgent.md => inbound-agent.md} (92%) diff --git a/README.md b/README.md index b67f7dc93..ac2f155ba 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ Feel free to contribute. User documentation: * [Changelog](CHANGELOG.md) - Remoting release notes -* [Launching inbound TCP agents](docs/tcpAgent.md) - Mechanisms and parameters for launching inbound TCP agents +* [Launching inbound agents](docs/inbound-agent.md) - Mechanisms and parameters for launching inbound agents * [Remoting Protocols](docs/protocols.md) - Overview of protocols integrated with Jenkins * [Remoting Configuration](docs/configuration.md) - Configuring remoting agents * [Logging](docs/logging.md) - Logging diff --git a/docs/tcpAgent.md b/docs/inbound-agent.md similarity index 92% rename from docs/tcpAgent.md rename to docs/inbound-agent.md index 004923ca1..d58fa1a63 100644 --- a/docs/tcpAgent.md +++ b/docs/inbound-agent.md @@ -1,17 +1,17 @@ -# Launching inbound TCP agents +# Launching inbound agents Jenkins provides a number of ways of connecting remote agents. -Two of the most popular are outbound SSH agents and inbound TCP agents. +Two of the most popular are outbound SSH agents and inbound agents. SSH agents, most commonly used on Unix platforms, are master initiated. The master creates the connection when it needs. -Inbound TCP agents, most commonly used on Windows platforms, are agent initiated. +Inbound agents, most commonly used on Windows platforms, are agent initiated. The agent must first connect to the master and then the master sends commands as needed. These were formerly known as JNLP agents, but that name was erroneous and confusing. -This document describes some of the primary mechanisms for launching inbound TCP agents. +This document describes some of the primary mechanisms for launching inbound agents. For additional information about Jenkins agents see [Distributed builds](https://wiki.jenkins.io/display/JENKINS/Distributed+builds#Distributedbuilds-HavemasterlaunchagentonWindows). ## Launch mechanisms -Part of the agent status page for TCP agents looks something like this: +Part of the agent status page for inbound agents looks something like this: ![Tcp agent status UI](tcpAgentStatus.jpg) @@ -33,7 +33,11 @@ This mechanism is not recommended, is deprecated, and may not be supported or av ### Download JNLP file Another mechanism, shown in the above status page fragment, runs the agent from a script or command-line to retrieve the JNLP file. This mechanism does not use JNLP or WebStart but uses the downloaded file to obtain connection information. -Internally, this is a two-step process: 1) connect to an HTTP(S) port to retrieve the connection information, and 2) extract that information and connect to the TCP port. + +For an inbound agent using the TCP port, this is a two-step process: +* Connect to an HTTP(S) port to retrieve the connection information. +* Extract that information and connect to the TCP port. +(When using the `-webSocket` option, only a single connection needs to be made.) Before invoking this command you must download the "agent.jar" file. The correct version can be obtained from your Jenkins server at something like "https://myjenkins.example.com/jnlpJars/agent.jar". @@ -114,7 +118,7 @@ Similar to the usage in a number of other applications, this controls which host ### The '@' argument annotation If any command-line argument is prepended with the '@' symbol, special behavior is invoked. -For example, more recent versions of Jenkins show in the UI that one form of launching inbound TCP agents is +For example, more recent versions of Jenkins show in the UI that one form of launching inbound agents is ``` echo > secret-file java -jar agent.jar -jnlpUrl -secret @secret-file -workDir diff --git a/docs/protocols.md b/docs/protocols.md index de1880f73..7c6c63e72 100644 --- a/docs/protocols.md +++ b/docs/protocols.md @@ -1,8 +1,7 @@ Remoting protocols ==== -Remoting library provides extension points, which allow implementing custom communication protocols. -For example, Jenkins project defines its own protocols for the CLI client. +The Remoting library provides APIs which allow custom communication protocols to be implemented. This section describes only the protocols available within the remoting library. @@ -27,10 +26,15 @@ If stronger algorithms are needed (for example, AES with 256-bit keys), the [JCE Protocol uses non-blocking I/O wherever possible which removes the performance bottleneck of the JNLP3-connect protocol. +### WebSocket + +Uses WebSocket over an HTTP(S) port to handle handshakes, encryption, framing, etc. +See [JEP-222](https://jenkins.io/jep/222) for background. + ## Plugin protocols ### Remoting Kafka Plugin * [Remoting Kafka Plugin](https://github.com/jenkinsci/remoting-kafka-plugin) uses Kafka as fault-tolerant communication layer to support command invocation between Jenkins master and agent. * The plugin gets rid of current direct TCP connection between master and agent. -* More info can be found in the technical [documentation](https://github.com/jenkinsci/remoting-kafka-plugin/blob/master/docs/DOCUMENTATION.md) of the plugin. \ No newline at end of file +* More info can be found in the technical [documentation](https://github.com/jenkinsci/remoting-kafka-plugin/blob/master/docs/DOCUMENTATION.md) of the plugin. From 0682149e21a8aa8648108152e5a73171ec3f57b7 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 2 Jan 2020 22:55:05 -0500 Subject: [PATCH 25/33] Fixed formatting --- docs/inbound-agent.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/inbound-agent.md b/docs/inbound-agent.md index d58fa1a63..2a28581bd 100644 --- a/docs/inbound-agent.md +++ b/docs/inbound-agent.md @@ -37,6 +37,7 @@ This mechanism does not use JNLP or WebStart but uses the downloaded file to obt For an inbound agent using the TCP port, this is a two-step process: * Connect to an HTTP(S) port to retrieve the connection information. * Extract that information and connect to the TCP port. + (When using the `-webSocket` option, only a single connection needs to be made.) Before invoking this command you must download the "agent.jar" file. From 87ff8af84c1cc27e9a8e637fa7645be536d2d911 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Sat, 4 Jan 2020 09:40:55 -0500 Subject: [PATCH 26/33] Apply suggestions from code review Co-Authored-By: Jeff Thompson <37345299+jeffret-b@users.noreply.github.com> --- src/main/java/hudson/remoting/Capability.java | 2 +- src/main/java/hudson/remoting/jnlp/Main.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/hudson/remoting/Capability.java b/src/main/java/hudson/remoting/Capability.java index 09d83bcc7..6177c5a2b 100644 --- a/src/main/java/hudson/remoting/Capability.java +++ b/src/main/java/hudson/remoting/Capability.java @@ -194,7 +194,7 @@ public void close() throws IOException { public String toASCII() throws IOException { try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { write(baos); - return baos.toString("US-ASCII"); + return baos.toString(StandardCharsets.US_ASCII.name()); } } diff --git a/src/main/java/hudson/remoting/jnlp/Main.java b/src/main/java/hudson/remoting/jnlp/Main.java index a98ee16c9..49c0235b6 100644 --- a/src/main/java/hudson/remoting/jnlp/Main.java +++ b/src/main/java/hudson/remoting/jnlp/Main.java @@ -273,7 +273,7 @@ public static void _main(String[] args) throws IOException, InterruptedException throw new CmdLineException(p, "-url is required in -webSocket mode", null); } if (m.urls.size() > 1) { - throw new CmdLineException(p, "multiple -url is not currently supported in -webSocket mode", null); + throw new CmdLineException(p, "multiple -url are not currently supported in -webSocket mode", null); } if (m.directConnection != null) { throw new CmdLineException(p, "-webSocket and -direct are mutually exclusive", null); From d220515a7415f5a5165ad17732738921de0c5143 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jan 2020 15:41:55 -0500 Subject: [PATCH 27/33] Expect to indicate version in which -webSocket was introduced. --- docs/protocols.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/protocols.md b/docs/protocols.md index 7c6c63e72..37bb5429c 100644 --- a/docs/protocols.md +++ b/docs/protocols.md @@ -28,8 +28,9 @@ Protocol uses non-blocking I/O wherever possible which removes the performance b ### WebSocket +* Introduced in: Remoting version TODO, [JEP-222](https://jenkins.io/jep/222) + Uses WebSocket over an HTTP(S) port to handle handshakes, encryption, framing, etc. -See [JEP-222](https://jenkins.io/jep/222) for background. ## Plugin protocols From 977bcc13523220b275fdef28a42002a845bc98bb Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jan 2020 15:56:31 -0500 Subject: [PATCH 28/33] Introduced constant for X-Remoting-Minimum-Version. --- src/main/java/hudson/remoting/Engine.java | 8 +++++++- .../remoting/engine/JnlpAgentEndpointResolver.java | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 67d6bcbf0..5c6c1b79f 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -99,6 +99,12 @@ */ @NotThreadSafe // the fields in this class should not be modified by multiple threads concurrently public class Engine extends Thread { + + /** + * HTTP header sent by Jenkins to indicate the earliest version of Remoting it is prepared to accept connections from. + */ + public static final String REMOTING_MINIMUM_VERSION_HEADER = "X-Remoting-Minimum-Version"; + /** * Thread pool that sets {@link #CURRENT}. */ @@ -535,7 +541,7 @@ public void beforeRequest(Map> headers) { @Override public void afterResponse(HandshakeResponse hr) { LOGGER.fine(() -> "Receiving: " + hr.getHeaders()); - List remotingMinimumVersion = hr.getHeaders().get("X-Remoting-Minimum-Version"); + List remotingMinimumVersion = hr.getHeaders().get(REMOTING_MINIMUM_VERSION_HEADER); if (remotingMinimumVersion != null && !remotingMinimumVersion.isEmpty()) { VersionNumber minimumSupportedVersion = new VersionNumber(remotingMinimumVersion.get(0)); VersionNumber currentVersion = new VersionNumber(Launcher.VERSION); diff --git a/src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java b/src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java index 71b4f14d1..086132a45 100644 --- a/src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java +++ b/src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java @@ -24,6 +24,7 @@ package org.jenkinsci.remoting.engine; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import hudson.remoting.Engine; import hudson.remoting.Launcher; import hudson.remoting.NoProxyEvaluator; import org.jenkinsci.remoting.util.VersionNumber; @@ -223,7 +224,7 @@ public JnlpAgentEndpoint resolve() throws IOException { } // Check if current version of agent is supported - String minimumSupportedVersionHeader = first(header(con, "X-Remoting-Minimum-Version")); + String minimumSupportedVersionHeader = first(header(con, Engine.REMOTING_MINIMUM_VERSION_HEADER)); if (minimumSupportedVersionHeader != null) { VersionNumber minimumSupportedVersion = new VersionNumber(minimumSupportedVersionHeader); VersionNumber currentVersion = new VersionNumber(Launcher.VERSION); From 91f8990b3f10db4b7afc612908b049e7d9dc9d27 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jan 2020 16:12:15 -0500 Subject: [PATCH 29/33] Take advantage of args4j declarative depends/forbids. Also reword error about multiple -url. --- src/main/java/hudson/remoting/jnlp/Main.java | 34 +++----------------- 1 file changed, 5 insertions(+), 29 deletions(-) diff --git a/src/main/java/hudson/remoting/jnlp/Main.java b/src/main/java/hudson/remoting/jnlp/Main.java index 49c0235b6..ed710acdb 100644 --- a/src/main/java/hudson/remoting/jnlp/Main.java +++ b/src/main/java/hudson/remoting/jnlp/Main.java @@ -81,7 +81,9 @@ public class Main { public List urls = new ArrayList<>(); @Option(name="-webSocket", - usage="Make a WebSocket connection to Jenkins rather than using the TCP port.") + usage="Make a WebSocket connection to Jenkins rather than using the TCP port.", + depends="-url", + forbids={"-direct", "-tunnel", "-credentials", "-proxyCredentials", "-candidateCertificates", "-disableHttpsCertValidation", "-noKeepAlive", "-noReconnect"}) public boolean webSocket; @Option(name="-credentials",metaVar="USER:PASSWORD", @@ -269,35 +271,9 @@ public static void _main(String[] args) throws IOException, InterruptedException throw new CmdLineException(p, "At least one -url option is required.", null); } if (m.webSocket) { - if (m.urls.isEmpty()) { - throw new CmdLineException(p, "-url is required in -webSocket mode", null); - } + assert !m.urls.isEmpty(); // depends="-url" if (m.urls.size() > 1) { - throw new CmdLineException(p, "multiple -url are not currently supported in -webSocket mode", null); - } - if (m.directConnection != null) { - throw new CmdLineException(p, "-webSocket and -direct are mutually exclusive", null); - } - if (m.tunnel != null) { - throw new CmdLineException(p, "-tunnel is not currently supported in -webSocket mode", null); - } - if (m.credentials != null) { - throw new CmdLineException(p, "-credentials is not currently supported in -webSocket mode", null); - } - if (m.proxyCredentials != null) { - throw new CmdLineException(p, "-proxyCredentials is not currently supported in -webSocket mode", null); - } - if (m.candidateCertificates != null) { - throw new CmdLineException(p, "-candidateCertificates is not currently supported in -webSocket mode", null); - } - if (m.disableHttpsCertValidation) { - throw new CmdLineException(p, "-disableHttpsCertValidation is not currently supported in -webSocket mode", null); - } - if (m.noKeepAlive) { - throw new CmdLineException(p, "-noKeepAlive is not currently supported in -webSocket mode", null); - } - if (m.noReconnect) { - throw new CmdLineException(p, "-noReconnect is not currently supported in -webSocket mode", null); + throw new CmdLineException(p, "-webSocket supports only a single -url", null); } } m.main(); From 739e33898fab5354d6b55af9915fd0234d13b451 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jan 2020 16:25:17 -0500 Subject: [PATCH 30/33] No need to explicitly close connections after EngineListener.error: Main.CuiListener etc. already exit the JVM in this case. --- src/main/java/hudson/remoting/Engine.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index 5c6c1b79f..b3e7baf57 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -546,7 +546,6 @@ public void afterResponse(HandshakeResponse hr) { VersionNumber minimumSupportedVersion = new VersionNumber(remotingMinimumVersion.get(0)); VersionNumber currentVersion = new VersionNumber(Launcher.VERSION); if (currentVersion.isOlderThan(minimumSupportedVersion)) { - // TODO these errors should trigger a connection close events.error(new IOException("Agent version " + minimumSupportedVersion + " or newer is required.")); } } From 7eda1936cb48701887bee1b9b48f8efdfd98345b Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jan 2020 17:54:07 -0500 Subject: [PATCH 31/33] Implement reconnection in WebSocket mode. --- src/main/java/hudson/remoting/Engine.java | 201 +++++++++++-------- src/main/java/hudson/remoting/jnlp/Main.java | 2 +- 2 files changed, 114 insertions(+), 89 deletions(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index b3e7baf57..e641927df 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -29,6 +29,7 @@ import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.IOException; +import java.net.HttpURLConnection; import java.net.Socket; import java.net.URI; import java.net.URL; @@ -526,107 +527,131 @@ public void run() { @SuppressFBWarnings(value = "REC_CATCH_EXCEPTION", justification = "checked exceptions were a mistake to begin with") private void runWebSocket() { try { - AtomicReference ch = new AtomicReference<>(); - String localCap = new Capability().toASCII(); - class HeaderHandler extends ClientEndpointConfig.Configurator { - Capability remoteCapability = new Capability(); - @Override - public void beforeRequest(Map> headers) { - headers.put(JnlpConnectionState.CLIENT_NAME_KEY, Collections.singletonList(slaveName)); - headers.put(JnlpConnectionState.SECRET_KEY, Collections.singletonList(secretKey)); - headers.put(Capability.KEY, Collections.singletonList(localCap)); - // TODO use JnlpConnectionState.COOKIE_KEY somehow (see EngineJnlpConnectionStateListener.afterChannel) - LOGGER.fine(() -> "Sending: " + headers); - } - @Override - public void afterResponse(HandshakeResponse hr) { - LOGGER.fine(() -> "Receiving: " + hr.getHeaders()); - List remotingMinimumVersion = hr.getHeaders().get(REMOTING_MINIMUM_VERSION_HEADER); - if (remotingMinimumVersion != null && !remotingMinimumVersion.isEmpty()) { - VersionNumber minimumSupportedVersion = new VersionNumber(remotingMinimumVersion.get(0)); - VersionNumber currentVersion = new VersionNumber(Launcher.VERSION); - if (currentVersion.isOlderThan(minimumSupportedVersion)) { - events.error(new IOException("Agent version " + minimumSupportedVersion + " or newer is required.")); - } - } - try { - remoteCapability = Capability.fromASCII(hr.getHeaders().get(Capability.KEY).get(0)); - LOGGER.fine(() -> "received " + remoteCapability); - } catch (IOException x) { - events.error(x); + while (true) { + AtomicReference ch = new AtomicReference<>(); + String localCap = new Capability().toASCII(); + class HeaderHandler extends ClientEndpointConfig.Configurator { + Capability remoteCapability = new Capability(); + @Override + public void beforeRequest(Map> headers) { + headers.put(JnlpConnectionState.CLIENT_NAME_KEY, Collections.singletonList(slaveName)); + headers.put(JnlpConnectionState.SECRET_KEY, Collections.singletonList(secretKey)); + headers.put(Capability.KEY, Collections.singletonList(localCap)); + // TODO use JnlpConnectionState.COOKIE_KEY somehow (see EngineJnlpConnectionStateListener.afterChannel) + LOGGER.fine(() -> "Sending: " + headers); } - } - } - HeaderHandler headerHandler = new HeaderHandler(); - class AgentEndpoint extends Endpoint { - @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") - AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; - @Override - public void onOpen(Session session, EndpointConfig config) { - events.status("WebSocket connection open"); - session.addMessageHandler(byte[].class, this::onMessage); - try { - ch.set(new ChannelBuilder(slaveName, executor). - withJarCacheOrDefault(jarCache). // unless EngineJnlpConnectionStateListener can be used for this purpose - build(new Transport(session))); - } catch (IOException x) { - events.error(x); + @Override + public void afterResponse(HandshakeResponse hr) { + LOGGER.fine(() -> "Receiving: " + hr.getHeaders()); + List remotingMinimumVersion = hr.getHeaders().get(REMOTING_MINIMUM_VERSION_HEADER); + if (remotingMinimumVersion != null && !remotingMinimumVersion.isEmpty()) { + VersionNumber minimumSupportedVersion = new VersionNumber(remotingMinimumVersion.get(0)); + VersionNumber currentVersion = new VersionNumber(Launcher.VERSION); + if (currentVersion.isOlderThan(minimumSupportedVersion)) { + events.error(new IOException("Agent version " + minimumSupportedVersion + " or newer is required.")); + } + } + try { + remoteCapability = Capability.fromASCII(hr.getHeaders().get(Capability.KEY).get(0)); + LOGGER.fine(() -> "received " + remoteCapability); + } catch (IOException x) { + events.error(x); + } } } - private void onMessage(byte[] message) { - LOGGER.finest(() -> "received message of length " + message.length); - receiver.handle(message); - } - @Override - public void onClose(Session session, CloseReason closeReason) { - LOGGER.fine(() -> "onClose: " + closeReason); - receiver.terminate(new ChannelClosedException(ch.get(), null)); - } - @Override - public void onError(Session session, Throwable x) { - LOGGER.log(Level.FINE, null, x); - receiver.terminate(new ChannelClosedException(ch.get(), x)); - } - class Transport extends AbstractByteArrayCommandTransport { - final Session session; - Transport(Session session) { - this.session = session; - } + HeaderHandler headerHandler = new HeaderHandler(); + class AgentEndpoint extends Endpoint { + @SuppressFBWarnings(value = "UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", justification = "just trust me here") + AbstractByteArrayCommandTransport.ByteArrayReceiver receiver; @Override - public void setup(AbstractByteArrayCommandTransport.ByteArrayReceiver _receiver) { - events.status("Setting up channel"); - receiver = _receiver; + public void onOpen(Session session, EndpointConfig config) { + events.status("WebSocket connection open"); + session.addMessageHandler(byte[].class, this::onMessage); + try { + ch.set(new ChannelBuilder(slaveName, executor). + withJarCacheOrDefault(jarCache). // unless EngineJnlpConnectionStateListener can be used for this purpose + build(new Transport(session))); + } catch (IOException x) { + events.error(x); + } } - @Override - public void writeBlock(Channel channel, byte[] payload) throws IOException { - LOGGER.finest(() -> "sending message of length " + payload.length); - session.getBasicRemote().sendBinary(ByteBuffer.wrap(payload)); + private void onMessage(byte[] message) { + LOGGER.finest(() -> "received message of length " + message.length); + receiver.handle(message); } @Override - public Capability getRemoteCapability() throws IOException { - return headerHandler.remoteCapability; + public void onClose(Session session, CloseReason closeReason) { + LOGGER.fine(() -> "onClose: " + closeReason); + receiver.terminate(new ChannelClosedException(ch.get(), null)); } @Override - public void closeWrite() throws IOException { - events.status("Write side closed"); - session.close(); + public void onError(Session session, Throwable x) { + // TODO or would events.error(x) be better? + LOGGER.log(Level.FINE, null, x); + receiver.terminate(new ChannelClosedException(ch.get(), x)); } - @Override - public void closeRead() throws IOException { - events.status("Read side closed"); - session.close(); + class Transport extends AbstractByteArrayCommandTransport { + final Session session; + Transport(Session session) { + this.session = session; + } + @Override + public void setup(AbstractByteArrayCommandTransport.ByteArrayReceiver _receiver) { + events.status("Setting up channel"); + receiver = _receiver; + } + @Override + public void writeBlock(Channel channel, byte[] payload) throws IOException { + LOGGER.finest(() -> "sending message of length " + payload.length); + session.getBasicRemote().sendBinary(ByteBuffer.wrap(payload)); + } + @Override + public Capability getRemoteCapability() throws IOException { + return headerHandler.remoteCapability; + } + @Override + public void closeWrite() throws IOException { + events.status("Write side closed"); + session.close(); + } + @Override + public void closeRead() throws IOException { + events.status("Read side closed"); + session.close(); + } } } + ContainerProvider.getWebSocketContainer().connectToServer(new AgentEndpoint(), + ClientEndpointConfig.Builder.create().configurator(headerHandler).build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); + while (ch.get() == null) { + Thread.sleep(100); + } + events.status("Connected"); + ch.get().join(); + events.status("Terminated"); + if (noReconnect) { + return; + } + events.onDisconnect(); + while (true) { + // Unlike JnlpAgentEndpointResolver, we do not use $jenkins/tcpSlaveAgentListener/, as that will be a 404 if the TCP port is disabled. + URL ping = new URL(candidateUrls.get(0), "login"); + try { + HttpURLConnection conn = (HttpURLConnection) ping.openConnection(); + int status = conn.getResponseCode(); + conn.disconnect(); + if (status == 200) { + break; + } else { + events.status(ping + " is not ready: " + status); + } + } catch (IOException x) { + events.status(ping + " is not ready", x); + } + Thread.sleep(10_000); + } + events.onReconnect(); } - ContainerProvider.getWebSocketContainer().connectToServer(new AgentEndpoint(), - ClientEndpointConfig.Builder.create().configurator(headerHandler).build(), URI.create(candidateUrls.get(0).toString().replaceFirst("^http", "ws") + "wsagents/")); - while (ch.get() == null) { - Thread.sleep(100); - } - LOGGER.info("Connected"); - ch.get().join(); - // TODO handle multiple candidate URLs - // TODO handle reconnection } catch (Exception e) { events.error(e); } diff --git a/src/main/java/hudson/remoting/jnlp/Main.java b/src/main/java/hudson/remoting/jnlp/Main.java index ed710acdb..34b5529ab 100644 --- a/src/main/java/hudson/remoting/jnlp/Main.java +++ b/src/main/java/hudson/remoting/jnlp/Main.java @@ -83,7 +83,7 @@ public class Main { @Option(name="-webSocket", usage="Make a WebSocket connection to Jenkins rather than using the TCP port.", depends="-url", - forbids={"-direct", "-tunnel", "-credentials", "-proxyCredentials", "-candidateCertificates", "-disableHttpsCertValidation", "-noKeepAlive", "-noReconnect"}) + forbids={"-direct", "-tunnel", "-credentials", "-proxyCredentials", "-candidateCertificates", "-disableHttpsCertValidation", "-noKeepAlive"}) public boolean webSocket; @Option(name="-credentials",metaVar="USER:PASSWORD", From b779630b94ad895678b897e34d4736c7284ca24c Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jan 2020 17:55:58 -0500 Subject: [PATCH 32/33] Correcting option name. --- src/main/java/hudson/remoting/jnlp/Main.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/jnlp/Main.java b/src/main/java/hudson/remoting/jnlp/Main.java index 34b5529ab..c39a4d36b 100644 --- a/src/main/java/hudson/remoting/jnlp/Main.java +++ b/src/main/java/hudson/remoting/jnlp/Main.java @@ -83,7 +83,7 @@ public class Main { @Option(name="-webSocket", usage="Make a WebSocket connection to Jenkins rather than using the TCP port.", depends="-url", - forbids={"-direct", "-tunnel", "-credentials", "-proxyCredentials", "-candidateCertificates", "-disableHttpsCertValidation", "-noKeepAlive"}) + forbids={"-direct", "-tunnel", "-credentials", "-proxyCredentials", "-cert", "-disableHttpsCertValidation", "-noKeepAlive"}) public boolean webSocket; @Option(name="-credentials",metaVar="USER:PASSWORD", From c238b7c99bb8ccb13cbe47ca000a62476c4b09e4 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jan 2020 18:11:19 -0500 Subject: [PATCH 33/33] FindSecBugs --- src/main/java/hudson/remoting/Engine.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/remoting/Engine.java b/src/main/java/hudson/remoting/Engine.java index e641927df..f8d35c016 100644 --- a/src/main/java/hudson/remoting/Engine.java +++ b/src/main/java/hudson/remoting/Engine.java @@ -524,7 +524,7 @@ public void run() { } } - @SuppressFBWarnings(value = "REC_CATCH_EXCEPTION", justification = "checked exceptions were a mistake to begin with") + @SuppressFBWarnings(value = {"REC_CATCH_EXCEPTION", "URLCONNECTION_SSRF_FD"}, justification = "checked exceptions were a mistake to begin with; connecting to Jenkins from agent") private void runWebSocket() { try { while (true) {