Investigate / remove custom downstream changes to PhabricatorClientRateLimit.php
Closed, DeclinedPublic
Actions

Assigned To

Authored By

	Aklapper
	May 14 2024, 11:21 AM

Description

As I've been going through our downstream Phab diff to reduce technical debt and maintenance costs,
https://gitlab.wikimedia.org/repos/phabricator/phabricator/-/blob/wmf/stable/support/startup/PhabricatorClientRateLimit.php has four custom downstream changes from the year 2018 per https://gitlab.wikimedia.org/repos/phabricator/phabricator/-/commits/wmf/stable/support/startup/PhabricatorClientRateLimit.php :

I assume that our more recent use of requestctl makes these our downstream changes rather moot and they could be removed?

Current upstream: https://we.phorge.it/source/phorge/browse/master/support/startup/PhabricatorClientRateLimit.php

Diff:

P62380 PhabricatorClientRateLimit.php Diff

1	diff --git a/phorge-upstream20230624-52be52d429ce/support/startup/PhabricatorClientRateLimit.php b/phab-wmf-20231125/support/startup/PhabricatorClientRateLimit.php
2	index 89a273e..7537334 100644
3	--- a/phorge-upstream20230624-52be52d429ce/support/startup/PhabricatorClientRateLimit.php
4	+++ b/phab-wmf-20231125/support/startup/PhabricatorClientRateLimit.php
5	@@ -3,6 +3,8 @@
6	final class PhabricatorClientRateLimit
7	extends PhabricatorClientLimit {
8
9	+ protected $whitelist = array('87.138.110.76', '198.73.209.241');
10	+
11	protected function getBucketDuration() {
12	return 60;
13	}
14	@@ -13,12 +15,24 @@ final class PhabricatorClientRateLimit
15
16	protected function shouldRejectConnection($score) {
17	$limit = $this->getLimit();
18	+ if ($limit == 0) {
19	+ return false;
20	+ }
21
22	// Reject connections if the average score across all buckets exceeds the
23	// limit.
24	$average_score = $score / $this->getBucketCount();
25
26	- return ($average_score > $limit);
27	+ if ($average_score <= $limit) {
28	+ return false;
29	+ }
30	+
31	+ // don't reject whitelisted connections
32	+ $key = $this->getClientKey();
33	+ if (in_array($key, $this->whitelist)) {
34	+ return false;
35	+ }
36	+ return true;
37	}
38
39	protected function getConnectScore() {
40	@@ -26,16 +40,21 @@ final class PhabricatorClientRateLimit
41	}
42
43	protected function getPenaltyScore() {
44	- return 1;
45	+ return 0;
46	}
47
48	protected function getDisconnectScore(array $request_state) {
49	$score = 1;
50
51	- // If the user was logged in, let them make more requests.
52	+ $key = $this->getClientKey();
53	+ // whitelisted ips get unlimited requests
54	+ if (in_array($key, $this->whitelist)) {
55	+ $score = 0;
56	+ }
57	+
58	if (isset($request_state['viewer'])) {
59	$viewer = $request_state['viewer'];
60	- if ($viewer->isOmnipotent()) {
61	+ if ($viewer->isOmnipotent() \|\| $viewer->getIsSystemAgent()) {
62	// If the viewer was omnipotent, this was an intracluster request or
63	// some other kind of special request, so don't give it any points
64	// toward rate limiting.
65	@@ -44,10 +63,9 @@ final class PhabricatorClientRateLimit
66	// If the viewer was logged in, give them fewer points than if they
67	// were logged out, since this traffic is much more likely to be
68	// legitimate.
69	- $score = 0.25;
70	+ $score = $score / 4;
71	}
72	}
73	-
74	return $score;
75	}
76

Details

	Title	Reference	Author	Source Branch	Dest Branch
	Revert "Revert custom changes to PhabricatorClientRateLimit.php"	repos/phabricator/phabricator!59	aklapper	T364839rerevert	wmf/stable
	Revert custom changes to PhabricatorClientRateLimit.php	repos/phabricator/phabricator!52	aklapper	T364839rateLimit	wmf/stable

Customize query in GitLab

Related Objects

Mentioned In: T367775: Deploy Phabricator/Phorge 2024-06-18
rPHDEPebe3a947ad22: update submodules for 2024-06-18 release
Mentioned Here: T367775: Deploy Phabricator/Phorge 2024-06-18
P62380 PhabricatorClientRateLimit.php Diff
rPHAB11134c1dbca0: Exempt bot users from rate limiting.
rPHAB49ad8413aec8: Whitelist WMF and WMDE office IPs from rate limiting
rPHAB0716d5f927b3: Whitelist connections rather than just zero-rating them
rPHAB640c5beae64f: Don't rate limit when the limit is set to zero.

Event Timeline

Aklapper triaged this task as Low priority.May 14 2024, 11:21 AM

Aklapper created this task.

aklapper opened https://gitlab.wikimedia.org/repos/phabricator/phabricator/-/merge_requests/52

Draft: Revert custom changes to PhabricatorClientRateLimit.php

Aklapper added a project: serviceops.May 27 2024, 11:12 AM

@Jelto Hi, would you have any input or recommendations on this? If not that's fine, I could also remove this custom code and see what may (not) happen. :)

I left a comment in https://gitlab.wikimedia.org/repos/phabricator/phabricator/-/merge_requests/52#note_86778. +1 for removing the custom rate limiting code. I'd try to move any existing extra logic to a single place (requestctl).

Jelto edited projects, added collaboration-services; removed serviceops.Wed, Jun 5, 8:47 AM

LSobanski moved this task from Incoming to Consultation on the collaboration-services board.Mon, Jun 10, 3:35 PM

brennen merged https://gitlab.wikimedia.org/repos/phabricator/phabricator/-/merge_requests/52

Revert custom changes to PhabricatorClientRateLimit.php

Aklapper edited projects, added Phabricator (2024-06-25); removed Patch-For-Review, Phabricator.Mon, Jun 17, 8:19 PM

brennen mentioned this in rPHDEPebe3a947ad22: update submodules for 2024-06-18 release.Tue, Jun 18, 3:03 PM

Aklapper mentioned this in T367775: Deploy Phabricator/Phorge 2024-06-18.Tue, Jun 18, 3:14 PM