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

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

Merged
merged 7 commits into from
Jan 2, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9922cfd
[JENKINS-72469] Avoid repeated tool downloads from misconfigured HTTP…
MarkEWaite Dec 31, 2023
cd0487e
Use equals instead of contains to check ETag
MarkEWaite Dec 31, 2023
df05da5
Use weak comparison for ETag values
MarkEWaite Dec 31, 2023
cc50570
Test comparison of weak and strong validators
MarkEWaite Dec 31, 2023
b068176
Do not duplicate test args, more readable
MarkEWaite Jan 1, 2024
056ec5d
Use better variable names in test
MarkEWaite Jan 1, 2024
aa3578d
Fix variable declaration order
MarkEWaite Jan 1, 2024
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
23 changes: 21 additions & 2 deletions core/src/main/java/hudson/FilePath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,7 +962,7 @@
* </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 +984,16 @@
try {
FilePath timestamp = this.child(".timestamp");
long lastModified = timestamp.lastModified();
String etag = timestamp.exists() ? timestamp.readToString().replace("\"", "") : null;
MarkEWaite marked this conversation as resolved.
Show resolved Hide resolved
URLConnection con;
try {
con = ProxyConfiguration.open(archive);
if (lastModified != 0) {
con.setIfModifiedSince(lastModified);
}
if (etag != null && !etag.isEmpty()) {
con.setRequestProperty("If-None-Match", etag);
}
con.connect();
} catch (IOException x) {
if (this.exists()) {
Expand All @@ -1016,7 +1020,7 @@
return false;
}
}
if (lastModified != 0) {
if (lastModified != 0 || (etag != null && !etag.isEmpty())) {

Check warning on line 1023 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 1023 is only partially covered, one branch is missing
if (responseCode == HttpURLConnection.HTTP_NOT_MODIFIED) {
return false;
} else if (responseCode != HttpURLConnection.HTTP_OK) {
Expand All @@ -1027,8 +1031,15 @@
}

long sourceTimestamp = con.getLastModified();
String resultEtag = con.getHeaderField("ETag");
MarkEWaite marked this conversation as resolved.
Show resolved Hide resolved
if (resultEtag != null && !resultEtag.isEmpty()) {

Check warning on line 1035 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 1035 is only partially covered, one branch is missing
resultEtag = resultEtag.replace("\"", "");
MarkEWaite marked this conversation as resolved.
Show resolved Hide resolved
}

if (this.exists()) {
if (resultEtag != null && !resultEtag.isEmpty() && etag != null && !etag.isEmpty() && resultEtag.equals(etag)) {

Check warning on line 1040 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 1040 is only partially covered, 3 branches are missing
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 @@
// First try to download from the agent machine.
try {
act(new Unpack(archive));
if (resultEtag != null && !resultEtag.isEmpty() && !resultEtag.equals(etag)) {
/* Store the ETag value in the timestamp file for later use */
timestamp.write(resultEtag, "UTF-8");

Check warning on line 1058 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 1056-1058 are not covered by tests
}
timestamp.touch(sourceTimestamp);
return true;
} catch (IOException x) {
Expand All @@ -1061,6 +1076,10 @@
throw new IOException(String.format("Failed to unpack %s (%d bytes read of total %d)",
archive, cis.getByteCount(), con.getContentLength()), e);
}
if (resultEtag != null && !resultEtag.isEmpty() && !resultEtag.equals(etag)) {

Check warning on line 1079 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 1079 is only partially covered, 2 branches are missing
/* Store the ETag value in the timestamp file for later use */
timestamp.write(resultEtag, "UTF-8");
}
timestamp.touch(sourceTimestamp);
return true;
} catch (IOException e) {
Expand Down
32 changes: 32 additions & 0 deletions core/src/test/java/hudson/FilePathTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,38 @@ private static void assertValidateAntFileMask(String expected, FilePath d, Strin
assertTrue(d.installIfNecessaryFrom(url, null, message));
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModified() 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("An-opaque-ETag-string");
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());
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());
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