provider/digitalocean: Add support for certificates #14578

caiofilipini · 2017-05-17T15:55:52Z

This PR addresses that by introducing the resource_digitalocean_certificate.

A few things to note:

I've upgraded godo to get support for Certificate. This upgrade bought a few breaking changes which I've addressed in caiofilipini@6681a86 (i.e. every call now takes a context.Context, and a few things got deprecated/removed).
I've introduced a RandTLSCert function in helper/acctest/random.go that generates a valid, self-signed TLS certificate so that we can properly test the integration.
I was getting a Step 0 error: After applying this step, the plan was not empty: while running the new tests and I couldn't really figure it out. I think it's because the attributes that DO's API returns for a certificate are different from the ones we use fore creating one. For that reason, I've set ExpectNonEmptyPlan to true, but I'd love some guidance here.

$ make testacc DIGITALOCEAN_TOKEN=XXX TEST=./builtin/providers/digitalocean TESTARGS='-run=TestAccDigitalOceanCertificate'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/17 17:54:50 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/digitalocean -v -run=TestAccDigitalOceanCertificate -timeout 120m
=== RUN   TestAccDigitalOceanCertificate_Basic
--- PASS: TestAccDigitalOceanCertificate_Basic (6.84s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/digitalocean   6.867s

There are three "deeper" changes included with this update: 1) The `Detach` function got removed from the `StorageActionsService` in favor of `DetachByDropletID` (which is now used in `resource_digitalocean_volume.go`). 2) The `Update` function got removed from `TagsService` (renaming a tag has been deprecated in the API). 3) Every function in godo now takes a `context.Context` as first argument, so I've changed all calls to send in a `context.Background()`.

Besides the support for DO certificates themselves, this commit also includes: 1) A new `RandTLSCert` function that generates a valid, self-signed TLS certificate to be used in the test 2) A fix for the PEM encoding of the private key generated in `RandSSHKeyPair`: the PEM was always empty

caiofilipini · 2017-05-17T18:08:30Z

Just realized this is missing updates to docs, so marking it as WIP.

caiofilipini · 2017-05-17T19:35:25Z

Docs added in a565001. Removing [WIP].

grubernaut

Overall, looks good. Great work on this! Just a few minor nits/questions.

grubernaut · 2017-05-18T14:59:28Z

builtin/providers/digitalocean/resource_digitalocean_certificate.go

+		Name:             d.Get("name").(string),
+		PrivateKey:       d.Get("private_key").(string),
+		LeafCertificate:  d.Get("leaf_certificate").(string),
+		CertificateChain: d.Get("certificate_chain").(string),


Are there any undesired effects to certificate_chain being nil in the CertificateRequest struct? As certificate_chain is an optional parameter, might be worthwhile to verify it's populated with d.GetOk(). If it doesn't cause any undesired effects, looks good as-is.

@grubernaut AFAIK, there are no undesired effects to certificate_chain being nil. But I'd rather play safe here, so I'm gonna add a check around it.

Although I just checked the docs for Get on ResourceData and it seems to be safe for TypeString (it just returns the zero value, or ""). I think it's safe to leave it as is.

That's correct about Get returning the nil type for the native golang string type, however, does that have any adverse effect during the Create request in the DO API?
IE: will setting certificate_chain to "", cause the API request to attempt to set certificate_chain to ""?

@grubernaut It doesn't seem to have any adverse effects.

grubernaut · 2017-05-18T15:01:07Z

builtin/providers/digitalocean/resource_digitalocean_certificate.go

+
+	d.Set("name", cert.Name)
+	d.Set("not_after", cert.NotAfter)
+	d.Set("sha1_fingerprint", cert.SHA1Fingerprint)


Are the other attributes listed in the schema not available in the cert object? These will need to be set as well so Terraform can detect any configuration drift between config/state.

@grubernaut Correct, they're available only for creation, but not returned by the API (for security reasons, I assume).

This is the example response from the API docs:

{ "certificate": { "id": "892071a0-bb95-49bc-8021-3afd67a210bf", "name": "web-cert-01", "not_after": "2017-02-22T00:23:00Z", "sha1_fingerprint": "dfcc9f57d86bf58e321c2c6c31c7a971be244ac7", "created_at": "2017-02-08T16:02:37Z" } }

grubernaut · 2017-05-18T15:02:24Z

builtin/providers/digitalocean/resource_digitalocean_certificate.go

+		return fmt.Errorf("Error deleting Certificate: %s", err)
+	}
+
+	d.SetId("")


The Delete method doesn't need to set the ID to nil, as a non-error return from the Delete method already does this. However, it's not critical to remove, just noting for future knowledge 😄

Cool, I'm gonna remove it anyway. Thanks for the tip!

grubernaut · 2017-05-18T15:11:56Z

