Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Resource: aws_ram_resource_association #7449

Merged
merged 2 commits into from
Feb 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,7 @@ func Provider() terraform.ResourceProvider {
"aws_organizations_policy_attachment": resourceAwsOrganizationsPolicyAttachment(),
"aws_placement_group": resourceAwsPlacementGroup(),
"aws_proxy_protocol_policy": resourceAwsProxyProtocolPolicy(),
"aws_ram_resource_association": resourceAwsRamResourceAssociation(),
"aws_ram_resource_share": resourceAwsRamResourceShare(),
"aws_rds_cluster": resourceAwsRDSCluster(),
"aws_rds_cluster_endpoint": resourceAwsRDSClusterEndpoint(),
Expand Down
210 changes: 210 additions & 0 deletions aws/resource_aws_ram_resource_association.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
package aws

import (
"fmt"
"log"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ram"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
)

func resourceAwsRamResourceAssociation() *schema.Resource {
return &schema.Resource{
Create: resourceAwsRamResourceAssociationCreate,
Read: resourceAwsRamResourceAssociationRead,
Delete: resourceAwsRamResourceAssociationDelete,

Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},

Schema: map[string]*schema.Schema{
"resource_arn": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},

"resource_share_arn": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
},
}
}

func resourceAwsRamResourceAssociationCreate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ramconn
resourceARN := d.Get("resource_arn").(string)
resourceShareARN := d.Get("resource_share_arn").(string)

input := &ram.AssociateResourceShareInput{
ClientToken: aws.String(resource.UniqueId()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a comment indicating that ClientToken is unique case-sensitive id used for tracking the request, and not an AWS Account ID? Or maybe just adding a link to https://docs.aws.amazon.com/ram/latest/APIReference/API_AssociateResourceShare.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this has to do with an AWS account ID and we don't tend to re-document the AWS Go SDK structs inline. Using resource.UniqueId() is just for a 26 digit randomly generated number behind a mutex to help protect from concurrency issues. We do this rather than implementing our function when one is readily available. We could use resource.PrefixedUniqueId() which would allow for some form of prefix beyond the resource.UniqueId() default of "terraform-", etc. but many of these ClientToken/IdempotencyToken fields have byte length restrictions so in my opinion seems safer to just default to a short standard.

If you would like anything changed in this regard, we should create an issue and apply the change across the codebase.

Copy link
Member

@nywilken nywilken Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you would like anything changed in this regard, we should create an issue and apply the change across the codebase.

Sorry for the confusion I was actually referring to the field name ClientToken. At first glance it seemed like an AWS specific thing but after reading the documentation I understood why resource.UniqueId() was being used. No action necessary. Thanks for the explanation of resource.UniqueId() and resource.PrefixUniqueId() btw.

ResourceArns: aws.StringSlice([]string{resourceARN}),
ResourceShareArn: aws.String(resourceShareARN),
}

log.Printf("[DEBUG] Associating RAM Resource Share: %s", input)
_, err := conn.AssociateResourceShare(input)
if err != nil {
return fmt.Errorf("error associating RAM Resource Share: %s", err)
}

d.SetId(fmt.Sprintf("%s,%s", resourceShareARN, resourceARN))

if err := waitForRamResourceShareResourceAssociation(conn, resourceShareARN, resourceARN); err != nil {
return fmt.Errorf("error waiting for RAM Resource Share (%s) Resource Association (%s): %s", resourceShareARN, resourceARN, err)
}

return resourceAwsRamResourceAssociationRead(d, meta)
}

func resourceAwsRamResourceAssociationRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ramconn

resourceShareARN, resourceARN, err := decodeRamResourceAssociationID(d.Id())
if err != nil {
return err
}

resourceShareAssociation, err := getRamResourceShareAssociation(conn, resourceShareARN, resourceARN)

if err != nil {
return fmt.Errorf("error reading RAM Resource Share (%s) Resource Association (%s): %s", resourceShareARN, resourceARN, err)
}

if resourceShareAssociation == nil {
log.Printf("[WARN] RAM Resource Share (%s) Resource Association (%s) not found, removing from state", resourceShareARN, resourceARN)
d.SetId("")
return nil
}

if aws.StringValue(resourceShareAssociation.Status) != ram.ResourceShareAssociationStatusAssociated {
log.Printf("[WARN] RAM Resource Share (%s) Resource Association (%s) not associated, removing from state", resourceShareARN, resourceARN)
d.SetId("")
return nil
}

d.Set("resource_arn", resourceARN)
d.Set("resource_share_arn", resourceShareARN)

return nil
}

func resourceAwsRamResourceAssociationDelete(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ramconn

resourceShareARN, resourceARN, err := decodeRamResourceAssociationID(d.Id())
if err != nil {
return err
}

input := &ram.DisassociateResourceShareInput{
ResourceArns: aws.StringSlice([]string{resourceARN}),
ResourceShareArn: aws.String(resourceShareARN),
}

log.Printf("[DEBUG] Disassociating RAM Resource Share: %s", input)
_, err = conn.DisassociateResourceShare(input)

if isAWSErr(err, ram.ErrCodeUnknownResourceException, "") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a pattern we follow: check for isAWSErr than check err != nil. Is there a reason, other than the nested if statements, that we don't do something like the following to reduce the number if err checks?

if err != nil {
  if isAWSErr(err, ram.ErrCodeUnkonwnResourceException, "") {
    return nil
  }

return fmt.Errorf("error disassociating RAM Resource Share (%s) Resource Association (%s): %s", resourceShareARN, resourceARN, err)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two good reasons:

  • It follows a good (in my opinion easier to read) coding style practice of aligning the happy path on the left and encourages returning early
  • It prevents subtle error handling bugs, which are hard to catch during review. I caught this one in a review just the other day (I can dig up the reference if necessary):
if err != nil {
  if isAWSErr(err, "...", "...") {
    return nil
  }
}

Or the occasional:

if err != nil {
  if isAWSErr(err, "...", "...") {
    log.Printf("[WARN] Thing X (%s) not found, removing from state", d.Id())
    d.SetId("")
    return nil
  }
}

These look trivial when isolated, but when reviewing hundreds of lines of code they can be easy to miss. I'm not sure a linter can catch them either unless there is one out there that requires combining a nested if with its parent when its the only logic in the conditional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look trivial when isolated, but when reviewing hundreds of lines of code they can be easy to miss. I'm not sure a linter can catch them either unless there is one out there that requires combining a nested if with its parent when its the only logic in the conditional.

Great point! Thanks for sharing from your experiences 💯

return nil
}

if err != nil {
return fmt.Errorf("error disassociating RAM Resource Share (%s) Resource Association (%s): %s", resourceShareARN, resourceARN, err)
}

if err := waitForRamResourceShareResourceDisassociation(conn, resourceShareARN, resourceARN); err != nil {
return fmt.Errorf("error waiting for RAM Resource Share (%s) Resource Association (%s) disassociation: %s", resourceShareARN, resourceARN, err)
}

return nil
}

func decodeRamResourceAssociationID(id string) (string, string, error) {
idFormatErr := fmt.Errorf("unexpected format of ID (%s), expected SHARE,RESOURCE", id)

parts := strings.SplitN(id, ",", 2)
if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
return "", "", idFormatErr
}

return parts[0], parts[1], nil
}

