Proposal: Combine Singular Data Source and Resource Acceptance Testing

[bflad]

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 "me too" comments, 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

Background

We have hit another regression scenario where a resource update for new functionality broke the respective data source.

Historically, the main reason for these types of issues was that the data source shared the Read function with the resource. When the data source does not have its schema (and preferably its acceptance testing) also updated, the mismatch in the data source can cause it return errors and prevent usage. We have since gone through and refactored many of the data sources to implement their own Read function to make the appropriate root level attribute d.Set() calls.

This last occurrence was more subtle though and involved a new nested schema attribute under a configuration block. Typically for these configuration block attributes, the data source and resource Read functions call out to a shared flattenXXX() function to convert an SDK object from the API response into the expected Terraform state. Since there is yet another tightly coupled implementation and most d.Set() calls for configuration blocks are now correctly performing error checking, the mismatch of schemas causes the data source to return errors and prevent usage.

Maintainers are procedurally accustomed to running acceptance testing for only the particular resource pattern (e.g. TestAccAWSServiceThing_) OR data source pattern (e.g. TestAccAWSServiceThingDataSource_) affected by code changes as running the entire acceptance test suite is unrealistic due to time and money constraints.

Currently, we write bespoke data source acceptance testing in aws/data_source_RESOURCE_test.go files. While many variations of resource test cases are created in the aws/resource_RESOURCE_test.go file, there is very little parity to when a matching data source test case is created. The data source testing typically only covers basic use cases and the test configurations are usually exact duplicates of resource test configurations plus a data source declaration.

Respective data sources are very common feature requests which are showing no signs of decrease, so problems outlined above will only continue to grow in size over time.

Regression Issue References:

• error setting health_check with AWS 2.5.0 when performing a target_group data lookup #8209
• Add support for importing VPC route table routes #5689
• error setting enabled_cloudwatch_logs_exports: Invalid address to set: []string{"enabled_cloudwatch_logs_exports"} #4922

Proposal

Migrate singular data source testing to the majority of resource test cases by adding a data source declaration to the each resource configuration along with relevant data source attribute checks.

This solves a few problems:

• Prevents data source testing from being missed during code reviews
• Ensures data source testing covers more scenarios
• Does not require twice the infrastructure, time, and code maintenance burden (reminder: data source configurations are mostly a duplicate of the resource configurations plus the data source declaration)

Acceptance Testing Configuration Updates

Generally data source configurations can use a straightforward argument reference to the resource identifier

# existing test configuration
resource "aws_example" "test" {
  # ... other configuration ...
}

# new configuration
data "aws_example" "test" {
  id = "${aws_example.test.id}"
}

More advanced data source cases can be handled by keeping the existing data source configurations and test checks as-is but updating the data source test names to match the resource test naming pattern.

Acceptance Testing Function Updates

Essentially we can take the current style of data source testing that prefers to utilize resource.TestCheckResourceAttrPair() to ensure the resource and data source attributes properly exist and have matching values where expected.

// ... existing checks ....
resource.TestCheckResourceAttr(resourceName, "name", rName),
// new checks:
resource.TestCheckResourceAttrPair(datasourceName, "arn", resourceName, "arn"),
// ... repeat for relevant attributes ...

Affected Resources and Data Sources

• aws_acm_certificate
• aws_acmpca_certificate_authority
• aws_alb/aws_lb
• aws_alb_listener/aws_lb_listener
• aws_alb_target_group/aws_lb_target_group
• aws_ami
• aws_api_gateway_api_key
• aws_api_gateway_resource
• aws_api_gateway_rest_api
• aws_api_gateway_vpc_link
• aws_autoscaling_group
• aws_batch_compute_environment
• aws_batch_job_queue
• aws_cloudformation_stack
• aws_cloudhsm_v2_cluster
• aws_cloudwatch_log_group
• aws_codecommit_repository
• aws_cur_report_definition
• aws_db_cluster_snapshot
• aws_db_instance
• aws_db_snapshot
• aws_dx_gateway
• aws_dynamodb_table
• aws_ebs_snapshot
• aws_ebs_volume
• aws_ec2_transit_gateway
• aws_ec2_transit_gateway_route_table
• aws_ec2_transit_gateway_vpc_attachment
• aws_ecr_repository
• aws_ecs_cluster
• aws_ecs_service
• aws_ecs_task_definition
• aws_efs_file_system
• aws_efs_mount_target
• aws_eip
• aws_eks_cluster
• aws_elastic_beanstalk_application
• aws_elasticache_cluster
• aws_elasticache_replication_group
• aws_elb
• aws_iam_group
• aws_iam_instance_profile
• aws_iam_policy
• aws_iam_role
• aws_iam_server_certificate
• aws_iam_user
• aws_instance
• aws_internet_gateway
• aws_iot_endpoint
• aws_kinesis_stream
• aws_kms_alias
• aws_kms_key
• aws_lambda_function
• aws_launch_configuration
• aws_launch_template
• aws_mq_broker
• aws_nat_gateway
• aws_network_acl
• aws_network_interface
• aws_rds_cluster
• aws_redshift_cluster
• aws_route
• aws_route53_delegation_set
• aws_route53_zone
• aws_route_table
• aws_s3_bucket
• aws_s3_bucket_object
• aws_secretsmanager_secret
• aws_secretsmanager_secret_version
• aws_security_group
• aws_sns_topic
• aws_sqs_queue
• aws_ssm_document
• aws_ssm_parameter
• aws_subnet
• aws_transfer_server
• aws_vpc
• aws_vpc_dhcp_options
• aws_vpc_endpoint
• aws_vpc_endpoint_service
• aws_vpc_peering_connection
• aws_vpn_gateway

Abandoned Ideas

• Introduce further code duplication to fully decouple the data source Read from the resource Read by splitting the flattenXXX() functions into resource and data source variants. Bug fixes will need to be implemented twice and adds unnecessary code complexity.
• Create a common schema between the data source and resource. While this solves the root of the issues, the differences between the two schemas usually require separate Required/Optional/Computed handling for every attribute. Reconciling this manually would be error prone and add unnecessary schema complexity.
• Add maintainer procedural burden to always call both the resource and data source acceptance testing patterns when a resource has a respective data source. This is subject to human error and not all resource and data source naming patterns are aligned properly today. e.g. one resource and data source might line up with TestAccAWSServiceThing(DataSource)?_ while others would need complex matching like TestAcc(DataSource)?A[wW][sS](S(ervice|ERVICE))?Thing(DataSource)?_
• In a broad sense, we have had very preliminary discussions about potentially combining data sources with their respective resources conceptually in Terraform. There is no future timeline for design, planning, or potential voting/implementation of this, so we cannot reliably wait for this type of conceptual change to potentially solve this situation.

[github-actions]

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

[bflad]

Bumping to keep this open.

[github-actions]

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!