From 8908239882c07aeb206c7f51aa3e45152ca98e7f Mon Sep 17 00:00:00 2001 From: Carroll Chiou Date: Wed, 29 Nov 2023 02:02:20 -0700 Subject: [PATCH 1/3] [JENKINS-71737] fix redirect when submitting cloud changes (#8505) Co-authored-by: Alexander Brandes (cherry picked from commit ec27a074c9bc3a7b72e2c6c373d7c5a1ec85ff11) --- core/src/main/java/hudson/slaves/Cloud.java | 3 ++- core/src/main/java/jenkins/agents/CloudSet.java | 1 - core/src/main/resources/hudson/slaves/Cloud/configure.jelly | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/slaves/Cloud.java b/core/src/main/java/hudson/slaves/Cloud.java index 6ec3ed3b489d..76cf075c2634 100644 --- a/core/src/main/java/hudson/slaves/Cloud.java +++ b/core/src/main/java/hudson/slaves/Cloud.java @@ -331,7 +331,8 @@ public HttpResponse doConfigSubmit(StaplerRequest req, StaplerResponse rsp) thro j.clouds.replace(this, result); j.save(); // take the user back to the cloud top page. - return FormApply.success("."); + return FormApply.success("../" + result.name + '/'); + } public Cloud reconfigure(@NonNull final StaplerRequest req, JSONObject form) throws Descriptor.FormException { diff --git a/core/src/main/java/jenkins/agents/CloudSet.java b/core/src/main/java/jenkins/agents/CloudSet.java index 00a27b04693d..e69a3f9add3c 100644 --- a/core/src/main/java/jenkins/agents/CloudSet.java +++ b/core/src/main/java/jenkins/agents/CloudSet.java @@ -231,7 +231,6 @@ private void handleNewCloudPage(Descriptor descriptor, String name, Stapl checkName(name); JSONObject formData = req.getSubmittedForm(); formData.put("name", name); - formData.put("cloudName", name); // ec2 uses that field name formData.remove("mode"); // Cloud descriptors won't have this field. req.setAttribute("instance", formData); req.setAttribute("descriptor", descriptor); diff --git a/core/src/main/resources/hudson/slaves/Cloud/configure.jelly b/core/src/main/resources/hudson/slaves/Cloud/configure.jelly index 3e122734d601..f7479a743176 100644 --- a/core/src/main/resources/hudson/slaves/Cloud/configure.jelly +++ b/core/src/main/resources/hudson/slaves/Cloud/configure.jelly @@ -40,7 +40,6 @@ THE SOFTWARE. - From 987b147ba2ec0ddb635bb9c862695426b540d06e Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Mon, 1 Jan 2024 20:09:42 -0700 Subject: [PATCH 2/3] [JENKINS-72469] Avoid repeated tool downloads from misconfigured HTTP servers (#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 https://github.com/jenkinsci/jenkins/pull/8814#discussion_r1438909824 * 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 c8156d41f2e6abf52b41669287e9ab771080b8e4) --- core/src/main/java/hudson/FilePath.java | 42 +++++++++++- core/src/test/java/hudson/FilePathTest.java | 71 +++++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index cdc057ef9c54..1da338d6e549 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -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; @@ -962,7 +963,7 @@ public Void invoke(File dir, VirtualChannel channel) throws IOException { * * * @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 @@ -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()) { @@ -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) { @@ -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(); @@ -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) { @@ -1061,6 +1076,10 @@ 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) { @@ -1068,6 +1087,25 @@ private boolean installIfNecessaryFrom(@NonNull URL archive, @NonNull TaskListen } } + /* 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 { private final URL archive; diff --git a/core/src/test/java/hudson/FilePathTest.java b/core/src/test/java/hudson/FilePathTest.java index fe152a25ba49..54d37c2b3dcd 100644 --- a/core/src/test/java/hudson/FilePathTest.java +++ b/core/src/test/java/hudson/FilePathTest.java @@ -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() { From 914f0d46ebd92ab718b0d657a7dd241fc4e029d4 Mon Sep 17 00:00:00 2001 From: Alexander Brandes Date: Sun, 7 Jan 2024 13:42:29 +0100 Subject: [PATCH 3/3] Increase MavenTest#sensitiveParameters() timeout to 500s (#8840) Infrastructure issue that we don't want to allow to delay the release candidate of 2.426.3. https://github.com/jenkins-infra/helpdesk/issues/3890 is the issue (cherry picked from commit 3dbbf2689489c620ef2adeb82af8deaf6fd41cf2) --- test/src/test/java/hudson/tasks/MavenTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/src/test/java/hudson/tasks/MavenTest.java b/test/src/test/java/hudson/tasks/MavenTest.java index bd366adce71d..dbf745b672d9 100644 --- a/test/src/test/java/hudson/tasks/MavenTest.java +++ b/test/src/test/java/hudson/tasks/MavenTest.java @@ -66,6 +66,7 @@ import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.ToolInstallations; +import org.jvnet.hudson.test.recipes.WithTimeout; import org.kohsuke.stapler.jelly.JellyFacet; /** @@ -190,7 +191,7 @@ private void verify() throws Exception { assertNotNull(isp.installers.get(MavenInstaller.class)); } - @Test public void sensitiveParameters() throws Exception { + @Test @WithTimeout(500) public void sensitiveParameters() throws Exception { FreeStyleProject project = j.createFreeStyleProject(); ParametersDefinitionProperty pdb = new ParametersDefinitionProperty( new StringParameterDefinition("string", "defaultValue", "string description"),