From 8b88f68fcef0c8d67c46831e9b8a45560bc88183 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Mon, 30 Sep 2019 20:46:00 -0400 Subject: [PATCH 1/2] internal/keyvaluetags: Generate Ec2UpdateTags Updated via: ```console $ make gen ``` --- .../generators/updatetags/main.go | 59 ++++++++++++++++--- .../service_generation_customizations.go | 3 + aws/internal/keyvaluetags/update_tags_gen.go | 37 ++++++++++++ 3 files changed, 91 insertions(+), 8 deletions(-) diff --git a/aws/internal/keyvaluetags/generators/updatetags/main.go b/aws/internal/keyvaluetags/generators/updatetags/main.go index 8ff741e619e..b3d083481ca 100644 --- a/aws/internal/keyvaluetags/generators/updatetags/main.go +++ b/aws/internal/keyvaluetags/generators/updatetags/main.go @@ -43,6 +43,7 @@ var serviceNames = []string{ "directoryservice", "docdb", "dynamodb", + "ec2", "ecr", "ecs", "efs", @@ -100,14 +101,16 @@ func main() { ServiceNames: serviceNames, } templateFuncMap := template.FuncMap{ - "ClientType": keyvaluetags.ServiceClientType, - "TagFunction": ServiceTagFunction, - "TagInputIdentifierField": ServiceTagInputIdentifierField, - "TagInputResourceTypeField": ServiceTagInputResourceTypeField, - "TagInputTagsField": ServiceTagInputTagsField, - "Title": strings.Title, - "UntagFunction": ServiceUntagFunction, - "UntagInputTagsField": ServiceUntagInputTagsField, + "ClientType": keyvaluetags.ServiceClientType, + "TagFunction": ServiceTagFunction, + "TagInputIdentifierField": ServiceTagInputIdentifierField, + "TagInputIdentifierRequiresSlice": ServiceTagInputIdentifierRequiresSlice, + "TagInputResourceTypeField": ServiceTagInputResourceTypeField, + "TagInputTagsField": ServiceTagInputTagsField, + "Title": strings.Title, + "UntagFunction": ServiceUntagFunction, + "UntagInputRequiresTagType": ServiceUntagInputRequiresTagType, + "UntagInputTagsField": ServiceUntagInputTagsField, } tmpl, err := template.New("updatetags").Funcs(templateFuncMap).Parse(templateBody) @@ -168,11 +171,19 @@ func {{ . | Title }}UpdateTags(conn {{ . | ClientType }}, identifier string{{ if if removedTags := oldTags.Removed(newTags); len(removedTags) > 0 { input := &{{ . }}.{{ . | UntagFunction }}Input{ + {{- if . | TagInputIdentifierRequiresSlice }} + {{ . | TagInputIdentifierField }}: aws.StringSlice([]string{identifier}), + {{- else }} {{ . | TagInputIdentifierField }}: aws.String(identifier), + {{- end }} {{- if . | TagInputResourceTypeField }} {{ . | TagInputResourceTypeField }}: aws.String(resourceType), {{- end }} + {{- if . | UntagInputRequiresTagType }} + {{ . | UntagInputTagsField }}: removedTags.IgnoreAws().{{ . | Title }}Tags(), + {{- else }} {{ . | UntagInputTagsField }}: aws.StringSlice(removedTags.Keys()), + {{- end }} } _, err := conn.{{ . | UntagFunction }}(input) @@ -184,7 +195,11 @@ func {{ . | Title }}UpdateTags(conn {{ . | ClientType }}, identifier string{{ if if updatedTags := oldTags.Updated(newTags); len(updatedTags) > 0 { input := &{{ . }}.{{ . | TagFunction }}Input{ + {{- if . | TagInputIdentifierRequiresSlice }} + {{ . | TagInputIdentifierField }}: aws.StringSlice([]string{identifier}), + {{- else }} {{ . | TagInputIdentifierField }}: aws.String(identifier), + {{- end }} {{- if . | TagInputResourceTypeField }} {{ . | TagInputResourceTypeField }}: aws.String(resourceType), {{- end }} @@ -214,6 +229,8 @@ func ServiceTagFunction(serviceName string) string { return "AddTagsToResource" case "docdb": return "AddTagsToResource" + case "ec2": + return "CreateTags" case "efs": return "CreateTags" case "elasticache": @@ -268,6 +285,8 @@ func ServiceTagInputIdentifierField(serviceName string) string { return "ResourceId" case "docdb": return "ResourceName" + case "ec2": + return "Resources" case "efs": return "FileSystemId" case "elasticache": @@ -321,6 +340,16 @@ func ServiceTagInputIdentifierField(serviceName string) string { } } +// ServiceTagInputIdentifierRequiresSlice determines if the service tagging resource field requires a slice. +func ServiceTagInputIdentifierRequiresSlice(serviceName string) string { + switch serviceName { + case "ec2": + return "yes" + default: + return "" + } +} + // ServiceTagInputTagsField determines the service tagging tags field. func ServiceTagInputTagsField(serviceName string) string { switch serviceName { @@ -356,6 +385,8 @@ func ServiceUntagFunction(serviceName string) string { return "RemoveTagsFromResource" case "docdb": return "RemoveTagsFromResource" + case "ec2": + return "DeleteTags" case "efs": return "DeleteTags" case "elasticache": @@ -389,6 +420,16 @@ func ServiceUntagFunction(serviceName string) string { } } +// ServiceUntagInputRequiresTagType determines if the service untagging requires full Tag type. +func ServiceUntagInputRequiresTagType(serviceName string) string { + switch serviceName { + case "ec2": + return "yes" + default: + return "" + } +} + // ServiceUntagInputTagsField determines the service untagging tags field. func ServiceUntagInputTagsField(serviceName string) string { switch serviceName { @@ -398,6 +439,8 @@ func ServiceUntagInputTagsField(serviceName string) string { return "TagKeyList" case "datasync": return "Keys" + case "ec2": + return "Tags" case "glue": return "TagsToRemove" default: diff --git a/aws/internal/keyvaluetags/service_generation_customizations.go b/aws/internal/keyvaluetags/service_generation_customizations.go index 8dee8812376..852918752e7 100644 --- a/aws/internal/keyvaluetags/service_generation_customizations.go +++ b/aws/internal/keyvaluetags/service_generation_customizations.go @@ -34,6 +34,7 @@ import ( "github.com/aws/aws-sdk-go/service/directoryservice" "github.com/aws/aws-sdk-go/service/docdb" "github.com/aws/aws-sdk-go/service/dynamodb" + "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/ecr" "github.com/aws/aws-sdk-go/service/ecs" "github.com/aws/aws-sdk-go/service/efs" @@ -146,6 +147,8 @@ func ServiceClientType(serviceName string) string { funcType = reflect.TypeOf(docdb.New) case "dynamodb": funcType = reflect.TypeOf(dynamodb.New) + case "ec2": + funcType = reflect.TypeOf(ec2.New) case "ecr": funcType = reflect.TypeOf(ecr.New) case "ecs": diff --git a/aws/internal/keyvaluetags/update_tags_gen.go b/aws/internal/keyvaluetags/update_tags_gen.go index 5bcc5a20d23..443456c0fb5 100644 --- a/aws/internal/keyvaluetags/update_tags_gen.go +++ b/aws/internal/keyvaluetags/update_tags_gen.go @@ -32,6 +32,7 @@ import ( "github.com/aws/aws-sdk-go/service/directoryservice" "github.com/aws/aws-sdk-go/service/docdb" "github.com/aws/aws-sdk-go/service/dynamodb" + "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/ecr" "github.com/aws/aws-sdk-go/service/ecs" "github.com/aws/aws-sdk-go/service/efs" @@ -1013,6 +1014,42 @@ func DynamodbUpdateTags(conn *dynamodb.DynamoDB, identifier string, oldTagsMap i return nil } +// Ec2UpdateTags updates ec2 service tags. +// The identifier is typically the Amazon Resource Name (ARN), although +// it may also be a different identifier depending on the service. +func Ec2UpdateTags(conn *ec2.EC2, identifier string, oldTagsMap interface{}, newTagsMap interface{}) error { + oldTags := New(oldTagsMap) + newTags := New(newTagsMap) + + if removedTags := oldTags.Removed(newTags); len(removedTags) > 0 { + input := &ec2.DeleteTagsInput{ + Resources: aws.StringSlice([]string{identifier}), + Tags: removedTags.IgnoreAws().Ec2Tags(), + } + + _, err := conn.DeleteTags(input) + + if err != nil { + return fmt.Errorf("error untagging resource (%s): %s", identifier, err) + } + } + + if updatedTags := oldTags.Updated(newTags); len(updatedTags) > 0 { + input := &ec2.CreateTagsInput{ + Resources: aws.StringSlice([]string{identifier}), + Tags: updatedTags.IgnoreAws().Ec2Tags(), + } + + _, err := conn.CreateTags(input) + + if err != nil { + return fmt.Errorf("error tagging resource (%s): %s", identifier, err) + } + } + + return nil +} + // EcrUpdateTags updates ecr service tags. // The identifier is typically the Amazon Resource Name (ARN), although // it may also be a different identifier depending on the service. From a23d496abf52f7d6c5c547e162e749a66ec58efd Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Mon, 30 Sep 2019 21:48:23 -0400 Subject: [PATCH 2/2] resource/aws_vpc: Refactor to use keyvaluetags and call Read after Create Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/7926 In preparation for provider-wide ignore and default tag logic, here we refactor this resource to use the consistent keyvaluetags handling. The previous `setTags()` logic was always performing retries only necessary for resource creation and not following recommended practices to call the `Read` function after `Create`. Output from acceptance testing: ``` --- PASS: TestAccAWSVpc_disappears (16.21s) --- PASS: TestAccAWSVpc_coreMismatchedDiffs (23.55s) --- PASS: TestAccAWSVpc_DisabledDnsSupport (28.46s) --- PASS: TestAccAWSVpc_classiclinkOptionSet (28.64s) --- PASS: TestAccAWSVpc_bothDnsOptionsSet (28.66s) --- PASS: TestAccAWSVpc_classiclinkDnsSupportOptionSet (29.10s) --- PASS: TestAccAWSVpc_basic (29.31s) --- PASS: TestAccAWSVpc_update (40.02s) --- PASS: TestAccAWSVpc_tags (45.37s) --- PASS: TestAccAWSVpc_AssignGeneratedIpv6CidrBlock (62.90s) --- PASS: TestAccAWSVpc_Tenancy (62.94s) ``` --- aws/resource_aws_vpc.go | 112 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 103 insertions(+), 9 deletions(-) diff --git a/aws/resource_aws_vpc.go b/aws/resource_aws_vpc.go index f0a12162622..f540e8e45b7 100644 --- a/aws/resource_aws_vpc.go +++ b/aws/resource_aws_vpc.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/helper/validation" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" ) func resourceAwsVpc() *schema.Resource { @@ -171,8 +172,96 @@ func resourceAwsVpcCreate(d *schema.ResourceData, meta interface{}) error { } } - // Update our attributes and return - return resourceAwsVpcUpdate(d, meta) + // You cannot modify the DNS resolution and DNS hostnames attributes in the same request. Use separate requests for each attribute. + // Reference: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_ModifyVpcAttribute.html + + if d.Get("enable_dns_hostnames").(bool) { + input := &ec2.ModifyVpcAttributeInput{ + EnableDnsHostnames: &ec2.AttributeBooleanValue{ + Value: aws.Bool(true), + }, + VpcId: aws.String(d.Id()), + } + + if _, err := conn.ModifyVpcAttribute(input); err != nil { + return fmt.Errorf("error enabling VPC (%s) DNS hostnames: %s", d.Id(), err) + } + + d.SetPartial("enable_dns_hostnames") + } + + // By default, only the enableDnsSupport attribute is set to true in a VPC created any other way. + // Reference: https://docs.aws.amazon.com/vpc/latest/userguide/vpc-dns.html#vpc-dns-support + + if !d.Get("enable_dns_support").(bool) { + input := &ec2.ModifyVpcAttributeInput{ + EnableDnsSupport: &ec2.AttributeBooleanValue{ + Value: aws.Bool(false), + }, + VpcId: aws.String(d.Id()), + } + + if _, err := conn.ModifyVpcAttribute(input); err != nil { + return fmt.Errorf("error disabling VPC (%s) DNS support: %s", d.Id(), err) + } + + d.SetPartial("enable_dns_support") + } + + if d.Get("enable_classiclink").(bool) { + input := &ec2.EnableVpcClassicLinkInput{ + VpcId: aws.String(d.Id()), + } + + if _, err := conn.EnableVpcClassicLink(input); err != nil { + return fmt.Errorf("error enabling VPC (%s) ClassicLink: %s", d.Id(), err) + } + + d.SetPartial("enable_classiclink") + } + + if d.Get("enable_classiclink_dns_support").(bool) { + input := &ec2.EnableVpcClassicLinkDnsSupportInput{ + VpcId: aws.String(d.Id()), + } + + if _, err := conn.EnableVpcClassicLinkDnsSupport(input); err != nil { + return fmt.Errorf("error enabling VPC (%s) ClassicLink DNS support: %s", d.Id(), err) + } + + d.SetPartial("enable_classiclink_dns_support") + } + + if v := d.Get("tags").(map[string]interface{}); len(v) > 0 { + // Handle EC2 eventual consistency on creation + err := resource.Retry(5*time.Minute, func() *resource.RetryError { + err := keyvaluetags.Ec2UpdateTags(conn, d.Id(), nil, v) + + if isAWSErr(err, "InvalidVpcID.NotFound", "") { + return resource.RetryableError(err) + } + + if err != nil { + return resource.NonRetryableError(err) + } + + return nil + }) + + if isResourceTimeoutError(err) { + err = keyvaluetags.Ec2UpdateTags(conn, d.Id(), nil, v) + } + + if err != nil { + return fmt.Errorf("error adding tags: %s", err) + } + + d.SetPartial("tags") + } + + d.Partial(false) + + return resourceAwsVpcRead(d, meta) } func resourceAwsVpcRead(d *schema.ResourceData, meta interface{}) error { @@ -205,8 +294,9 @@ func resourceAwsVpcRead(d *schema.ResourceData, meta interface{}) error { }.String() d.Set("arn", arn) - // Tags - d.Set("tags", tagsToMap(vpc.Tags)) + if err := d.Set("tags", keyvaluetags.Ec2KeyValueTags(vpc.Tags).IgnoreAws().Map()); err != nil { + return fmt.Errorf("error setting tags: %s", err) + } d.Set("owner_id", vpc.OwnerId) @@ -404,7 +494,7 @@ func resourceAwsVpcUpdate(d *schema.ResourceData, meta interface{}) error { d.SetPartial("enable_classiclink_dns_support") } - if d.HasChange("assign_generated_ipv6_cidr_block") && !d.IsNewResource() { + if d.HasChange("assign_generated_ipv6_cidr_block") { toAssign := d.Get("assign_generated_ipv6_cidr_block").(bool) log.Printf("[INFO] Modifying assign_generated_ipv6_cidr_block to %#v", toAssign) @@ -445,7 +535,7 @@ func resourceAwsVpcUpdate(d *schema.ResourceData, meta interface{}) error { d.SetPartial("assign_generated_ipv6_cidr_block") } - if d.HasChange("instance_tenancy") && !d.IsNewResource() { + if d.HasChange("instance_tenancy") { modifyOpts := &ec2.ModifyVpcTenancyInput{ VpcId: aws.String(vpcid), InstanceTenancy: aws.String(d.Get("instance_tenancy").(string)), @@ -460,9 +550,13 @@ func resourceAwsVpcUpdate(d *schema.ResourceData, meta interface{}) error { d.SetPartial("instance_tenancy") } - if err := setTags(conn, d); err != nil { - return err - } else { + if d.HasChange("tags") { + o, n := d.GetChange("tags") + + if err := keyvaluetags.Ec2UpdateTags(conn, d.Id(), o, n); err != nil { + return fmt.Errorf("error updating tags: %s", err) + } + d.SetPartial("tags") }