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
[FIXED JENKINS-26558] Clients should provide a unique ID to be used for ... #26
Changes from 2 commits
ab37bc8
0871658
2132772
7984ef2
640637c
78c8650
b7e1470
ff5dd53
96a1a84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,58 +2,73 @@ | |
|
||
import hudson.remoting.Launcher; | ||
import hudson.remoting.jnlp.Main; | ||
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.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.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, | ||
|
@@ -191,7 +206,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) { | ||
|
@@ -292,7 +307,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()) | ||
+ param("hash", hash) | ||
); | ||
|
||
post.setDoAuthentication(true); | ||
|
||
|
@@ -306,6 +323,16 @@ protected void createSwarmSlave(Candidate target) throws IOException, Interrupte | |
throw new RetryException( | ||
"Failed to create a slave on Jenkins CODE: " + responseCode); | ||
} | ||
String name = post.getResponseBodyAsString(); | ||
if (name == null) { | ||
this.name = options.name; | ||
return; | ||
} | ||
name = name.trim(); | ||
if (name.isEmpty()) { | ||
return; | ||
} | ||
this.name = name; | ||
} | ||
|
||
private String encode(String value) throws UnsupportedEncodingException { | ||
|
@@ -368,6 +395,53 @@ 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'); | ||
} | ||
} | ||
byte[] hardwareAddress = ni.getHardwareAddress(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Sent from my phone There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 getDigestOf(buf.toString()).substring(0, 8); | ||
} | ||
|
||
public static String getDigestOf(String text) { | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apache commons has some nice util classes ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But that would bloat the swarm-client jar file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I assumed that the inclusion of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The client already has |
||
return toHexString(MessageDigest.getInstance("MD5").digest(text.getBytes("UTF-8"))); | ||
} catch (NoSuchAlgorithmException e) { | ||
throw new IllegalStateException("MD5 not installed", e); // impossible according to JLS | ||
} catch (UnsupportedEncodingException e) { | ||
throw new IllegalStateException("UTF-8 not supported", e); // impossible according to JLS | ||
} | ||
} | ||
|
||
public static String toHexString(byte[] bytes) { | ||
return String.format("%0" + (bytes.length * 2) + "x", new BigInteger(1, bytes)); | ||
} | ||
|
||
private static class DefaultTrustManager implements X509TrustManager { | ||
|
||
public void checkClientTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
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.Descriptor.FormException; | ||
|
@@ -10,19 +10,21 @@ | |
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.IOException; | ||
import java.io.Writer; | ||
import java.util.List; | ||
|
||
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. | ||
|
@@ -36,7 +38,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); | ||
|
@@ -54,9 +56,24 @@ 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 When a swarm save disconnects its node is removed. Should be fine On Friday, April 24, 2015, James Nord notifications@github.com wrote:
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Legacy clients will not display the returned text anyway... Might add it On Friday, April 24, 2015, James Nord notifications@github.com wrote:
Sent from my phone There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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 && n.toComputer().isOnline()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// this is an existing connection, we'll only cause issues if we trample over an online connection | ||
rsp.setStatus(SC_CONFLICT); | ||
return; | ||
} | ||
} | ||
|
||
SwarmSlave slave = new SwarmSlave(name, "Swarm slave from " + req.getRemoteHost() + " : " + description, | ||
|
@@ -70,6 +87,12 @@ public void doCreateSlave(StaplerRequest req, StaplerResponse rsp, @QueryParamet | |
} | ||
jenkins.addNode(slave); | ||
} | ||
rsp.setContentType("text/plain; UTF-8"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Properties are written in ISO 8859-1 not UTF-8 |
||
byte[] response = name.getBytes("UTF-8"); | ||
rsp.setContentLength(response.length); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Sent from my phone |
||
ServletOutputStream outputStream = rsp.getOutputStream(); | ||
outputStream.write(response); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potential case issues in whacky locales... supply a defaultLocale.