func getRamResourceShareAssociation(conn *ram.RAM, resourceShareARN, resourceARN string) (*ram.ResourceShareAssociation, error) {
input := &ram.GetResourceShareAssociationsInput{
AssociationType: aws.String(ram.ResourceShareAssociationTypeResource),
ResourceArn: aws.String(resourceARN),
ResourceShareArns: aws.StringSlice([]string{resourceShareARN}),
}

output, err := conn.GetResourceShareAssociations(input)

if isAWSErr(err, ram.ErrCodeUnknownResourceException, "") {
return nil, nil
}

if err != nil {
return nil, err
}

if output == nil || len(output.ResourceShareAssociations) == 0 || output.ResourceShareAssociations[0] == nil {
return nil, nil
}

return output.ResourceShareAssociations[0], nil
}

func ramResourceAssociationStateRefreshFunc(conn *ram.RAM, resourceShareARN, resourceARN string) resource.StateRefreshFunc {
return func() (interface{}, string, error) {
resourceShareAssociation, err := getRamResourceShareAssociation(conn, resourceShareARN, resourceARN)

if err != nil {
return nil, ram.ResourceShareAssociationStatusFailed, err
}

if resourceShareAssociation == nil {
return nil, ram.ResourceShareAssociationStatusDisassociated, nil
}

if aws.StringValue(resourceShareAssociation.Status) == ram.ResourceShareAssociationStatusFailed {
extendedErr := fmt.Errorf("association status message: %s", aws.StringValue(resourceShareAssociation.StatusMessage))
return resourceShareAssociation, aws.StringValue(resourceShareAssociation.Status), extendedErr
}

return resourceShareAssociation, aws.StringValue(resourceShareAssociation.Status), nil
}
}

func waitForRamResourceShareResourceAssociation(conn *ram.RAM, resourceShareARN, resourceARN string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The func name waitForRamResourceShareResourceAssociation has a bit of a stutter. What do you think shortening this to waitForResourceAssociationShare(...) as the other parts are implied by the function signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard. 😅 In previous code reviews I was asked to do waitForServiceThingAction before for a consistent pattern, but everything in this regard is all over the place if you look across the codebase. If you feel strongly about this, I can certainly go back and adjust these (this isn't the only stuttering one simply due to the action being similar to the thing).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard.

Probably the first biggest problem in programming.

I don't feel strongly enough about it to adjust right now. I would be interested in thinking about it more when we start to think about large resource refactors or at least looking at what is the most consistent. Thanks again for the background context.

stateConf := &resource.StateChangeConf{
Pending: []string{ram.ResourceShareAssociationStatusAssociating},
Target: []string{ram.ResourceShareAssociationStatusAssociated},
Refresh: ramResourceAssociationStateRefreshFunc(conn, resourceShareARN, resourceARN),
Timeout: 5 * time.Minute,
}

_, err := stateConf.WaitForState()

return err
}

func waitForRamResourceShareResourceDisassociation(conn *ram.RAM, resourceShareARN, resourceARN string) error {
stateConf := &resource.StateChangeConf{
Pending: []string{ram.ResourceShareAssociationStatusAssociated, ram.ResourceShareAssociationStatusDisassociating},
Target: []string{ram.ResourceShareAssociationStatusDisassociated},
Refresh: ramResourceAssociationStateRefreshFunc(conn, resourceShareARN, resourceARN),
Timeout: 5 * time.Minute,
}

_, err := stateConf.WaitForState()

return err
}
172 changes: 172 additions & 0 deletions aws/resource_aws_ram_resource_association_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
package aws

import (
"fmt"
"testing"

"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ram"
)

func TestAccAwsRamResourceAssociation_basic(t *testing.T) {
var resourceShareAssociation1 ram.ResourceShareAssociation
resourceName := "aws_ram_resource_association.test"
rName := acctest.RandomWithPrefix("tf-acc-test")

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAwsRamResourceAssociationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAwsRamResourceAssociationConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsRamResourceAssociationExists(resourceName, &resourceShareAssociation1),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccAwsRamResourceAssociation_disappears(t *testing.T) {
var resourceShareAssociation1 ram.ResourceShareAssociation
resourceName := "aws_ram_resource_association.test"
rName := acctest.RandomWithPrefix("tf-acc-test")

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAwsRamResourceAssociationDestroy,
Steps: []resource.TestStep{
{
Config: testAccAwsRamResourceAssociationConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsRamResourceAssociationExists(resourceName, &resourceShareAssociation1),
testAccCheckAwsRamResourceAssociationDisappears(&resourceShareAssociation1),
),
ExpectNonEmptyPlan: true,
},
},
})
}

