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

chore(api, shared, app-generic): Convert API rate limit FF to be LaunchDarkly compatible #4857

Merged
merged 8 commits into from
Nov 17, 2023

Conversation

rifont
Copy link
Collaborator

@rifont rifont commented Nov 16, 2023

What change does this PR introduce?

  • Convert API rate limit flag from system-critical to feature-flag
  • Rename IS_REQUEST_RATE_LIMITING_ENABLED -> IS_API_RATE_LIMITING_ENABLED for naming consistency with features under implementation

Why was this change needed?

  • The feature flag was not compatible with LaunchDarkly and could not be resolved at runtime. This change enables rate limiting to be rolled out gradually through strategies such as A/B.

Other information (Screenshots)

N/A

Copy link

linear bot commented Nov 16, 2023

NV-3061 🏎️ Rate Limiting NestJS Guard

What?

Create the NestJS guard which is attached to the global App, Controllers and implements the TokenBucketRateLimit use case from NV-3060

  • add attachment capability for Methods
  • add rate limiting for IP addresses
  • add rate limiting headers to responses

Why? (Context

NestJS guards provide a capability to block traffic from the services it protects. We need to be able to protect specific resources from high traffic to enable service resiliency and reliability.

Definition of Done

  • Unit test on mock endpoints for both single & multiple resource category by injecting a mock in-memory Redis implementation. Verify that a correct rate limit indicator is returned for each case
  • E2E test on mock endpoints for single/multiple resource category
  • Rate limit headers present on all responses

@@ -25,7 +25,7 @@ REDIS_CACHE_FAMILY=
REDIS_CACHE_KEY_PREFIX=
REDIS_CACHE_ENABLE_AUTOPIPELINING=true

IS_REQUEST_RATE_LIMITING_ENABLED=false
IS_API_RATE_LIMITING_ENABLED=false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"API Rate Limiting" aligns naming with the use-cases under development. If we decide to add Web Socket rate limiting or other types, we can then toggle these features independently.

@rifont rifont self-assigned this Nov 16, 2023
…//github.com/novuhq/novu into nv-3061-convert-rate-limit-ff-for-launchdarkly
@@ -0,0 +1,22 @@
import { Injectable } from '@nestjs/common';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file was also renamed from get-is-request-rate-limiting-enabled -> get-is-api-rate-limiting-enabled

Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

looks good to me 🎉

@rifont rifont merged commit e8fbc29 into next Nov 17, 2023
21 of 23 checks passed
@rifont rifont deleted the nv-3061-convert-rate-limit-ff-for-launchdarkly branch November 17, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants