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.

Sign up for GitHub

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

Jump to bottom

ratelimits: Add a feature-flag which makes key-value implementation authoritative #7666

Merged
merged 5 commits into from
Aug 22, 2024
Merged

ratelimits: Add a feature-flag which makes key-value implementation authoritative #7666

merged 5 commits into from
Aug 22, 2024

Conversation

beautifulentropy
Copy link
Member

@beautifulentropy beautifulentropy commented Aug 13, 2024 •
edited
Loading

  • Add feature flag UseKvLimitsForNewOrder
  • Add feature flag UseKvLimitsForNewAccount
  • Flush all Redis shards before running integration or unit tests, this avoids false positives between local testing runs

Fixes #7664
Blocked by #7676

@beautifulentropy beautifulentropy force-pushed the key-value-limits-authoritative branch 2 times, most recently from 3711e8c to 34a846d Compare August 13, 2024 17:40
@beautifulentropy beautifulentropy mentioned this pull request Aug 15, 2024
Merged
@beautifulentropy beautifulentropy force-pushed the key-value-limits-authoritative branch 12 times, most recently from 9dda074 to 4e6bcc0 Compare August 16, 2024 17:57
@beautifulentropy beautifulentropy marked this pull request as ready for review August 16, 2024 18:13
@beautifulentropy beautifulentropy requested a review from a team as a code owner August 16, 2024 18:13
@github-actions GitHub Actions
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.

@github-actions GitHub Actions
Copy link
Contributor

@beautifulentropy, this PR adds one or more new feature flags: UseKvLimitsForNewAccount,UseKvLimitsForNewOrder. As such, this PR must be accompanied by a review of the Let's Encrypt CP/CPS to ensure that our behavior both before and after this flag is flipped is compliant with that document.

Please conduct such a review, then add your findings to the PR description in a paragraph beginning with "CPS Compliance Review:".

@beautifulentropy beautifulentropy force-pushed the key-value-limits-authoritative branch from 4e6bcc0 to 2e28ad9 Compare August 16, 2024 18:55
aarongable
aarongable reviewed Aug 19, 2024
View reviewed changes
wfe2/wfe.go Outdated Show resolved Hide resolved
wfe2/wfe.go Show resolved Hide resolved
test/integration/otel_test.go Outdated Show resolved Hide resolved
test/boulder-tools/flushredis.go Outdated Show resolved Hide resolved
ra/ra.go Outdated Show resolved Hide resolved
ra/ra.go Outdated Show resolved Hide resolved
ra/ra.go Outdated Show resolved Hide resolved
@beautifulentropy beautifulentropy mentioned this pull request Aug 21, 2024
Merged
@beautifulentropy beautifulentropy force-pushed the key-value-limits-authoritative branch from 76ebf98 to 27dc3a9 Compare August 21, 2024 18:42
beautifulentropy added a commit that referenced this pull request Aug 21, 2024
@beautifulentropy
ratelimits: Skip Spends on CertificatesPerDomain for renewals (#7676)
4bf6e2f 
This bug was introduced in
#7669.

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

Part of #7664
Blocks #7666
@beautifulentropy beautifulentropy force-pushed the key-value-limits-authoritative branch from b3cb54f to 6d04882 Compare August 21, 2024 19:45
aarongable
aarongable previously approved these changes Aug 21, 2024
View reviewed changes
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 modulo two test nits

test/integration/otel_test.go Outdated Show resolved Hide resolved
test/integration/ratelimit_test.go Outdated Show resolved Hide resolved
@beautifulentropy
Copy link
Member Author

Working through the deployment tickets for this change today.

@beautifulentropy beautifulentropy merged commit c9be034 into main Aug 22, 2024
14 checks passed
@beautifulentropy beautifulentropy deleted the key-value-limits-authoritative branch August 22, 2024 19:56
aarongable added a commit that referenced this pull request Aug 23, 2024
@aarongable
Fix two lints introduced by #7666
a1713af
@aarongable aarongable mentioned this pull request Sep 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarongable aarongable aarongable approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a feature-flag which makes key-value rate limits implementation authoritative
2 participants
@beautifulentropy @aarongable