Merge pull request #5 from terraform-providers/master

merge
pulumi · Oct 26, 2019 · c0f6d5e · c0f6d5e
2 parents e2687e5 + e2553d8
commit c0f6d5e
Show file tree

Hide file tree

Showing 22 changed files with 532 additions and 489 deletions.
diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md
@@ -25,6 +25,7 @@ ability to merge PRs and respond to issues.
         - [Documentation Update](#documentation-update)
         - [Enhancement/Bugfix to a Resource](#enhancementbugfix-to-a-resource)
         - [Adding Resource Import Support](#adding-resource-import-support)
+        - [Adding Resource Tagging Support](#adding-resource-tagging-support)
         - [New Resource](#new-resource)
         - [New Service](#new-service)
         - [New Region](#new-region)
@@ -213,6 +214,211 @@ In addition to the below checklist and the items noted in the Extending Terrafor
 - [ ] _Resource Acceptance Testing Implementation_: In the resource acceptance testing (e.g. `aws/resource_aws_service_thing_test.go`), implementation of `TestStep`s with `ImportState: true`
 - [ ] _Resource Documentation Implementation_: In the resource documentation (e.g. `website/docs/r/service_thing.html.markdown`), addition of `Import` documentation section at the bottom of the page
 
+#### Adding Resource Tagging Support
+
+AWS provides key-value metadata across many services and resources, which can be used for a variety of use cases including billing, ownership, and more. See the [AWS Tagging Strategy page](https://aws.amazon.com/answers/account-management/aws-tagging-strategies/) for more information about tagging at a high level.
+
+Implementing tagging support for Terraform AWS Provider resources requires the following, each with its own section below:
+
+- [ ] _Generated Service Tagging Code_: In the internal code generators (e.g. `aws/internal/keyvaluetags`), implementation and customization of how a service handles tagging, which is standardized for the resources.
+- [ ] _Resource Tagging Code Implementation_: In the resource code (e.g. `aws/resource_aws_service_thing.go`), implementation of `tags` schema attribute, along with handling in `Create`, `Read`, and `Update` functions.
+- [ ] _Resource Tagging Acceptance Testing Implementation_: In the resource acceptance testing (e.g. `aws/resource_aws_service_thing_test.go`), implementation of new acceptance test function and configurations to exercise new tagging logic.
+- [ ] _Resource Tagging Documentation Implementation_: In the resource documentation (e.g. `website/docs/r/service_thing.html.markdown`), addition of `tags` argument
+
+See also a [full example pull request for implementing EKS tagging](https://github.com/terraform-providers/terraform-provider-aws/pull/10307).
+
+##### Adding Service to Tag Generating Code
+
+This step is only necessary for the first implementation and may have been previously completed. If so, move on to the next section.
+
+More details about this code generation, including fixes for potential error messages in this process, can be found in the [keyvaluetags documentation](aws/internal/keyvaluetags/README.md).
+
+- Open the AWS Go SDK documentation for the service, e.g. for [`service/eks`](https://docs.aws.amazon.com/sdk-for-go/api/service/eks/). Note: there can be a delay between the AWS announcement and the updated AWS Go SDK documentation.
+- Determine the "type" of tagging implementation. Some services will use a simple map style (`map[string]*string` in Go) while others will have a separate structure shape (`[]service.Tag` struct with `Key` and `Value` fields).
+
+  - If the type is a map, add the AWS Go SDK service name (e.g. `eks`) to `mapServiceNames` in `aws/internal/keyvaluetags/generators/servicetags/main.go`
+  - Otherwise, if the type is a struct, add the AWS Go SDK service name (e.g. `eks`) to `sliceServiceNames` in `aws/internal/keyvaluetags/generators/servicetags/main.go`. If the struct name is not exactly `Tag`, it can be customized via the `ServiceTagType` function. If the struct key field is not exactly `Key`, it can be customized via the `ServiceTagTypeKeyField` function. If the struct value field is not exactly `Value`, it can be customized via the `ServiceTagTypeValueField` function.
+
+- Determine if the service API includes functionality for listing tags (usually a `ListTags` or `ListTagsForResource` API call) or updating tags (usually `TagResource` and `UntagResource` API calls). If so, add the AWS Go SDK service client information to `ServiceClientType` (along with the new required import) in `aws/internal/keyvaluetags/service_generation_customizations.go`, e.g. for EKS:
+
+  ```go
+  case "eks":
+    funcType = reflect.TypeOf(eks.New)
+  ```
+
+  - If the service API includes functionality for listing tags, add the AWS Go SDK service name (e.g. `eks`) to `serviceNames` in `aws/internal/keyvaluetags/generators/listtags/main.go`.
+  - If the service API includes functionality for updating tags, add the AWS Go SDK service name (e.g. `eks`) to `serviceNames` in `aws/internal/keyvaluetags/generators/updatetags/main.go`.
+
+- Run `make gen` (`go generate ./...`) and ensure there are no errors via `make test` (`go test ./...`)
+
+##### Resource Tagging Code Implementation
+
+- In the resource Go file (e.g. `aws/resource_aws_eks_cluster.go`), add the following Go import: `"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"`
+- In the resource schema, add `"tags": tagsSchema(),`
+- If the API supports tagging on creation (the `Input` struct accepts a `Tags` field), in the resource `Create` function, implement the logic to convert the configuration tags into the service tags, e.g. with EKS Clusters:
+
+  ```go
+  input := &eks.CreateClusterInput{
+    /* ... other configuration ... */
+    Tags: keyvaluetags.New(d.Get("tags").(map[string]interface{})).IgnoreAws().EksTags(),
+  }
+  ```
+
+  If the service API does not allow passing an empty list, the logic can be adjusted similar to:
+
+  ```go
+  input := &eks.CreateClusterInput{
+    /* ... other configuration ... */
+  }
+
+  if v := d.Get("tags").(map[string]interface{}); len(v) > 0 {
+    input.Tags = keyvaluetags.New(v).IgnoreAws().EksTags()
+  }
+  ```
+
+- Otherwise if the API does not support tagging on creation (the `Input` struct does not accept a `Tags` field), in the resource `Create` function, implement the logic to convert the configuration tags into the service API call to tag a resource, e.g. with CloudHSM v2 Clusters:
+
+  ```go
+  if v := d.Get("tags").(map[string]interface{}); len(v) > 0 {
+    if err := keyvaluetags.Cloudhsmv2UpdateTags(conn, d.Id(), nil, v); err != nil {
+      return fmt.Errorf("error adding CloudHSM v2 Cluster (%s) tags: %s", d.Id(), err)
+    }
+  }
+  ```
+
+- In the resource `Read` function, implement the logic to convert the service tags to save them into the Terraform state for drift detection, e.g. with EKS Clusters (which had the tags available in the DescribeCluster API call):
+
+  ```go
+  if err := d.Set("tags", keyvaluetags.EksKeyValueTags(cluster.Tags).IgnoreAws().Map()); err != nil {
+    return fmt.Errorf("error setting tags: %s", err)
+  }
+  ```
+
+  If the service API does not return the tags directly from reading the resource and requires a separate API call, its possible to use the `keyvaluetags` functionality like the following, e.g. with Athena Workgroups:
+
+  ```go
+  tags, err := keyvaluetags.AthenaListTags(conn, arn.String())
+
+  if err != nil {
+    return fmt.Errorf("error listing tags for resource (%s): %s", arn, err)
+  }
+
+  if err := d.Set("tags", tags.IgnoreAws().Map()); err != nil {
+    return fmt.Errorf("error setting tags: %s", err)
+  }
+  ```
+
+- In the resource `Update` function (this may be the first functionality requiring the creation of the `Update` function), implement the logic to handle tagging updates, e.g. with EKS Clusters:
+
+  ```go
+  if d.HasChange("tags") {
+    o, n := d.GetChange("tags")
+    if err := keyvaluetags.EksUpdateTags(conn, d.Get("arn").(string), o, n); err != nil {
+      return fmt.Errorf("error updating tags: %s", err)
+    }
+  }
+  ```
+
+##### Resource Tagging Acceptance Testing Implementation
+
+- In the resource testing (e.g. `aws/resource_aws_eks_cluster_test.go`), verify that existing resources without tagging are unaffected and do not have tags saved into their Terraform state. This should be done in the `_basic` acceptance test by adding a line similar to `resource.TestCheckResourceAttr(resourceName, "tags.%s", "0"),`
+- In the resource testing, implement a new test named `_Tags` with associated configurations, that verifies creating the resource with tags and updating tags. e.g. EKS Clusters:
+
+  ```go
+  func TestAccAWSEksCluster_Tags(t *testing.T) {
+    var cluster1, cluster2, cluster3 eks.Cluster
+    rName := acctest.RandomWithPrefix("tf-acc-test")
+    resourceName := "aws_eks_cluster.test"
+
+    resource.ParallelTest(t, resource.TestCase{
+      PreCheck:     func() { testAccPreCheck(t); testAccPreCheckAWSEks(t) },
+      Providers:    testAccProviders,
+      CheckDestroy: testAccCheckAWSEksClusterDestroy,
+      Steps: []resource.TestStep{
+        {
+          Config: testAccAWSEksClusterConfigTags1(rName, "key1", "value1"),
+          Check: resource.ComposeTestCheckFunc(
+            testAccCheckAWSEksClusterExists(resourceName, &cluster1),
+            resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
+            resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"),
+          ),
+        },
+        {
+          ResourceName:      resourceName,
+          ImportState:       true,
+          ImportStateVerify: true,
+        },
+        {
+          Config: testAccAWSEksClusterConfigTags2(rName, "key1", "value1updated", "key2", "value2"),
+          Check: resource.ComposeTestCheckFunc(
+            testAccCheckAWSEksClusterExists(resourceName, &cluster2),
+            resource.TestCheckResourceAttr(resourceName, "tags.%", "2"),
+            resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1updated"),
+            resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"),
+          ),
+        },
+        {
+          Config: testAccAWSEksClusterConfigTags1(rName, "key2", "value2"),
+          Check: resource.ComposeTestCheckFunc(
+            testAccCheckAWSEksClusterExists(resourceName, &cluster3),
+            resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
+            resource.TestCheckResourceAttr(resourceName, "tags.key2", "value2"),
+          ),
+        },
+      },
+    })
+  }
+
+  func testAccAWSEksClusterConfigTags1(rName, tagKey1, tagValue1 string) string {
+    return testAccAWSEksClusterConfig_Base(rName) + fmt.Sprintf(`
+  resource "aws_eks_cluster" "test" {
+    name     = %[1]q
+    role_arn = "${aws_iam_role.test.arn}"
+
+    tags = {
+      %[2]q = %[3]q
+    }
+
+    vpc_config {
+      subnet_ids = ["${aws_subnet.test.*.id[0]}", "${aws_subnet.test.*.id[1]}"]
+    }
+
+    depends_on = ["aws_iam_role_policy_attachment.test-AmazonEKSClusterPolicy", "aws_iam_role_policy_attachment.test-AmazonEKSServicePolicy"]
+  }
+  `, rName, tagKey1, tagValue1)
+  }
+
+  func testAccAWSEksClusterConfigTags2(rName, tagKey1, tagValue1, tagKey2, tagValue2 string) string {
+    return testAccAWSEksClusterConfig_Base(rName) + fmt.Sprintf(`
+  resource "aws_eks_cluster" "test" {
+    name     = %[1]q
+    role_arn = "${aws_iam_role.test.arn}"
+
+    tags = {
+      %[2]q = %[3]q
+      %[4]q = %[5]q
+    }
+
+    vpc_config {
+      subnet_ids = ["${aws_subnet.test.*.id[0]}", "${aws_subnet.test.*.id[1]}"]
+    }
+
+    depends_on = ["aws_iam_role_policy_attachment.test-AmazonEKSClusterPolicy", "aws_iam_role_policy_attachment.test-AmazonEKSServicePolicy"]
+  }
+  `, rName, tagKey1, tagValue1, tagKey2, tagValue2)
+  }
+  ```
+
+- Verify all acceptance testing passes for the resource (e.g. `make testacc TESTARGS='-run=TestAccAWSEksCluster_'`)
+
+#### Resource Tagging Documentation Implementation
+
+- In the resource documentation (e.g. `website/docs/r/eks_cluster.html.markdown`), add the following to the arguments reference:
+
+  ```markdown
+  * `tags` - (Optional) Key-value mapping of resource tags
+  ```
+
 #### New Resource
 
 Implementing a new resource is a good way to learn more about how Terraform

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,4 +1,10 @@
 ## 2.34.0 (Unreleased)
+
+BUG FIXES:
+
+* resource/aws_cloudhsm_v2_cluster: Ensure multiple tag configurations are applied correctly [GH-10309]
+* resource/aws_cloudhsm_v2_cluster: Perform drift detection with tags [GH-10309]
+
 ## 2.33.0 (October 17, 2019)
 
 FEATURES:

diff --git a/aws/data_source_aws_ecr_repository.go b/aws/data_source_aws_ecr_repository.go
@@ -7,6 +7,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ecr"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+	"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
 )
 
 func dataSourceAwsEcrRepository() *schema.Resource {
@@ -54,17 +55,22 @@ func dataSourceAwsEcrRepositoryRead(d *schema.ResourceData, meta interface{}) er
 	}
 
 	repository := out.Repositories[0]
-
-	log.Printf("[DEBUG] Received ECR repository %s", out)
+	arn := aws.StringValue(repository.RepositoryArn)
 
 	d.SetId(aws.StringValue(repository.RepositoryName))
-	d.Set("arn", repository.RepositoryArn)
+	d.Set("arn", arn)
 	d.Set("registry_id", repository.RegistryId)
 	d.Set("name", repository.RepositoryName)
 	d.Set("repository_url", repository.RepositoryUri)
 
-	if err := getTagsECR(conn, d); err != nil {
-		return fmt.Errorf("error getting ECR repository tags: %s", err)
+	tags, err := keyvaluetags.EcrListTags(conn, arn)
+
+	if err != nil {
+		return fmt.Errorf("error listing tags for ECR Repository (%s): %s", arn, err)
+	}
+
+	if err := d.Set("tags", tags.IgnoreAws().Map()); err != nil {
+		return fmt.Errorf("error setting tags: %s", err)
 	}
 
 	return nil

diff --git a/aws/data_source_aws_subnet.go b/aws/data_source_aws_subnet.go
@@ -7,6 +7,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+	"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
 )
 
 func dataSourceAwsSubnet() *schema.Resource {
@@ -162,7 +163,11 @@ func dataSourceAwsSubnetRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("cidr_block", subnet.CidrBlock)
 	d.Set("default_for_az", subnet.DefaultForAz)
 	d.Set("state", subnet.State)
-	d.Set("tags", tagsToMap(subnet.Tags))
+
+	if err := d.Set("tags", keyvaluetags.Ec2KeyValueTags(subnet.Tags).IgnoreAws().Map()); err != nil {
+		return fmt.Errorf("error setting tags: %s", err)
+	}
+
 	d.Set("assign_ipv6_address_on_creation", subnet.AssignIpv6AddressOnCreation)
 	d.Set("map_public_ip_on_launch", subnet.MapPublicIpOnLaunch)
 

diff --git a/aws/data_source_aws_vpc.go b/aws/data_source_aws_vpc.go
@@ -8,6 +8,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+	"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
 )
 
 func dataSourceAwsVpc() *schema.Resource {
@@ -176,7 +177,11 @@ func dataSourceAwsVpcRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("instance_tenancy", vpc.InstanceTenancy)
 	d.Set("default", vpc.IsDefault)
 	d.Set("state", vpc.State)
-	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)
 
 	arn := arn.ARN{