Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-26580] Activate JNLP3 support #2010

Merged
merged 16 commits into from
Mar 9, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
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 @@ -27,7 +28,7 @@
@Extension
public class DefaultJnlpSlaveReceiver extends JnlpAgentReceiver {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm this is approach is slightly different than what I had in mind when you mentioned refactoring the remoting module. I was wondering if we could do-away with the server-side implementing JNLP version specific protocol handlers entirely? ie. The master would just make one call to the remoting library and the implementation there would try protocol 3, 2, 1.

This way we can add protocols to the remoting library without needing to update the master, and makes the remoting library more re-usable and out-of-the-box ready.

Regardless this is fine since it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see, that makes sense. I think we can take on that as a follow-up change.

@Override
public boolean handle(String nodeName, JnlpSlaveHandshake handshake) throws IOException, InterruptedException {
public boolean handle(String nodeName, JnlpServerHandshake handshake) throws IOException, InterruptedException {
SlaveComputer computer = (SlaveComputer) Jenkins.getInstance().getComputer(nodeName);

if (computer==null) {
Expand All @@ -43,9 +44,7 @@ public boolean handle(String nodeName, JnlpSlaveHandshake handshake) throws IOEx
LOGGER.info("Disconnecting "+nodeName+" as we are reconnected from the current peer");
try {
computer.disconnect(new ConnectionFromCurrentPeer()).get(15, TimeUnit.SECONDS);
} catch (ExecutionException e) {
throw new IOException("Failed to disconnect the current client",e);
} catch (TimeoutException e) {
} catch (ExecutionException | TimeoutException e) {
throw new IOException("Failed to disconnect the current client",e);
}
} else {
Expand Down Expand Up @@ -87,7 +86,7 @@ public boolean handle(String nodeName, JnlpSlaveHandshake handshake) throws IOEx
* @return
* true if the slave secret matches the handshake secret, false otherwise.
*/
private boolean matchesSecret(String nodeName, JnlpSlaveHandshake handshake){
private boolean matchesSecret(String nodeName, JnlpServerHandshake 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: 12 additions & 1 deletion core/src/main/java/jenkins/slaves/JnlpAgentReceiver.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
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 @@ -55,7 +57,16 @@ 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, JnlpSlaveHandshake handshake) throws IOException, InterruptedException;
public abstract boolean handle(String name, JnlpServerHandshake handshake) throws IOException, InterruptedException;
Copy link
Member

Choose a reason for hiding this comment

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

It still breaks the binary compatibility, because external classes won't implement the new abstract method. Maybe NIT in the case of remoting

Copy link
Member

Choose a reason for hiding this comment

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

BTW the invocation below catches the AbstractMethodError


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

Choose a reason for hiding this comment

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

Missing @Deprecated - given the javadoc.

Copy link
Member Author

Choose a reason for hiding this comment

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

One or the other is sufficient.

return handle(name,(JnlpServerHandshake)handshake);
}


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
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

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 @@ -11,6 +12,7 @@
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 @@ -67,7 +69,7 @@ public void handle(Socket socket) throws IOException, InterruptedException {
new Handler(hub.getHub(),socket).run();
}

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

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

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

protected void run() throws IOException, InterruptedException {
Expand Down
10 changes: 8 additions & 2 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol2.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
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 @@ -54,8 +55,13 @@ protected void run() throws IOException, InterruptedException {
final String nodeName = request.getProperty("Node-Name");

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

Choose a reason for hiding this comment

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

Looks alarming. Why not just retain the original method signature, deprecate it, and delegate?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was much shorter than the ping-pong and loop detection, plus it has the benefit of abstract method clearly marked as such.

return;
}
}

error("JNLP2-connect: rejected connection for node: " + nodeName);
Expand Down
137 changes: 137 additions & 0 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol3.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
package jenkins.slaves;

import hudson.AbortException;
import hudson.Extension;
import hudson.Util;
import hudson.model.Computer;
import hudson.remoting.Channel;
import hudson.remoting.ChannelBuilder;
import hudson.slaves.SlaveComputer;
import jenkins.AgentProtocol;
import jenkins.model.Jenkins;
import jenkins.security.ChannelConfigurator;
import org.jenkinsci.remoting.engine.JnlpServer3Handshake;
import org.jenkinsci.remoting.nio.NioChannelHub;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.inject.Inject;
import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.net.Socket;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
* Master-side implementation for JNLP3-connect protocol.
*
* <p>@see {@link org.jenkinsci.remoting.engine.JnlpProtocol3} for more details.
*
* @author Akshay Dayal
Copy link
Member

Choose a reason for hiding this comment

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

@since would be very useful in this case

* @since 1.XXX
*/
@Extension
Copy link
Contributor

Choose a reason for hiding this comment

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

Initial manual testing looks good - the JNLP node connects with JNLP3 protocol and I can build a project on the node.

I think more stress testing needs to be done before JNLP3 is made the default protocol. In this initial release JNLP3 should only be available if the user explicitly asks for it. There are 3 approaches:

Master-side control
1. Making master support JNLP3 through a flag
2. Making master support JNLP3 through UI option
Node-side control
3. Making node try JNLP3 first using flag

Option 1 can be done in "easy" ways I suppose, this class can check for a boolean system property, if its not set fail and let JNLP2 take over.
Option 2 is a lot of work and I would not favor it.
Option 3 is easy, but requires some extra work for users starting up nodes with JNLP3.

I'm ok with either option 1 or 3, prefer 1 slightly since I like controlling the behavior from the master side, not the node.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll ease this in by making this feature optional in the beginning for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to spend some time dusting out the stress-test suites that we used before and run them for JNLP3. Will probably have to modify them a bit to have significant "work" being done for each build instead of just "sleep 5". Will let you know when I'm done.

public class JnlpSlaveAgentProtocol3 extends AgentProtocol {
@Inject
NioChannelSelector hub;

@Override
public String getName() {
if (ENABLED) return "JNLP3-connect";
else return "JNLP3-disabled";
}

@Override
public void handle(Socket socket) throws IOException, InterruptedException {
new Handler(hub.getHub(), socket).run();
}

static class Handler extends JnlpServer3Handshake {
private SlaveComputer computer;
private PrintWriter logw;
private OutputStream log;

public Handler(NioChannelHub hub, Socket socket) throws IOException {
super(hub, Computer.threadPoolForRemoting, socket);
}

protected void run() throws IOException, InterruptedException {
try {
Channel channel = connect();

computer.setChannel(channel, log,
new Channel.Listener() {
@Override
public void onClosed(Channel channel, IOException cause) {
if (cause != null)
LOGGER.log(Level.WARNING,
Thread.currentThread().getName() + " for + " +
getNodeName() + " terminated", cause);
try {
socket.close();
} catch (IOException e) {
// Do nothing.
}
}
});
} catch (AbortException e) {
logw.println(e.getMessage());
logw.println("Failed to establish the connection with the agent");
throw e;
} catch (IOException e) {
logw.println("Failed to establish the connection with the agent " + getNodeName());
e.printStackTrace(logw);
throw e;
}
}

@Override
public ChannelBuilder createChannelBuilder(String nodeName) {
log = computer.openLogFile();
logw = new PrintWriter(log,true);
logw.println("JNLP agent connected from " + socket.getInetAddress());

ChannelBuilder cb = super.createChannelBuilder(nodeName).withHeaderStream(log);

for (ChannelConfigurator cc : ChannelConfigurator.all()) {
cc.onChannelBuilding(cb, computer);
}

return cb;
}

@Override
protected String getNodeSecret(String nodeName) throws Failure {
computer = (SlaveComputer) Jenkins.getInstance().getComputer(nodeName);
if (computer == null) {
throw new Failure("Agent trying to register for invalid node: " + nodeName);
}
return computer.getJnlpMac();
}

}

private static final Logger LOGGER = Logger.getLogger(JnlpSlaveAgentProtocol3.class.getName());

/**
* Flag to control the activation of JNLP3 protocol.
* This feature is being A/B tested right now.
*
* <p>
* Once this will be on by default, the flag and this field will disappear. The system property is
* an escape hatch for those who hit any issues and those who are trying this out.
*/
@Restricted(NoExternalUse.class)
public static boolean ENABLED;

static {
String propName = JnlpSlaveAgentProtocol3.class.getName() + ".enabled";
if (System.getProperties().containsKey(propName))
ENABLED = Boolean.getBoolean(propName);
else {
byte hash = Util.fromHexString(Jenkins.getActiveInstance().getLegacyInstanceId())[0];
ENABLED = (hash%10)==0;
}
}
}
110 changes: 7 additions & 103 deletions core/src/main/java/jenkins/slaves/JnlpSlaveHandshake.java
Original file line number Diff line number Diff line change
@@ -1,118 +1,22 @@
package jenkins.slaves;

import hudson.model.Computer;
import hudson.remoting.Channel;
import hudson.remoting.ChannelBuilder;
import hudson.remoting.Engine;
import org.jenkinsci.remoting.engine.JnlpServerHandshake;
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.Map.Entry;
import java.util.Properties;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.concurrent.ExecutorService;

/**
* 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 {
/**
* 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 class JnlpSlaveHandshake extends JnlpServerHandshake {
/*package*/ JnlpSlaveHandshake(JnlpServerHandshake rhs) {
super(rhs);
}

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
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>remoting</artifactId>
<version>2.53.3</version>
<version>2.56</version>
</dependency>

<dependency>
Expand Down