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

Move DNS providers into a separate package #112

Closed
wants to merge 3 commits into from

Conversation

willglynn
Copy link
Contributor

This changeset moves the DNS challenge providers into a new dns_provider package.

This package a sibling of acme. I felt this was appropriate given the low amount of coupling between acme and dns_provider, and that this coupling can be reduced further. (The DNS providers don't need to know anything ACME-specific – they only need to be able to create and destroy TXT records with short TTLs – but that's a topic for a later changeset.)

All the providers are together in the dns_provider package to keep this changeset minimal. Similarly, all the providers are initialized the way they were before – usually by passing in empty strings – even though there are clear opportunities for cleanup.

The lego CLI interacts with this package via dns_provider.Registry. This registry provides a list of entries describing the available DNS challenge providers in exactly the way that lego help describes them, and lego help output is constructed based on the contents of this registry. Once the CLI decides it wants a specific entry, the CLI calls NewChallengeProvider() to yield an acme.ChallengeProvider.

Individual providers register themselves like:

func init() {
    Registry.addProvider("digitalocean", "DO_AUTH_TOKEN", func() (acme.ChallengeProvider, error) {
        return NewDNSProviderDigitalOcean(os.Getenv("DO_AUTH_TOKEN"))
    })
}

This pattern permits both additional refactoring into separate packages and conditional compilation (#111). Go guarantees that init() gets called after initializing imports and before main(), so there's no fuss for either of those changes.

@xenolf
Copy link
Member

xenolf commented Feb 13, 2016

Are you going to make some changes to this as #111 progresses?

I like the idea of a registry but does that mean we have a registry for every challenge?

@willglynn
Copy link
Contributor Author

Sure, I'll update this if it seems like it's on the right track. Would you like me to push each provider into a separate package as part of this PR?

I'm not sure if we'll need a registry for every challenge. For DNS specifically, I expect DNS providers to continue multiplying more rapidly than other types of providers, and I think DNS providers should ultimately satisfy an interface more specialized than acme.ChallengeProvider, so I think a dedicated DNS provider registry would make sense.

@willglynn willglynn force-pushed the dns_provider_refactor branch 3 times, most recently from bf762c5 to 9f9c00a Compare February 14, 2016 02:12
@willglynn
Copy link
Contributor Author

I rebased to the current master one step at a time.

(For my records, in case I screwed something up: 83d5b00 -> 15f5b71 -> bf762c5 -> 9f9c00a -> 03be013.)

@xenolf: please let me know how you'd like me to proceed in this PR.

@xenolf
Copy link
Member

xenolf commented Feb 14, 2016

Yes I feel like moving each provider into its own package is the way to go.
Something like lego/providers/dns for example.

How would you specialize the interface for DNS providers while still being able to use them with the current structure though?

@willglynn
Copy link
Contributor Author

I was leaning towards putting dns first in the package naming:

  • lego/dns: DNS support functions and type definitions (e.g. FQDN)
  • lego/dns/provider: home of the DNS provider Registry
  • lego/dns/provider/digitalocean: a DNS provider for DigitalOcean (imports lego/dns/provider, registers itself)
  • lego/dns/all_providers: a package that import _s each provider, registering them all

Providers would need to import lego/dns/provider but not lego/acme.

You mentioned in another issue that you'd like every challenge type to be solvable with just lego/acme, but DNS-01 has a lot of supporting machinery, and my first thought is that it should be an exception. Some applications simply do not require DNS-01 support, and if we pushed DNS-01 support entirely into lego/dns, we could free lego/acme of the rather heavy github.com/miekg/dns library. In that case, lego/dns could have all the DNS-specific logic and all the DNS-specific dependencies.

The key is that lego/acme would be entirely decoupled from DNS-01. If you want DNS-01, you'd import lego/acme, lego/dns, and lego/dns/provider/yourfavorite or lego/dns/all_providers; if you don't, you'd just import lego/acme. Pulling DNS out of lego/acme would mean that if you want to satisfy a DNS challenge automatically, you'd need to make a lego/acme.Client, find and instantiate the DNS provider you want, then client.SetChallengeProvider(DNS01, your_dns_provider). This sounds like a lot of effort, but in fact it's exactly how things work today, so I don't see any real downside beyond sacrificing the DNS-01 challenge type if you didn't import lego/dns.

Then I had a second thought: we could make a lightweight DNS-01 provider and ship it inside lego/acme. It would be similar to the current manual provider, except instead of asking you to create the record and then checking that your record made it to all the right places, it would ask you to create the record and wait until you're ready to proceed with the challenge. lego/acme would thus still be able to complete DNS-01 challenges right out of the box without needing to pull in lego/dns. You wouldn't have DNS-related automation, sure, but that's because you opted out – and as a result you also wouldn't have the dependency chain.

@xenolf
Copy link
Member

xenolf commented Feb 15, 2016

If you want DNS-01, you'd import lego/acme, lego/dns, and lego/dns/provider/yourfavorite or lego/dns/all_providers

This seems not intuitive to me. If I'd import lego/acme I'd expect it to be all of ACME. Importing lego/acme and then having to import lego/providers/dns/cloudflare would make sense to me.
You could still have all the heavy stuff inside the lego/providers/dns package.

Then I had a second thought: we could make a lightweight DNS-01 provider and ship it inside lego/acme. It would be similar to the current manual provider, except instead of asking you to create the record and then checking that your record made it to all the right places, it would ask you to create the record and wait until you're ready to proceed with the challenge.

This is exactly how the manual provider works. The DNS propagation check is done in the challenge code and was even moved out into the preCheckDNS variable. We basically can switch out the check on the fly. If we export that, we can change the pre-check if there is need for an actual DNS check and just have it being a no-op in the ACME package - this also gets rid of the github.com/miekg/dns dependency as that gets pushed to wherever we implement the function.

@xenolf
Copy link
Member

xenolf commented Mar 14, 2016

Closing this as #144 is now merged.

@xenolf xenolf closed this Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants