Skip to content

Commit

Permalink
Revert "Merge pull request #2010 from jenkinsci/jnlp3"
Browse files Browse the repository at this point in the history
This reverts commit af1a53d.
  • Loading branch information
olivergondza committed Mar 31, 2016
1 parent ce866dd commit 343e65f
Show file tree
Hide file tree
Showing 7 changed files with 116 additions and 173 deletions.
Expand Up @@ -7,7 +7,6 @@
import hudson.remoting.Channel;
import hudson.slaves.SlaveComputer;
import jenkins.model.Jenkins;
import org.jenkinsci.remoting.engine.JnlpServerHandshake;

import java.io.IOException;
import java.security.SecureRandom;
Expand All @@ -28,7 +27,7 @@
@Extension
public class DefaultJnlpSlaveReceiver extends JnlpAgentReceiver {
@Override
public boolean handle(String nodeName, JnlpServerHandshake handshake) throws IOException, InterruptedException {
public boolean handle(String nodeName, JnlpSlaveHandshake handshake) throws IOException, InterruptedException {
SlaveComputer computer = (SlaveComputer) Jenkins.getInstance().getComputer(nodeName);

if (computer==null) {
Expand All @@ -44,7 +43,9 @@ public boolean handle(String nodeName, JnlpServerHandshake handshake) throws IOE
LOGGER.info("Disconnecting "+nodeName+" as we are reconnected from the current peer");
try {
computer.disconnect(new ConnectionFromCurrentPeer()).get(15, TimeUnit.SECONDS);
} catch (ExecutionException | TimeoutException e) {
} catch (ExecutionException e) {
throw new IOException("Failed to disconnect the current client",e);
} catch (TimeoutException e) {
throw new IOException("Failed to disconnect the current client",e);
}
} else {
Expand Down Expand Up @@ -86,7 +87,7 @@ public boolean handle(String nodeName, JnlpServerHandshake handshake) throws IOE
* @return
* true if the slave secret matches the handshake secret, false otherwise.
*/
private boolean matchesSecret(String nodeName, JnlpServerHandshake handshake){
private boolean matchesSecret(String nodeName, JnlpSlaveHandshake handshake){
SlaveComputer computer = (SlaveComputer) Jenkins.getInstance().getComputer(nodeName);
String handshakeSecret = handshake.getRequestProperty("Secret-Key");
// Verify that the slave secret matches the handshake secret.
Expand Down
13 changes: 1 addition & 12 deletions core/src/main/java/jenkins/slaves/JnlpAgentReceiver.java
Expand Up @@ -3,8 +3,6 @@
import hudson.ExtensionList;
import hudson.ExtensionPoint;
import hudson.model.Slave;
import jenkins.model.Jenkins;
import org.jenkinsci.remoting.engine.JnlpServerHandshake;

import java.io.IOException;
import java.util.Properties;
Expand Down Expand Up @@ -57,16 +55,7 @@ public abstract class JnlpAgentReceiver implements ExtensionPoint {
* @throws Exception
* Any exception thrown from this method will fatally terminate the connection.
*/
public abstract boolean handle(String name, JnlpServerHandshake handshake) throws IOException, InterruptedException;

/**
* @deprecated
* Use {@link #handle(String, JnlpServerHandshake)}
*/
public boolean handle(String name, JnlpSlaveHandshake handshake) throws IOException, InterruptedException {
return handle(name,(JnlpServerHandshake)handshake);
}

public abstract boolean handle(String name, JnlpSlaveHandshake handshake) throws IOException, InterruptedException;

public static ExtensionList<JnlpAgentReceiver> all() {
return ExtensionList.lookup(JnlpAgentReceiver.class);
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java
Expand Up @@ -2,7 +2,6 @@

import hudson.AbortException;
import hudson.Extension;
import hudson.model.Computer;
import hudson.remoting.Channel;
import hudson.remoting.Channel.Listener;
import hudson.remoting.ChannelBuilder;
Expand All @@ -12,7 +11,6 @@
import jenkins.model.Jenkins;
import jenkins.security.ChannelConfigurator;
import jenkins.security.HMACConfidentialKey;
import org.jenkinsci.remoting.engine.JnlpServerHandshake;
import org.jenkinsci.remoting.nio.NioChannelHub;

import javax.inject.Inject;
Expand Down Expand Up @@ -69,7 +67,7 @@ public void handle(Socket socket) throws IOException, InterruptedException {
new Handler(hub.getHub(),socket).run();
}

protected static class Handler extends JnlpServerHandshake {
protected static class Handler extends JnlpSlaveHandshake {

/**
* @deprecated as of 1.559
Expand All @@ -81,7 +79,9 @@ public Handler(Socket socket) throws IOException {
}

public Handler(NioChannelHub hub, Socket socket) throws IOException {
super(hub, Computer.threadPoolForRemoting, socket);
super(hub,socket,
new DataInputStream(socket.getInputStream()),
new PrintWriter(new BufferedWriter(new OutputStreamWriter(socket.getOutputStream(),"UTF-8")),true));
}

protected void run() throws IOException, InterruptedException {
Expand Down
10 changes: 2 additions & 8 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol2.java
@@ -1,7 +1,6 @@
package jenkins.slaves;

import hudson.Extension;
import org.jenkinsci.remoting.engine.JnlpServerHandshake;
import org.jenkinsci.remoting.nio.NioChannelHub;

import java.io.ByteArrayInputStream;
Expand Down Expand Up @@ -55,13 +54,8 @@ protected void run() throws IOException, InterruptedException {
final String nodeName = request.getProperty("Node-Name");

for (JnlpAgentReceiver recv : JnlpAgentReceiver.all()) {
try {
if (recv.handle(nodeName,this))
return;
} catch (AbstractMethodError e) {
if (recv.handle(nodeName,new JnlpSlaveHandshake(this)))
return;
}
if (recv.handle(nodeName,this))
return;
}

error("JNLP2-connect: rejected connection for node: " + nodeName);
Expand Down
137 changes: 0 additions & 137 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol3.java

This file was deleted.

110 changes: 103 additions & 7 deletions core/src/main/java/jenkins/slaves/JnlpSlaveHandshake.java
@@ -1,22 +1,118 @@
package jenkins.slaves;

import org.jenkinsci.remoting.engine.JnlpServerHandshake;
import hudson.model.Computer;
import hudson.remoting.Channel;
import hudson.remoting.ChannelBuilder;
import hudson.remoting.Engine;
import org.jenkinsci.remoting.nio.NioChannelHub;

import java.io.DataInputStream;
import java.io.DataOutputStream;
import java.io.IOException;
import java.io.PrintWriter;
import java.net.Socket;
import java.util.concurrent.ExecutorService;
import java.util.Map.Entry;
import java.util.Properties;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
* Palette of objects to talk to the incoming JNLP slave connection.
*
* @author Kohsuke Kawaguchi
* @since 1.561
* @deprecated as of 1.609
* Use {@link JnlpServerHandshake}
*/
public class JnlpSlaveHandshake extends JnlpServerHandshake {
/*package*/ JnlpSlaveHandshake(JnlpServerHandshake rhs) {
super(rhs);
public class JnlpSlaveHandshake {
/**
* Useful for creating a {@link Channel} with NIO as the underlying transport.
*/
/*package*/ final NioChannelHub hub;

/**
* Socket connection to the slave.
*/
/*package*/ final Socket socket;

/**
* Wrapping Socket input stream.
*/
/*package*/ final DataInputStream in;

/**
* For writing handshaking response.
*
* This is a poor design choice that we just carry forward for compatibility.
* For better protocol design, {@link DataOutputStream} is preferred for newer
* protocols.
*/
/*package*/ final PrintWriter out;

/**
* Bag of properties the JNLP agent have sent us during the hand-shake.
*/
/*package*/ final Properties request = new Properties();


/*package*/ JnlpSlaveHandshake(NioChannelHub hub, Socket socket, DataInputStream in, PrintWriter out) {
this.hub = hub;
this.socket = socket;
this.in = in;
this.out = out;
}

public NioChannelHub getHub() {
return hub;
}

public Socket getSocket() {
return socket;
}

public DataInputStream getIn() {
return in;
}

public PrintWriter getOut() {
return out;
}

public Properties getRequestProperties() {
return request;
}

public String getRequestProperty(String name) {
return request.getProperty(name);
}


/**
* Sends the error output and bail out.
*/
public void error(String msg) throws IOException {
out.println(msg);
LOGGER.log(Level.WARNING,Thread.currentThread().getName()+" is aborted: "+msg);
socket.close();
}

/**
* {@link JnlpAgentReceiver} calls this method to tell the client that the server
* is happy with the handshaking and is ready to move on to build a channel.
*/
public void success(Properties response) {
out.println(Engine.GREETING_SUCCESS);
for (Entry<Object, Object> e : response.entrySet()) {
out.println(e.getKey()+": "+e.getValue());
}
out.println(); // empty line to conclude the response header
}

public ChannelBuilder createChannelBuilder(String nodeName) {
if (hub==null)
return new ChannelBuilder(nodeName, Computer.threadPoolForRemoting);
else
return hub.newChannelBuilder(nodeName, Computer.threadPoolForRemoting);
}


private static final Logger LOGGER = Logger.getLogger(JnlpSlaveHandshake.class.getName());
}
2 changes: 1 addition & 1 deletion pom.xml
Expand Up @@ -179,7 +179,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>remoting</artifactId>
<version>2.56</version>
<version>2.53.3</version>

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 13, 2016

Member

@olivergondza @jglick @oleg-nenashev THIS LOOKS LIKE A SERIOUS OOPS!!!

We should not have rolled back remoting... the rest OK... but remoting?

Was this discussed properly?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 13, 2016

Member

IIRC no. There are known issues in remoting 2.54, but other versions seem to be more or less fine.
I suppose we also need jenkinsci/remoting#79, hence @kohsuke should finally release 2.53.4 at least (maybe in .2).

I'm also aware about JNLP3 revert since it's the only version with encryption support. BTW, there are reported issues in this protocol, so I more-or-less agree.

This comment has been minimized.

Copy link
@olivergondza

olivergondza Apr 13, 2016

Author Member

@stephenc, this revert just un-backported the fix we decided not to put in on meeting. We have not rolled back to anything older than what was in baseline.

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 13, 2016

Member

2.54 includes the ability to configure slaves with the TLS certs easily... that is a big miss IMHO... (as should the TLS cert CLI option have been in this LTS line)

as an aside I have just found and (hopefully) fixed also a memory leak JENKINS-34213 that I think should be an LTS candidate

This comment has been minimized.

Copy link
@olivergondza

olivergondza Apr 14, 2016

Author Member

@stephenc, I do not object. Please make sure those are flagged properly so they will not miss .2.

</dependency>

<dependency>
Expand Down

0 comments on commit 343e65f

Please sign in to comment.