-
Notifications
You must be signed in to change notification settings - Fork 258
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
feat: protect pro tier projects and our services #1445
feat: protect pro tier projects and our services #1445
Conversation
While implementing the ability to allow pro users to start project above the soft limit I could not understand why the tests kept failing. Eventually found is was because gateway thought these were basic users. This happened because our mock auth only tracked scopes and not the actual tiers of users it managed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I left one comment about observability, but it's not blocking.
let has_capacity = if current_container_count < self.cch_container_limit { | ||
true | ||
} else if current_container_count < self.soft_container_limit { | ||
!is_cch_project | ||
} else if current_container_count < self.hard_container_limit { | ||
matches!(account_tier, AccountTier::Pro) | ||
} else { | ||
false | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add tracing events with the count if we are over any of the limits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly not sure. Because of the ambulance event (in the other PR) we can already setup a honeycomb alert. So I want to add a warn!
in this function but don't see how it will be useful considering the ambulance event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it's likely overkill. 👍
Description of change
Prevent the creation of new projects or the starting of idle projects that will overload our servers
How has this been tested? (if applicable)
Updating the tests