Skip to content

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

Merged
beautifulentropy merged 5 commits into
mainfrom
key-value-limits-authoritative
Aug 22, 2024
Merged

ratelimits: Add a feature-flag which makes key-value implementation authoritative#7666
beautifulentropy merged 5 commits into
mainfrom
key-value-limits-authoritative

Conversation

@beautifulentropy

@beautifulentropy beautifulentropy commented Aug 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Member
  • 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 Bot commented Aug 16, 2024

Copy link
Copy Markdown
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 Bot commented Aug 16, 2024

Copy link
Copy Markdown
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
Comment thread wfe2/wfe.go Outdated
Comment thread wfe2/wfe.go
Comment thread test/integration/otel_test.go Outdated
Comment thread test/boulder-tools/flushredis.go
Comment thread ra/ra.go
Comment thread ra/ra.go Outdated
Comment thread ra/ra.go Outdated
@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

@aarongable aarongable left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread test/integration/otel_test.go Outdated
Comment thread test/integration/ratelimit_test.go Outdated
@beautifulentropy

beautifulentropy commented Aug 22, 2024

Copy link
Copy Markdown
Member Author

Working through the deployment tickets for this change today.

@beautifulentropy beautifulentropy merged commit c9be034 into main Aug 22, 2024
@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
Closed
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