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

Alias domain #175

Closed
wants to merge 4 commits into from
Closed

Alias domain #175

wants to merge 4 commits into from

Conversation

aphexer
Copy link

@aphexer aphexer commented May 7, 2020

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?)

Add support for alias domains.

Why(Why did you change it?)

See https://letsencrypt.org/2019/10/09/onboarding-your-customers-with-lets-encrypt-and-acme.html

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #175 into master will decrease coverage by 0.05%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
- Coverage   86.94%   86.89%   -0.06%     
==========================================
  Files          19       19              
  Lines        1088     1091       +3     
==========================================
+ Hits          946      948       +2     
- Misses        142      143       +1     
Impacted Files Coverage Δ
sewer/dns_providers/common.py 95.45% <75.00%> (-4.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1012d76...bc64175. Read the comment docs.

@mmaney
Copy link
Collaborator

mmaney commented May 25, 2020

Before diving into the details, a big thank you, @aphexer, for posting this, and for including the link to that document/page at LE. I wish that had been around back when I was setting up something similar (on a smaller scale than they suggest).

I like the idea, but this simple, minimally invasive, implementation has a few problems. Beginning with the fact that it simply can't work without modifying each of the existing DNS providers to accept and pass through the alias_domain parameter. :-(

And that's not the only modification that would be needed to these legacy drivers, since they always add the _acme-challenge prefix to the identifier. That could be made to work, but I'd rather not implement something that conflicts with the advice in that onboarding document. And while I'm thinking about it, it really would be the polite thing to do to have sewer verify that the CNAME entries exist before attempting to setup an aliased challenge.

All of these things suggest to me that this is a feature that will be added for the unified provider that's landed in master since you posted this. My hidden agenda with doing it this way is that alias support makes a nice carrot to encourage authors to migrate those legacy providers to the new interface. :-)

I'm going to keep this in mind but not do any implementation until I can get 0.8.2 out. The code is done, aside from the last polishing, and I've been working on documentation this weekend... except when writing the docs for something makes me revisit the code.

@mmaney
Copy link
Collaborator

mmaney commented Jun 3, 2020

Plans are just predictions, and predictions are often... changed. In the end, it was a small change in the UnifiedProvider bundle of stuff, but it's going to require porting the legacy DNS provider to the new-model interface and then using the new alias support calls in the driver. Want to look at #178 and let me know if this looks like it will resolve your issue (when the driver you're using gets migrated, of course - leaving that for after 0.8.2 gets released).

Thanks!

@mmaney
Copy link
Collaborator

mmaney commented Aug 19, 2020

Closing this, as the issue is resolved - it just needs a bunch of per-driver work by someone who can test that the changes work.... or even make sense in the driver's context (eg., would an AWS customer (using route53 driver) ever need aliasing?)

@mmaney mmaney closed this Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants