Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove restriction that the zone apex has to be in a two-part domain.… #176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aphexer
Copy link

@aphexer aphexer commented May 7, 2020

Remove restriction that the zone apex has to be in a two-part domain.tld format.

Now domains such as bbc.co.uk should work fine with cloudns.

Thank you for contributing to sewer.
Every contribution to sewer is important to us.
You may not know it, but you have just contributed to making the world a more safer and secure place.

Contributor offers to license certain software (a “Contribution” or multiple
“Contributions”) to sewer, and sewer agrees to accept said Contributions,
under the terms of the MIT License.
Contributor understands and agrees that sewer shall have the irrevocable and perpetual right to make
and distribute copies of any Contribution, as well as to create and distribute collective works and
derivative works of any Contribution, under the MIT License.

Now,

What(What have you changed?)

For the CloudNS dns provider, remove the restriction that the zone apex has to be in a two-part domain.tld format.

Why(Why did you change it?)

Now domains such as bbc.co.uk should work fine with cloudns. See also #164

…tld format.

Now domains such as bbc.co.uk should work fine with cloudns.
@aphexer
Copy link
Author

aphexer commented May 7, 2020

I'm not very good with the mock testutils. The code now does an extra zone list request which the mock response is not handling. Hoping someone can help fix the test.

@mmaney
Copy link
Collaborator

mmaney commented May 8, 2020

Yes, the mocked tests are a pain. I can summarize most of what I've learned about working with them to date in just a couple bullet points (or are they pain points?):

  • errors are reported through tracebacks
  • all errors/tracebacks follow all the normal spew - scroll 'way down, then up to first usually helps
  • you have to grok each test in fullness :-( but sometimes fixing one takes care of many :-)

I've seen this sort of service-API introspection used by other providers. Is this something you feel is necessary for $REASONS beyond those such as bbc.co.uk? Because tldextract probably fixes those, and does so in a way that could be shared by all providers, rather than doing a bespoke fix for each one.

@aphexer
Copy link
Author

aphexer commented May 9, 2020

For the reasons: tldextract doesn't work when the domain name you want to get a certificate for is a subdomain, e.g. sub.domain.tld.

In my case, I have zones in CloudNS for sub.domain.tld, where domain.tld is managed elsewhere and NS records delegate authority for sub to CloudNS.

@mmaney
Copy link
Collaborator

mmaney commented May 10, 2020

@aphexer thanks - that's something I had wondered about. I'm only superficially familiar with tldextract, and that much only because it's been used by a few of the existing providers. I don't suppose tldextract's "private domains" thing would address that? Even aside from the issue of getting a current version of the list it uses...

I'll get back to your testing issues if you haven't sorted them out later. Hard to say just when, my time is not yet my own, and this e-learning-in-place thing seems to consume even more of our time than classes run the old way. Since we're making it up as we go, still, I guess that's to be expected. :-/

From looking at the error, I'm guessing that the existing tests mock the requests.* calls, and that the return values they provide don't contain the right structure(s) for your service query. This is a fundamental issue with testing ACME clients in general and DNS providers in particular, needing to mock responses where you can't access the actual service in the tests.

Making sense of the mocks' setup in typical sewer tests is nearly impossible given black's one-way-no-exceptions formatting. I had to de-black a few to gain some understanding of them, undoing black's obfuscation of the long with clauses. :-(

You might also look at route53's tests, as that uses the same sort of zone list examination that you're adding to cloudns here. It looks like quite a bit of work to me - maybe you could add a proper UNIT test(s) of _find_dns_zone in isolation, then mock it out entirely for testing the rest of the module?

@mmaney
Copy link
Collaborator

mmaney commented Aug 28, 2020

@aphexer sorry it's been so long, but I'm trying to sort out the things that missed 0.8.3 for whatever reason, and I would think this would be a candidate for 0.8.4. Am I correct in thinking that you've used this patch with a sub.domain.tld registration and at least tested it with a two-part domain.tld as well? If it's just the testing that's still holding this up, I think we can beat the mocks into submission. :-)

@mmaney mmaney added awaiting feedback legacy driver a dns_providers driver issue labels Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback legacy driver a dns_providers driver issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants