Improve renewal rate limiting #2832

cpu · 2017-06-23T14:56:08Z

As described in Boulder issue #2800 the implementation of the SA's
countCertificates function meant that the renewal exemption for the
Certificates Per Domain rate limit was difficult to work with. To
maximize allotted certificates clients were required to perform all new
issuances first, followed by the "free" renewals. This arrangement was
difficult to coordinate.

In this PR countCertificates is updated such that renewals are
excluded from the count reliably. To do so the SA takes the serials it
finds for a given domain from the issuedNames table and cross references
them with the FQDN sets it can find for the associated serials. With the
FQDN sets a second query is done to find all the non-renewal FQDN sets
for the serials, giving a count of the total non-renewal issuances to
use for rate limiting.

Resolves #2800

As described in Boulder issue #2800 the implementation of the SA's `countCertificates` function meant that the renewal exemption for the Certificates Per Domain rate limit was difficult to work with. To maximize allotted certificates clients were required to perform all new issuances first, followed by the "free" renewals. This arrangement was difficult to coordinate. In this commit `countCertificates` is updated such that renewals are excluded from the count reliably. To do so the SA takes the serials it finds for a given domain from the issuedNames table and cross references it with the FQDN sets it can find for the associated serials. With the FQDN sets a second query is done to find all the non-renewal FQDN sets for the serials, giving a count of the total non-renewal issuances to use for rate limiting.

jsha · 2017-06-23T20:10:04Z

To do so the SA takes the serials it finds for a given domain from the issuedNames table and cross references it with the FQDN sets

s/it with/them with/

jsha · 2017-06-23T20:33:41Z

sa/sa.go

@@ -434,19 +434,18 @@ func (ssa *SQLStorageAuthority) countCertificatesByExactName(domain string, earl
 }

 // countCertificates returns, for a single domain, the count of
-// certificates issued in the given time range for that domain using the
+// non-renewal certificates issued in the given time range for that domain using the


Let's change this to "non-renewal certificate issuances," which I think parses a bit better.

jsha · 2017-06-23T20:41:00Z

sa/sa.go

 // provided query, assumed to be either `countCertificatesExactSelect` or
-// `countCertificatesSelect`.
+// `countCertificatesSelect`. Renewals of certificates issued within the same
+// window are considered "free" and not returned as part of this count.


"and are not counted" instead of "not returned as part of this count."

jsha · 2017-06-23T20:47:43Z

sa/sa.go

@@ -1031,6 +1046,101 @@ func (ssa *SQLStorageAuthority) CountFQDNSets(ctx context.Context, window time.D
 	return count, err
 }

+// getFQDNSetsBySerials returns a slice of []byte `setHash` entries from the


Let's do type setHash []byte so we can use types to clearly express the relationship between what this function returns and what the other function takes.

Also, this comment mostly documents the return type and parameters (which can be deduced from the code). Instead, it's better to talk more about the "why." E.g. "finds the setHashes corresponding to a set of certificate serials so we can check whether any previous certificate was issued with the same set of names."

jsha · 2017-06-23T20:52:06Z

sa/sa.go

+		qmarks[i] = "?"
+	}
+	query := "SELECT setHash FROM fqdnSets " +
+		"WHERE serial IN (" + strings.Join(qmarks, ",") + ") "


Trailing space in string.

jsha · 2017-06-23T20:53:23Z

sa/sa.go

+		query,
+		params...)
+
+	if err != nil && err != sql.ErrNoRows {


ErrNoRows should be considered an error here because it's an internal consistency violation. If the serial existed in issuedNames, it should exist here.

jsha · 2017-06-23T22:23:34Z

sa/sa.go

+}
+
+// getNewIssuancesByFQDNSet returns a count of new issuances (renewals are not
+// included) for a given slice of fqdnSets that were issued after the earliest


s/that were issued after/that occurred after/

jsha · 2017-06-23T22:25:09Z

sa/sa.go

+
+	processedSetHashes := make(map[string]struct{})
+	issuanceCount := 0
+	// Loop through each set hash result, counting issuance per unique set hash


jsha · 2017-06-23T22:31:21Z

sa/sa.go

