Hey @bmw, do you have any advice RE the checks I suggested in the PR description?

We should definitely make sure a retry is likely to succeed before retrying, but I don't know exactly what errors are going to be recoverable (or if I should just stick to CAA errors).

Currently this rescues any ACME error, and retries with the domains minus the identifiers of any subproblems that might exist, which is almost certainly too general.

Hey @JamesBalazs. Sorry for the delayed response here. I've been busy.

Three high level thoughts:

1. I think we should probably implement this at a lower level than renewal.py. I think with the way --allow-subset-of-names works in master, the flag works when obtaining a certificate for the first time. I think by catching the problem in renewal.py, we're only catching this problem on "renewal" and not initial issuance.
2. As for what errors are worth retrying, my initial thought is we should be pretty aggressive about retrying here. That initial idea would be to do something like as long as it errors out with subproblems and that have at least 1 identifier field, remove that identifier and try again. A slightly more conservative approach would be to only do this if all subproblems have an identifier.
3. I think we should add a changelog entry about this although you may want to wait to write this until we've nailed down the behavior here.

It sounds like you think (2) is too aggressive though. Why is that? What types of problems are you worried about here? I certainly could be overlooking something.

Hey @bmw, in response to your points:

1. Does obtain_certificate sound like the right place to move the catch / retry logic? It looks like that is called both on issuance of a new cert, and renewal. Presumably I can just try / catch around the call to obtain_certificate_from_csr then have obtain_certificate call itself to retry, similar to how allow_subset_of_names already works in that method, if there's at least one subproblem with an identifier.
2. We have been quite cautious internally about retrying with LetsEncrypt due to a limit on retries for the same list of domains. That doesn't apply here as the list of domains will change for the retry, so I'm happy to be more aggressive with retrying if there aren't any other retry limits we could expect to run into.
3. Yeah definitely - I was going to wait until the behaviour was better defined and we're closer to something mergeable before adding to the changelog. Just to reduce the chances of conflicts, the current version changing while this is in progress etc.

Thanks for taking a look at this!

1. That sounds exactly right to me.
2. OK cool. I think we should do the most aggressive version of what I described in my previous post and retry if there is at least one subproblem with an identifier. This feels safe to me based on Let's Encrypt's current behavior, it gives Certbot the best chance to issue a certificate for at least some domains, and seems appropriate based on this text from the RFC:

ACME clients may choose to use the "identifier" field of a subproblem
as a hint that an operation would succeed if that identifier were
omitted.

3. SGTM!

@bmw I've implemented the change we discussed along with a simple test for the behaviour in client_test.py.
Let me know if there's anything I've missed or any extra tests are needed for this.

I'll review this soon :) !