Only check allow_domain when we don't already have a cert

[gohai]

The allow_domain check might be somewhat costly - involving e.g. an external HTTP request to query a central registry. Make it so that this gets only done before requesting new certificates, not for every HTTPS connection.

[GUI]

@gohai: Thanks for the pull request, but sorry for the belated response! I've been busy with some other things, but I'll definitely try to take a closer look at this in the next week or two and try to get this merged and released. Thanks again!

[luto]

This would cause domains, for which allow_domain returned true in the past, but does not now, to still get that "old" certificate, right? I understand that the check might be costly, but this doesn't seem like the original purpose of that setting to me. Maybe caching (either in your code, or in auto-ssl) would be a better approach here.

[GUI]

@gohai: Thanks bringing this up and this PR. I've tweaked this a bit and made it part of the v0.12.0 release, if you'd like to take a look at that.

You can see be9eb90 for the changes I made, but to summarize:

• I tweaked the error handling to ensure we continue logging the "domain not allowed" at the NOTICE level, rather than the ERROR level.

• I shifted the allow_domain callback to happen after we lookup certs in memory, but before we check for them in the storage adapter. I did this since the storage adapter lookup could also be costly depending on your setup, and I was worried about non-allowed domains being able to trigger an indefinite number of storage lookups. So by shifting the allow_domain callback to be handled after the memory lookup, this should hopefully alleviate most of the pressure, since the lookup will still be avoided once the certificate is cached in memory on the specific server.

  However, this approach still might not be perfect in all situations, particularly if you're dealing with a lot of non-allowed domains (so the certs will never be cached or in storage), and your allow_domain callback is more costly than your storage adapter lookup. As @luto suggested, we could try to implement some type of caching on the allow_domain callback for those situations, but the more I think about it, I think that type caching might be best left to each user to implement themselves inside allow_domain (rather than trying to implement this within lua-resty-auto-ssl), since there's a lot of variables there (where to store the cache, how long caching should be valid for, whether negative non-allowed responses should be cached, etc). But if this is a common issue, we could see about implementing some basic caching options within lua-resty-auto-ssl.

  But if you have any further thoughts on this, I'm happy to continue this conversation, since I do see how this could be an issue.

[gohai]

@GUI Thank you for your in-depth reply and picking up of my patch. We should be able to use the new behavior you implemented as-is: the eventual query to our backend to check whether we should approve of the site is really fine, as long as it is only done on cache-miss, and not for every new connection!