Revert "Expiry mailer: fetch certificates in bulk" (letsencrypt#5780)

aarongable · web-flow · commit bbe53e92d0bc · 2021-11-05T13:26:06.000-07:00
This reverts commit e3ce816, which was reviewed in letsencrypt#5607. This change caused database queries to exceed the maximum packet size and fail. Because this was an opportunistic optimization, reverting it is the safest course moving forward.
diff --git a/cmd/expiration-mailer/main.go b/cmd/expiration-mailer/main.go
@@ -13,7 +13,6 @@ import (
 	netmail "net/mail"
 	"net/url"
 	"os"
-	"regexp"
 	"sort"
 	"strings"
 	"text/template"
@@ -335,34 +334,24 @@ func (m *mailer) findExpiringCertificates() error {
 		}
 		m.stats.nagsAtCapacity.With(prometheus.Labels{"nag_group": expiresIn.String()}).Set(atCapacity)
 
-		if len(serials) == 0 {
-			continue // nothing to do
-		}
-
-		// Wrap every serial in quotes so they can be interpolated into the query.
-		quotedSerials := make([]string, len(serials))
-		serialRegexp := regexp.MustCompile("^[0-9a-f]+$")
-		for i, s := range serials {
-			if !serialRegexp.MatchString(s) {
-				return fmt.Errorf("encountered malformed serial %q", s)
+		// Now we can sequentially retrieve the certificate details for each of the
+		// certificate status rows
+		var certs []core.Certificate
+		for _, serial := range serials {
+			var cert core.Certificate
+			cert, err := sa.SelectCertificate(m.dbMap, serial)
+			if err != nil {
+				// We can get a NoRowsErr when processing a serial number corresponding
+				// to a precertificate with no final certificate. Since this certificate
+				// is not being used by a subscriber, we don't send expiration email about
+				// it.
+				if db.IsNoRows(err) {
+					continue
+				}
+				m.log.AuditErrf("expiration-mailer: Error loading cert %q: %s", cert.Serial, err)
+				return err
 			}
-			quotedSerials[i] = fmt.Sprintf("'%s'", s)
-		}
-
-		// Now we can retrieve the certificate details for all of the status rows.
-		certWithIDs, err := sa.SelectCertificates(
-			m.dbMap,
-			fmt.Sprintf("WHERE serial IN (%s)", strings.Join(quotedSerials, ",")),
-			nil,
-		)
-		if err != nil {
-			m.log.AuditErrf("expiration-mailer: error retrieving certs: %s", err)
-			return err
-		}
-
-		certs := make([]core.Certificate, len(certWithIDs))
-		for i, c := range certWithIDs {
-			certs[i] = c.Certificate
+			certs = append(certs, cert)
 		}
 
 		m.log.Infof("Found %d certificates expiring between %s and %s", len(certs),
diff --git a/cmd/expiration-mailer/main_test.go b/cmd/expiration-mailer/main_test.go
@@ -724,12 +724,12 @@ func TestDedupOnRegistration(t *testing.T) {
 	}
 	regA, err = testCtx.ssa.NewRegistration(ctx, regA)
 	test.AssertNotError(t, err, "Couldn't store regA")
-
 	rawCertA := newX509Cert("happy A",
 		testCtx.fc.Now().Add(72*time.Hour),
 		[]string{"example-a.com", "shared-example.com"},
 		serial1,
 	)
+
 	certDerA, _ := x509.CreateCertificate(rand.Reader, rawCertA, rawCertA, &testKey.PublicKey, &testKey)
 	certA := &core.Certificate{
 		RegistrationID: regA.Id,
@@ -764,8 +764,11 @@ func TestDedupOnRegistration(t *testing.T) {
 
 	err = testCtx.m.findExpiringCertificates()
 	test.AssertNotError(t, err, "error calling findExpiringCertificates")
-	if len(testCtx.mc.Messages) != 1 {
-		t.Fatalf("wrong num of messages, want 1, got %d", len(testCtx.mc.Messages))
+	if len(testCtx.mc.Messages) > 1 {
+		t.Errorf("num of messages, want %d, got %d", 1, len(testCtx.mc.Messages))
+	}
+	if len(testCtx.mc.Messages) == 0 {
+		t.Fatalf("no messages sent")
 	}
 	domains := "example-a.com\nexample-b.com\nshared-example.com"
 	expected := mocks.MailerMessage{
diff --git a/test/v1_integration.py b/test/v1_integration.py
@@ -402,13 +402,9 @@ def test_expiration_mailer():
 
     requests.post("http://localhost:9381/clear", data='')
     for time in (no_reminder, first_reminder, last_reminder):
-        try:
-            print(get_future_output(
-                ["./bin/expiration-mailer", "--config", "%s/expiration-mailer.json" % config_dir],
-                time))
-        except subprocess.CalledProcessError as e:
-            print(e.output.decode("unicode-escape"))
-            raise
+        print(get_future_output(
+            ["./bin/expiration-mailer", "--config", "%s/expiration-mailer.json" % config_dir],
+            time))
     resp = requests.get("http://localhost:9381/count?to=%s" % email_addr)
     mailcount = int(resp.text)
     if mailcount != 2:
