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

resource/aws_cloudfront_distribution: Get rid of flatmap dependency #8902

Closed
radeksimko opened this issue Jun 7, 2019 · 9 comments · Fixed by #10013 or #14339
Closed

resource/aws_cloudfront_distribution: Get rid of flatmap dependency #8902

radeksimko opened this issue Jun 7, 2019 · 9 comments · Fixed by #10013 or #14339
Assignees
Labels
service/cloudfront Issues and PRs that pertain to the cloudfront service. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Milestone

Comments

@radeksimko
Copy link
Member

As part of some preparation work for SDK decoupling I found this provider imports a legacy flatmap package - this isn't the way we plan to represent data in the SDK in the future anymore and it would be awesome if we could reduce the "core surface" exposed to providers by removing this dependency/import.

The CloudFront Distribution resource apparently uses this for "flattening" SDK struct into a map here

https://github.com/terraform-providers/terraform-provider-aws/blob/b13ffce6248b6eadda3cd9ffd1664ac1aafe5f39/aws/cloudfront_distribution_configuration_structure.go#L1100-L1116

the affected field active_trusted_signers is TypeMap
https://github.com/terraform-providers/terraform-provider-aws/blob/b13ffce6248b6eadda3cd9ffd1664ac1aafe5f39/aws/resource_aws_cloudfront_distribution.go#L697-L700

which makes me doubt whether this actually ever worked - there isn't any test to prove it and I don't think we have much successful history of using TypeMap for nested structures.

I suppose the easiest way to resolve this would be migrate the schema to TypeList & MaxItems: 1?

@radeksimko radeksimko added the technical-debt Addresses areas of the codebase that need refactoring or redesign. label Jun 7, 2019
@bflad bflad added the service/cloudfront Issues and PRs that pertain to the cloudfront service. label Jun 10, 2019
@apparentlymart
Copy link
Contributor

In Terraform 0.12 I would expect Terraform Core to be picky about values for this argument being exactly lists of strings, and so this would be appearing in the rest of the configuration as a map with the keys items.#, items.0.aws_account_number, items.0.key_pair_ids.#, items.0.key_pair_ids.0, etc.

Probably in 0.11 this tricked the interpolator into converting this structure into a list of maps, because it only had the flatmap keys to work from, but the new protocol shims will see that this is a map and will serialize it as one so Terraform 0.12 will not be so easily fooled.

Changing this to be (essentially) list(object({aws_account_number=string, key_pair_ids=list(string)})) would make it behave differently in Terraform 0.12, because the language will then see it as a real nested data structure, rather than as a flat map. It might also require a state migration for anyone who already created their cloudfront distribution with Terraform 0.12 and so has this saved in the new structured storage format.

Leaving it as a map of string using flatmap-style keys -- either by hand-writing some simpler, more specialized code to generate it just for this case, or by copying the flatmap package into the AWS provider -- is probably the best way to keep this compatible for both Terraform 0.11 and 0.12 users.

@bflad bflad self-assigned this Sep 5, 2019
bflad added a commit that referenced this issue Sep 5, 2019
…tribute for Terraform 0.12

Reference: #8902

This attribute implemented a legacy Terraform library (`flatmap`), which does not work with Terraform 0.12's data types and whose only usage was on this single attribute. This was missed during the Terraform 0.12 upgrade since there were no covering acceptance tests and the CloudFront setup itself is fairly esoteric (requires root AWS account login to manage CloudFront Keys).

The attribute now correctly implements (in the closest approximation) the standard Terraform Provider SDK types and methodology for saved nested object data to the Terraform state. The `items` subattribute list is now accessible again in Terraform 0.12. It does, however, unavoidably switch the root `active_trusted_signers` attribute to `TypeList` instead of `TypeMap` (which does not support map elements of different types in the current Terraform Provider SDK), so any potential references to nested attributes will need to be changed in user configurations, e.g.

Previously:

```
aws_cloudfront_distribution.example.active_trusted_signers.enabled
```

Now:

```
aws_cloudfront_distribution.example.active_trusted_signers.0.enabled
```

Output from acceptance testing:

```
--- PASS: TestAccAWSCloudFrontDistribution_DefaultCacheBehavior_TrustedSigners (611.33s)
```
@bflad
Copy link
Contributor

bflad commented Sep 5, 2019

Pull request submitted: #10013

@bflad bflad added this to the v2.28.0 milestone Sep 6, 2019
@bflad
Copy link
Contributor

bflad commented Sep 6, 2019

flatmap usage has been removed. 🎉

@ghost
Copy link

ghost commented Sep 12, 2019

This has been released in version 2.28.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!

@bflad bflad removed this from the v2.28.0 milestone Sep 12, 2019
@bflad
Copy link
Contributor

bflad commented Sep 12, 2019

Reopening as #10013 will be reverted by #10093

@bflad bflad reopened this Sep 12, 2019
@bflad bflad added this to the v3.0.0 milestone Sep 23, 2019
bflad added a commit that referenced this issue Sep 25, 2019
…lly for now, add test covering existing active_trusted_signers behavior

Reference: #8902
Reference: #10013
Reference: #10093

Temporarily keep the existing behavior for the only provider attribute using the legacy package so we can migrate to the standalone Terraform Plugin SDK from the github.com/hashicorp/terraform dependency in the future. Switching this attribute to not use the flatmap package will require state migration or deprecation.

Output from acceptance testing:

```
--- PASS: TestAccAWSCloudFrontDistribution_DefaultCacheBehavior_TrustedSigners (1351.69s)
```
@bflad
Copy link
Contributor

bflad commented Sep 25, 2019

For now, this will move the flatmap package internally, so we can still migrate away from using the github.com/hashicorp/terraform dependency: #10244

@anGie44 anGie44 self-assigned this Jul 24, 2020
anGie44 pushed a commit that referenced this issue Jul 24, 2020
…tribute for Terraform 0.12

Reference: #8902

This attribute implemented a legacy Terraform library (`flatmap`), which does not work with Terraform 0.12's data types and whose only usage was on this single attribute. This was missed during the Terraform 0.12 upgrade since there were no covering acceptance tests and the CloudFront setup itself is fairly esoteric (requires root AWS account login to manage CloudFront Keys).

The attribute now correctly implements (in the closest approximation) the standard Terraform Provider SDK types and methodology for saved nested object data to the Terraform state. The `items` subattribute list is now accessible again in Terraform 0.12. It does, however, unavoidably switch the root `active_trusted_signers` attribute to `TypeList` instead of `TypeMap` (which does not support map elements of different types in the current Terraform Provider SDK), so any potential references to nested attributes will need to be changed in user configurations, e.g.

Previously:

```
aws_cloudfront_distribution.example.active_trusted_signers.enabled
```

Now:

```
aws_cloudfront_distribution.example.active_trusted_signers.0.enabled
```

Output from acceptance testing:

```
--- PASS: TestAccAWSCloudFrontDistribution_DefaultCacheBehavior_TrustedSigners (611.33s)
```

(cherry picked from commit ebdc976)
@anGie44
Copy link
Contributor

anGie44 commented Jul 29, 2020

Re-introducing the changes originally made in #10013 is now merged with #14339 and will release with the upcoming v3.0.0 of the Terraform AWS Provider

@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 29, 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 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/cloudfront Issues and PRs that pertain to the cloudfront service. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Projects
None yet
4 participants