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

ratelimits: Check at NewOrder and SpendOnly later #7669

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

beautifulentropy
Copy link
Member

@beautifulentropy beautifulentropy commented Aug 15, 2024

  • Check CertificatesPerDomain at newOrder and spend at Finalize time.
  • Check CertificatesPerAccountPerDomain at newOrder and spend at Finalize time.
  • Check CertificatesPerFQDNSet at newOrder and spend at Finalize time.
  • Fix a bug inFailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction() which results in failed authorizations being spent for the exact FQDN, not the eTLD+1.
  • Remove redundant "max names" check at transaction construction time
  • Enable key-value rate limits in the RA

@beautifulentropy beautifulentropy force-pushed the ratelimits-spend-only-later branch 2 times, most recently from 73ed05b to 315ab1a Compare August 15, 2024 21:51
@beautifulentropy beautifulentropy marked this pull request as ready for review August 15, 2024 21:51
@beautifulentropy beautifulentropy requested a review from a team as a code owner August 15, 2024 21:51
@beautifulentropy beautifulentropy requested review from jsha and aarongable and removed request for jsha August 15, 2024 21:51
aarongable
aarongable previously approved these changes Aug 15, 2024
ra/ra.go Outdated Show resolved Hide resolved
aarongable
aarongable previously approved these changes Aug 15, 2024
Copy link
Contributor

@beautifulentropy, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

LGTM % filing a ticket for the RA to be hooked up to the rate limits redis cluster

@beautifulentropy
Copy link
Member Author

@beautifulentropy, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

Filing a ticket with #7666 to ensure that the RA gets hooked up and I'll ensure that there's no dependence on it unless it's configured and the flag is flipped.

@beautifulentropy beautifulentropy merged commit 14c0b2c into main Aug 15, 2024
13 checks passed
@beautifulentropy beautifulentropy deleted the ratelimits-spend-only-later branch August 15, 2024 23:08
beautifulentropy added a commit that referenced this pull request Aug 16, 2024
- Check `CertificatesPerDomain` at newOrder and spend at Finalize time.
- Check `CertificatesPerAccountPerDomain` at newOrder and spend at
Finalize time.
- Check `CertificatesPerFQDNSet` at newOrder and spend at Finalize time.
- Fix a bug
in`FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction()` which
results in failed authorizations being spent for the exact FQDN, not the
eTLD+1.
- Remove redundant "max names" check at transaction construction time
- Enable key-value rate limits in the RA
beautifulentropy added a commit that referenced this pull request Aug 21, 2024
This bug was introduced in
#7669.

Also, make calls to ra.countCertificateIssued() non-blocking like
ra.countFailedValidation().

Part of #7664
Blocks #7666
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.

2 participants