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

Cache permanent redirects #3389

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

HebaruSan
Copy link
Member

Motivation

This is a follow-up to #3387. Passing the token after a redirect solved the error spam, but the Inflator will still attempt to retrieve out of date URLs even after the server tells us it is out of date.

Changes

Now we save HTTP redirects that we receive in a Dictionary and use them to skip the old URL if it is needed again. This will somewhat reduce our GitHub API usage, leaving more slack to index more mods.

@HebaruSan HebaruSan added Enhancement New features or functionality Easy This is easy to fix Core (ckan.dll) Issues affecting the core part of CKAN Pull request Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN labels May 31, 2021
@HebaruSan HebaruSan requested a review from DasSkelett May 31, 2021 16:22
@DasSkelett
Copy link
Member

We should make sure to cache only actually permanent redirects, i.e. those returned with a status code of 301 and 308.
For example, we also see some dynamically generated, authenticated temporary redirects for release assets:

1917 [1] WARN CKAN.Net (null) - Redirecting https://github.com/UmbraSpaceIndustries/USI-LS/releases/download/1.4.0/USI-LS_1.4.0.0.zip 
with 302 to 
https://github-releases.githubusercontent.com/34215491/25592580-43ab-11eb-91ba-f787d066b019?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=<IDontKnowIfThisShouldBeSharedPublicly>%2F20210608%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20210608T143049Z&X-Amz-Expires=300&X-Amz-Signature=<IDontKnowIfThisShouldBeSharedPublicly>&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=34215491&response-content-disposition=attachment%3B%20filename%3DUSI-LS_1.4.0.0.zip&response-content-type=application%2Foctet-stream

@DasSkelett
Copy link
Member

The permanentRedirects dict needs to be static so it's shared between all instances of RedirectingTimeoutWebClient, otherwise it's not of much use, because the web clients are very short lived (created and disposed within Download()/DownloadText().

@HebaruSan HebaruSan force-pushed the feature/perm-redirects branch from c8ba80a to 9d086b2 Compare June 8, 2021 15:16
@HebaruSan
Copy link
Member Author

We should make sure to cache only actually permanent redirects, i.e. those returned with a status code of 301 and 308.

The permanentRedirects dict needs to be static so it's shared between all instances of RedirectingTimeoutWebClient

Oof, good catches. Both done in force push.
(I wanted to use the HttpStatusCode.PermanentRedirect enum element, but that isn't defined in our version of .NET, so I used ints instead.)

@DasSkelett
Copy link
Member

I'm not sure if restricting the Authorization header carry over logic to 301s and 308s as well is what we want here, in theory it also applies to other redirects. In practice though, we only get 301s where it's relevant, the only other redirect I see is the 302 from above, which also leads to another host and has separate authorization data baked into the URL.
So I think we're fine, if not, GitHub will tell us loudly :P

@HebaruSan
Copy link
Member Author

So I think we're fine, if not, GitHub will tell us loudly :P

Ohhh. No, let me fix that. I forgot that there was more logic going on here than just the caching...

@HebaruSan HebaruSan force-pushed the feature/perm-redirects branch from 9d086b2 to 442cb62 Compare June 8, 2021 16:06
Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Thanks, looks very good now, and works fine in my hacky testing.

@HebaruSan HebaruSan merged commit af50400 into KSP-CKAN:master Jun 8, 2021
@HebaruSan HebaruSan deleted the feature/perm-redirects branch June 8, 2021 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Easy This is easy to fix Enhancement New features or functionality Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants