DigitalOcean DNS-01 support #345

dl00 · 2018-02-24T23:08:11Z

What this PR does / why we need it:

This PR adds DigitalOcean DNS-01 challenge support.

Which issue this PR fixes

No issue, I just wanted it.

Special notes for your reviewer:

There are included tests for the DigitalOcean API, using the DIGITALOCEAN_TOKEN and DIGITALOCEAN_DOMAIN environment variables to run live.
I have not tested E2E in a real system yet, partially because I was not able to find any docs on how to build the image in this repo
There is a license file some of the other DNS-01 challenge implementations, and I was not sure what to do with it :) I am of course happy to put this code on whatever the most appropriate license is supposed to be for this project.

Release note:

munnerz · 2018-02-27T08:07:23Z

/ok-to-test

dl00 · 2018-02-27T08:40:18Z

W0227 08:20:46.640] The Service "nginx-ingress-controller" is invalid: spec.clusterIP: Invalid value: "10.0.0.15": provided IP is already allocated

Any idea how I can cause an error like that? That appears to be the root cause.

munnerz · 2018-02-27T08:41:10Z

That's a really unfortunate test flake that occasionally comes up. I've not had a chance to fix it yet 😬

/retest

munnerz · 2018-03-08T16:43:54Z

I have not tested E2E in a real system yet, partially because I was not able to find any docs on how to build the image in this repo

There's been some work by @euank here: #381 to document how to build/deploy a custom version of cert-manager if you're able to try this out on DigitialOcean!

I've not got an account, nor anything set up over there so if you could test it and let me know, I'd be happy to merge this if all is okay! (I've taken a look, and there's nothing really to change! 😄 💯)

euank · 2018-03-09T07:20:44Z

pkg/issuer/acme/dns/digitalocean/digitalocean.go

+	return nil, fmt.Errorf("No existing record found for %s", fqdn)
+}
+
+func (c *DNSProvider) makeRequest(method, uri string, body io.Reader) (json.RawMessage, error) {


Any reason for hand-writing the 50 lines of client code we need vs using the digital ocean godo library to make these calls?

I am aware of the godo library. However, it is a library for the entire DO API, vs just 50 lines to make the 1 or 2 API calls needed. I was also observing the convention of the other DNS providers making similar choices.

I think the other DNS providers all use the cloud-provider provided library, excepting cloudflare.

I'm personally in favour of using those libraries when possible since auth and retry logic can be a bit tricky otherwise.
I'll defer to @munnerz if he wishes to weigh in differently.

+1 using the official go library reduces the amount of testing and maintenance for this area of code

I'm open to doing this in future - so far, we've copied across all the DNS providers from xenolf/lego when implementing them here. There's nothing wrong with us changing/improving them, but I don't think it's a requirement in order to accept it initially.

euank · 2018-03-09T07:22:05Z

pkg/issuer/acme/dns/digitalocean/digitalocean.go

+			return nil, err
+		}
+
+		return nil, fmt.Errorf("DigitalOcean API Error \n%d: %s", r.Code, r.Message)


In general, it's preferable to not have newlines in error strings.

I don't know go very well, I was just copying from the other DNS providers. Will fix if @munnerz agrees.

yep could you remove the \n 😄

euank · 2018-03-09T07:29:26Z

pkg/issuer/acme/dns/dns.go

+	case providerConfig.DigitalOcean != nil:
+		apiTokenSecret, err := s.secretLister.Secrets(s.resourceNamespace).Get(providerConfig.DigitalOcean.Token.Name)
+		if err != nil {
+			return nil, fmt.Errorf("error getting digitalocean token: %s", err.Error())


There's no need to call .Error here (I know other parts of this file do too, they should be fixed as well).

The %s format string will automatically call .Error for anything that implements error (with a higher priority than calling .String even, see playground)

sylvainfilteau · 2018-03-18T17:59:52Z

I would really like to test this PR, what are the steps to run this branch locally ?

I tried the doc/devel instructions without success. I'm unfortunately completely in the dark with the go development stack.

dl00 · 2018-03-18T18:11:37Z

@sylvainfilteau seems like what you have to do is run make build. You can then push whatever images it generates to your registry or wherever. You may need to download the repository using go get github.com/jetstack/cert-manager/... before that works though.

Your testing would really help out, since I have been too busy! I also still have to do a couple small corrections in the code as mentioned earlier anyway. Since munnerz has voiced support for using godo, I will probably add it.

munnerz · 2018-03-19T13:35:34Z

@dl00 if it's easy enough to switch to godo, I'm happy for it to happen, although would like to see some automated tests running to ensure it works if you do.

Otherwise I'm happy to accept this as is given it's been verified in the upstream xenolf/lego repo.

sylvainfilteau · 2018-03-21T23:52:53Z

I opened an issue #401 because of my incapacity to build the code.

ulrichSchreiner · 2018-05-24T17:39:18Z

hi,

i would really appreciate to see this code in cert-manager ... is anything that can be done to get this merged?

dl00 · 2018-05-24T17:50:26Z

I have some free time. I can do the rebase and updates tomorrow.Sorry for delays. On May 24, 2018 10:39 AM, Ulrich Schreiner <notifications@github.com> wrote:hi, i would really appreciate to see this code in cert-manager ... is anything that can be done to get this merged? —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

oliverkane · 2018-05-25T21:43:47Z

Nothing to add except I was looking at forking this myself, and then saw there was much progress. Commenting to subscribe to updates as this is something I'm interested in.

dl00 · 2018-05-29T22:34:39Z

@munnerz what is the appropriate way to run the E2E tests on my local machine? If I try to run make e2e_test, it says error installing cm.

I am nearly done with the switch to Godo, just testing.

katcheLS · 2018-07-01T05:52:11Z

What’s the status on this PR? I’m interested in using digital ocean DNS-01 challenge later this year, and I hope this hasn’t dropped off the radar too far. If no one is working on it, I might be able to contribute in a couple months.

dl00 · 2018-07-01T07:41:45Z

i have made some changes, but I am unsure why the checks are failing. and then I forgot about this issue again because of life. I can take another look in a few days.

Richard87 · 2018-07-13T14:17:18Z

I'm testing a little bit, I would love to se DO-DNS provider!

@dl00 to fix the cm errors, I had to create a namespace "cert-manager" , and update tiller with a proper service account :)

