Skip to content

ratelimits: Exempt renewals from NewOrdersPerAccount and CertificatesPerDomain#7513

Merged
beautifulentropy merged 5 commits into
mainfrom
ratelimits-renewal-exceptions
Jun 27, 2024
Merged

ratelimits: Exempt renewals from NewOrdersPerAccount and CertificatesPerDomain#7513
beautifulentropy merged 5 commits into
mainfrom
ratelimits-renewal-exceptions

Conversation

@beautifulentropy

@beautifulentropy beautifulentropy commented May 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member
  • Rename NewOrderRequest field LimitsExempt to IsARIRenewal
  • Introduce a new NewOrderRequest field, IsRenewal
  • Introduce a new (temporary) feature flag, CheckRenewalExemptionAtWFE

WFE:

  • Perform renewal detection in the WFE when CheckRenewalExemptionAtWFE is set
  • Skip (key-value) NewOrdersPerAccount and CertificatesPerDomain limit checks when renewal detection indicates the the order is a renewal.

RA:

  • Leave renewal detection in the RA intact
  • Skip renewal detection and (legacy) NewOrdersPerAccount and CertificatesPerDomain limit checks when CheckRenewalExemptionAtWFE is set and the NewOrderRequest indicates that the order is a renewal.

Fixes #7508
Part of #5545

@beautifulentropy beautifulentropy force-pushed the ratelimits-renewal-exceptions branch 3 times, most recently from 4e6bfed to 17eb6c0 Compare May 29, 2024 21:25
@beautifulentropy beautifulentropy force-pushed the ratelimits-renewal-exceptions branch from 17eb6c0 to 9bd02cb Compare June 14, 2024 15:42
@beautifulentropy beautifulentropy force-pushed the ratelimits-renewal-exceptions branch from 9bd02cb to 596a850 Compare June 18, 2024 21:11
@beautifulentropy beautifulentropy marked this pull request as ready for review June 18, 2024 21:24
@beautifulentropy beautifulentropy requested a review from a team as a code owner June 18, 2024 21:24
@github-actions

github-actions Bot commented Jun 18, 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 Jun 18, 2024

Copy link
Copy Markdown
Contributor

@beautifulentropy, this PR adds one or more new feature flags: CheckRenewalExemptionAtWFE. 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:".

aarongable
aarongable reviewed Jun 20, 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.

I must be missing something, because I don't understand why this requires a new isRenewal boolean instead of re-using the limitsExempt boolean.

@beautifulentropy

beautifulentropy commented Jun 21, 2024

Copy link
Copy Markdown
Member Author

I must be missing something, because I don't understand why this requires a new isRenewal boolean instead of re-using the limitsExempt boolean.

limitsExempt is used for ARI renewals, which are exempt from all rate limits:

boulder/ra/ra.go

Lines 2526 to 2535 in 7a6632d

// Renewal orders, indicated by ARI, are exempt from NewOrder rate limits.
if !req.LimitsExempt {
// Check if there is rate limit space for issuing a certificate.
err = ra.checkNewOrderLimits(ctx, newOrder.Names, newOrder.RegistrationID, req.IsRenewal)
if err != nil {
return nil, err
}
}

isRenewal is used for detected renewals, which are exempt from just two rate limits:

boulder/ra/ra.go

Lines 1589 to 1618 in 7a6632d

func (ra *RegistrationAuthorityImpl) checkNewOrderLimits(ctx context.Context, names []string, regID int64, isRenewal bool) error {
newOrdersPerAccountLimits := ra.rlPolicies.NewOrdersPerAccount()
// TODO(#7511): Remove the feature flag check.
skipCheck := features.Get().CheckRenewalExemptionAtWFE && isRenewal
if newOrdersPerAccountLimits.Enabled() && !skipCheck {
started := ra.clk.Now()
err := ra.checkNewOrdersPerAccountLimit(ctx, regID, names, newOrdersPerAccountLimits)
elapsed := ra.clk.Since(started)
if err != nil {
if errors.Is(err, berrors.RateLimit) {
ra.rlCheckLatency.WithLabelValues(ratelimit.NewOrdersPerAccount, ratelimits.Denied).Observe(elapsed.Seconds())
}
return err
}
ra.rlCheckLatency.WithLabelValues(ratelimit.NewOrdersPerAccount, ratelimits.Allowed).Observe(elapsed.Seconds())
}
certNameLimits := ra.rlPolicies.CertificatesPerName()
if certNameLimits.Enabled() && !skipCheck {
started := ra.clk.Now()
err := ra.checkCertificatesPerNameLimit(ctx, names, certNameLimits, regID)
elapsed := ra.clk.Since(started)
if err != nil {
if errors.Is(err, berrors.RateLimit) {
ra.rlCheckLatency.WithLabelValues(ratelimit.CertificatesPerName, ratelimits.Denied).Observe(elapsed.Seconds())
}
return err
}
ra.rlCheckLatency.WithLabelValues(ratelimit.CertificatesPerName, ratelimits.Allowed).Observe(elapsed.Seconds())
}

@beautifulentropy beautifulentropy force-pushed the ratelimits-renewal-exceptions branch from fbdf682 to 13a7de6 Compare June 26, 2024 19:23
aarongable
aarongable approved these changes Jun 27, 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.

Thanks, this makes a lot more sense to my head with the updated field names. LGTM.

pgporada
pgporada approved these changes Jun 27, 2024
View reviewed changes
Comment thread wfe2/wfe.go
wfe.stats.ariReplacementOrders.With(prometheus.Labels{
"isReplacement": fmt.Sprintf("%t", replaces != ""),
"limitsExempt": fmt.Sprintf("%t", limitsExempt),
"limitsExempt": fmt.Sprintf("%t", isARIRenewal),

@pgporada pgporada Jun 27, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"limitsExempt": fmt.Sprintf("%t", isARIRenewal),
"isARIRenewal": fmt.Sprintf("%t", isARIRenewal),

@beautifulentropy beautifulentropy merged commit 55c274d into main Jun 27, 2024
@beautifulentropy beautifulentropy deleted the ratelimits-renewal-exceptions branch June 27, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pgporada pgporada pgporada approved these changes

@aarongable aarongable aarongable approved these changes

@jsha jsha Awaiting requested review from jsha

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ratelimits: Exempt renewals from NewOrdersPerAccount and CertificatesPerDomain limits

3 participants

@beautifulentropy @pgporada @aarongable