func testAccCheckAwsRamResourceAssociationDisappears(resourceShareAssociation *ram.ResourceShareAssociation) resource.TestCheckFunc {
return func(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).ramconn

input := &ram.DisassociateResourceShareInput{
ResourceArns: []*string{resourceShareAssociation.AssociatedEntity},
ResourceShareArn: resourceShareAssociation.ResourceShareArn,
}

_, err := conn.DisassociateResourceShare(input)
if err != nil {
return err
}

return waitForRamResourceShareResourceDisassociation(conn, aws.StringValue(resourceShareAssociation.ResourceShareArn), aws.StringValue(resourceShareAssociation.AssociatedEntity))
}
}

func testAccCheckAwsRamResourceAssociationExists(resourceName string, association *ram.ResourceShareAssociation) resource.TestCheckFunc {
return func(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).ramconn

rs, ok := s.RootModule().Resources[resourceName]

if !ok {
return fmt.Errorf("Not found: %s", resourceName)
}

resourceShareARN, resourceARN, err := decodeRamResourceAssociationID(rs.Primary.ID)

if err != nil {
return err
}

resourceShareAssociation, err := getRamResourceShareAssociation(conn, resourceShareARN, resourceARN)

if err != nil {
return fmt.Errorf("error reading RAM Resource Share (%s) Resource Association (%s): %s", resourceShareARN, resourceARN, err)
}

if resourceShareAssociation == nil {
return fmt.Errorf("RAM Resource Share (%s) Resource Association (%s) not found", resourceShareARN, resourceARN)
}

if aws.StringValue(resourceShareAssociation.Status) != ram.ResourceShareAssociationStatusAssociated {
return fmt.Errorf("RAM Resource Share (%s) Resource Association (%s) not associated: %s", resourceShareARN, resourceARN, aws.StringValue(resourceShareAssociation.Status))
}

*association = *resourceShareAssociation

return nil
}
}

func testAccCheckAwsRamResourceAssociationDestroy(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).ramconn

for _, rs := range s.RootModule().Resources {
if rs.Type != "aws_ram_resource_association" {
continue
}

resourceShareARN, resourceARN, err := decodeRamResourceAssociationID(rs.Primary.ID)

if err != nil {
return err
}

resourceShareAssociation, err := getRamResourceShareAssociation(conn, resourceShareARN, resourceARN)

if err != nil {
return err
}

if resourceShareAssociation != nil && aws.StringValue(resourceShareAssociation.Status) != ram.ResourceShareAssociationStatusDisassociated {
return fmt.Errorf("RAM Resource Share (%s) Resource Association (%s) not disassociated: %s", resourceShareARN, resourceARN, aws.StringValue(resourceShareAssociation.Status))
}
}

return nil
}

func testAccAwsRamResourceAssociationConfig(rName string) string {
return fmt.Sprintf(`
resource "aws_vpc" "test" {
cidr_block = "10.0.0.0/16"
tags {
Name = "tf-acc-test-ram-resource-association"
}
}
resource "aws_subnet" "test" {
cidr_block = "10.0.0.0/24"
vpc_id = "${aws_vpc.test.id}"
tags {
Name = "tf-acc-test-ram-resource-association"
}
}
resource "aws_ram_resource_share" "test" {
name = %q
}
resource "aws_ram_resource_association" "test" {
resource_arn = "${aws_subnet.test.arn}"
resource_share_arn = "${aws_ram_resource_share.test.id}"
}
`, rName)
}
Loading