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

fix(site/htaccess) only use Apache mod_rewrite by replacing RedirectMatch by RewriteRule directives #812

Merged

Conversation

dduportal
Copy link
Contributor

Related to jenkins-infra/helpdesk#2649 (comment) and jenkins-infra/helpdesk#4311

After a discussion with @daniel-beck , we realized that some of the issues on the new update center could be solved using smart request distribution on Apache.

The mix of mod_rewrite (RewriteRule) and mod_alias (RedirectMatch ) in the generated htaccess files makes the rules ordering hard to determine as Apache evaluates all rewrite rules and apply the module's own internal priority, and then evaluate the redirect matchings.

We were forced to use RedirectMatch in #790 to ensure the new UC fallback to mirrors.updates.jenkins.io was working as expected without breaking existing redirections.

Using rewrite rules allows more powerful setup such as using internal variables, applying conditions, etc.

This PR:

  • Add test case to cover all current RedirectMatch rules (and the test works obviously)
  • Replaces the RedirectMatch rules by RewriteRule directives (with test still passing)
  • Changing the new UC fallback rule to a RewriteRule which tests the absence of the file specified by the request in order to send to the mirrors. As such, if we copy any file (such as uctest.json or /download/**/*.html) in the Apache document roots, then we can control which files are served from Azure or from the mirrors.

@dduportal dduportal force-pushed the fix/site/htaccess--only-rewriterules branch from 5118169 to 61e5de4 Compare October 18, 2024 14:45
@dduportal dduportal marked this pull request as ready for review October 18, 2024 14:49
@dduportal dduportal changed the title fix(site/htaccess) only use Apache mod_rewrite by replacing RedirectMatch by RewriteRule driectives fix(site/htaccess) only use Apache mod_rewrite by replacing RedirectMatch by RewriteRule directives Oct 21, 2024
@dduportal
Copy link
Contributor Author

@daniel-beck shall we test this change on the current production (pkg VM) before reviewing/merging this PR? Or do you think it's enough to go as it?

@daniel-beck
Copy link
Contributor

@dduportal Up to you. I kinda accepted a mess in the wrapper files in this repo already 😉

@daniel-beck daniel-beck added the wrapper PR affects wrapper behavior and will be effective immediately when merged. label Oct 21, 2024
@dduportal
Copy link
Contributor Author

@dduportal Up to you. I kinda accepted a mess in the wrapper files in this repo already 😉

Better safe than sorry (on this service): I'll check with @smerle33 and we'll test a manually modified .htaccess before merging this PR then.

Thanks @daniel-beck !

@dduportal
Copy link
Contributor Author

Tested in production (pkg VM) with the help of @smerle33:

diff -u .htaccess .htaccess.bkp 
--- .htaccess   2024-10-21 15:18:30.760592178 +0000
+++ .htaccess.bkp       2024-10-21 15:13:34.189052454 +0000
@@ -511,9 +511,9 @@
 # download/* directories contain virtual URL spaces for redirecting download traffic to mirrors.
 
 # 'latest' need special handling here since they're not getting mirrored properly to get.jenkins.io
-RewriteRule ^download/war/latest/jenkins[.]war$ https://updates.jenkins.io/latest/jenkins.war [NC,L,R=302]
-RewriteRule ^download/plugins/(.*)/latest/(.+)[.]hpi$ https://updates.jenkins.io/latest/$2.hpi [NC,L,R=302]
+RedirectMatch 302 /download/war/latest/jenkins[.]war$ https://updates.jenkins.io/latest/jenkins.war
+RedirectMatch 302 /download/plugins/(.*)/latest/(.+)[.]hpi$ https://updates.jenkins.io/latest/$2.hpi
 
-RewriteRule ^download/war/([0-9]+[.][0-9]+[.][0-9]+/jenkins)[.]war$ https://get.jenkins.io/war-stable/$1.war [NC,L,R=302]
-RewriteRule ^download/war/(.+)[.]war$ https://get.jenkins.io/war/$1.war [NC,L,R=302]
-RewriteRule ^download/plugins/(.+)[.]hpi$ https://get.jenkins.io/plugins/$1.hpi [NC,L,R=302]
+RedirectMatch 302 /download/war/([0-9]+[.][0-9]+[.][0-9]+/jenkins)[.]war$ https://get.jenkins.io/war-stable/$1.war
+RedirectMatch 302 /download/war/(.+)[.]war$ https://get.jenkins.io/war/$1.war
+RedirectMatch 302 /download/plugins/(.+)[.]hpi$ https://get.jenkins.io/plugins/$1.hpi

We triggered our monitoring + did a few manual curl request: LGT(Us). We're proceeding to merge and deploy

@dduportal dduportal merged commit 333f7c8 into jenkins-infra:master Oct 21, 2024
2 checks passed
@dduportal dduportal deleted the fix/site/htaccess--only-rewriterules branch October 21, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wrapper PR affects wrapper behavior and will be effective immediately when merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants