New resource/datasource: SSL Policy #1247

nickjacques · 2018-03-23T15:37:02Z

Add new datasource for SSL Policy
Add new resource for SSL Policy
Add docs for datasource and resource
Add unit tests for datasource and resource

Open questions:

In scenarios where the user has not provided an alphabetized list of custom_features, TF will show a "perpetual diff" of the same contents in a different order. Since I'm a bit new to both developing TF resources and developing in Go, I'm seeking suggestions in how to best handle this.

example terraform:

resource "google_compute_ssl_policy" "test" {
  name = "test"
  profile         = "CUSTOM"
  custom_features = ["TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", "TLS_RSA_WITH_3DES_EDE_CBC_SHA"]
}

example terraform plan output, post-creation of resource:

Terraform will perform the following actions:

  ~ google_compute_ssl_policy.test
      custom_features.0: "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384" => "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"
      custom_features.1: "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384" => "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384"


Plan: 0 to add, 1 to change, 0 to destroy.

In scenarios where the user attempts to update two resource attributes at once, terraform apply will fail with:
```
~ google_compute_ssl_policy.nlj-test2
    min_tls_version:   "TLS_1_2" => "TLS_1_0"
    profile:           "RESTRICTED" => "COMPATIBLE"

...

  Error: Error applying plan:

1 error(s) occurred:

* google_compute_ssl_policy.test: 1 error(s) occurred:

* google_compute_ssl_policy.test: Error updating SSL Policy: googleapi: Error 412: Invalid fingerprint., conditionNotMet
```
- Presumably this is related to partial updates and the fingerprint changing between the two. Is there a good reference for updates that are not partial? I think a "consolidated" or full update may prevent this issue, as it will build the entire object (including all changed parameters) and thus the fingerprint won't change until that update has been processed by the API.

This PR will resolve #1228 and paves the way for #1229 and #1230

danawillow

Thanks @nickjacques, this looks awesome!

danawillow · 2018-03-23T16:20:35Z

google/data_source_google_compute_ssl_policy.go

+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func dataSourceGoogleComputeSslPolicy() *schema.Resource {


We actually have some helpers that can simplify this code a fair bit- take a look at data_source_google_container_cluster.go for an example. These'll help with code re-use, making it easier to keep the datasource/resource in sync in the future.

danawillow · 2018-03-23T16:22:45Z

google/resource_compute_ssl_policy.go

+		Schema: map[string]*schema.Schema{
+
+			"custom_features": &schema.Schema{
+				Type:     schema.TypeList,


To answer your question- what you want here is a schema.TypeSet. There are a whole bunch of examples of these around the codebase but let me know if you get stuck with how to use it!

danawillow · 2018-03-23T16:23:53Z

google/resource_compute_ssl_policy.go

+				ValidateFunc: validation.StringInSlice([]string{"TLS_1_0", "TLS_1_1", "TLS_1_2", "TLS_1_3"}, false),
+			},
+
+			"name": &schema.Schema{


We're trying right now to put attributes in the order Required -> Optional -> Computed-only, if you don't mind reordering these

danawillow · 2018-03-23T16:25:35Z

google/resource_compute_ssl_policy.go

+		return fmt.Errorf("Error creating SSL Policy: %s", err)
+	}
+
+	err = computeSharedOperationWaitTime(config.clientCompute, op, project, int(d.Timeout(schema.TimeoutDelete).Minutes()), "Creating SSL Policy")


typo: schema.TimeoutCreate

danawillow · 2018-03-23T16:27:09Z

google/resource_compute_ssl_policy.go

+		return err
+	}
+
+	d.SetId(sslPolicy.Name)


I usually prefer to set the id between the Insert call and the Wait call- this way, if the terraform process stops for whatever reason, the Id is still set in state, meaning a subsequent refresh will get the created resource. Then if the wait operation stuff fails, you can do a d.SetId("") before returning the error

danawillow · 2018-03-23T17:01:49Z

google/resource_compute_ssl_policy_test.go

+						"google_compute_ssl_policy.update", &sslPolicy),
+					resource.TestCheckResourceAttr(
+						"google_compute_ssl_policy.update", "profile", "MODERN"),
+					//resource.TestCheckResourceAttr(


