search

hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.81k stars 9.16k forks source link

Tech debt: Consistently handle overlapping resource tags when updating #7052

Closed ewbankkit closed 4 years ago

ewbankkit commented 5 years ago

Community Note

Prompted by https://github.com/terraform-providers/terraform-provider-aws/pull/6911#discussion_r242696623 which references https://github.com/terraform-providers/terraform-provider-aws/pull/5108. Ensure that we are consistently handling overlapping resource tags when updating a resource. Because each AWS service has its own types and methods for resource tagging, code has been copy-pasted and is now inconsistent.

ewbankkit commented 5 years ago

This brings up the bigger issue of making tag handling functions generic via Go reflection.

bflad commented 5 years ago

I really don't want to start any sort of flame war on the subject, but I think a few of the maintainers are strongly opposed to reflection. 😅 Type assertion (via creating our own generic type, call it something like keyValueResourceTag, that can be implemented easily for each of the AWS Go SDK service types, and paring down our functions to just those operations with our type) is more likely to be accepted.

ewbankkit commented 5 years ago

I wasn't expressing any preference or opinion 😄. For this issue I'd like to identify and fix the services which are inconsistent WRT overlapping tags. This will give us a known good state from which to explore a generic solution.

ewbankkit commented 5 years ago
Service Source File Correct
Amazon API Gateway tags_apigateway.go
diffTagsGeneric()		 ❌
AWS App Mesh tagsAppmesh.go
https://github.com/terraform-providers/terraform-provider-aws/pull/8111		 ❌
Amazon Athena tagsAthena.go
https://github.com/terraform-providers/terraform-provider-aws/pull/8136		 ❌
AWS Auto Scaling autoscaling_tags.go ❌
AWS Certificate Manager (ACM) tagsACM.go ❌
AWS Certificate Manager (ACM) Private Certificate Authority (CA) tagsACMPCA.go ❌
Amazon CloudFront tagsCloudFront.go ❌
AWS CloudHSM v2 resource_aws_cloudhsm2_cluster.go
diffTagsGeneric()		 ❌
Amazon CloudWatch Events tagsCloudWatchEvent..go
https://github.com/terraform-providers/terraform-provider-aws/pull/8076		 ❌
Amazon CloudWatch Metrics tagsCloudWatch.go
https://github.com/terraform-providers/terraform-provider-aws/pull/8168		 ❌
AWS CloudTrail tagsCloudtrail.go ❌
AWS Direct Connect tagsDX.go ❌
AWS Directory Service tagsDS.go ❌
Amazon DocumentDB tagsDocDB.go ✅
Amazon DynamoDB tags.go
diffTagsDynamoDb()		 ❌
Amazon DynamoDB Accelerator (DAX) tagsDAX.go ❌
Amazon EC2 tags.go
diffTags()		 ✅
AWS Elastic Beanstalk tagsBeanstalk.go ❌
Amazon ElastiCache tagsEC.go ❌
Amazon Elastic Container Registry (ECR) tagsECR.go ✅
Amazon Elastic Container Service (ECS) tagsECS.go ✅
Amazon Elastic File System (EFS) tagsEFS.go ❌
AWS Elastic Load Balancing (ELB) tagsELB.go ❌
AWS Elastic Load Balancing (ELB) v2 tags.go
diffElbV2Tags()		 ❌
Amazon Elasticsearch Service tags_elasticsearchservice.go ❌
AWS Elemental MediaPackage tagsMediapackage.go
diffTagsGeneric()
https://github.com/terraform-providers/terraform-provider-aws/pull/7984		 ❌
AWS Identity and Access Management (IAM) tagsIAM.go ✅
AWS Key Management Service (KMS) tagsKMS.go ❌
Amazon Kinesis tags_kinesis.go ❌
Amazon Kinesis Data Firehose tagsKinesisFirehose.go ❌
AWS Lambda tagsLambda.go
diffTagsGeneric()		 ❌
AWS License Manager tagsLicenseManager.go ❌
Amazon MQ diffTagsGeneric()
https://github.com/terraform-providers/terraform-provider-aws/pull/7193		 ❌
Amazon Neptune tagsNeptune.go ❌
AWS OpsWorks tagsOpsworks.go
diffTagsGeneric()		 ❌
Amazon Relational Database Service (RDS) tagsRDS.go ❌
Amazon Redshift tagsRedshift.go ❌
AWS Resource Access Manager (RAM) tagsRAM.go
https://github.com/terraform-providers/terraform-provider-aws/pull/6528		 ✅
Amazon Route 53 tags_route53.go ❌
Amazon Route 53 Resolver tagsRoute53Resolver.go
https://github.com/terraform-providers/terraform-provider-aws/pull/6574		 ✅
Amazon S3 s3_tags.go ❌
AWS Secrets Manager tagsSecretsManager.go ❌
AWS SNS tagsSNS.go
https://github.com/terraform-providers/terraform-provider-aws/pull/8468		 ✅
Amazon Simple Queue Service (SQS) resource_aws_sqs_queue.go
diffTagsGeneric()		 ❌
AWS Step Functions tagsSfn.go ✅
AWS Systems Manager tagsSSM.go ❌
AWS Transfer for SFTP tagsTransfer.go ✅
ewbankkit commented 5 years ago

Note that some of these diffTags functions may not be needed as the underlying AWS API doesn't deal with tag set differences but instead just deletes all tags and applies a tag set. e.g. for S3: https://github.com/terraform-providers/terraform-provider-aws/blob/9a2ba295b2f1ccbb54fc2c62a31312962f362da1/aws/s3_tags.go#L25-L28 and https://github.com/terraform-providers/terraform-provider-aws/blob/9a2ba295b2f1ccbb54fc2c62a31312962f362da1/aws/s3_tags.go#L36-L45.

ewbankkit commented 5 years ago

We need to also consider the issues brought up in https://github.com/terraform-providers/terraform-provider-aws/issues/7323, https://github.com/terraform-providers/terraform-provider-aws/pull/7342 for those APIs that don't deal with tag set differences but may have system tags.

ewbankkit commented 5 years ago

I will close this issue when https://github.com/terraform-providers/terraform-provider-aws/pull/10018 is merged as the functionality that PR implements will fix this for all existing services and new services in the future.

ewbankkit commented 4 years ago

I'm closing this issue in favor of https://github.com/terraform-providers/terraform-provider-aws/issues/10688, implementation of which will ensure overlapping tags are handled correctly.

ghost commented 4 years ago

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!