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

api: acl: bring limit for hackers down to 5 per node #2058

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

gioelecerati
Copy link
Member

What does this pull request do? Explain your changes. (required)

Specific updates (required)

How did you test each of these updates (required)

Does this pull request close any open issues?

Screenshots (optional)

Checklist

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@gioelecerati gioelecerati requested a review from a team as a code owner February 15, 2024 14:51
Copy link

vercel bot commented Feb 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
livepeer-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 15, 2024 3:14pm

@@ -23,7 +23,7 @@ import { isFreeTierUser } from "./helpers";
import { cache } from "../store/cache";

const WEBHOOK_TIMEOUT = 30 * 1000;
const MAX_ALLOWED_VIEWERS_FOR_FREE_TIER = 30;
const MAX_ALLOWED_VIEWERS_FOR_FREE_TIER = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change the variable name to MAX_ALLOWED_VIEWERS_FOR_HACKER_TIER_PER_NODE or comment that it's per-node value. It's not obvious in this context.

@pwilczynskiclearcode
Copy link
Contributor

pwilczynskiclearcode commented Feb 15, 2024

With this implementation lower limit is the only option. The problem is that with scaling up the number of nodes (like for Fishtank) we raise the global viewers limit for the hacker plan. Maybe it's acceptable for now.

Also with current 29 nodes. 29 * 5 = 145 which is quite high and still above our alert's thresholds

@gioelecerati
Copy link
Member Author

With this implementation lower limit is the only option. The problem is that with scaling up the number of nodes (like for Fishtank) we raise the global viewers limit for the hacker plan. Maybe it's acceptable for now.

Also with current 29 nodes. 29 * 5 = 145 which is quite high and still above our alert's thresholds

Yes, until we have a catalyst shared state with the amount of viewers globally, this is the only space we have to move around.
I agree that 145 is high, but consider that playback would need to happen in all regions to reach that number, right?

Copy link
Contributor

@thomshutt thomshutt left a comment

Choose a reason for hiding this comment

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

I think that bringing this down makes sense - hopefully 5 per node is low enough that the system's not usable for people trying to abuse the free tier

@pwilczynskiclearcode
Copy link
Contributor

@gioelecerati

I agree that 145 is high, but consider that playback would need to happen in all regions to reach that number, right?

Right. I guess that rate-limiting is done after geolocation... so if we reach the limit on a node we won't redirect viewer to another node.

@gioelecerati gioelecerati merged commit 550371a into master Feb 15, 2024
13 checks passed
@gioelecerati gioelecerati deleted the gio/acl/limit-change branch February 15, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants