Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

[FIXED JENKINS-26558] Clients should provide a unique ID to be used for ... #26

Merged
merged 9 commits into from Apr 27, 2015
Merged

[FIXED JENKINS-26558] Clients should provide a unique ID to be used for ... #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ab37bc8
[FIXED JENKINS-26558] Clients should provide a unique ID to be used f…
stephenc Apr 24, 2015
0871658
Actually need to keep `options.name` as immutable or else it will kee…
stephenc Apr 24, 2015
2132772
extra null safety
stephenc Apr 24, 2015
7984ef2
Add support for IPv6 addresses
stephenc Apr 27, 2015
640637c
Return key-value properties to support future extensions
stephenc Apr 27, 2015
78c8650
Explain what's going wrong
stephenc Apr 27, 2015
b7e1470
Use commons-codec DigestUtils
stephenc Apr 27, 2015
ff5dd53
Specify locale
stephenc Apr 27, 2015
96a1a84
oops missed the charset fix
stephenc Apr 27, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/src/main/java/hudson/plugins/swarm/Client.java
View file
Open in desktop
Expand Up @@ -77,7 +77,7 @@ public void run() throws InterruptedException {
swarmClient.verifyThatUrlIsHudson(target);
}

System.out.println("Attempting to connect to " + target.url + " " + target.secret);
System.out.println("Attempting to connect to " + target.url + " " + target.secret + " with ID " + swarmClient.getHash());

// create a new swarm slave
swarmClient.createSwarmSlave(target);
Expand Down
124 changes: 100 additions & 24 deletions client/src/main/java/hudson/plugins/swarm/SwarmClient.java
View file
Open in desktop
Expand Up @@ -2,58 +2,77 @@

import hudson.remoting.Launcher;
import hudson.remoting.jnlp.Main;
import org.apache.commons.codec.digest.DigestUtils;
import org.apache.commons.httpclient.HttpClient;
import org.apache.commons.httpclient.HttpStatus;
import org.apache.commons.httpclient.UsernamePasswordCredentials;
import org.apache.commons.httpclient.auth.AuthScope;
import org.apache.commons.httpclient.methods.GetMethod;
import org.apache.commons.httpclient.methods.PostMethod;
import org.apache.commons.lang.StringUtils;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
import org.w3c.dom.Node;
import org.w3c.dom.Text;
import org.xml.sax.SAXException;

import javax.net.ssl.KeyManager;
import javax.net.ssl.SSLContext;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.UnsupportedEncodingException;
import java.math.BigInteger;
import java.net.DatagramPacket;
import java.net.DatagramSocket;
import java.net.HttpURLConnection;
import java.net.Inet4Address;
import java.net.Inet6Address;
import java.net.InetAddress;
import java.net.MalformedURLException;
import java.net.NetworkInterface;
import java.net.SocketException;
import java.net.SocketTimeoutException;
import java.net.URL;
import java.net.URLEncoder;
import java.security.KeyManagementException;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Properties;
import java.util.Random;

import javax.net.ssl.KeyManager;
import javax.net.ssl.SSLContext;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;

import org.apache.commons.httpclient.HttpClient;
import org.apache.commons.httpclient.HttpStatus;
import org.apache.commons.httpclient.UsernamePasswordCredentials;
import org.apache.commons.httpclient.auth.AuthScope;
import org.apache.commons.httpclient.methods.GetMethod;
import org.apache.commons.httpclient.methods.PostMethod;
import org.apache.commons.lang.StringUtils;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
import org.w3c.dom.Node;
import org.w3c.dom.Text;
import org.xml.sax.SAXException;

public class SwarmClient {

private final Options options;

private final String hash;

private String name;

public SwarmClient(Options options) {
this.options = options;
this.hash = hash(options.remoteFsRoot);
this.name = options.name;
}

public String getHash() {
return hash;
}

public Candidate discoverFromBroadcast() throws IOException,
Expand Down Expand Up @@ -191,7 +210,7 @@ protected void connect(Candidate target) throws InterruptedException {
try {
Launcher launcher = new Launcher();

launcher.slaveJnlpURL = new URL(target.url + "computer/" + options.name
launcher.slaveJnlpURL = new URL(target.url + "computer/" + name
+ "/slave-agent.jnlp");

if (options.username != null && options.password != null) {
Expand Down Expand Up @@ -292,7 +311,9 @@ protected void createSwarmSlave(Candidate target) throws IOException, Interrupte
+ param("labels", labelStr)
+ param("toolLocations", toolLocationsStr)
+ "&secret=" + target.secret
+ param("mode", options.mode.toUpperCase()));
+ param("mode", options.mode.toUpperCase(Locale.ENGLISH))
+ param("hash", hash)
);

post.setDoAuthentication(true);

Expand All @@ -303,9 +324,29 @@ protected void createSwarmSlave(Candidate target) throws IOException, Interrupte

int responseCode = client.executeMethod(post);
if (responseCode != 200) {
throw new RetryException(
"Failed to create a slave on Jenkins CODE: " + responseCode);
throw new RetryException(String.format("Failed to create a slave on Jenkins CODE: %s%n%s",responseCode,
post.getResponseBodyAsString()) );
}
Properties props = new Properties();
InputStream stream = post.getResponseBodyAsStream();
if (stream != null) {
try {
props.load(stream);
} finally {
stream.close();
}
}
String name = props.getProperty("name");
if (name == null) {
this.name = options.name;
return;
}
name = name.trim();
if (name.isEmpty()) {
this.name = options.name;
return;
}
this.name = name;
}

private String encode(String value) throws UnsupportedEncodingException {
Expand Down Expand Up @@ -368,6 +409,41 @@ private static String getChildElementString(Element parent, String tagName) {
return null;
}

/**
* Returns a hash that should be consistent for any individual swarm client (as long as it has a persistent IP)
* and should be unique to that client.
*
* @param remoteFsRoot the file system root should be part of the hash (to support multiple swarm clients from
* the same machine)
* @return our best effort at a consistent hash
*/
public static String hash(File remoteFsRoot) {
StringBuilder buf = new StringBuilder();
try {
buf.append(remoteFsRoot.getCanonicalPath()).append('\n');
} catch (IOException e) {
buf.append(remoteFsRoot.getAbsolutePath()).append('\n');
}
try {
for (NetworkInterface ni : Collections.list(NetworkInterface.getNetworkInterfaces())) {
for (InetAddress ia : Collections.list(ni.getInetAddresses())) {
if (ia instanceof Inet4Address) {
buf.append(ia.getHostAddress()).append('\n');
} else if (ia instanceof Inet6Address) {
buf.append(ia.getHostAddress()).append('\n');
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you don't want to include non IP adapters (it's just more entropy?) or collapse this into on if but not enough to fail the review.

}
}
byte[] hardwareAddress = ni.getHardwareAddress();
Copy link
Member

Choose a reason for hiding this comment

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

so for IPv6 hosts when a machine is cloned this is going to break?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not think that Jenkins works on IPv6.

It'll most likely be dhcp for its v4 address anyway

On Friday, April 24, 2015, James Nord notifications@github.com wrote:

In client/src/main/java/hudson/plugins/swarm/SwarmClient.java
#26 (comment):

  • */
  • public static String hash(File remoteFsRoot) {
  •    StringBuilder buf = new StringBuilder();

  •    try {

  •        buf.append(remoteFsRoot.getCanonicalPath()).append('\n');

  •    } catch (IOException e) {

  •        buf.append(remoteFsRoot.getAbsolutePath()).append('\n');

  •    }

  •    try {

  •        for (NetworkInterface ni : Collections.list(NetworkInterface.getNetworkInterfaces())) {

  •            for (InetAddress ia : Collections.list(ni.getInetAddresses())) {

  •                if (ia instanceof Inet4Address) {

  •                    buf.append(ia.getHostAddress()).append('\n');

  •                }

  •            }

  •            byte[] hardwareAddress = ni.getHardwareAddress();

so for IPv6 hosts when a machine is cloned this is going to break?

—
Reply to this email directly or view it on GitHub
https://github.com/jenkinsci/swarm-plugin/pull/26/files#r29063667.

Sent from my phone

Copy link
Member

Choose a reason for hiding this comment

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

I think Jenkins on the whole does work on ipV6 and if there are some areas that don't I would consider it a bug and I would be against introducing new bugs.

if (hardwareAddress != null) {
buf.append(Arrays.toString(hardwareAddress));
}
}
} catch (SocketException e) {
// oh well we tried
}
return DigestUtils.md5Hex(buf.toString()).substring(0, 8);
}

private static class DefaultTrustManager implements X509TrustManager {

public void checkClientTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException {
Expand Down
64 changes: 52 additions & 12 deletions plugin/src/main/java/hudson/plugins/swarm/PluginImpl.java
View file
Open in desktop
@@ -1,28 +1,33 @@
package hudson.plugins.swarm;

import static javax.servlet.http.HttpServletResponse.*;
import com.google.common.collect.Lists;
import hudson.Plugin;
import hudson.Util;
import hudson.model.Computer;
import hudson.model.Descriptor.FormException;
import hudson.model.Node;
import hudson.slaves.SlaveComputer;
import hudson.tools.ToolDescriptor;
import hudson.tools.ToolInstallation;
import hudson.tools.ToolLocationNodeProperty;
import hudson.tools.ToolLocationNodeProperty.ToolLocation;

import java.io.IOException;
import java.io.Writer;
import java.util.List;

import jenkins.model.Jenkins;

import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;

import com.google.common.collect.Lists;
import javax.servlet.ServletOutputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.Writer;
import java.util.List;
import java.util.Properties;

import static javax.servlet.http.HttpServletResponse.SC_CONFLICT;
import static javax.servlet.http.HttpServletResponse.SC_EXPECTATION_FAILED;
import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE;

/**
* Exposes an entry point to add a new swarm slave.
Expand All @@ -36,7 +41,7 @@ public class PluginImpl extends Plugin {
*/
public void doCreateSlave(StaplerRequest req, StaplerResponse rsp, @QueryParameter String name, @QueryParameter String description, @QueryParameter int executors,
@QueryParameter String remoteFsRoot, @QueryParameter String labels, @QueryParameter String secret, @QueryParameter Node.Mode mode,
@QueryParameter String toolLocations) throws IOException {
@QueryParameter String toolLocations, @QueryParameter(fixEmpty = true) String hash) throws IOException {

if (!getSwarmSecret().equals(secret)) {
rsp.setStatus(SC_FORBIDDEN);
Expand All @@ -54,9 +59,34 @@ public void doCreateSlave(StaplerRequest req, StaplerResponse rsp, @QueryParamet
nodeProperties = Lists.newArrayList(new ToolLocationNodeProperty(parsedToolLocations));
}

// try to make the name unique. Swarm clients are often replicated VMs, and they may have the same name.
if (jenkins.getNode(name) != null) {
name = name + '-' + req.getRemoteAddr();
if (hash == null && jenkins.getNode(name) != null) {
Copy link
Member

Choose a reason for hiding this comment

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

does this mean that no existing swarm clients will be able to reconnect when Jenkins restarts - or when a swarm node disconnects its node is removed (and hence there would be no previous swamr nodes on a Jenkins restart?)

Copy link
Member Author

Choose a reason for hiding this comment

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

SwarmSlave is ephemeral so on restart none will exist.

Plus this branch is only if using a legacy client, wherein we can do
nothing as legacy cannot pick up the new name.

When a swarm save disconnects its node is removed. Should be fine

On Friday, April 24, 2015, James Nord notifications@github.com wrote:

In plugin/src/main/java/hudson/plugins/swarm/PluginImpl.java
#26 (comment):

@@ -54,9 +57,28 @@ public void doCreateSlave(StaplerRequest req, StaplerResponse rsp, @QueryParamet
nodeProperties = Lists.newArrayList(new ToolLocationNodeProperty(parsedToolLocations));
}

  •        // try to make the name unique. Swarm clients are often replicated VMs, and they may have the same name.

  •        if (jenkins.getNode(name) != null) {

  •            name = name + '-' + req.getRemoteAddr();

  •        if (hash == null && jenkins.getNode(name) != null) {

does this mean that no existing swarm clients will be able to reconnect
when Jenkins restarts - or when a swarm node disconnects its node is
removed (and hence there would be no previous swamr nodes on a Jenkins
restart?)

—
Reply to this email directly or view it on GitHub
https://github.com/jenkinsci/swarm-plugin/pull/26/files#r29063891.

Sent from my phone

// this is a legacy client, they won't be able to pick up the new name, so throw them away
// perhaps they can find another master to connect to
rsp.setStatus(SC_CONFLICT);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be wise to include a plain text response here for the poor admin that tries to debug this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Legacy clients will not display the returned text anyway... Might add it
for the second conflict and add logging to Jenkins though to assist an admin

On Friday, April 24, 2015, James Nord notifications@github.com wrote:

In plugin/src/main/java/hudson/plugins/swarm/PluginImpl.java
#26 (comment):

@@ -54,9 +57,28 @@ public void doCreateSlave(StaplerRequest req, StaplerResponse rsp, @QueryParamet
nodeProperties = Lists.newArrayList(new ToolLocationNodeProperty(parsedToolLocations));
}

  •        // try to make the name unique. Swarm clients are often replicated VMs, and they may have the same name.

  •        if (jenkins.getNode(name) != null) {

  •            name = name + '-' + req.getRemoteAddr();

  •        if (hash == null && jenkins.getNode(name) != null) {

  •            // this is a legacy client, they won't be able to pick up the new name, so throw them away

  •            // perhaps they can find another master to connect to

  •            rsp.setStatus(SC_CONFLICT);

I think it would be wise to include a plain text response here for the
poor admin that tries to debug this.

—
Reply to this email directly or view it on GitHub
https://github.com/jenkinsci/swarm-plugin/pull/26/files#r29063947.

Sent from my phone

Copy link
Member

Choose a reason for hiding this comment

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

sometimes - esp with JNLP and http (non secured interfaces) it's very nice to see this information in the network capture.

rsp.setContentType("text/plain; UTF-8");
rsp.getWriter().printf(
"A slave called '%s' already exists and legacy clients do not support name disambiguation%n",
name);
return;
}
if (hash != null) {
// try to make the name unique. Swarm clients are often replicated VMs, and they may have the same name.
name = name + '-' + hash;
}
// check for existing connections
{
Node n = jenkins.getNode(name);
if (n != null) {
Computer c = n.toComputer();
if (c != null && c.isOnline()) {
// this is an existing connection, we'll only cause issues if we trample over an online connection

rsp.setStatus(SC_CONFLICT);
Copy link
Member

Choose a reason for hiding this comment

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

ditto some plain text in the response for the admin that tries to debug this.

rsp.setContentType("text/plain; UTF-8");
rsp.getWriter().printf("A slave called '%s' is already created and on-line%n", name);
return;
}
}
}

SwarmSlave slave = new SwarmSlave(name, "Swarm slave from " + req.getRemoteHost() + " : " + description,
Expand All @@ -70,6 +100,16 @@ public void doCreateSlave(StaplerRequest req, StaplerResponse rsp, @QueryParamet
}
jenkins.addNode(slave);
}
rsp.setContentType("text/plain; charset=iso-8859-1");
Properties props = new Properties();
props.put("name", name);
ByteArrayOutputStream bos = new ByteArrayOutputStream();
props.store(bos, "");
byte[] response = bos.toByteArray();
Copy link
Member

Choose a reason for hiding this comment

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

this is a byte array of ISO 8859-1 characters not UTF-8 ones.

rsp.setContentLength(response.length);
Copy link
Member

Choose a reason for hiding this comment

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

may I recommend adding a terminating \r\n (or \n) or perhaps even a swarmName=blah rather than just blah so that it is easier to evolve this in the future if need be without breaking older clients.

Copy link
Member Author

Choose a reason for hiding this comment

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

A fair point

On Friday, April 24, 2015, James Nord notifications@github.com wrote:

In plugin/src/main/java/hudson/plugins/swarm/PluginImpl.java
#26 (comment):

@@ -70,6 +92,12 @@ public void doCreateSlave(StaplerRequest req, StaplerResponse rsp, @QueryParamet
}
jenkins.addNode(slave);
}

  •        rsp.setContentType("text/plain; UTF-8");

  •        byte[] response = name.getBytes("UTF-8");

  •        rsp.setContentLength(response.length);

may I recommend adding a terminating \r\n (or \n) or perhaps even a
swarmName=blah rather than just blah so that it is easier to evolve this
in the future if need be without breaking older clients.

—
Reply to this email directly or view it on GitHub
https://github.com/jenkinsci/swarm-plugin/pull/26/files#r29064189.

Sent from my phone

ServletOutputStream outputStream = rsp.getOutputStream();
outputStream.write(response);
Copy link
Member

Choose a reason for hiding this comment

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

and its sent with charset UTF-8 here.

outputStream.flush();
} catch (FormException e) {
e.printStackTrace();
}
Expand Down