Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix update of imported ACM certificates #9685

Merged
merged 13 commits into from
Jul 14, 2020

Conversation

julienduchesne
Copy link
Contributor

The update for the ACM certificates did not work for imported (external) certificates. The reason for that is that only the hash of the certificates was stored in the state. The API call requires all three parts (chain, body and private key) of a certificate when updating.

Therefore, an update would only work if you changed all three parts, otherwise, it would try to send the hash instead of the actual certificate.

I tried to hack something together with CustomizeDiff but I couldn't manage to force Terraform to go re-read the source of the unchanged parts instead of getting them from the state when one of the three parts has changed.

The way I made it work was by putting the certificate in the state

Output from acceptance testing:

TF_ACC=1 go test  github.com/terraform-providers/terraform-provider-aws/aws -run "^(TestAccAWSAcmCertificate_imported_DomainName)$" -v
=== RUN   TestAccAWSAcmCertificate_imported_DomainName
=== PAUSE TestAccAWSAcmCertificate_imported_DomainName
=== CONT  TestAccAWSAcmCertificate_imported_DomainName
--- PASS: TestAccAWSAcmCertificate_imported_DomainName (76.78s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       76.819s

Unverified

No user is associated with the committer email.
@julienduchesne julienduchesne requested a review from a team August 8, 2019 16:56
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/acm Issues and PRs that pertain to the acm service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Aug 8, 2019

Unverified

No user is associated with the committer email.
@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Oct 22, 2019

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…ted-certificates-update
@jocgir jocgir force-pushed the fix-imported-certificates-update branch from f65576f to 21dd050 Compare October 22, 2019 16:24
Julien Duchesne added 3 commits October 22, 2019 12:44

Unverified

No user is associated with the committer email.
…o fix-imported-certificates-update

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.
…' into fix-imported-certificates-update
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Oct 22, 2019
jocgir and others added 2 commits October 22, 2019 14:53

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Remove an extra empty line

Unverified

No user is associated with the committer email.
@edahlseng
Copy link

Linking everything up to be pedantic, it looks like this should address #10847, and perhaps #9809.

@bflad bflad added this to the v3.0.0 milestone Dec 2, 2019
Julien Duchesne and others added 4 commits December 3, 2019 09:51

Unverified

No user is associated with the committer email.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…ted-certificates-update

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…ted-certificates-update

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…ted-certificates-update
@bflad bflad self-assigned this Jul 14, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulling this in as part of our 3.0.0 work, thanks @julienduchesne 🚀 Please note that the major version will not include the DiffSuppressFunc (and some logic to not call the API on hash removal only updates) to encourage updating the state immediately. 👍

Output from acceptance testing (failure related to other upcoming 3.0.0 work):

--- FAIL: TestAccAWSAcmCertificate_san_multiple (23.72s)

--- PASS: TestAccAWSAcmCertificate_disableCTLogging (14.97s)
--- PASS: TestAccAWSAcmCertificate_dnsValidation (16.95s)
--- PASS: TestAccAWSAcmCertificate_emailValidation (18.91s)
--- PASS: TestAccAWSAcmCertificate_imported_DomainName (28.06s)
--- PASS: TestAccAWSAcmCertificate_imported_IpAddress (11.75s)
--- PASS: TestAccAWSAcmCertificate_privateCert (20.73s)
--- PASS: TestAccAWSAcmCertificate_root (14.59s)
--- PASS: TestAccAWSAcmCertificate_root_TrailingPeriod (15.02s)
--- PASS: TestAccAWSAcmCertificate_rootAndWildcardSan (15.84s)
--- PASS: TestAccAWSAcmCertificate_san_single (19.04s)
--- PASS: TestAccAWSAcmCertificate_san_TrailingPeriod (19.81s)
--- PASS: TestAccAWSAcmCertificate_tags (39.78s)
--- PASS: TestAccAWSAcmCertificate_wildcard (20.89s)
--- PASS: TestAccAWSAcmCertificate_wildcardAndRootSan (19.73s)

bflad added a commit that referenced this pull request Jul 14, 2020
…cate_body, certificate_chain, and private_key arguments

Reference: #9685
Reference: #13053
Reference: #13406

Output from acceptance testing (failure related to other upcoming 3.0.0 work):

```
--- FAIL: TestAccAWSAcmCertificate_san_multiple (23.72s)

--- PASS: TestAccAWSAcmCertificate_disableCTLogging (14.97s)
--- PASS: TestAccAWSAcmCertificate_dnsValidation (16.95s)
--- PASS: TestAccAWSAcmCertificate_emailValidation (18.91s)
--- PASS: TestAccAWSAcmCertificate_imported_DomainName (28.06s)
--- PASS: TestAccAWSAcmCertificate_imported_IpAddress (11.75s)
--- PASS: TestAccAWSAcmCertificate_privateCert (20.73s)
--- PASS: TestAccAWSAcmCertificate_root (14.59s)
--- PASS: TestAccAWSAcmCertificate_root_TrailingPeriod (15.02s)
--- PASS: TestAccAWSAcmCertificate_rootAndWildcardSan (15.84s)
--- PASS: TestAccAWSAcmCertificate_san_single (19.04s)
--- PASS: TestAccAWSAcmCertificate_san_TrailingPeriod (19.81s)
--- PASS: TestAccAWSAcmCertificate_tags (39.78s)
--- PASS: TestAccAWSAcmCertificate_wildcard (20.89s)
--- PASS: TestAccAWSAcmCertificate_wildcardAndRootSan (19.73s)
```
@bflad bflad merged commit 8cd743b into hashicorp:master Jul 14, 2020
bflad added a commit that referenced this pull request Jul 14, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ghost
Copy link

ghost commented Jul 31, 2020

This has been released in version 3.0.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Aug 14, 2020

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. Thanks!

@ghost ghost locked and limited conversation to collaborators Aug 14, 2020
@julienduchesne julienduchesne deleted the fix-imported-certificates-update branch September 8, 2020 16:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/acm Issues and PRs that pertain to the acm service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants