New Resource: aws_ram_resource_association

[bflad]

Reference: #6527

Output from acceptance testing:

--- PASS: TestAccAwsRamResourceAssociation_disappears (29.50s)
--- PASS: TestAccAwsRamResourceAssociation_basic (31.78s)

[dbektas]

Hi @bflad !

as far as I am aware, one needs to switch to Master Account within a certain AWS organisation -> go to AWS RAM -> Settings -> enable Sharing.
This step is a requirement in order to be able to share resource with other principals within a certain organisation. Just then you can switch back to your previous account and be able to share resources with others.

However I am not sure why you are not able to share resources from standalone account (meaning share with external accounts)? I was expecting this test to pass as there are no prior requirements to be conducted.

Please let me know if I can be of further help.

[bflad]

@dbektas I can understand the error with our "normal" testing AWS account that is a member of an AWS Organizations Organization, but the error is really odd with the standalone AWS account (AWS Organizations disabled / not a member or master of an Organization).

In both my manual and automated testing it is not trying to associate any principals with the Resource Share, just associating resources with it via AssociateResourceShare, to manage that piece of functionality with the service. The fact the error doesn't occur during Resource Share creation, but only after, is strange. 🙁 In any event, I have opened AWS Support Case 5755832021 to track this and will report back any findings from that.

[dbektas]

Yeah, I just checked RAM whitepaper and I would suggest that this is not even foreseen to be supported by AWS.

Meaning, the only way to share resources is to be within an organisation.

However, im subscribed to this, keenly waiting for the resolution.

[bflad]

Enabling Resource Sharing in the master AWS Organizations account got past the AssociateResourceShare error in the account that is part of an organization. I'll try to post further updates from support regarding whether standalone accounts are supported at all or if they plan on having CreateResourceShare fail the same way, fixing the AssociateResourceShare error, or improving the error message/documentation.

Terraform resource has been successfully acceptance tested and is ready for review.

[sudomann]

When should one expect to be able to use this?

[bflad]

When should one expect to be able to use this?

When it is reviewed by another maintainer, any feedback is addressed, and the merged code is released. My best guess would be either today (if someone is available) or middle of next week.

[nywilken]

This looks good to me. I left a few questions and minor suggestions but nothing that would prevent this from being merged.

[nywilken]

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

[bflad]

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.

[nywilken]

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.

[nywilken]

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?

[bflad]

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).

[nywilken]

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.

[nywilken]

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)
}

[bflad]

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.

[nywilken]

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 💯

[bflad]

This has been released in version 1.59.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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!