implement custom ssl resource

[jterzis]

Implementation of custom ssl resource in terraform.
See v4 library https://github.com/cloudflare/cloudflare-go/blob/master/ssl.go.

Tested with terraform v0.11.0

Create a file main.tf after building the plugin with make fmt && make build and insert the following:

provider "cloudflare" {
  email = "${var.cloudflare_email}"
  token = "${var.cloudflare_token}"
}

resource "cloudflare_custom_ssl" "ssl" {
  zone_id = "${var.cloudflare_zone_id}"
  custom_ssl_options = "${var.cloudflare_custom_ssl_options}"
  custom_ssl_priority = "${var.cloudflare_custom_ssl_priority}"
}

variable "cloudflare_zone_id" {
  type = "string"
  default = ""
}

variable "cloudflare_custom_ssl_priority" {
  type = "list"
  default = [{
    "id" = ""
    "priority" = 3
  }]
}

variable "cloudflare_custom_ssl_options" {
  type = "map"
  default = {
    "certificate" = ""
    "private_key" = ""
    "bundle_method" = "ubiquitous"
  }
}

Interpolate the desired variables in the file and run terraform init && terraform plan. Then terraform apply to build resource.

[jacobbednarz]

Thanks for this @jterzis! Appreciate the effort. Few questions from me (other specific notes inline):

• This looks related to Add custom certificate resource #224, are you able to confirm?
• Would you be able to go through and remove your debugging comments like
  //return mtSlice, fmt.Errorf("FAILURE: %#v %#v", newData, zcsp)? If you'd like to leave a debug point in there, log.Printf("[DEBUG] ...") should suffice.
• We'll need to add documentation that appears on https://www.terraform.io/docs/providers/cloudflare/. Would you mind adding that (including examples) without the use of all the variables for a clear and concise usage guide? I find the overuse of variables here to be quite confusing for the bare bones use case. https://github.com/terraform-providers/terraform-provider-cloudflare/pull/145/files#diff-6603c294cf5f2bae75be0d66736e8b29 includes an example of adding these new sections to the website if you get stuck.
• Can we also look to add some acceptance tests for this new functionality? This ensures we have a reproducible set of scenarios to test against when we refactor or identify regressions.

[jacobbednarz]

As the API has restrictions on what values can be used here, it's helpful to provide validation before the HTTP interactions are made.

Suggested change

	Computed: true,
	Computed: true,
	ValidateFunc: validation.StringInSlice([]string{"ubiquitous", "optimal", "force"}, false),

[jacobbednarz]

We have bundle_method here as well as nested under custom_ssl_options. Is there a difference for these two?

[jterzis]

The one under custom_ssl_options is passed in during creation. The other is returned by the /zones/:zone_id/custom_certificates/:cert_id which the Read method calls. Added it to top level as a convenience since on Read, the custom_ssl_options map is not set.

[jacobbednarz]

🤔 Generally, we've followed what the public API has returned to ensure we had something consistent to map it off. This feels like it will cause some confusion if the value is present in two spots but not matching.

[jterzis]

I've strived to follow the public API however, the return body from the public api for the POST method on this resource differs from the request body passed into POST in fields and nesting of fields. I've tried to account for both by mapping the POST request body field custom_ssl_options as a nested map and then mapping the return body separately where bundle_method is non-nested (see below return body POST zones/:zone_identifier/custom_certificates from public api).

{
  "success": true,
  "errors": [],
  "messages": [],
  "result": {
    "id": "2458ce5a-0c35-4c7f-82c7-8e9487d3ff60",
    "hosts": [
      "example.com"
    ],
    "issuer": "GlobalSign",
    "signature": "SHA256WithRSA",
    "status": "active",
    "bundle_method": "ubiquitous",
    "geo_restrictions": {
      "label": "us"
    },
    "zone_id": "023e105f4ecef8ad9ca31a8372d0c353",
    "uploaded_on": "2014-01-01T05:20:00Z",
    "modified_on": "2014-01-01T05:20:00Z",
    "expires_on": "2016-01-01T05:20:00Z",
    "priority": 1
  }
}

I think I can set the bundle_method on READ within the nested map, custom_ssl_options and then remove bundle_method from the top level in the schema so we don't have it specified in two places as you've pointed out. But then the return body won't match 1-1 with our public api. I don't have a strong preference here and want to go with our convention so let me if you are okay with a missing bundle_method field in top level of return body.

[jterzis]

@jacobbednarz Thanks for adding some clarity to the request. cloudflare-go separates the req/resp payloads into 2 different structs for an underlying POST and returns the same response payload on GETS as it does on the POST (https://github.com/cloudflare/cloudflare-go/blob/master/ssl.go#L65-L92) so Im not sure there is much more to do there.

What I am doing in the schema amounts to merging the 2 structs from cloudflare-go (ZoneCustomSSLOptions, ZoneCustomSSL). In fact, there shouldn't be much confusion in the schema as to which bundle_method is needed for CREATING the resource (at time apply or plan) versus which is READ in since the latter is a Computed attribute in the schema. Maybe it would help if you describe the specific case you are trying to mitigate ?

[jterzis]

Now that I think about it more, I could probably remove the computed bundle_method from the schema since it's not required to be set in READ only on CREATE.

[jacobbednarz]

Maybe it would help if you describe the specific case you are trying to mitigate ?

Good idea, let's start with the issue first and work backwards from there 😄 My main concern is that we're duplicating schema fields for the purposes of making the internals happy. That tells me that something isn't encapsulated or structured correctly.

[patryk]

Let's abstract away from internals and present the struct that makes sense. And please with no Request/Response fields 🙏

[jterzis]

Sounds good. I've normalized the schema struct and removed bundle_method from the top level struct which was duplicative as @jacobbednarz indicated.

[jacobbednarz]

We're missing some fields from the schema, is this intentional? The API shows the following also available:

• type
• geo_restrictions

[jterzis]

These are not supported by v4api https://github.com/cloudflare/cloudflare-go/blob/master/ssl.go#L40-L44. Ill cut a PR against that repo and then update this one likely in a separate PR.

[jacobbednarz]

🙇 thanks for getting that updated! PR is at cloudflare/cloudflare-go#327

[jacobbednarz]

As the API has restrictions on what values can be used here, it's helpful to provide validation before the HTTP interactions are made.

Suggested change

	Optional: true,
	Optional: true,
	ValidateFunc: validation.StringInSlice([]string{"ubiquitous", "optimal", "force"}, false),

[jterzis]

I was planning on adding validations as a separate ticket. This one establishes minimal functionality for creating, updating, deleting custom_ssl resource. I can add validation for requisite parameters on POST , PUT encapsulating functions though no problem.

[jacobbednarz]

Understandable. If we can, I prefer to have as much validation up front as (even with MVP style PRs) to speed up the feedback loop that users get. For example, our Terraform configuration is across ~40 domains and if we apply changes we can wait up to a couple of minutes for the payload to hit the Cloudflare API only to inform us that we've incorrectly formatted a field or gave it an unexpected value. Compared to schema based validation, we get that almost instantly. It does make the user UX quite a bit nicer and doesn't waste as much time on larger installs 🙂

[jterzis]

Forgot to update this after I added Validation for post fields that take on a set of string values, namely bundle_method, geo_restrictions, type.

[jacobbednarz]

Suggested change

[jterzis]

👍

[jacobbednarz]

Personal preference here (so take it or leave it 😛) but I'd look to return early here and instead of nesting another level as while it's not too big of a deal now, Go is pretty good at getting gnarly nested if expressions. I.e.

data, dataOk := d.GetOk("custom_ssl_priority")

// ..

if !dataOk {
    // Perhaps a better exception than `nil` but 🤷‍♂ 
    return mtSlice, nil
}

for _, innerData := range data.([]interface{}) { 
    newData := make(map[string]interface{})
    // .. continue on
}

[jterzis]

This appears to be more idiomatic so Ill take your suggestion, thanks!

[jacobbednarz]

Suggested change

//newData[id] = newValue

[jterzis]

👍

[jacobbednarz]

If this isn't needed, let's get rid of it.

Suggested change

//return mtSlice, fmt.Errorf("FAILURE: %#v %#v", newData, zcsp)

[jterzis]

👍

[jacobbednarz]

Suggested change

//type mt map[string]interface{}

[jterzis]

👍

[jacobbednarz]

Let's keep it at zoneID as we're in the process of deprecating zoneName from this provider due to the non-unique nature of it in Cloudflare.

[jterzis]

cool, updated

[jacobbednarz]

CLOUDFLARE_ZONE_ID is preferred to CLOUDFLARE_DOMAIN for the uniqueness of it.

[jterzis]

Ok i was applying the pattern we currently have elsewhere. I agree zone id a unique identifier is preferable esp. since the tests create actual resources and cost $ potentially.

[jacobbednarz]

Yeah, sorry about that. We've put the deprecation in place as of the next minor release and I'll be swapping everything over internally once that lands.

[jterzis]

no problem, updated!

[jacobbednarz]

Let's use the generateRandomResourceName() method here to build a resource name for the entropy to denote this isn't a static resource (only used for testing) and in the future when we swap all resource names to begin with tf-acc, this will be one less place we need to manually update.

Suggested change

	resourceName := fmt.Sprintf("cloudflare_custom_ssl.foossl")
	rnd := generateRandomResourceName()
	resourceName := "cloudflare_custom_ssl." + rnd

[jacobbednarz]

This will require a couple of flow on updates in this test but ends up being quite a bit more flexible.

[jterzis]

Sounds good. Updated in commit: 8fa7bea

[patryk]

Should use partial state mode here: https://www.terraform.io/docs/plugins/provider.html#resource-data

[jacobbednarz]

This is causing a failure for the integration tests

------- Stdout: -------
=== RUN   TestAccCloudflareCustomSSL_Basic
=== PAUSE TestAccCloudflareCustomSSL_Basic
=== CONT  TestAccCloudflareCustomSSL_Basic
--- FAIL: TestAccCloudflareCustomSSL_Basic (0.02s)
    testing.go:568: Step 0 error: config is invalid: Unsupported argument: An argument named "options" is not expected here.
FAIL

Suggested change

	options = {
	custom_ssl_options = {

[jterzis]

Odd that this was not picked up by CI. Good catch! I fixed it.

[jacobbednarz]

We have two types of CI running here and only one hooked up to GitHub 🙁 The unit tests passed but these were making the integration tests fail which run on a different system.

We're looking to see if we can have them both hooked into GitHub but it's a chat with Hashicorp as they own the repositories.

[jacobbednarz]

After fixing the issue above, this is now failing with the following error

=== RUN   TestAccCloudflareCustomSSL_Basic
=== PAUSE TestAccCloudflareCustomSSL_Basic
=== CONT  TestAccCloudflareCustomSSL_Basic
--- FAIL: TestAccCloudflareCustomSSL_Basic (0.02s)
    testing.go:568: Step 0 error: config is invalid: Incorrect attribute value type: Inappropriate value for attribute "custom_ssl_options": element "geo_restrictions": string required.
FAIL
FAIL	github.com/terraform-providers/terraform-provider-cloudflare/cloudflare	0.050s

We will need to look at getting these fixed before we merge this in.

[jacobbednarz]

Also, the value usd is invalid here. The schema only allows "us", "eu", "highest_security"

[jterzis]

Fixed as well

[jacobbednarz]

I (and integration tests) are still seeing failures here. It doesn't look like you've addressed the issue with geo_restrictions yet.

[jacobbednarz]

Remainder of the code looks good however the tests are failing. We'll need to sort that out before merge.

[snoopdouglas]

Are we including origin certificates as part of this PR, or will this need to be a separate resource?

Edit: sorry, origin certificates are a discrete resource (API) - so this won't apply