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.

Sign up for GitHub

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

Jump to bottom

Remove Hashing from State Storage #13406

Closed
breathingdust opened this issue May 19, 2020 · 5 comments · Fixed by #14187
Closed

Remove Hashing from State Storage #13406

breathingdust opened this issue May 19, 2020 · 5 comments · Fixed by #14187
Assignees
@bflad
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Milestone
v3.0.0

Comments

@breathingdust
Copy link
Member

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Hashing schema attribute values in the Terraform state storage was implemented as a workaround for attribute-level encryption of some sensitive values. However, Terraform is designed to work with wholly known values during resource operations and performing differences. For example, if a resource only saves a hashed value in the state, during update operations the resource will incorrectly send the hashed value in API operations.

To remove the hashing from resources, any StateFunc must be replaced with a DiffSuppressFunc that does not show a difference between the whole value and the hashed value. State migration is not possible since the previous state value is only the hash of the real value, however any configured values that omit setting the state explicitly via (helper/schema.ResourceData).Set(), should implicitly update the state value with the configuration value on first refresh.

References
@breathingdust breathingdust added breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. technical-debt Addresses areas of the codebase that need refactoring or redesign. labels May 19, 2020
@breathingdust breathingdust added this to the v3.0.0 milestone May 19, 2020
@bflad
Copy link
Contributor

bflad commented May 20, 2020

Affected Resources

  • aws_acm_certificate: certificate_body, certificate_chain, private_key
  • aws_iam_server_certificate: certificate_body, certificate_chain, private_key
  • aws_sns_platform_application: platform_credential, platform_principal

These attributes need separate discussion and should probably remain out of scope:

  • aws_instance: user_data
  • aws_launch_configuration: user_data
  • aws_spot_fleet_request: user_data

Other References

@bflad bflad mentioned this issue May 21, 2020
Closed
bflad added a commit that referenced this issue Jul 14, 2020
@bflad
resource/aws_sns_platform_application: Finalize platform_credential a…
a05aa9e 
…nd platform_princial hash removal

Reference: #3894
Reference: #9951
Reference: #12085
Reference: #13406

This also attempts to prevent the SetPlatformApplicationAttributes API call if no API updates need to occur. While we are in the midst of breaking changes and since this resource cannot be acceptance tested by the HashiCorp maintainers, this also fixes some other technical debt issues.
@bflad
Copy link
Contributor

bflad commented Jul 14, 2020

All set with aws_sns_platform_application -- will work on aws_acm_certificate and aws_iam_server_certificate later today or tomorrow using the community contributions where possible.

@bflad bflad self-assigned this Jul 14, 2020
bflad added a commit that referenced this issue Jul 14, 2020
@bflad
resource/aws_acm_certificate: Finalize state hash removal for certifi…
a299da7 
…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 mentioned this issue Jul 15, 2020
Merged
bflad added a commit that referenced this issue Jul 15, 2020
@bflad
resource/aws_iam_server_certificate: Remove state hashing from certif…
ad459ec 
…icate_body, certificate_chain, and private_key arguments

Reference: #13406

Output from acceptance testing:

```
--- PASS: TestAccAWSIAMServerCertificate_name_prefix (6.05s)
--- PASS: TestAccAWSIAMServerCertificate_disappears (6.10s)
--- PASS: TestAccAWSIAMServerCertificate_Path (6.48s)
--- PASS: TestAccAWSIAMServerCertificate_basic (6.54s)
--- PASS: TestAccAWSIAMServerCertificate_file (10.07s)
```
@bflad bflad mentioned this issue Jul 15, 2020
Merged
@bflad
Copy link
Contributor

bflad commented Jul 23, 2020

Final PR pending team review: #14187

bflad added a commit that referenced this issue Jul 29, 2020
@bflad
resource/aws_iam_server_certificate: Remove state hashing from certif…
e2cea22 
…icate_body, certificate_chain, and private_key arguments

Reference: #13406

Output from acceptance testing:

```
--- PASS: TestAccAWSIAMServerCertificate_name_prefix (6.05s)
--- PASS: TestAccAWSIAMServerCertificate_disappears (6.10s)
--- PASS: TestAccAWSIAMServerCertificate_Path (6.48s)
--- PASS: TestAccAWSIAMServerCertificate_basic (6.54s)
--- PASS: TestAccAWSIAMServerCertificate_file (10.07s)
```
bflad added a commit that referenced this issue Jul 29, 2020
@bflad
resource/aws_iam_server_certificate: Remove state hashing from certif…
a59cbd7 
…icate_body, certificate_chain, and private_key arguments (#14187)

Reference: #13406

Output from acceptance testing:

```
--- PASS: TestAccAWSIAMServerCertificate_name_prefix (6.05s)
--- PASS: TestAccAWSIAMServerCertificate_disappears (6.10s)
--- PASS: TestAccAWSIAMServerCertificate_Path (6.48s)
--- PASS: TestAccAWSIAMServerCertificate_basic (6.54s)
--- PASS: TestAccAWSIAMServerCertificate_file (10.07s)
```
@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 28, 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 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bflad bflad

Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Projects
None yet
Milestone
v3.0.0
2 participants
@bflad @breathingdust