provider: Stabilization of singular data source id attributes with clear identifiers #15399
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ear identifiers Reference: #14579 Reference: https://github.com/bflad/tfproviderlint/tree/master/passes/R015 Reference: https://github.com/bflad/tfproviderlint/tree/master/passes/R016 Reference: https://github.com/bflad/tfproviderlint/tree/master/passes/R017 Reference: https://www.terraform.io/docs/extend/best-practices/versioning.html#example-major-number-increments Reference: https://registry.terraform.io/providers/hashicorp/random/ Reference: https://registry.terraform.io/providers/hashicorp/time/ Terraform 0.13 reworked data source reads into the plan graph, which had some unintentional consequences with Terraform Plugin SDK and provider behaviors that were previously ignored when displaying plan differences. While some of these differences dealing with empty and missing blocks could be addressed with extra graph logic, there remains problematic behaviors that will not be addressable in the near future in core or the SDK. This change set uses the `tfproviderlint` R015, R016, and R017 checks to find `(*schema.ResourceData).SetId()` usage with unstable values such as the current time and per-execution random identifiers and where the identifier can be stablized based on the data source purpose and surrounding context. For example, singular data sources that represent an analogous managed resource can use the same identifier (e.g. `aws_acm_certificate`, `aws_autoscaling_group`). Other cases where the data source represents information from an AWS Partition or AWS Region are stabilized with those as identifiers (e.g. `aws_regions`). Importantly, its worth noting that while the unstable `id` attribute is the most visible in the plan difference output, it does not necessarily represent the underlying issue that is causing the output to show. There are two known cases, first with providers unexpectedly writing values to unconfigured and uncomputed attributes and second with Default usage in data source schemas, that are the real triggers of the unexpected difference output. Additional upstream bug reports to properly show difference sigils in the plan difference output are likely necessary, since in many of the real world cases of this particular issue they are missing. Potential future bug reports containing these data sources may help guide those. Another important note here is that per the Extending Terraform documentation for versioning, that resource identifier changes typically fall under a best practice of a major version increment for Terraform Providers. Given the widespread reports of unexpected behavior as practitioners are upgrading to Terraform 0.13 and since the old identifiers did not represent meaningful information for the lookup context, these changes are not lightly being considered a bug fix. If the usage of a changing time stamp is necessary, the `timestamp()` function and Terraform Time Provider are recommended methodologies for this information. If the usage of a changing random identifier is necessary, the Terraform Random Provider is the recommended methodology. There still remains other data sources and resources that suffer from known unstable identifiers, such as many plural data sources. Determination of the path forward for these is still undetermined, since ideally Terraform Providers should no longer need to set an `id` attribute after Terraform 0.12, however the Terraform Plugin SDK does not provide functionality to avoid this yet. Future changes may involve a standardized pattern for these within the provider itself. Changes: ``` NOTES: * data-source/aws_acm_certificate: The `id` attribute has changed to the ARN of the ACM Certificate. The first apply of this updated data source may show this difference. * data-source/aws_autoscaling_group: The `id` attribute has changed to the name of the Auto Scaling Group. The first apply of this updated data source may show this difference. * data-source/aws_availability_zones: The `id` attribute has changed to the name of the AWS Region. The first apply of this updated data source may show this difference. * data-source/aws_db_event_categories: The `id` attribute has changed to the name of the AWS Region. The first apply of this updated data source may show this difference. * data-source/aws_ebs_default_kms_key: The `id` attribute has changed to the name of the AWS Region. The first apply of this updated data source may show this difference. * data-source/aws_ebs_encryption_by_default: The `id` attribute has changed to the name of the AWS Region. The first apply of this updated data source may show this difference. * data-source/aws_ec2_instance_type_offering: The `id` attribute has changed to the EC2 Instance Type. The first apply of this updated data source may show this difference. * data-source/aws_ecr_authorization_token: The `id` attribute has changed to the AWS Region. The first apply of this updated data source may show this difference. * data-source/aws_ecr_image: The `id` attribute has changed to the SHA256 digest of the ECR Image. The first apply of this updated data source may show this difference. * data-source/aws_eks_cluster_auth: The `id` attribute has changed to the name of the EKS Cluster. The first apply of this updated data source may show this difference. * data-source/aws_iam_account_alias: The `id` attribute has changed to the AWS Account Alias. The first apply of this updated data source may show this difference. * data-source/aws_kms_alias: The `id` attribute has changed to the ARN of the KMS Alias. The first apply of this updated data source may show this difference. * data-source/aws_partition: The `id` attribute has changed to the identifier of the AWS Partition. The first apply of this updated data source may show this difference. * data-source/aws_regions: The `id` attribute has changed to the identifier of the AWS Partition. The first apply of this updated data source may show this difference. * data-source/aws_sns_topic: The `id` attribute has changed to the ARN of the SNS Topic. The first apply of this updated data source may show this difference. FIXES: * data-source/aws_acm_certificate: Prevent plan differences with the `id` attribute * data-source/aws_autoscaling_group: Prevent plan differences with the `id` attribute * data-source/aws_availability_zones: Prevent plan differences with the `id` attribute * data-source/aws_db_event_categories: Prevent plan differences with the `id` attribute * data-source/aws_ebs_default_kms_key: Prevent plan differences with the `id` attribute * data-source/aws_ebs_encryption_by_default: Prevent plan differences with the `id` attribute * data-source/aws_ec2_instance_type_offering: Prevent plan differences with the `id` attribute * data-source/aws_ecr_authorization_token: Prevent plan differences with the `id` attribute * data-source/aws_ecr_image: Prevent plan differences with the `id` attribute * data-source/aws_eks_cluster_auth: Prevent plan differences with the `id` attribute * data-source/aws_iam_account_alias: Prevent plan differences with the `id` attribute * data-source/aws_kms_alias: Prevent plan differences with the `id` attribute * data-source/aws_partition: Prevent plan differences with the `id` attribute * data-source/aws_regions: Prevent plan differences with the `id` attribute * data-source/aws_sns_topic: Prevent plan differences with the `id` attribute ``` Output from acceptance testing: ``` --- PASS: TestAccAwsAutoScalingGroupDataSource_basic (38.65s) --- PASS: TestAccAWSAvailabilityZones_AllAvailabilityZones (23.55s) --- PASS: TestAccAWSAvailabilityZones_basic (23.40s) --- PASS: TestAccAWSAvailabilityZones_ExcludeNames (14.32s) --- PASS: TestAccAWSAvailabilityZones_ExcludeZoneIds (24.34s) --- PASS: TestAccAWSAvailabilityZones_Filter (23.32s) --- PASS: TestAccAWSAvailabilityZones_stateFilter (23.23s) --- PASS: TestAccAWSDbEventCategories_basic (15.76s) --- PASS: TestAccAWSDbEventCategories_sourceType (14.71s) --- PASS: TestAccAWSEc2InstanceTypeOfferingDataSource_Filter (23.91s) --- PASS: TestAccAWSEc2InstanceTypeOfferingDataSource_LocationType (24.65s) --- PASS: TestAccAWSEc2InstanceTypeOfferingDataSource_PreferredInstanceTypes (23.58s) --- PASS: TestAccAWSEcrAuthorizationTokenDataSource_basic (23.96s) --- PASS: TestAccAWSEcrDataSource_ecrImage (24.22s) --- PASS: TestAccAWSEksClusterAuthDataSource_basic (19.59s) --- PASS: TestAccDataSourceAwsEBSDefaultKmsKey_basic (20.11s) --- PASS: TestAccDataSourceAwsEBSEncryptionByDefault_basic (23.26s) --- PASS: TestAccDataSourceAwsKmsAlias_AwsService (24.27s) --- PASS: TestAccDataSourceAwsKmsAlias_CMK (28.03s) --- PASS: TestAccDataSourceAwsRegions_AllRegions (23.30s) --- PASS: TestAccDataSourceAwsRegions_basic (23.44s) --- PASS: TestAccDataSourceAwsRegions_Filter (22.16s) --- PASS: TestAccDataSourceAwsSnsTopic_basic (26.41s) ```
ghost
added
size/M
YakDriver
approved these changes
Oct 2, 2020
There was a problem hiding this comment.
Looks great!
--- SKIP: TestAccAWSAcmCertificateDataSource_noMatchReturnsError (0.00s)
--- SKIP: TestAccAWSAcmCertificateDataSource_multipleIssued (0.00s)
--- PASS: TestAvailabilityZonesSort (0.90s)
--- SKIP: TestAccAWSAcmCertificateDataSource_singleIssued (0.00s)
--- PASS: TestAccAWSEc2InstanceTypeOfferingDataSource_PreferredInstanceTypes (111.73s)
--- PASS: TestAccDataSourceAwsEBSDefaultKmsKey_basic (112.43s)
--- PASS: TestAccAWSAvailabilityZones_ExcludeZoneIds (112.62s)
--- PASS: TestAccAWSEksClusterAuthDataSource_basic (111.94s)
--- PASS: TestAccAWSAvailabilityZones_Filter (113.82s)
--- PASS: TestAccAWSEcrDataSource_ecrImage (113.38s)
--- PASS: TestAccAWSAvailabilityZones_AllAvailabilityZones (114.05s)
--- PASS: TestAccAWSDbEventCategories_sourceType (114.36s)
--- PASS: TestAccAWSAvailabilityZones_ExcludeNames (114.92s)
--- PASS: TestAccAWSAvailabilityZones_stateFilter (115.12s)
--- PASS: TestAccDataSourceAwsKmsAlias_AwsService (114.43s)
--- PASS: TestAccAWSEc2InstanceTypeOfferingDataSource_Filter (115.35s)
--- PASS: TestAccDataSourceAwsEBSEncryptionByDefault_basic (116.00s)
--- PASS: TestAccAWSAvailabilityZones_basic (116.07s)
--- PASS: TestAccAWSEc2InstanceTypeOfferingDataSource_LocationType (116.61s)
--- PASS: TestAccAWSDbEventCategories_basic (116.73s)
--- PASS: TestAccDataSourceAwsKmsAlias_CMK (116.99s)
--- PASS: TestAccAWSAcmCertificateDataSource_KeyTypes (120.93s)
--- PASS: TestAccAWSEcrAuthorizationTokenDataSource_basic (140.57s)
--- PASS: TestAccDataSourceAwsRegions_Filter (31.50s)
--- PASS: TestAccAWSPartition_basic (33.07s)
--- PASS: TestAccAwsAutoScalingGroupDataSource_basic (144.87s)
--- PASS: TestAccDataSourceAwsRegions_AllRegions (32.29s)
--- PASS: TestAccDataSourceAwsRegions_basic (32.75s)
--- PASS: TestAccDataSourceAwsSnsTopic_basic (31.93s)
|
This has been released in version 3.10.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! |
|
Looks |
|
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
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Community Note
Reference: #14579
Reference: #15199
Reference: hashicorp/terraform-provider-google#7375
Reference: https://github.com/bflad/tfproviderlint/tree/master/passes/R015
Reference: https://github.com/bflad/tfproviderlint/tree/master/passes/R016
Reference: https://github.com/bflad/tfproviderlint/tree/master/passes/R017
Reference: https://www.terraform.io/docs/extend/best-practices/versioning.html#example-major-number-increments
Reference: https://registry.terraform.io/providers/hashicorp/random/
Reference: https://registry.terraform.io/providers/hashicorp/time/
Release note for CHANGELOG:
Terraform 0.13 reworked data source reads into the plan graph, which had some unintentional consequences with Terraform Plugin SDK and provider behaviors that were previously ignored when displaying plan differences. While some of these differences dealing with empty and missing blocks could be addressed with extra graph logic, there remains problematic behaviors that will not be addressable in the near future in core or the SDK.
This change set uses the
tfproviderlintR015, R016, and R017 checks to find(*schema.ResourceData).SetId()usage with unstable values such as the current time and per-execution random identifiers and where the identifier can be stablized based on the data source purpose and surrounding context. For example, singular data sources that represent an analogous managed resource can use the same identifier (e.g.aws_acm_certificate,aws_autoscaling_group). Other cases where the data source represents information from an AWS Partition or AWS Region are stabilized with those as identifiers (e.g.aws_regions).Importantly, its worth noting that while the unstable
idattribute is the most visible in the plan difference output, it does not necessarily represent the underlying issue that is causing the output to show. There are two known cases, first with providers unexpectedly writing values to unconfigured and uncomputed attributes and second with Default usage in data source schemas, that are the real triggers of the unexpected difference output. Additional upstream bug reports to properly show difference sigils in the plan difference output are likely necessary, since in many of the real world cases of this particular issue they are missing. Potential future bug reports containing these data sources may help guide those.Another important note here is that per the Extending Terraform documentation for versioning, that resource identifier changes typically fall under a best practice of a major version increment for Terraform Providers. Given the widespread reports of unexpected behavior as practitioners are upgrading to Terraform 0.13 and since the old identifiers did not represent meaningful information for the lookup context, these changes are not lightly being considered a bug fix. If the usage of a changing time stamp is necessary, the
timestamp()function and Terraform Time Provider are recommended methodologies for this information. If the usage of a changing random identifier is necessary, the Terraform Random Provider is the recommended methodology.There still remains other data sources and resources that suffer from known unstable identifiers, such as many plural data sources. Determination of the path forward for these is still undetermined, since ideally Terraform Providers should no longer need to set an
idattribute after Terraform 0.12, however the Terraform Plugin SDK does not provide functionality to avoid this yet. Future changes may involve a standardized pattern for these within the provider itself.In the future, Terraform core is moving towards the direction of always outputting any data source attributes changes between runs. There is no timeframe for this functionality, however fixing these here and now is a positive step should that upstream change be implemented.
Output from acceptance testing: