Update GlotPress `export-translations` requests to avoid rate limiting #361

mokagio · 2022-05-04T06:54:24Z

~~Left to do: Make the "Error downloading locale..." print the URL used for the download, to make it easier to triage.~~ See #362.

Starting yesterday, both I and @AliSoftware have been experiencing rate-liming when trying to download translations for the iOS app via this tooling:

[05:56:38]: -------------------------------------------------------
[05:56:38]: --- Step: ios_download_strings_files_from_glotpress ---
[05:56:38]: -------------------------------------------------------
[05:56:38]: Downloading translations for 'ar' from GlotPress (ar) [{:status=>"current"}]...
[05:56:39]: Error downloading locale `ar` — 429 Too Many Requests
[05:56:39]: Downloading translations for 'bg' from GlotPress (bg) [{:status=>"current"}]...
[05:56:40]: Error downloading locale `bg` — 429 Too Many Requests
...

I reached out in the WordPress.org Slack and the issue was identified as genuine rate-limiting applied to our requests (I still don't know why it started only recently, though).

@dd32 suggested in the WordPress.org Slack to:

Set the user agent

Fix the URI you’re requesting, you need to request …./export-translations/?… otherwise you’re doubling the requests needed (It doesn’t include a trailing slash at present)

Check if your SSL dependancies are up-to-date, older SSL libraries might trigger some new checks

This PR address the first two points. Regarding SSL, we don't have any specific dependency, so I'm guessing it's all up to the URI.open implementation from the Ruby runtime on which the tooling runs. We try to keep those fairly up to date. Our major apps are all on Ruby 2.7.4 (not the latest, but fairly recent still)

This will avoid doubling up each request we make. More details at https://wordpress.slack.com/archives/C02RP50LK/p1651640726217119?thread_ts=1651607938.132789&cid=C02RP50LK

This way, people looking at the request logs (and machines, too, I hope) will know it is a legit source that's making the requests. The value originally suggested was `WordPress iOS App Release Packager; https://apps.wordpress.com/ ` but I chose a more generic one because this tooling is used for more than WordPress alone. See https://wordpress.slack.com/archives/C02RP50LK/p1651640726217119?thread_ts=1651607938.132789&cid=C02RP50LK

This will avoid doubling up each request we make. More details at https://wordpress.slack.com/archives/C02RP50LK/p1651640726217119?thread_ts=1651607938.132789&cid=C02RP50LK

…otPress This way, people looking at the request logs (and machines, too, I hope) will know it is a legit source that's making the requests. The value originally suggested was `WordPress iOS App Release Packager; https://apps.wordpress.com/ ` but I chose a more generic one because this tooling is used for more than WordPress alone. See https://wordpress.slack.com/archives/C02RP50LK/p1651640726217119?thread_ts=1651607938.132789&cid=C02RP50LK

mokagio

While working and reviewing the code, the fact that the logic to build the URL is duplicated in several places clearly stood out to me. I decided not to refactor it to keep this fix focused and fast to review, given this is currently a blocking issue for at least the WordPress iOS release automation.

mokagio · 2022-05-04T06:55:52Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/user_agent.rb

+module Fastlane
+  module Wpmreleasetoolkit
+    USER_AGENT = 'Automattic App Release Automator; https://github.com/wordpress-mobile/release-toolkit/'.freeze
+  end
+end


I'm not sure whether this is the best way to define a constant scoped to the whole toolkit codebase. I copied this approach from version.rb.

mokagio · 2022-05-04T06:57:27Z

spec/ios_l10n_helper_spec.rb

+    it 'sets a predefined User Agent so GlotPress will not rate-limit us' do
+      # Arrange
+      stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations/")
+             .with(
+               query: { format: 'strings' },
+               # Note that the syntax below merely checks that the given headers are present in the request,
+               # it does not restrict the request to have only those headers.
+               #
+               # See:
+               # - https://github.com/bblimke/webmock/tree/33d8810c2828fc17010e15cc3f21ad2c726a966f#matching-requests
+               # - https://github.com/bblimke/webmock/issues/276#issuecomment-28625436
+               headers: { 'User-Agent' => 'Automattic App Release Automator; https://github.com/wordpress-mobile/release-toolkit/' }
+             ).to_return(body: 'content')
+      dest = StringIO.new
+      # Act
+      described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
+      # Assert
+      expect(stub).to have_been_made.once
+      expect(dest.string).to eq('content')
+    end


Nitpick: The android test has this example at the bottom, while here is at the top. I think we can live with that...

mokagio · 2022-05-04T07:09:59Z

@AliSoftware this is still a draft, but I'd love to hear your thoughts on it. Thanks!

I think the approach is on the right track. However... I tested it locally from WordPress iOS (using path: 'PATH TO MY RELEASE TOOLKIT') in the Pluginfile and I still got 429s 🤔 But that might be due to my IP being in a block list now, or GlotPress not knowing it should allow our User Agent.

AliSoftware · 2022-05-04T11:42:47Z

spec/android_localize_helper_spec.rb

+               # Note that the syntax below merely checks that the given headers are present in the request,
+               # it does not restrict the request to have only those headers.


AliSoftware · 2022-05-04T12:27:26Z

@mokagio Decided to go ahead and merge it because I need to do a hotfix (that has already been delayed a bit in last 2 days so I don't want to delay it more) which means that I'll also need to do a new beta (for Android versionCode technical reasons) before I can submit the hotfix to PlayStore alongside with it… so I'm hoping to point the release-toolkit to trunk to include that fix while I try to do the beta. Hopefully I'll avoid the 429 with that fix of yours, and if not that would have been a good excuse to test it anyway.

That being said I'm indeed not sure that's enough for the server to not rate-limit us. Sure it will divide the requests by 2 by avoiding the intermediate redirect, but from my initial tests setting the User-Agent was still not reducing the rate limiting. So we'll still have to do some follow-up with folks who handle the server side of this.

Some options I can imagine:

Quickfix / Temporary fix to land something asap: ask them to whitelist our special User-Agent for the time being, until they have time to implement a better and stronger solution (see below), to at least unblock us for the next betas to come (otherwise that will really become a big problem real quick…).
Ask them to lift the rate-limiting if we use an authenticated request using a token that they would whitelist on their side. That way they'd still rate-limit malicious actors against attacks but still allow trusted actors like us to do the downloads
Ask them to implement the bulk-download logic so that we can download a zip of all the locales at once.
- That would require work from both sides (changing the implementation logic of our toolkit a bit more than just adding an auth token to the request, for example) so will take a tad more time to make something land, but if they agree to lift the rate limiting at least for our User-Agent as a temporary patch in the meantime, that could be acceptable.
- That being said, while bulk-download would divide our number of requests by 30+ (the number of locales we download individually nowadays) we could still hit the rate-limit in the case where e.g. our linter notice some issues in the downloaded translations and we have to fix them in GlotPress and redownload them soon after. Way less likely to hit rate-limit in those cases as it won't be as many requests and as close to one another than currently, but still not ideal to have the mere possibility that betas and final builds would be blocked by possibility of that odd occurrence.

My main concern with all that is timing. We really have to find a solution to stop having those 429 soon, because we will definitively need them for the next betas we do, be it the one for localization testing next Wednesday… or more importantly, the final build at the end of the sprint.

In hope to limit the risk of "429 Too Many Requests" errors when downloading the translations See wordpress-mobile/release-toolkit#361

mokagio · 2022-05-05T07:16:35Z

So we'll still have to do some follow-up with folks who handle the server side of this.

For reference: https://make.wordpress.org/systems/2022/05/05/mobile-release-tools-unable-to-fetch-from-glotpress/

Add CHANGELOG entries for #361 and #362

mokagio added 5 commits May 4, 2022 16:43

Add trailing slash to GlotPress iOS export-translations URL

e40bf5d

This will avoid doubling up each request we make. More details at https://wordpress.slack.com/archives/C02RP50LK/p1651640726217119?thread_ts=1651607938.132789&cid=C02RP50LK

Add trailing slash to GlotPress Android export-translations URL

449159d

This will avoid doubling up each request we make. More details at https://wordpress.slack.com/archives/C02RP50LK/p1651640726217119?thread_ts=1651607938.132789&cid=C02RP50LK

Extract shared User Agent value in constant

1995bdd

mokagio commented May 4, 2022

View reviewed changes

mokagio requested a review from AliSoftware May 4, 2022 07:10

mokagio mentioned this pull request May 4, 2022

Print URLs that failed to download from GlotPress #362

Merged

mokagio marked this pull request as ready for review May 4, 2022 10:20

mokagio self-assigned this May 4, 2022

mokagio mentioned this pull request May 4, 2022

Add duplicate keys detection to ios_lint_localization action – Optional, but active by default #360

Merged

AliSoftware approved these changes May 4, 2022

View reviewed changes

AliSoftware merged commit ee92ff1 into trunk May 4, 2022

AliSoftware deleted the comply-with-glotpress-conventions-for-localization-download branch May 4, 2022 11:43

AliSoftware added a commit to wordpress-mobile/WordPress-Android that referenced this pull request May 4, 2022

Point release-toolkit to trunk

e7b1a4f

In hope to limit the risk of "429 Too Many Requests" errors when downloading the translations See wordpress-mobile/release-toolkit#361

AliSoftware mentioned this pull request May 4, 2022

Merge release/19.8 — 19.8-rc-2 + 19.7.1 hotfix + Release Notes wordpress-mobile/WordPress-Android#16476

Merged

mokagio mentioned this pull request May 6, 2022

Merge changes from beta 19.8.0.1 wordpress-mobile/WordPress-iOS#18526

Merged

3 tasks

AliSoftware added a commit that referenced this pull request May 6, 2022

Add CHANGELOG entries for #361 and #362

b391d39

AliSoftware added a commit that referenced this pull request May 9, 2022

Merge pull request #363 from wordpress-mobile/changelog/361-362

329effd

Add CHANGELOG entries for #361 and #362

This was referenced May 12, 2022

Merge release/19.8 (19.8.0.2 beta) wordpress-mobile/WordPress-iOS#18584

Merged

Merge finalized release/19.8 wordpress-mobile/WordPress-iOS#18612

Merged

mokagio mentioned this pull request May 26, 2022

Add WordPress and Jetpack Installable Builds wordpress-mobile/WordPress-Android#16642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update GlotPress `export-translations` requests to avoid rate limiting #361

Update GlotPress `export-translations` requests to avoid rate limiting #361

mokagio commented May 4, 2022 •

edited

Loading

mokagio left a comment

mokagio May 4, 2022

mokagio May 4, 2022

mokagio commented May 4, 2022 •

edited

Loading

AliSoftware May 4, 2022

AliSoftware commented May 4, 2022 •

edited

Loading

mokagio commented May 5, 2022

		# Note that the syntax below merely checks that the given headers are present in the request,
		# it does not restrict the request to have only those headers.

Update GlotPress export-translations requests to avoid rate limiting #361

Update GlotPress export-translations requests to avoid rate limiting #361

Conversation

mokagio commented May 4, 2022 • edited Loading

mokagio left a comment

Choose a reason for hiding this comment

mokagio May 4, 2022

Choose a reason for hiding this comment

mokagio May 4, 2022

Choose a reason for hiding this comment

mokagio commented May 4, 2022 • edited Loading

AliSoftware May 4, 2022

Choose a reason for hiding this comment

AliSoftware commented May 4, 2022 • edited Loading

mokagio commented May 5, 2022

Update GlotPress `export-translations` requests to avoid rate limiting #361

Update GlotPress `export-translations` requests to avoid rate limiting #361

mokagio commented May 4, 2022 •

edited

Loading

mokagio commented May 4, 2022 •

edited

Loading

AliSoftware commented May 4, 2022 •

edited

Loading