Fix pre-existing VirtualHost object paths when adding a new one

[joohoi]

When installing certificates for Apache, if there is a need to create multiple VirtualHosts to the same file, the first one created gets an Augeas DOM path that has no index, as it's the only one in that path at the very moment. If said VirtualHost has multiple domains, the deploy_cert might get called to another domain for that VirtualHost later on, after creating another VirtualHost to the same file. Augeas then fails to manage path without index, crashing the process.

This PR fixes the pre-existing VirtualHost object paths when creating a new one, if needed. It also adds a debug log message to the deploy_cert function, logging the processed domain name to help with debugging in the future.

[bmw]

When we chatted about this issue before, we agreed that the real fix for this would be to always fetch vhosts dynamically but we couldn't do that because of our reliance on association between vhosts.

I spent a couple minutes trying to see how strong our reliance on this was and all I could come up with was our use of vhost.ancestor here. If I remember correctly, Noah added this not out of necessity, but because he thought it was silly we were searching for a vhost we had already found once. Do you have the same understanding of this feature?

Are there any associations I missed? If not, perhaps we can implement the "real" fix and it might even be less code than this already small PR.

[joohoi]

In configurator object we have things like self.assoc, self._wildcard_vhosts and self._enhanced_vhosts that keep some sort of state mapping to actual VirtualHost objects. We could possibly work our way through this by changing the __eq__ magic method in obj.VirtualHost to match no index to 1-index with the minor changes to the actual configurator code, but I feel that it'd be a lot more work, and would also involve almost exactly the same functionality that's implemented in this PR.

[bmw]

Not sure how I missed all the other state set up in __init__. Thanks for the pointer.

To be honest, path surgery likes this makes me nervous. Are we sure this properly handles any vhost paths that could be thrown at us by Augeas' match function? Is it possible two vhosts refer to the same file but use different Augeas paths? See https://github.com/hercules-team/augeas/wiki/Path-expressions#formal-grammar for examples.

Additionally, are we sure we'll always be adding vhosts to the bottom of the file both now and in the future? If it changes in the future, will we remember this code won't work?

If you're confident this will work and especially confident this won't cause issues for the 99.99%+ of certbot-apache users who have never hit this issue, we can go forward with this , otherwise, I think we may want to consider the "real" fix that doesn't involve path surgery. Out of the variables you mentioned, I think we could remove all with some performance hits and small changes in Certbot's core except self._wildcard_vhosts. I don't have a good solution there other than just asking the user to pick the vhost once for deploy_cert and once again for every enhancement which obviously isn't ideal.

Another possible solution is to reload Augeas after making changes and for each vhost in self.vhost in the same file as the new vhost, try and match it with a loaded vhost (largely) ignoring only the value of vhost.path. We'll have to do something in the somewhat unlikely case there are multiple matches though. Perhaps we could just remove any cache entries containing the vhost we couldn't find again?

[bmw]

@joohoi, I assigned myself this during a triage we did as a team of all open PRs after your contract ended. The thought was I was going to own figuring out what to do with this since I had the most context.

Now that you're back though, I'm curious what you think we should do with this PR.

[bmw]

I don't think there's a rush on this, but I'm assigning @joohoi based on #6256 (comment) which needs a response.

We probably can just close this PR based on #6665 though.