Improve performance of GetOrderForNames.

[jsha]

When there are a lot of potential orders to reuse, the query could scan
unnecessary rows, sometimes leading to timeouts. The new query uses the
available index more effectively and adds a LIMIT clause.

There are no new unittest cases because I think the cases at https://github.com/letsencrypt/boulder/blob/master/sa/sa_test.go#L2046-L2086 already cover what I would want to test for this change, but I'm definitely open to ideas for additional test cases.

Fixes #4325

[cpu]

We talked offline about putting this behind a feature flag.

[jsha]

I pushed changes to add the feature flag.

However, in local testing (both before and after the feature flag) I reliably got failures for the test_http2_http01_challenge test case under config-next:

Internal error - Error creating new order - getAllOrderAuthorizationStatuses returned the wrong number of authorization statuses (0 vs expected 1) for order 4

When I check my local integration test database (which is persisted across runs), this checked out. Order 4 was created last Wednesday (less than a week ago), and there was one entry in the orderToAuthz table for it, but the corresponding authz ID did not exist in either pendingAuthorizations or authz.

I initially thought this could be an edge case where the order was originally created with reused authzs, and the authzs expired before the order did and were cleaned up by the expired-authz-purger. But when that happens, the order expiry is shortened to match the min expiry time on any of the authzs involved. In this case, the order had its full one-week lifetime, suggesting there was no authz reuse involved.

It is worth noting that this case always happens for test_http2_http01_challenge, which is unusual among our integration test cases because it always attempts to issue for the same hostname. Also, each integration test run generates a new client (and so a new account key), which means that previously, orders generated from prior integration test runs would have been ignored (they didn't match the account ID). However, with this new code, we pick the oldest order as the sole likely candidate for reuse, and that order happens to be one that is broken (i.e. one that fails during GetOrder).

One other data point: When I switched the ORDER BY expires ASC to ORDER BY expires DESC, I still get the same error.

Note that this error case could be avoided by moving the check for account ID so it's before the GetOrder call, but that might just be masking the symptoms.

[jsha]

Found the problem:

expired-authz-purger is run, during the integration test, with a date 1 year in the future, to delete all authzs:

boulder/test/integration-test.py

Lines 69 to 70 in e3f797f

	# Run the purger once to clear out any backlog so we have a clean slate.
	expect(now+datetime.timedelta(days=+365), None, "")

However, since the purger doesn't delete orders, that leaves dangling orders whose authorizations have been deleted. Before this change, that wasn't a big deal. Since each run of the integration test would generate fresh accounts, and the SELECT used by this function included the account ID as one parameter, there would never be an attempt to load orders (and therefore authorizations) associated with an account from a previous run.

I verified this by disabling the purger step in the integration test, and found that the test passed as expected.

I'm going to fix this by doing this (from previous comment):

moving the check for account ID so it's before the GetOrder call, but that might just be masking the symptoms.

This is nice because it saves a query in some cases. Based on the above testing, I'm satisfied that this isn't just masking a bigger problem.

[cpu]

Nice work figuring out the integration test problem. I agree that moving the acct ID check earlier seems like the right fix here 👍

This looks good to me, I'm going to merge with the one 🔍 based on available reviewers.