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.

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

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

Merged

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Dec 31, 2023

[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 (based on the ETag specification) 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.

JENKINS-72469 has more details.

The community post 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.

Proposed changelog entries

  • Avoid repeated tool downloads from misconfigured HTTP servers.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

N/A

Before the changes are marked as ready-for-merge:

Maintainer checklist

… 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.
@MarkEWaite MarkEWaite added the bug For changelog: Minor bug. Will be listed after features label Dec 31, 2023
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.
core/src/main/java/hudson/FilePath.java Outdated Show resolved Hide resolved
core/src/main/java/hudson/FilePath.java Outdated Show resolved Hide resolved
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)
@MarkEWaite MarkEWaite added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Dec 31, 2023
Copy link
Contributor

@mawinter69 mawinter69 left a comment

Choose a reason for hiding this comment

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

Looks good now

@MarkEWaite MarkEWaite requested a review from a team January 1, 2024 18:03
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jan 1, 2024
@MarkEWaite
Copy link
Contributor Author

Thanks for the approval @timja.

I plan to merge this pull request shortly before I go to sleep tonight so that it will be included in the weekly build tomorrow. I'd like it in at least one weekly before we select the next LTS baseline.

@MarkEWaite MarkEWaite merged commit c8156d4 into jenkinsci:master Jan 2, 2024
17 checks passed
@MarkEWaite MarkEWaite deleted the add-etag-support-to-tool-downloader branch January 2, 2024 03:09
krisstern pushed a commit to krisstern/jenkins that referenced this pull request Jan 8, 2024
… 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback squash-merge-me Unclean or useless commit history, should be merged only with squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants