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

Long-term rate limits for package uploads. #8063

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/config/dartlang-pub.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ rateLimits:
burst: 3
hourly: 6
daily: 12
weekly: 20 # 20 versions/week
# monthly: 32 # ~ 8 versions/week
# quarterly: 50 # ~ 4 versions/week
# yearly: 100 # ~ 2 versions/week
Comment on lines +79 to +81
Copy link
Member

Choose a reason for hiding this comment

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

I think the problem with monthly, quarterly and yearly is that these limits are so long that we need a mechanism to override them.

If you publish ~50 versions per quarter, then suddenly you can't publish for 6 months.
I don't think that's viable.


I do think that 20 publications per week seems like a reasonable limit. And having to wait 5 days is not an entirely unreasonable ask (in such scenarios, hopefully there are few).

Copy link
Member

Choose a reason for hiding this comment

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

I think a client-side warning or error is probably better:
dart-lang/pub#4392

We just want a soft limit, really.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you publish ~50 versions per quarter, then suddenly you can't publish for 6 months.

The daily, weekly and monthly limit still apply, and it is unlikely that you would trigger only the quarterly and nothing before. However, if you do hit this scenario, your events are stretched out over the quarter. As the window is rolling, your prior events will move out of it, you won't need to wait for the entire quarter to get more quota.

Maybe the events could have an exponential decay in them. E.g. after a month an event may count as only 0.5 event, and as such, the emergency quota gets freed up a bit faster?

- operation: package-published
scope: user
burst: 200
Expand Down
13 changes: 7 additions & 6 deletions app/lib/package/backend.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1085,11 +1085,17 @@ class PackageBackend {
'Package "${newVersion.package}" has no admin email to notify.');
}

// check rate limits before the transaction
final existingVersions = await db
.query<PackageVersion>(ancestorKey: newVersion.packageKey!)
.run()
.toList();

// check long term (over a day) rate limits with version created timestamps
await verifyPackageUploadRateLimit(
agent: agent,
package: newVersion.package,
isNew: isNew,
timestamps: existingVersions.map((v) => v.created!).toList(),
);

