Refactor Lexicon-based DNS plugins#9746
Conversation
5882c58 to
2ead6b4
Compare
bmw
left a comment
There was a problem hiding this comment.
Very very nice Adrien! Thanks a lot for writing this (and your other PRs). It took me a bit to fully grok the structure of the changes here, but once I did, I can see that this is actually quite elegant (and results in removing almost 200 lines of code). Nicely done.
Other than my minor inline comments about deprecations, I think we should add a changelog entry or two about this.
Also, while the code LGTM, I don't happen to have credentials for any of the DNS APIs whose plugins we're changing here currently set up. Do you (or anyone else who may be reading this)? I think us doing a manual unmocked test with one of these plugins would be nice as an extra sanity check here. If you (or anyone else) doesn't have credentials currently set up either, I can poke around and see if I can come up with something myself.
Finally, I think think we may want to merge #9754 (or something like it) or do the same in this PR as I otherwise believe the full/nightly test suite would fail once this is merged. Speaking of which, it's probably not a bad idea for one of us to run those tests this branch before merging to try and help ensure everything still works. I can do that myself once this is updated if you like.
Thanks again!
|
|
||
|
|
||
| class LexiconClient: | ||
| class LexiconClient: # pragma: no cover |
There was a problem hiding this comment.
nit: For this (and maybe build_lexicon_config below as well because why not?) can we use the approach we've used before in PRs like #8989? I think it warns about more edge cases.
There was a problem hiding this comment.
Well, I wanted to leverage this hook to gain something very interesting compared to the approach we have used before.
Basically __init__subclass__ is a class-level hook introduced in recent versions of Python that is triggered when the concerned class is subclassed. It means that it is called even when no instance of the class is actually created, and by extension, any attribute or method called, whether they are at instance, class or static level.
Consider now the situation of a certbot installation with a plugin that is impacted by the deprecation. The DNS authenticator will be instantiated only when a certificate has to be renewed, so a warning associated to instances will be triggered in the future, two months in worst case scenario.
Here, the warning will be issued as soon as certbot starts and load the plugins, even if there is nothing to do like with certbot renew outside renewal windows for a certificate.
I think it gives us the possibility to issue the deprecation warning way faster.
There was a problem hiding this comment.
I think our previous approach warns faster no? It raises a warning as soon as you try to access the deprecated attribute of the module for any reason whether that be for subclassing or for code like if isinstance(obj, dns_common_lexicon.LexiconClient) which the subclass approach doesn't warn about at all.
With that said, I think code like the isinstance example is quite unlikely here so if you have a strong preference for your current approach, you can keep it as is.
There was a problem hiding this comment.
I was trying to find a less hacky way to get this kind of deprecation warnings on larger audience, but I have to acknowledge that there are some corner cases with classes, and it is not covering functions. I have switched back to the approach we used before.
| # pylint: disable=no-member | ||
|
|
||
| class BaseLexiconAuthenticatorTest(dns_test_common.BaseAuthenticatorTest): | ||
| class BaseLexiconAuthenticatorTest(dns_test_common.BaseAuthenticatorTest): # pragma: no cover |
There was a problem hiding this comment.
Same suggestion as above about maybe raising a deprecation on every access of this attribute rather than just on every call. What do you think?
You are welcome! In fact it is more 500 lines of code removed ultimately, since we have to keep the old classes part of the public API for now. Indeed the plugins implementation is now mostly conventions and almost no code, hopefully this will help migrations we could do in the future. I have realigned the PR with the latest updates on versions pinning. I have OVH accounts, so I have been able to test, with success, that certificates were indeed issued properly using the |
|
Sorry for the delay here Adrien. Basically, after thinking about this more, I'm wondering if we should redesign the changes here to make it as easy as possible for 3rd party plugins to work with both new and old versions of Certbot. Why? Because based on our experience with the zope transition, 3rd party plugin authors and their users often want to support both versions. See posts like #9485, linuxserver/docker-swag#297, and #9486 to see what happened with that. Because of that, I've been thinking about how we can do something similar to what we did with zope here, but it's tricky and complicated by the fact that we don't just have to worry about new and old versions of Certbot, but new and old versions of dns-lexicon as well. I was hoping to have something more concrete before writing a comment here, but since it's been a week and I still don't have anything, I thought I'd give you an update about where I'm at. Please let me know if you have any ideas on how we could handle this. |
|
Hello @bmw, also sorry for my delay. I thought about it and I have not a clear strategy either, or more precisely I do not have a strategy that would avoid a significant amount of complexity added that would break the basic purpose of this PR in the first place. The thing is that we are not talking here about a "drop-in" replacement of the interfaces using a different technology, but a complete set of different interfaces with a big shift of paradigm: we move from a custom implementation of the For instance we could think about a duck type approach where the new However for the sake of this PR, I want to stress on the fact that nothing is removed from the public API so far. Things could stay like this for the time needed to prepare the shift, or to find an alternative to not require any shift for existing implementations. We could acknowledge right now that the new classes are good for new implementations, and find a retro-compatibility mechanism later. I would like to propose to do an additional effort on that matter. I am really willing to look at the various implementations of the DNS plugins that exists in the community, and propose the shift to the new system, or a dual implementation depending on the version of Certbot used. I think this would be the best benefit to everyone at the end. |
8601708 to
efe279b
Compare
There was a problem hiding this comment.
Sorry for yet another delay. Sometime between when I posted my last comment and you responded I caught COVID and took some time off work 😩 I'm back now and feeling fine though.
High level comments
Honestly, these changes make me kind of nervous based on how the zope.interface changes went as I described in my last post. There were enough problems there that we felt that undoing the changes on our end was the best approach. If we go forward with this PR, I personally really hope we don't experience the same thing here.
Luckily, these changes only affect Certbot plugins using Lexicon rather than all plugins. Additionally, unless I want to push for undoing the changes in Lexicon, these plugins are going to have to make changes so we may as well take the opportunity to change things, however, we like.
Suggested community outreach
With all that said, I am fine going forward and trying this change, but I would like to take you up on your offer to help the 3rd party plugin community with the transition here to the degree that you're willing and able to do so. Basically, I'm thinking we should use the list of Certbot authenticator plugins with 100+ users as seen by Let's Encrypt that I posted at https://gist.github.com/bmw/aceb69020dceee50ba827ec17b22e08a and go around to each Lexicon based plugin and open an issue/PR describing the changes to be made here. Based on how that goes, we then may also want to write more generic instructions like we did here before we rip out the old interfaces. What do you think? Anything else you think we should do? I think we/I should create an issue in our current milestone to track doing whatever we decide here (although I expect fully completing this to take months due to the time it takes to have the back and forth with other developers).
Other considerations before merging
Finally, assuming you still want to go forward with this change, in addition to my minor inline comments, I think we should
- Make sure the full test suite passes which I kicked off at https://dev.azure.com/certbot/certbot/_build/results?buildId=7103&view=results. EDIT: The test farm test failure should be fixed by merging in
master. - Add changelog entries like:
### CHANGED
* The `LexiconClient ` and `build_lexicon_config` attributes of the `certbot.plugins.dns_common_lexicon` have been deprecated in favor of `LexiconDNSAuthenticator` which was added to the same module.
* The `BaseLexiconAuthenticatorTest` and `BaseLexiconClientTest` attributes of `certbot.plugins.dns_test_common_lexicon` have been deprecated in favor of `BaseLexiconDNSAuthenticatorTest` which was added to the same module.
|
Really sorry for you! I hope you have got fully recovered. So, for the high level comments. I want to clarify my plan for Lexicon. Yes, I want to release a version 4 at some point, and this version would introduce breaking changes. However, I have no intention to do it in the near future (at least 6 months), and I have not decided yet to include the breaking changes that concerns this PR in this future major version. There are already older changes mostly on the CLI on the pipe and they have been already migrated on Certbot side long time ago. For Lexicon 4, it could be simply that providers stay available on their original package and I give myself freedom to break retro-compatibility on the providers API for an even further future. Anyway all of this would happen only after:
As a side note, I would like to say that you have not undone the migration for the Zope interfaces. It is more a compatibility shim optionally enabled when Zope is available on the DNS plugin environment. Shims are well suited at the edge of a system, typically on the public API, but here we are talking about the inner mechanics of the plugins. Now, for the community support. Yes I am still willing to do it, given that we would end with a cleaner situation for everyone. This list, after been expunged of Certbot-owned plugins + non Lexicon-based plugins, should be around 30 plugins to migrate. This is globally the effort I anticipated. The effort by itself is minimal in term of code. As a reminder with my changes, implementing a DNS plugin takes around 50 to 60 LOCs. Remains mostly the time and inertia to have the changes merged and released. So I agree with the commitment it implies, including the efforts to provide a decent documentation on the migration steps. |
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
|
Changelog added. |
…ion (#9821) The Lexicon-based DNS plugins use a mechanism to determine which actual segment of the input domain is actually the DNS zone in which the DNS-01 challenge has to be initiated (eg. `subdomain.domain.com` or `domain.com` for input `subdomain.domain.com`): they tries recursively to configure Lexicon and initiate authentication from the most specific to most generic domain segment, and select the first segment where Lexicon stop erroring out. This mechanism broke with #9746 because now the plugins call Lexicon client instead of the underlying providers, and the client makes guess on the actual domain requested. Typically for `subdomain.domain.com` it will actually try to authenticate against `domain.com`, and so the mechanism above does not work anymore. This PR fixes the issue by using the `delegated` field in Lexicon config each time the plugin needs it. This field is designed for this kind of purpose: it will instruct Lexicon what is the actual DNS zone domain instead of guessing it. I tested the change with one of my OVH account. The expected behavior is re-established and the plugin is able to test `subdomain.domain.com` then `domain.com` as before. Fixes #9791 Fixes #9818
…ion (#9821) The Lexicon-based DNS plugins use a mechanism to determine which actual segment of the input domain is actually the DNS zone in which the DNS-01 challenge has to be initiated (eg. `subdomain.domain.com` or `domain.com` for input `subdomain.domain.com`): they tries recursively to configure Lexicon and initiate authentication from the most specific to most generic domain segment, and select the first segment where Lexicon stop erroring out. This mechanism broke with #9746 because now the plugins call Lexicon client instead of the underlying providers, and the client makes guess on the actual domain requested. Typically for `subdomain.domain.com` it will actually try to authenticate against `domain.com`, and so the mechanism above does not work anymore. This PR fixes the issue by using the `delegated` field in Lexicon config each time the plugin needs it. This field is designed for this kind of purpose: it will instruct Lexicon what is the actual DNS zone domain instead of guessing it. I tested the change with one of my OVH account. The expected behavior is re-established and the plugin is able to test `subdomain.domain.com` then `domain.com` as before. Fixes #9791 Fixes #9818 (cherry picked from commit cf4f07d)
* Set the delegated field in Lexicon config to bypass subdomain resolution (#9821) The Lexicon-based DNS plugins use a mechanism to determine which actual segment of the input domain is actually the DNS zone in which the DNS-01 challenge has to be initiated (eg. `subdomain.domain.com` or `domain.com` for input `subdomain.domain.com`): they tries recursively to configure Lexicon and initiate authentication from the most specific to most generic domain segment, and select the first segment where Lexicon stop erroring out. This mechanism broke with #9746 because now the plugins call Lexicon client instead of the underlying providers, and the client makes guess on the actual domain requested. Typically for `subdomain.domain.com` it will actually try to authenticate against `domain.com`, and so the mechanism above does not work anymore. This PR fixes the issue by using the `delegated` field in Lexicon config each time the plugin needs it. This field is designed for this kind of purpose: it will instruct Lexicon what is the actual DNS zone domain instead of guessing it. I tested the change with one of my OVH account. The expected behavior is re-established and the plugin is able to test `subdomain.domain.com` then `domain.com` as before. Fixes #9791 Fixes #9818 (cherry picked from commit cf4f07d) * add changelog entry for 9821 (#9822) (cherry picked from commit 7bb85f8) --------- Co-authored-by: Adrien Ferrand <adferrand@users.noreply.github.com>
The upcoming version of Lexicon 4 will rationalize the public API proposed by the project. The global spirit of this rationalization is that the provider implementations are not supposed to be used directly, and the
lexicon.client.Clientclass should be the central entrypoint for any DNS operation.For that purpose, the providers implementations are moved to a private package, while the
Clientget new methods more suitable for a usage of Lexicon in third-party projects.This will impact Certbot on the Lexicon-base DNS plugins as of version
3.14, because of the deprecation warnings, and4obviously because of removals of deprecated class/methods and changes in the packaging.One quick win fix with
3.14could be to just change the packages fromlexicon.providerstolexicon._internal.providersand make pylint happy about it, but this will not solve the problem in the long term.Instead, I think this is a good opportunity to refactor the code on Certbot side to offer implementations and interfaces that are less complex, and not coupled to Lexicon internals. We would all benefit from these simplifications, including the developers that are maintaining third-parties DNS plugins based on Certbot interfaces for that purpose.
This PR does this refactoring. In details:
certbot.plugins.dns_common_lexicon.LexiconDNSAuthenticator, that allows to write a Lexicon-based DNS plugin directly by subclassingLexiconDNSAuthenticatorwith as few code as possible.certbot.plugins.dns_common_lexiconare deprecated.certbot.plugins.dns_common_lexicon_test.BaseLexiconDNSAuthenticatorTestthat is the counter-part ofLexiconDNSAuthenticatorto test Lexicon-based DNS pluginsLexiconDNSAuthenticatorBaseLexiconDNSAuthenticatorTestdns_common_lexicon_testis dropped as not relevant anymore3.14.1in current and oldest testsThe main impact of this PR are the changes on the public API. The old classes are kept for retro-compatibility purpose. But they will emit a deprecation warning when subclassed starting with the next version of Certbot, inviting the developpers to switch to the new interfaces. They can be dropped with the next major version of Certbot.
I would gladly provide a migration path documentation for third-party plugins maintainers if needed.