kragniz · 2018-08-03T15:23:29Z

@dl00 thanks!

dl00 · 2018-08-03T15:45:53Z

/retest

retest-bot · 2018-08-03T17:19:17Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

retest-bot · 2018-08-03T17:59:16Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

dl00 · 2018-08-03T18:00:26Z

/lgtm cancel

munnerz · 2018-08-03T20:53:05Z

pkg/issuer/acme/dns/digitalocean/digitalocean.go

+		context.Background(),
+		util.UnFqdn(zoneName),
+		createRequest,
+	)


Will this return an error if the record already exists?

(it should 'upsert', i.e. absorb errors if the record already exists)

In my testing digital ocean will simply post a duplicate of the record without error even if the challenge is exactly the same. Later it deletes any matching records found during clean up.

This can be added to the tests explicitly if you prefer.

munnerz · 2018-08-03T20:55:50Z

@dl00 I've resolved the issues with the e2e environment that have hit all PRs this evening. A domain used during e2e tests had expired..!

I've just got that one Q, and if that's all okay then we can go ahead and apply lgtm again 😄

dl00 · 2018-08-03T21:15:43Z

I've responded, thanks for your help!

dl00 · 2018-08-04T03:27:42Z

/retest

Noah-Huppert · 2018-08-05T16:55:46Z

Super excited for this PR!

Being able to move over from kube-lego -> cert-manager with DigitalOcean DNS-01 will solve an issue I have been having with kube-lego for a while now.

🙌

If there is anything I can do to help please let me know!

dl00 · 2018-08-07T03:17:56Z

@munnerz ready to merge when you are

jetstack-bot · 2018-08-07T18:58:34Z

@dl00: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

munnerz · 2018-08-07T19:01:10Z

Hey - sorry there's been a few PRs merged today and this was obviously not merged beforehand! 😬

I'll drop a merge commit on top of your branch to bring this up to speed with the latest changes if you've got the 'Allow edits from maintainers' option enabled and get this merged to save you having to do more work!

Again, sorry for all the hassle!

dl00 · 2018-08-07T19:09:14Z

@munnerz the setting you mentioned is enabled, thanks for your help.

jetstack-bot · 2018-08-13T11:14:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kragniz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kragniz · 2018-08-13T11:25:55Z

Sorry, I pushed the wrong branch by mistake and closed this. I've continued in #821 since I can't push to this branch anymore.

jetstack-bot requested a review from munnerz February 24, 2018 23:08

jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 24, 2018

jetstack-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 27, 2018

euank reviewed Mar 9, 2018

View reviewed changes

sylvainfilteau mentioned this pull request Mar 20, 2018

Unable to build master using "devel" documentation #401

Closed

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2018

munnerz added area/acme Indicates a PR directly modifies the ACME Issuer code and removed area/acme Indicates a PR directly modifies the ACME Issuer code labels Apr 25, 2018

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2018

jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 14, 2018

jetstack-bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 3, 2018

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2018

munnerz reviewed Aug 3, 2018

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2018

kragniz closed this Aug 13, 2018

kragniz force-pushed the master branch from f4941c4 to 06b2af4 Compare August 13, 2018 11:13

jetstack-bot added retest-not-required-docs-only size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 13, 2018

munnerz removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2018

kragniz mentioned this pull request Aug 13, 2018

DigitalOcean DNS-01 support #821

Closed

Richard87 mentioned this pull request Sep 13, 2018

DigitalOcean DNS01 support #900

Closed

This was referenced Oct 19, 2018

DNS01 Challenge Provider for DigitalOcean #935

Closed

Add ACME DigitalOcean DNS01 provider #972

Merged

DigitalOcean DNS-01 support #345

DigitalOcean DNS-01 support #345

Conversation

dl00 commented Feb 24, 2018 • edited

munnerz commented Feb 27, 2018

dl00 commented Feb 27, 2018

munnerz commented Feb 27, 2018

munnerz commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvainfilteau commented Mar 18, 2018

dl00 commented Mar 18, 2018

munnerz commented Mar 19, 2018

sylvainfilteau commented Mar 21, 2018

ulrichSchreiner commented May 24, 2018

dl00 commented May 24, 2018 via email

oliverkane commented May 25, 2018

dl00 commented May 29, 2018

katcheLS commented Jul 1, 2018

dl00 commented Jul 1, 2018

Richard87 commented Jul 13, 2018

kragniz commented Aug 3, 2018

dl00 commented Aug 3, 2018

retest-bot commented Aug 3, 2018

retest-bot commented Aug 3, 2018

dl00 commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munnerz commented Aug 3, 2018

dl00 commented Aug 3, 2018

dl00 commented Aug 4, 2018

Noah-Huppert commented Aug 5, 2018 • edited

dl00 commented Aug 7, 2018

jetstack-bot commented Aug 7, 2018

munnerz commented Aug 7, 2018

dl00 commented Aug 7, 2018

jetstack-bot commented Aug 13, 2018

kragniz commented Aug 13, 2018

dl00 commented Feb 24, 2018 •

edited

Noah-Huppert commented Aug 5, 2018 •

edited