Skip to content

Commit

Permalink
[JENKINS-72469] Avoid repeated tool downloads from misconfigured HTTP…
Browse files Browse the repository at this point in the history 
… servers (jenkinsci#8814)

* [JENKINS-72469] Avoid repeated tool downloads from misconfigured HTTP servers

The Azul Systems content delivery network stopped providing the
last-modified header in their URL responses.  They only provide the
ETag header.

Add ETag support to the Jenkins FilePath URL download method so that if ETag is
provided, we use the ETag value.  If last-modified is provided and matches, we
continue to honor it as well.

https://issues.jenkins.io/browse/JENKINS-72469 has more details.

https://community.jenkins.io/t/job-stuck-on-unpacking-global-jdk-tool/11272
also includes more details.

Testing done

* Automated test added to FilePathTest for code changes on the controller.
  The automated test confirms that even without a last-modified value,
  the later downloads are skipped if a matching ETag is received.
  The automated test also confirms that download is skipped if OK is
  received with a matching ETag.  No automated test was added to confirm
  download on the agent because that path is not tested by any of the
  other test automation of this class.

* Interactive test with the Azul Systems JDK installer on the controller.
  I created a tool installer for the Azul JDK.  I verified that before
  this change it was downloaded each time the job was run.  I verified
  that after the change it was downloaded only once.

* Interactive test with the Azul Systems JDK installer on an agent.
  I created a tool installer for the Azul JDK.  I verified that before
  this change it was downloaded each time the job was run.  I verified
  that after the change it was downloaded only once.

* Interactive test on the controller with a file download from an NGINX
  web server confirmed that the tool is downloaded once and then later
  runs of the job did not download the file again.

* Use equals instead of contains to check ETag

Don't risk that a substring of an earlier ETag might cause a later
ETag to incorrectly assume it does not need to download a modified
installer.

* Use weak comparison for ETag values

https://httpwg.org/specs/rfc9110.html#field.etag describes weak comparison
cases and notes that content providers may provide weak or strong entity
tags.  Updated code to correctly compare weak and strong entity tags.

Also improves the null checks based on the suggestions from @mawinter69
in jenkinsci#8814 (comment)

* Test comparison of weak and strong validators

* Do not duplicate test args, more readable

* Use better variable names in test

Cover more branches in the equalEtags method as well

* Fix variable declaration order

(cherry picked from commit c8156d4)
  • Loading branch information
@MarkEWaite @krisstern
MarkEWaite authored and krisstern committed Jan 8, 2024
1 parent 8908239 commit 987b147
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 2 deletions.
42 changes: 40 additions & 2 deletions core/src/main/java/hudson/FilePath.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import static hudson.Util.fileToPath;
import static hudson.Util.fixEmpty;
import static hudson.Util.fixEmptyAndTrim;

import com.google.common.annotations.VisibleForTesting;
import com.jcraft.jzlib.GZIPInputStream;
Expand Down Expand Up @@ -962,7 +963,7 @@ public Void invoke(File dir, VirtualChannel channel) throws IOException {
* </ul>
*
* @param archive
* The resource that represents the tgz/zip file. This URL must support the {@code Last-Modified} header.
* The resource that represents the tgz/zip file. This URL must support the {@code Last-Modified} header or the {@code ETag} header.
* (For example, you could use {@link ClassLoader#getResource}.)
* @param listener
* If non-null, a message will be printed to this listener once this method decides to
Expand All @@ -984,12 +985,18 @@ private boolean installIfNecessaryFrom(@NonNull URL archive, @NonNull TaskListen
try {
FilePath timestamp = this.child(".timestamp");
long lastModified = timestamp.lastModified();
// https://httpwg.org/specs/rfc9110.html#field.etag is the ETag specification
// Read previously stored ETag if timestamp is available
String etag = timestamp.exists() ? fixEmptyAndTrim(timestamp.readToString()) : null;
URLConnection con;
try {
con = ProxyConfiguration.open(archive);
if (lastModified != 0) {
con.setIfModifiedSince(lastModified);
}
if (etag != null) {
con.setRequestProperty("If-None-Match", etag);
}
con.connect();
} catch (IOException x) {
if (this.exists()) {
Expand All @@ -1016,7 +1023,7 @@ private boolean installIfNecessaryFrom(@NonNull URL archive, @NonNull TaskListen
return false;
}
}
if (lastModified != 0) {
if (lastModified != 0 || etag != null) {
if (responseCode == HttpURLConnection.HTTP_NOT_MODIFIED) {
return false;
} else if (responseCode != HttpURLConnection.HTTP_OK) {
Expand All @@ -1027,8 +1034,12 @@ private boolean installIfNecessaryFrom(@NonNull URL archive, @NonNull TaskListen
}

long sourceTimestamp = con.getLastModified();
String resultEtag = fixEmptyAndTrim(con.getHeaderField("ETag"));

if (this.exists()) {
if (equalETags(etag, resultEtag)) {
return false; // already up to date
}
if (lastModified != 0 && sourceTimestamp == lastModified)
return false; // already up to date
this.deleteContents();
Expand All @@ -1042,6 +1053,10 @@ private boolean installIfNecessaryFrom(@NonNull URL archive, @NonNull TaskListen
// First try to download from the agent machine.
try {
act(new Unpack(archive));
if (resultEtag != null && !equalETags(etag, resultEtag)) {
/* Store the ETag value in the timestamp file for later use */
timestamp.write(resultEtag, "UTF-8");
}
timestamp.touch(sourceTimestamp);
return true;
} catch (IOException x) {
Expand All @@ -1061,13 +1076,36 @@ private boolean installIfNecessaryFrom(@NonNull URL archive, @NonNull TaskListen
throw new IOException(String.format("Failed to unpack %s (%d bytes read of total %d)",
archive, cis.getByteCount(), con.getContentLength()), e);
}
if (resultEtag != null && !equalETags(etag, resultEtag)) {
/* Store the ETag value in the timestamp file for later use */
timestamp.write(resultEtag, "UTF-8");
}
timestamp.touch(sourceTimestamp);
return true;
} catch (IOException e) {
throw new IOException("Failed to install " + archive + " to " + remote, e);
}
}

/* Return true if etag1 equals etag2 as defined by the etag specification
https://httpwg.org/specs/rfc9110.html#field.etag
*/
private boolean equalETags(String etag1, String etag2) {
if (etag1 == null || etag2 == null) {
return false;
}
if (etag1.equals(etag2)) {
return true;
}
/* Weak tags are identified by leading characters "W/" as a marker */
/* Weak tag marker must not be considered in tag comparison.
This implements the weak comparison in the specification at
https://httpwg.org/specs/rfc9110.html#field.etag */
String opaqueTag1 = etag1.startsWith("W/") ? etag1.substring(2) : etag1;
String opaqueTag2 = etag2.startsWith("W/") ? etag2.substring(2) : etag2;
return opaqueTag1.equals(opaqueTag2);
}

// this reads from arbitrary URL
private static final class Unpack extends MasterToSlaveFileCallable<Void> {
private final URL archive;
Expand Down
71 changes: 71 additions & 0 deletions core/src/test/java/hudson/FilePathTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,77 @@ private static void assertValidateAntFileMask(String expected, FilePath d, Strin
assertTrue(d.installIfNecessaryFrom(url, null, message));
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedStrongValidator() throws Exception {
String strongValidator = "\"An-ETag-strong-validator\"";
installIfNecessaryWithoutLastModified(strongValidator);
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedStrongValidatorNoQuotes() throws Exception {
// This ETag is a violation of the spec at https://httpwg.org/specs/rfc9110.html#field.etag
// However, better safe to handle without quotes as well, just in case
String strongValidator = "An-ETag-strong-validator-without-quotes";
installIfNecessaryWithoutLastModified(strongValidator);
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedWeakValidator() throws Exception {
String weakValidator = "W/\"An-ETag-weak-validator\"";
installIfNecessaryWithoutLastModified(weakValidator);
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedStrongAndWeakValidators() throws Exception {
String strongValidator = "\"An-ETag-validator\"";
String weakValidator = "W/" + strongValidator;
installIfNecessaryWithoutLastModified(strongValidator, weakValidator);
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedWeakAndStrongValidators() throws Exception {
String strongValidator = "\"An-ETag-validator\"";
String weakValidator = "W/" + strongValidator;
installIfNecessaryWithoutLastModified(weakValidator, strongValidator);
}

private void installIfNecessaryWithoutLastModified(String validator) throws Exception {
installIfNecessaryWithoutLastModified(validator, validator);
}

private void installIfNecessaryWithoutLastModified(String validator, String alternateValidator) throws Exception {
final HttpURLConnection con = mock(HttpURLConnection.class);
// getLastModified == 0 when last-modified header is not returned
when(con.getLastModified()).thenReturn(0L);
// An Etag is provided by Azul CDN without last-modified header
when(con.getHeaderField("ETag")).thenReturn(validator);
when(con.getInputStream()).thenReturn(someZippedContent());

final URL url = someUrlToZipFile(con);

File tmp = temp.getRoot();
final FilePath d = new FilePath(tmp);

/* Initial download expected to occur */
assertTrue(d.installIfNecessaryFrom(url, null, "message if failed first download"));

/* Timestamp last modified == 0 means the header was not provided */
assertThat(d.child(".timestamp").lastModified(), is(0L));

/* Second download should not occur if JENKINS-72469 is fixed and NOT_MODIFIED is returned */
when(con.getResponseCode()).thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED);
when(con.getInputStream()).thenReturn(someZippedContent());
when(con.getHeaderField("ETag")).thenReturn(alternateValidator);
assertFalse(d.installIfNecessaryFrom(url, null, "message if failed second download"));

/* Third download should not occur if JENKINS-72469 is fixed and OK is returned with matching ETag */
/* Unexpected to receive an OK and a matching ETag from a real web server, but check for safety */
when(con.getResponseCode()).thenReturn(HttpURLConnection.HTTP_OK);
when(con.getInputStream()).thenReturn(someZippedContent());
when(con.getHeaderField("ETag")).thenReturn(alternateValidator);
assertFalse(d.installIfNecessaryFrom(url, null, "message if failed third download"));
}

private URL someUrlToZipFile(final URLConnection con) throws IOException {

final URLStreamHandler urlHandler = new URLStreamHandler() {
Expand Down

0 comments on commit 987b147

Please sign in to comment.