+		key := string(result.SetHash)
+		// Skip set hashes that we have already processed - we only care about the
+		// first issuance
+		if _, exists := processedSetHashes[key]; exists {


Note: if we use a map[string]bool instead, this becomes much simpler:

if processedSetHashes[key] {

In general, I encourage maps of bools for sets rather than maps of empty struct unless the set is very large, since the readability is typically more important than size in memory. I acknowledge that my advice on this may have changed during the course of the project. :-)

jsha

Looking great!

jsha · 2017-06-26T18:11:55Z

sa/sa.go

 	_, err := ssa.dbMap.Select(
 		&fqdnSets,
 		query,
 		params...)

-	if err != nil && err != sql.ErrNoRows {
+	// NOTE(@cpu): We don't explicitly check if `err != sql.ErrNoRows` here


This is fine, but even better would be to use the same number of lines to explicitly check for ErrNoRows, and log an error saying something about "internal consistency violation" like above. Leans a little more in the "self-documenting code" direction.

Resolved (I pushed the commit but it hasn't shown up yet. Maybe some GH lag?)

There it is: Fixed in 76e30d1

rolandshoemaker · 2017-06-26T19:36:26Z

sa/sa.go

+	// to exist here. Otherwise an internal consistency violation occured and
+	// needs to be audit logged
+	if err != nil {
+		err := fmt.Errorf("getFQDNSetsBySerials returned no rows - internal consistency violation")


This will mask other non sql.ErrNoRows errors, should probably be a special case.

rolandshoemaker · 2017-06-26T19:39:13Z

sa/sa.go

+	for i, setHash := range fqdnSets {
+		// We have to cast the setHash back to []byte here since the sql package
+		// isn't able to convert `sa.setHash` for the parameter value itself
+		params[i] = []byte(setHash)


Side note: probably doesn't make sense for this since it's a single use but we could add a converter case to sa/type-converter.go so that gorp knows how to natively use the setHash type without having to cast it.

That's a good point. Agreed that its probably not worth it here, but good to know for the future.

rolandshoemaker · 2017-06-26T19:44:21Z

sa/sa.go

+	// were visible within our search window
+	fqdnSets, err := ssa.getFQDNSetsBySerials(serials)
+	if err != nil {
+		return -1, err


Do we actually use -1 here? Given our style is generally not to use trapdoor returns for cases where we fail to check the returned error I'd probably rather just use 0, but ¯_(ツ)_/¯.

I think I was following the convention that the function was using before I touched it: https://github.com/letsencrypt/boulder/blob/master/sa/sa.go#L469:L471

rolandshoemaker · 2017-06-26T19:46:50Z

sa/sa.go

+	// should loudly complain
+	if err == sql.ErrNoRows || len(results) == 0 {
+		ssa.log.AuditErr(fmt.Sprintf("Found no results from fqdnSets for setHashes known to exist: %#v", fqdnSets))
+		return 0, err


Per above comment you're also not consistently returning -1 so I'm not sure I see the value.

Good catch - in that case I'll just update the whole thing to use 0. I agree that a -1 sentinel value is un-gophery.

rolandshoemaker

A few small comments, otherwise looks good.

cpu · 2017-06-27T14:27:01Z

@rolandshoemaker I believe this is ready for another 🔍
@jsha I don't think the changes were significant enough to merit dismissing your review, but you may want to glance over the feature flag addition and the few other minor tweaks suggested by @rolandshoemaker's reviews.

Thanks!

jsha and others added 2 commits June 19, 2017 13:02

Add test for renewal exemption.

a79a73d

cpu self-assigned this Jun 23, 2017

jsha requested changes Jun 23, 2017

View reviewed changes

Daniel added 2 commits June 26, 2017 09:44

Editorial fixes

ed3ff70

Review feedback

369dfbc

cpu requested a review from rolandshoemaker June 26, 2017 17:02

jsha approved these changes Jun 26, 2017

View reviewed changes

Explicitly log getFQDNSetsBySerials consistency violation

76e30d1

rolandshoemaker reviewed Jun 26, 2017

View reviewed changes

Special case ErrNoRows check

4ec800d

rolandshoemaker reviewed Jun 26, 2017

View reviewed changes

rolandshoemaker suggested changes Jun 26, 2017

View reviewed changes

Daniel added 4 commits June 27, 2017 09:50

Add AllowRenewalFirstRL feature flag

712b4a6

Control renewal first RL exemption by feature flag.

8314b9e

Return 0 consistently

c32623c

Merge remote-tracking branch 'le/master' into cpu-rate-limit-renewal-OOO

10c32a6

rolandshoemaker approved these changes Jun 27, 2017

View reviewed changes

jsha approved these changes Jun 27, 2017

View reviewed changes

cpu merged commit 71f8ae0 into master Jun 27, 2017

cpu deleted the cpu-rate-limit-renewal-OOO branch June 27, 2017 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve renewal rate limiting #2832

Improve renewal rate limiting #2832

cpu commented Jun 23, 2017 •

edited

jsha commented Jun 23, 2017

jsha Jun 23, 2017

jsha Jun 23, 2017

jsha Jun 23, 2017

jsha Jun 23, 2017

jsha Jun 23, 2017

jsha Jun 23, 2017

jsha Jun 23, 2017

jsha Jun 23, 2017

jsha left a comment

jsha Jun 26, 2017

cpu Jun 26, 2017

cpu Jun 26, 2017

rolandshoemaker Jun 26, 2017

rolandshoemaker Jun 26, 2017

cpu Jun 26, 2017

rolandshoemaker Jun 26, 2017

cpu Jun 26, 2017

rolandshoemaker Jun 26, 2017

cpu Jun 26, 2017

rolandshoemaker left a comment

cpu commented Jun 27, 2017

Improve renewal rate limiting #2832

Improve renewal rate limiting #2832

Conversation

cpu commented Jun 23, 2017 • edited

jsha commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolandshoemaker left a comment

Choose a reason for hiding this comment

cpu commented Jun 27, 2017

cpu commented Jun 23, 2017 •

edited