Deprecate all tls-sni related objects in acme module

[adferrand]

This PR is a part of the tls-sni-01 removal plan described in #6849.

As acme is a library, we need to put some efforts to make a decent deprecation path before totally removing tls-sni in it. While initialization of acme.challenges.TLSSNI01 was already creating deprecation warning, not all cases were covered.

For instance, and innocent call like this ...

if not isinstance(challenge, acme.challenges.TLSSNI01):
    print('I am not using this TLS-SNI deprecated stuff, what could possibly go wrong?')

... would break if we suddenly remove all objects related to this challenge.

So, I use the Deprecator Warning Machine, Let's Pacify this Technical Debt (Guido ®), to make acme.challenges and acme.standalone patch themselves, and display a deprecation warning on stderr for any access to the tls-sni challenge objects.

No dev should be able to avoid the deprecation warning. I set the deprecation warning in the idea to remove the code on 0.34.0, but the exact deprecation window is open to discussion of course.

[adferrand]

Fixing TestSimpleTLSSNI01Server on Linux also corrected it on Windows. So I take it.

[bmw]

So, I use the Deprecator Warning Machine, Let's Pacify this Technical Debt (Guido ®)

Did the approach here come from Guido? If so, have a link?

[adferrand]

https://mail.python.org/pipermail/python-dev/2008-January/076194.html

[adferrand]

But it is not specific to monkey patch a module. I took the solution from you in fact (of a link you gave), from discussions about josepy. I will find the link tomorrow.

About the class approach, it is because __getattr__ is not always called for a missing property, depending of the Python version. Here again, a link tomorrow.

[bmw]

For what it's worth, cryptography takes a similar approach which is a big +1 to me about its sanity. Here's a deprecated attribute which uses the code at https://github.com/pyca/cryptography/blob/ac1d13f43dea5ebee0506dc229cd431660916c73/src/cryptography/utils.py#L154.

[adferrand]

It was exactly the example I saw, during the discussion about the retro-portage of new josepy module to jose module. Finally, a simple from ... import ... as did the trick, but this example was a very relevant way to handle non trivial monkey patch for existing modules.

[adferrand]

Also from this example it shows that __dict__ must also be exported to ensure a transparent behavior. I will add it.

[adferrand]

Here is the link about the problem of get attribute protocol using __getattr__ on modules, and the workaround that involve to use a class delegating to the module instead.

https://stackoverflow.com/questions/2447353/getattr-on-a-module

[bmw]

I don't think we should hardcode a port like this.

As a more minor point, why are these changes in this PR?

[adferrand]

In fact this code was subject to deadlock, and it was already the case on Windows. It started also to be it in this PR on Linux. So I refactor to not have this kind of deadlock.

For the port, I would argue that this kind of hardcoded port is in several places in certbot tests: I am not talking of parameters in mocked objects, but in integration tests with real bindings on the system that execute them.

[bmw]

In fact this code was subject to deadlock, and it was already the case on Windows. It started also to be it in this PR on Linux.

I don't see how the other changes in this PR cause a deadlock on Linux here.

For the port, I would argue that this kind of hardcoded port is in several places in certbot tests: I am not talking of parameters in mocked objects, but in integration tests with real bindings on the system that execute them.

I think the major difference is our unit tests are shipped in the Python package and usually run by OS packagers to see if the code is working correctly on their system. We don't have the same control on these environments as we do as on the environment for our integration tests so I think we should try and make our unit tests more robust and avoid doing things like hardcoding ports.

[adferrand]

I do not see neither why it suddenly deadlocked. But since it already happened, in similar code, I preferred to rewrite.

If I solve the static port attribution, to get randomly an available one, like in the previous implementation, will you be OK with the modifications I did here?

[bmw]

If I solve the static port attribution, to get randomly an available one, like in the previous implementation, will you be OK with the modifications I did here?

I'll need to review the test changes more closely when it's finished, but I should be!

[adferrand]

Done. I pick a random free port.

[bmw]

The changelog needs to be updated in this PR as well.

[bmw]

In fact this code was subject to deadlock, and it was already the case on Windows. It started also to be it in this PR on Linux.

I don't see how the other changes in this PR cause a deadlock on Linux here.

For the port, I would argue that this kind of hardcoded port is in several places in certbot tests: I am not talking of parameters in mocked objects, but in integration tests with real bindings on the system that execute them.

I think the major difference is our unit tests are shipped in the Python package and usually run by OS packagers to see if the code is working correctly on their system. We don't have the same control on these environments as we do as on the environment for our integration tests so I think we should try and make our unit tests more robust and avoid doing things like hardcoding ports.