final email = createPackageUploadedEmail(
Expand All @@ -1102,11 +1108,6 @@ class PackageBackend {
final outgoingEmail = emailBackend.prepareEntity(email);

Package? package;
final existingVersions = await db
.query<PackageVersion>(ancestorKey: newVersion.packageKey!)
.run()
.toList();

// Add the new package to the repository by storing the tarball and
// inserting metadata to datastore (which happens atomically).
final pv = await withRetryTransaction(db, (tx) async {
Expand Down
85 changes: 63 additions & 22 deletions app/lib/service/rate_limit/rate_limit.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@ import '../../shared/redis_cache.dart';

final _logger = Logger('rate_limit');

/// Verifies if the current package upload has a rate limit and throws
/// if the limit has been exceeded.
/// Verifies if the current package upload has an applicable rate limit
/// and throws if the limit has been exceeded.
Future<void> verifyPackageUploadRateLimit({
required AuthenticatedAgent agent,
required String package,
required bool isNew,
required List<DateTime> timestamps,
}) async {
final packagePublishedOp = AuditLogRecordKind.packagePublished;

await _verifyRateLimit(
rateLimit: _getRateLimit(packagePublishedOp, RateLimitScope.user),
rateLimit:
_getRateLimit(AuditLogRecordKind.packagePublished, RateLimitScope.user),
agentId: agent.agentId,
);

Expand All @@ -42,8 +42,10 @@ Future<void> verifyPackageUploadRateLimit({

// regular package-specific limits
await _verifyRateLimit(
rateLimit: _getRateLimit(packagePublishedOp, RateLimitScope.package),
rateLimit: _getRateLimit(
AuditLogRecordKind.packagePublished, RateLimitScope.package),
package: package,
timestamps: timestamps,
);
}

Expand Down Expand Up @@ -72,6 +74,7 @@ Future<void> _verifyRateLimit({
required RateLimit? rateLimit,
String? package,
String? agentId,
List<DateTime>? timestamps,
}) async {
assert(agentId != null || package != null);
if (agentId == KnownAgents.pubSupport) {
Expand All @@ -81,9 +84,7 @@ Future<void> _verifyRateLimit({
if (rateLimit == null) {
return;
}
if (rateLimit.burst == null &&
rateLimit.hourly == null &&
rateLimit.daily == null) {
if (rateLimit.isEmpty) {
return;
}

Expand Down Expand Up @@ -123,20 +124,34 @@ Future<void> _verifyRateLimit({

final now = clock.now().toUtc();
final windowStart = now.subtract(window);
final relevantEntries = auditEntriesFromLastDay!
.where((e) => e.kind == rateLimit.operation)
.where((e) => e.created!.isAfter(windowStart))
.where((e) => package == null || _containsPackage(e.packages, package))
.where((e) =>
agentId == null ||
e.agent == agentId ||
_containsUserId(e.users, agentId))
.toList();

if (relevantEntries.length >= maxCount) {
final firstTimestamp = relevantEntries

late List<DateTime> relevantTimestamps;
if (timestamps != null) {
relevantTimestamps =
timestamps.where((t) => t.isAfter(windowStart)).toList();
} else {
// Sanity check: most rate limit operations only support rate limit within a day.
// The ones which support longer window must provide their list of timestamps.
if (window.inDays > 1) {
throw ArgumentError(
'Rate limit "${rateLimit.operation}" does not support long-term rules.');
}
relevantTimestamps = auditEntriesFromLastDay!
.where((e) => e.kind == rateLimit.operation)
.where((e) => e.created!.isAfter(windowStart))
.where(
(e) => package == null || _containsPackage(e.packages, package))
.where((e) =>
agentId == null ||
e.agent == agentId ||
_containsUserId(e.users, agentId))
.map((e) => e.created!)
.reduce((a, b) => a.isBefore(b) ? a : b);
.toList();
}

if (relevantTimestamps.length >= maxCount) {
final firstTimestamp =
relevantTimestamps.reduce((a, b) => a.isBefore(b) ? a : b);
await entry.set(firstTimestamp.add(window), window);
throw RateLimitException(
operation: operation,
Expand Down Expand Up @@ -165,6 +180,32 @@ Future<void> _verifyRateLimit({
maxCount: rateLimit.daily,
windowAsText: 'last day',
);

// note: long-term limits are only checked if timestamps are provided
await check(
operation: rateLimit.operation,
window: Duration(days: 7),
maxCount: rateLimit.weekly,
windowAsText: 'last week',
);
await check(
operation: rateLimit.operation,
window: Duration(days: 30),
maxCount: rateLimit.monthly,
windowAsText: 'last month',
);
await check(
operation: rateLimit.operation,
window: Duration(days: 91),
maxCount: rateLimit.quarterly,
windowAsText: 'last 3 months',
);
await check(
operation: rateLimit.operation,
window: Duration(days: 365),
maxCount: rateLimit.yearly,
windowAsText: 'last year',
);
sw.stop();
_logger.info('[rate-limit-verified] Rate limit verified in ${sw.elapsed}');
}
Expand Down
24 changes: 24 additions & 0 deletions app/lib/shared/configuration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -543,16 +543,40 @@ class RateLimit {
/// Maximum number of operations in 24 hours.
final int? daily;

/// Maximum number of operations in 7 days.
final int? weekly;

/// Maximum number of operations in 30 days.
final int? monthly;

/// Maximum number of operations in 91 days.
final int? quarterly;

/// Maximum number of operations in 365 days.
final int? yearly;

RateLimit({
required this.operation,
required this.scope,
this.burst,
this.hourly,
this.daily,
this.weekly,
this.monthly,
this.quarterly,
this.yearly,
});

factory RateLimit.fromJson(Map<String, dynamic> json) =>
_$RateLimitFromJson(json);

Map<String, dynamic> toJson() => _$RateLimitToJson(this);

late final isEmpty = burst == null &&
hourly == null &&
daily == null &&
weekly == null &&
monthly == null &&
quarterly == null &&
yearly == null;
}
20 changes: 19 additions & 1 deletion app/lib/shared/configuration.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading