provider/heroku: Fix heroku_cert update of ssl cert #14240

wchrisjohnson · 2017-05-05T14:10:15Z

This change fixes #5930
Also:

slight refactor of the update method for the sake of logging.
beefed up tests around SSL endpoint
broke out tests to differentiate EU vs US endpoints.

catsby · 2017-05-05T20:57:23Z

builtin/providers/heroku/resource_heroku_cert.go

 		if err != nil {
-			return err
+			return fmt.Errorf("Error updating SSL endpoint: %s", err)
 		}

 		// Store the new ID
 		d.SetId(ad.ID)


I know this line isn't changing here, but it caught our eye. Is the ID of the SSL Cert changing? If so, either the private_key or certificate_chain attribute (or both?) should have ForceNew specified and instruct terraform to do a complete destroy and recreate. Updating ID here could result in unexpected behavior (dangling resources), or just undefined behavior, depending on the provider.

The client methods being called, and the underlying PATCH API call, suggests to me the ID is not actually changing and we should remove this line while we are here, if you could.

Wow, that's great feedback. This is my first PR against Terraform, and I'm still learning how it works. I will do some testing and handle this, or come back with additional questions...

@catsby I just verified that the ID is NOT changing between the create and the update, so I will remove this line.

Trying to surface this bug with a test: hashicorp#5930

wchrisjohnson · 2017-05-09T14:46:02Z

Could someone provide feedback on what, if anything, remaining to be done in order to get this merged?

catsby · 2017-05-09T16:23:38Z

Hey @wchrisjohnson – I ran the tests and got the following error:

[pr-14240]$ make testacc TEST=./builtin/providers/heroku TESTARGS="-run=TestAccHerokuCert"
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/09 11:20:12 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/heroku -v -run=TestAccHerokuCert -timeout 120m
=== RUN   TestAccHerokuCert_EU
--- FAIL: TestAccHerokuCert_EU (18.65s)
        testing.go:280: Step 0 error: Error applying: 1 error(s) occurred:

                * heroku_cert.ssl_certificate: 1 error(s) occurred:

                * heroku_cert.ssl_certificate: Error creating SSL endpoint: Post https://api.heroku.com/apps/tftest-jtvdruccq7/ssl-endpoints: Conflict
=== RUN   TestAccHerokuCert_US
--- PASS: TestAccHerokuCert_US (32.36s)
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/heroku 51.030s
make: *** [testacc] Error 1

Is that a limitation of my test account?

wchrisjohnson · 2017-05-09T16:58:37Z

@catsby It works on my end. Try the following:

$ HEROKU_API_KEY=<REDACTED> make testacc TEST=./builtin/providers/heroku TESTARGS="-run=TestAccHerokuCert"
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/09 12:55:29 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/heroku -v -run=TestAccHerokuCert -timeout 120m
=== RUN   TestAccHerokuCert_EU
--- PASS: TestAccHerokuCert_EU (23.20s)
=== RUN   TestAccHerokuCert_US
--- PASS: TestAccHerokuCert_US (20.11s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/heroku	43.333s

catsby · 2017-05-09T21:24:09Z

I have all the Heroku things I should need in the environment:

HEROKU_ORGANIZATION=terraform
HEROKU_EMAIL=terraform@...
HEROKU_API_KEY=abc123-redacted

I ran the tests again, strangely I'm getting a different error now, this time with the US SSL, not the EU one:

[pr-14240](4)$ make testacc TEST=./builtin/providers/heroku TESTARGS="-run=TestAccHerokuCert"
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/09 16:20:30 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/heroku -v -run=TestAccHerokuCert -timeout 120m
=== RUN   TestAccHerokuCert_EU
--- PASS: TestAccHerokuCert_EU (22.25s)
=== RUN   TestAccHerokuCert_US
--- FAIL: TestAccHerokuCert_US (13.14s)
        testing.go:280: Step 1 error: Error applying: 1 error(s) occurred:

                * heroku_cert.ssl_certificate: 1 error(s) occurred:

                * heroku_cert.ssl_certificate: Error updating SSL endpoint: Patch https://api.heroku.com/apps/tftest-yf43o3l4ph/ssl-endpoints/9609ba7d-9d82-4426-8cf9-fdbbd9464ec5: Could not complete the action. Please try again later.
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/heroku 35.422s
make: *** [testacc] Error 1

catsby · 2017-05-09T21:27:45Z

I did manage to get them to pass, but the inconsistency is troubling, do you have any thoughts there?

wchrisjohnson · 2017-05-10T00:38:28Z

yeah @catsby I have seen that specific error crop up periodically in my running of the test suite as well. It feels like some sort of race condition, as it doesn't occur consistently. Let me dig into this on my end a bit more.

catsby · 2017-05-10T15:54:29Z

@wchrisjohnson thanks! Feel free to ping me if/when you find out 😄

wchrisjohnson · 2017-05-10T17:31:09Z

@catsby I've been talking to some other engineers here at Heroku; there are some upstream conditions where rapid create/update of addon and SSL endpoints can fail, but can safely be retried. We're diagnosing the issue but believe it's unrelated to the changes in the PR.

catsby · 2017-05-10T20:52:26Z

OK, sounds good 👍

catsby

👍

* Attempt to write a new test for cert update Trying to surface this bug with a test: hashicorp#5930 * Fix the error * Fix the test for the update operation * Break apart tests for EU vs US to cleanse test run * Refactor Update to more closely match create, increase debug logging * Reflect differences of EU and US regions via separate tests * Add comment re: why of test breakout * Removed the “SetId” as it was unnecessary * Ensure the SSL Addon has been provisioned

ghost · 2020-04-12T02:35:45Z

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.

apparentlymart added provider/heroku bug labels May 5, 2017

wchrisjohnson changed the title ~~Fix heroku_cert update of ssl cert~~ provider/heroku Fix heroku_cert update of ssl cert May 5, 2017

wchrisjohnson changed the title ~~provider/heroku Fix heroku_cert update of ssl cert~~ provider/heroku: Fix heroku_cert update of ssl cert May 5, 2017

catsby reviewed May 5, 2017

View reviewed changes

catsby added the waiting-response An issue/pull request is waiting for a response from the community label May 5, 2017

wchrisjohnson added 8 commits May 5, 2017 21:04

Attempt to write a new test for cert update

3fa2dff

Trying to surface this bug with a test: hashicorp#5930

Fix the error

cf0b03c

Fix the test for the update operation

5e34eb9

Break apart tests for EU vs US to cleanse test run

28178b9

Refactor Update to more closely match create, increase debug logging

5c779d2

Reflect differences of EU and US regions via separate tests

8e91b6c

Add comment re: why of test breakout

da375cd

Removed the “SetId” as it was unnecessary

00d0061

wchrisjohnson force-pushed the cj-fix-ssl-cert-update branch from c5d78ad to 00d0061 Compare May 6, 2017 01:04

catsby removed the waiting-response An issue/pull request is waiting for a response from the community label May 8, 2017

catsby added the waiting-response An issue/pull request is waiting for a response from the community label May 9, 2017

Ensure the SSL Addon has been provisioned

3c05047

catsby removed the waiting-response An issue/pull request is waiting for a response from the community label May 10, 2017

catsby approved these changes May 10, 2017

View reviewed changes

catsby merged commit d1b5eac into hashicorp:master May 10, 2017

wchrisjohnson mentioned this pull request May 10, 2017

provider/heroku: Ensure the tests for heroku_cert work #14371

Merged

ghost locked and limited conversation to collaborators Apr 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/heroku: Fix heroku_cert update of ssl cert #14240

provider/heroku: Fix heroku_cert update of ssl cert #14240

wchrisjohnson commented May 5, 2017

catsby May 5, 2017

wchrisjohnson May 5, 2017

wchrisjohnson May 6, 2017

wchrisjohnson commented May 9, 2017

catsby commented May 9, 2017

wchrisjohnson commented May 9, 2017

catsby commented May 9, 2017

catsby commented May 9, 2017

wchrisjohnson commented May 10, 2017

catsby commented May 10, 2017

wchrisjohnson commented May 10, 2017

catsby commented May 10, 2017

catsby left a comment

ghost commented Apr 12, 2020

provider/heroku: Fix heroku_cert update of ssl cert #14240

provider/heroku: Fix heroku_cert update of ssl cert #14240

Conversation

wchrisjohnson commented May 5, 2017

catsby May 5, 2017

Choose a reason for hiding this comment

wchrisjohnson May 5, 2017

Choose a reason for hiding this comment

wchrisjohnson May 6, 2017

Choose a reason for hiding this comment

wchrisjohnson commented May 9, 2017

catsby commented May 9, 2017

wchrisjohnson commented May 9, 2017

catsby commented May 9, 2017

catsby commented May 9, 2017

wchrisjohnson commented May 10, 2017

catsby commented May 10, 2017

wchrisjohnson commented May 10, 2017

catsby commented May 10, 2017

catsby left a comment

Choose a reason for hiding this comment

ghost commented Apr 12, 2020