should this be uncommented?

Ah, this was a result of the Fingerprint issue (the test was originally written to update two attributes). I'll make the update a full instead of partial and these tests should work properly with two attributes.

danawillow · 2018-03-23T17:03:17Z

website/docs/d/datasource_compute_ssl_policy.html.markdown

@@ -0,0 +1,50 @@
+


nit: extra newline

danawillow · 2018-03-23T17:03:40Z

website/docs/d/datasource_compute_ssl_policy.html.markdown

+
+```tf
+data "google_compute_ssl_policy" "my-ssl-policy" {
+  name   = "production-ssl-policy"


nit: extra spaces between name and =

danawillow · 2018-03-23T17:04:31Z

website/docs/d/datasource_compute_ssl_policy.html.markdown

+
+* `custom_features` - If the `profile` is `CUSTOM`, these are the custom encryption
+    ciphers supported by the profile. If the `profile` is *not* `CUSTOM`, this
+    attribute will be nil.


nit: since nil isn't really a terraform concept, let's say empty instead

danawillow · 2018-03-23T17:06:16Z

google/resource_compute_ssl_policy.go

+			"description": &schema.Schema{
+				Type:     schema.TypeString,
+				Optional: true,
+				ForceNew: true,


this is surprising to me, can you confirm that the description can't be changed?

I went off of the details provided in gcloud beta compute ssl-policies update, which doesn't state that description is an updatable field, but I'll try against the API directly and see if it can be patched.

I just tried updating two fields via API Explorer: minTlsVersion and description. The change was applied to minTlsVersion but not description, so I believe the ForceNew: true is valid. If someone wants to change the resource description, a new resource must be created.

nickjacques · 2018-03-23T19:47:29Z

Okay, I think these latest commits resolve most of the comments - thanks for the tip on using Sets @danawillow, that worked perfectly! The only issue I'm encountering now is when updating a resource from:

profile         = "CUSTOM"
custom_features = ["TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", "TLS_RSA_WITH_3DES_EDE_CBC_SHA"]

to something like:

profile = "MODERN"

In this scenario, I receive an error from the underlying API:

google_compute_ssl_policy.test: Modifying... (ID: test)
  custom_features.#:          "3" => "0"
  custom_features.2273472042: "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384" => ""
  custom_features.3685213100: "TLS_RSA_WITH_3DES_EDE_CBC_SHA" => ""
  custom_features.933849229:  "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384" => ""
  profile:                    "CUSTOM" => "COMPATIBLE"

Error: Error applying plan:

1 error(s) occurred:

* google_compute_ssl_policy.test: 1 error(s) occurred:

* google_compute_ssl_policy.test: Error updating SSL Policy: googleapi: Error 400: Invalid value for field 'resource.customFeatures': ''. Custom features cannot be specified when the SslPolicy profile is not 'CUSTOM', invalid

So it seems that I need to delete the sslPolicy.CustomFeatures node before I send the request to the patch API, but I'm unsure of how to do that. I tried setting sslPolicy.CustomFeatures = nil if the length is 0, but I'm still hitting the same error. Once this defect is fixed, I can write a test to ensure we don't regress in the future.

danawillow

For the question about updating- the client library has a feature where it doesn't send fields that are set to the zero value to the API. You have to explicitly include them with ForceSendFields or NullFields. We use these a fair bit in the provider so you should be able to find an example. When you're testing it out, I also highly recommend running with debug logs on (TF_LOG=DEBUG) so you can see the requests/responses to/from the API. That'll help verify that the request you're sending is the one you expect. Hope that helps :)

danawillow · 2018-03-26T16:20:45Z

google/data_source_google_compute_ssl_policy.go

+	addRequiredFieldsToSchema(dsSchema, "name")
+
+	// Set 'Optional' schema elements
+	addOptionalFieldsToSchema(dsSchema, "custom_features", "description", "min_tls_version", "profile", "project")


You probably just want "project" here- an optional field in a data source is one that you can choose to set in the data source itself, rather than all the fields that it might return. Setting something like custom_features wouldn't have any effect, because the data source is read-only.

danawillow · 2018-03-26T16:22:06Z

google/resource_compute_ssl_policy.go

+				Optional: true,
+				Default:  "TLS_1_0",
+				// Although compute-gen.go says that TLS_1_3 is a valid value, the API currently (26 Mar 2018)
+				// responds with an HTTP 200 but doesn't actually create/update the policy.


Oh that's distressing. Is there an issue in the public issue tracker that we could link to here?

Not that I'm aware of, but I can file one if you'd like! Relevant lines are 21109-21118 in compute/v0.beta/compute-gen.go.

If you don't mind, that would be great! That way if someone stumbles across this part of the code and sees a comment with a link to an issue that's been fixed, we'd be able to add TLS_1_3 real quick

Ok, can do! Which product should I file it under? Cloud SDK?

I'm not super familiar with how the products are laid out in components, but if there's one for compute admin api, or networking api, or something along those lines. I'm not going to block this PR on getting that link in there though. I'll finish up the review now :)

No worries, I just added a link to the issue that Aaron created. (Thanks Aaron!)

danawillow · 2018-03-26T16:25:16Z

google/resource_compute_ssl_policy.go

+		return fmt.Errorf("Error updating SSL Policy: %s", err)
+	}
+
+	err = computeSharedOperationWaitTime(config.clientCompute, op, project, int(d.Timeout(schema.TimeoutCreate).Minutes()), "Updating SSL Policy")


This should probably be schema.TimeoutUpdate

danawillow · 2018-03-26T16:28:12Z

google/resource_compute_ssl_policy_test.go

+	return nil
+}
+
+func testAccCheckComputeSslPolicyCustomFeatures(sslPolicy *computeBeta.SslPolicy, n string) resource.TestCheckFunc {


You can actually get rid of this test function- the import check will verify this on its own (it'll call read and then try to populate state from it and compare it to the config, so if custom_features wasn't set upstream then the import would fail)

danawillow · 2018-03-26T16:28:47Z

website/docs/d/datasource_compute_ssl_policy.html.markdown

+  Gets an SSL Policy within GCE, for use with Target HTTPS and Target SSL Proxies.
+---
+
+# google\_compute\_ssl\_policies


s/policies/policy

danawillow · 2018-03-26T16:28:58Z

website/docs/r/compute_ssl_policy.html.markdown

+  Manages an SSL Policy within GCE, for use with Target HTTPS and Target SSL Proxies.
+---
+
+# google\_compute\_ssl\_policies


nickjacques · 2018-03-26T20:54:08Z

Thanks @danawillow NullFields was indeed exactly what I needed! Updates that enable and disable custom_features now work as expected, and I also have included tests for these scenarios. I've also added enabled_features as a computed field that folks can use to output what cipher suites resulted from their policy configuration. I think this is ready to go if things look good to you!

danawillow

Looks great! Just a few small nitpicks, if you don't mind :)

danawillow · 2018-03-26T22:31:03Z

google/resource_compute_ssl_policy.go

+		Fingerprint: d.Get("fingerprint").(string),
+	}
+
+	if v, ok := d.GetOk("profile"); ok {


nit: There isn't actually any difference between doing it this way and just setting it in the struct, since if the value is empty it'll be the empty string in the struct regardless.

danawillow · 2018-03-26T22:40:19Z

google/resource_compute_ssl_policy_test.go

+					resource.TestCheckResourceAttr(
+						"google_compute_ssl_policy.update", "min_tls_version", "TLS_1_0"),
+				),
+			},


let's also add the import test step here (and similarly after all test steps) juuuuuuuust in case

nickjacques · 2018-03-26T23:05:32Z

No worries at all! I've added Profile and minTlsVersion back into the struct, and dropped in imports between steps in the multi-step tests.

danawillow · 2018-03-26T23:53:30Z

👍

* Add SSL Policy to provider * Add resource for SSL Policy * Add SSL Policy data source * Add tests for SSL Policy resource * Add documentation for SSL Policy resource * Add SSL Policy datasource docs * Add test for SSL Policy datasource * Update SSL Policy datasource docs * Make full update for SSL Policy resource * SSL Policy resource test multi-attrib update * Clean up SSL Policy datasource * Set-ify custom_features in SSL Policy resource * Document description ForceNew rationale * Remove refs to TLS_1_3 * Update docs: plural -> singular * Remove extraneous attrs from datasource * Fix update logic for custom_features and add enabled_features * Update docs to include enabled_features * Add test for updating to/from custom_features * Add TLS 1.3 bug link * Add import between multi-step test configs * Move Profile and minTlsVersion back into sslPolicy struct

ghost · 2018-11-19T13:58:16Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

danawillow self-requested a review March 23, 2018 16:17

danawillow self-assigned this Mar 23, 2018

danawillow suggested changes Mar 23, 2018

View reviewed changes

danawillow reviewed Mar 26, 2018

View reviewed changes

nickjacques added 19 commits March 26, 2018 15:49

Add SSL Policy to provider

7857711

Add resource for SSL Policy

697f803

Add documentation for SSL Policy resource

7364017

Add SSL Policy data source

e68b281

Add SSL Policy datasource docs

df7dc62

Add test for SSL Policy datasource

1de363e

Add tests for SSL Policy resource

a6ebbdc

Update SSL Policy datasource docs

825efb9

Make full update for SSL Policy resource

1b0c44c

SSL Policy resource test multi-attrib update

efe226c

Clean up SSL Policy datasource

9a50864

Set-ify custom_features in SSL Policy resource

aaf6a86

Document description ForceNew rationale

2ed3f53

Remove refs to TLS_1_3

e0d6ab3

Update docs: plural -> singular

59072b5

Remove extraneous attrs from datasource

4b2513b

Fix update logic for custom_features and add enabled_features

650553a

Update docs to include enabled_features

d7fb0b7

Add test for updating to/from custom_features

98d06c0

Add TLS 1.3 bug link

3a9f2c3

danawillow approved these changes Mar 26, 2018

View reviewed changes

nickjacques added 2 commits March 26, 2018 18:02

Add import between multi-step test configs

bd84ef4

Move Profile and minTlsVersion back into sslPolicy struct

5c9ff2e

danawillow merged commit 72efdb1 into hashicorp:master Mar 26, 2018

ghost locked and limited conversation to collaborators Nov 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource/datasource: SSL Policy #1247

New resource/datasource: SSL Policy #1247

nickjacques commented Mar 23, 2018 •

edited

Loading

danawillow left a comment

danawillow Mar 23, 2018

danawillow Mar 23, 2018

danawillow Mar 23, 2018

danawillow Mar 23, 2018

danawillow Mar 23, 2018

danawillow Mar 23, 2018

nickjacques Mar 23, 2018

danawillow Mar 23, 2018

danawillow Mar 23, 2018

danawillow Mar 23, 2018

danawillow Mar 23, 2018

nickjacques Mar 23, 2018

nickjacques Mar 23, 2018

nickjacques commented Mar 23, 2018 •

edited

Loading

danawillow left a comment

danawillow Mar 26, 2018

danawillow Mar 26, 2018

nickjacques Mar 26, 2018

danawillow Mar 26, 2018

nickjacques Mar 26, 2018

danawillow Mar 26, 2018

nickjacques Mar 26, 2018

danawillow Mar 26, 2018

danawillow Mar 26, 2018

danawillow Mar 26, 2018

danawillow Mar 26, 2018

nickjacques commented Mar 26, 2018

danawillow left a comment

danawillow Mar 26, 2018

danawillow Mar 26, 2018

nickjacques commented Mar 26, 2018

danawillow commented Mar 26, 2018

ghost commented Nov 19, 2018

New resource/datasource: SSL Policy #1247

New resource/datasource: SSL Policy #1247

Conversation

nickjacques commented Mar 23, 2018 • edited Loading

danawillow 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

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

nickjacques commented Mar 23, 2018 • edited Loading

danawillow 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickjacques commented Mar 26, 2018

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickjacques commented Mar 26, 2018

danawillow commented Mar 26, 2018

ghost commented Nov 19, 2018

nickjacques commented Mar 23, 2018 •

edited

Loading

nickjacques commented Mar 23, 2018 •

edited

Loading