Also, as a follow-up, what attributes were changing in the acceptance test that causes the need for setting ExpectNonEmptyPlan to be true? We'll likely need to fix this issue prior to merging, as this will likely cause perpetual diff issues for users.

caiofilipini · 2017-05-19T15:33:14Z

Hey, @grubernaut, thanks a lot for the feedback! I'll address it soon and let you know when I think this is ready for a second pass.

caiofilipini · 2017-05-26T13:33:44Z

@grubernaut I collected the log from the failing test after removing ExpectNonEmptyPlan from it.

https://gist.github.com/caiofilipini/f0628fffd39abb631f2f374d62e4a057

Let me know if you have any insights so we can move this forward. Thank you!

grubernaut · 2017-05-26T14:07:18Z

Hey @caiofilipini, if you add Computed: true, to both sha1_fingerprint and next_after, the test should pass without the config/state diff that you're experiencing.

grubernaut · 2017-05-26T14:09:15Z

website/source/docs/providers/do/r/certificate.html.markdown

+* `certificate_chain` - (Optional) The full PEM-formatted trust chain
+between the certificate authority's certificate and your domain's TLS
+certificate.
+


Are not_after and sha1_fingerprint not allowed to be specified? In the resource schema, they're listed as Optional

@grubernaut They're not recognized (ignored) by the API and always computed based on the input. I wasn't sure how to best represent them so they would make it to the resulting state, but without them being required in the request. Is this the way to go, or is there a better way?

@caiofilipini ah okay awesome. If they aren't accepted inputs to the API request, we can remove Optional and ForceNew and the attributes will simply be Computed attributes that we set inside the Read method.

@grubernaut Ah, that makes sense. Done in c4bbb6e.

@caiofilipini Awesome, ready to review?

@grubernaut Yeah! 😄

caiofilipini · 2017-05-26T14:18:22Z

@grubernaut Computed: true indeed fixed it, thank you! 💯

grubernaut

Awesome work here! Thanks!
LGTM!

caiofilipini · 2017-05-26T20:24:57Z

Thanks for all your help, @grubernaut!

ghost · 2020-04-04T02:17:00Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

caiofilipini added 2 commits May 17, 2017 17:20

grubernaut added enhancement provider/digitalocean labels May 17, 2017

caiofilipini changed the title ~~provider/digitalocean: Add support for certificates~~ [WIP] provider/digitalocean: Add support for certificates May 17, 2017

provider/digitalocean: Add documentation for Certificate resource

a565001

caiofilipini changed the title ~~[WIP] provider/digitalocean: Add support for certificates~~ provider/digitalocean: Add support for certificates May 17, 2017

grubernaut suggested changes May 18, 2017

View reviewed changes

provider/digitalocean: Remove unnecessary SetId("") on cert destroy

32d724e

grubernaut reviewed May 26, 2017

View reviewed changes

caiofilipini added 2 commits May 26, 2017 16:21

provider/digitalocean: Mark not_after and sha1_fingerprint as computed

2b12ddf

provider/digitalocean: Remove Optional and ForceNew for computed attr

c4bbb6e

grubernaut approved these changes May 26, 2017

View reviewed changes

grubernaut merged commit 24202fb into hashicorp:master May 26, 2017

caiofilipini deleted the digitalocean-certs branch March 14, 2018 09:58

ghost locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/digitalocean: Add support for certificates #14578

provider/digitalocean: Add support for certificates #14578

caiofilipini commented May 17, 2017

caiofilipini commented May 17, 2017

caiofilipini commented May 17, 2017

grubernaut left a comment

grubernaut May 18, 2017

caiofilipini May 26, 2017

caiofilipini May 26, 2017

grubernaut May 26, 2017

caiofilipini May 26, 2017

grubernaut May 18, 2017

caiofilipini May 26, 2017

grubernaut May 18, 2017

caiofilipini May 26, 2017

grubernaut commented May 18, 2017

caiofilipini commented May 19, 2017

caiofilipini commented May 26, 2017

grubernaut commented May 26, 2017

grubernaut May 26, 2017

caiofilipini May 26, 2017

grubernaut May 26, 2017

caiofilipini May 26, 2017

grubernaut May 26, 2017

caiofilipini May 26, 2017

caiofilipini commented May 26, 2017

grubernaut left a comment

caiofilipini commented May 26, 2017

ghost commented Apr 4, 2020

provider/digitalocean: Add support for certificates #14578

provider/digitalocean: Add support for certificates #14578

Conversation

caiofilipini commented May 17, 2017

caiofilipini commented May 17, 2017

caiofilipini commented May 17, 2017

grubernaut left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grubernaut commented May 18, 2017

caiofilipini commented May 19, 2017

caiofilipini commented May 26, 2017

grubernaut commented May 26, 2017

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

caiofilipini commented May 26, 2017

grubernaut left a comment

Choose a reason for hiding this comment

caiofilipini commented May 26, 2017

ghost commented Apr 4, 2020