Support path-only when referencing ssl certificate in compute_target_https_proxy #210

rosbo · 2017-07-17T23:23:43Z

issue #47

Added support for both full url and path-only when referencing ssl certificate in * compute_target_https_proxy. This means both https://www.googleapis.com/compute/v1/projects/MY-PROJECT/global/sslCertificates/MY_CERT and projects/MY-PROJECT/global/sslCertificates/MY_CERT are valid.
Simplified the read/update logic for ssl certificate since a proxy must have exactly one ssl certificate.
Added more assertions to acceptance test
Added schema validation of ssl certificate field

…https_proxy

danawillow

Looks good overall, just a few things.

make testacc TEST=./google TESTARGS='-run=TestAccComputeTargetHttpsProxy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccComputeTargetHttpsProxy -timeout 120m
=== RUN   TestAccComputeTargetHttpsProxy_basic
--- PASS: TestAccComputeTargetHttpsProxy_basic (86.83s)
=== RUN   TestAccComputeTargetHttpsProxy_update
--- PASS: TestAccComputeTargetHttpsProxy_update (109.60s)
=== RUN   TestAccComputeTargetHttpsProxy_invalidCertificate
--- PASS: TestAccComputeTargetHttpsProxy_invalidCertificate (0.01s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	196.585s

@rileykarson want to do a quick check on the validate/statefunc stuff since you've done a lot of thinking around those?

danawillow · 2017-07-18T17:48:02Z

google/resource_compute_target_https_proxy.go

-		for k, _ := range _newMap {
-			sslCertificates = append(sslCertificates, k)
-		}
+		// Exactly one ssl certificate must be specified.


Can you confirm that the API call actually fails if more than one is specified? If it silently lets it go through, we might be breaking people with this.

The google cloud documentation for this resource is misleading, it says "Currently, exactly one SSL certificate must be specified." but you can specify more than one and it works. I sent feedback to that team to fix their documentation.

I updated the code, Terraform documentation and the test to confirm we can support more than one ssl certificate.

danawillow · 2017-07-18T17:49:47Z

google/resource_compute_target_https_proxy.go


 		cert_ref := &compute.TargetHttpsProxiesSetSslCertificatesRequest{
-			SslCertificates: sslCertificates,
+			SslCertificates: certs,


This can be simplified a bit to

SslCertificates: []string{d.Get("ssl_certificates.0")}

Not applicable anymore.

danawillow · 2017-07-18T17:54:40Z

google/resource_compute_target_https_proxy_test.go

+		CheckDestroy: testAccCheckComputeTargetHttpsProxyDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: `


Can you separate this config into a separate var at the bottom with the rest?

danawillow · 2017-07-18T17:59:28Z

google/resource_compute_target_https_proxy_test.go

-}
+func testAccComputeTargetHttpsProxy_basic1(id string) string {
+	return fmt.Sprintf(`
+		resource "google_compute_target_https_proxy" "foobar" {


It's nice to keep these all the way to the left- it's not really necessary, just makes it easier to copy and paste

rileykarson

Concerning Validate and State funcs: LGTM - I spotted a minor nit in the schema fields, but the ValidateFunc and StateFunc both look good.

rileykarson · 2017-07-18T18:38:40Z

google/resource_compute_target_https_proxy.go

+					ValidateFunc: validateRegexp(sslCertificateRegex),
+					StateFunc:    toCanonicalSslCertificate,
+				},
+				MinItems: 1,


Minor nit: having Required set implicitly sets MinItems to 1.

rileykarson · 2017-07-18T18:41:35Z

google/resource_compute_target_https_proxy.go

+	value := v.(string)
+
+	m := regexp.MustCompile(sslCertificateRegex).FindStringSubmatch(value)
+	if m == nil {


I'm just curious - will m ever be nil? I think our ValidateFunc makes sure that we will always see matches here.

It should never be nil. Removed this line.

danawillow · 2017-07-19T20:18:33Z

Looks good, thanks! Merge at will.

…https_proxy (hashicorp#210)

…ashicorp#210)

ghost · 2020-03-31T14:10:59Z

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!

Support path-only when referencing ssl certificate in compute_target_…

c838243

…https_proxy

danawillow suggested changes Jul 18, 2017

View reviewed changes

rileykarson approved these changes Jul 18, 2017

View reviewed changes

rosbo added 2 commits July 18, 2017 13:10

Address Dana and Riley's comments

2abe500

Fix test

71211fb

rosbo force-pushed the https_proxy_malformed_ssl_certificate_url branch from 84b1304 to 71211fb Compare July 18, 2017 20:24

danawillow approved these changes Jul 19, 2017

View reviewed changes

rosbo merged commit d9abcd8 into hashicorp:master Jul 19, 2017

rosbo deleted the https_proxy_malformed_ssl_certificate_url branch July 19, 2017 20:49

rosbo mentioned this pull request Jul 27, 2017

Resource: google_compute_target_https_proxy; apply/plan/refresh fails to update tfstate correctly #244

Closed

paddycarver mentioned this pull request Aug 10, 2017

provider/google: Improved SSL certs handling on target_https_proxy hashicorp/terraform#14264

Closed

negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017

Support path-only when referencing ssl certificate in compute_target_…

2ef3fd5

…https_proxy (hashicorp#210)

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018

Support path-only when referencing ssl certificate in compute_target_…

d6ea7db

…https_proxy (hashicorp#210)

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

send scheduling block with automaticrestart if there is none in cfg (h…

1663bb8

…ashicorp#210)

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support path-only when referencing ssl certificate in compute_target_https_proxy #210

Support path-only when referencing ssl certificate in compute_target_https_proxy #210

rosbo commented Jul 17, 2017 •

edited

Loading

danawillow left a comment

danawillow Jul 18, 2017

rosbo Jul 18, 2017

danawillow Jul 18, 2017

rosbo Jul 18, 2017 •

edited

Loading

danawillow Jul 18, 2017

rosbo Jul 18, 2017

danawillow Jul 18, 2017

rosbo Jul 18, 2017

rileykarson left a comment

rileykarson Jul 18, 2017

rosbo Jul 18, 2017

rileykarson Jul 18, 2017

rosbo Jul 18, 2017

danawillow commented Jul 19, 2017

ghost commented Mar 31, 2020

Support path-only when referencing ssl certificate in compute_target_https_proxy #210

Support path-only when referencing ssl certificate in compute_target_https_proxy #210

Conversation

rosbo commented Jul 17, 2017 • 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

rosbo Jul 18, 2017 • edited Loading

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

rileykarson 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

danawillow commented Jul 19, 2017

ghost commented Mar 31, 2020

rosbo commented Jul 17, 2017 •

edited

Loading

rosbo Jul 18, 2017 •

edited

Loading