Change auth_helpers GetAccountID to first try using caller-identity for the AccountID

[potto007]

Fixes #1189 and #5064

Changes proposed in this pull request:

• Change 1:

Add helper function to aws/auth_helpers.go called GetCallerID. Unlike the GetAccountID function at the top of auth_helpers, GetCallerID prefers AccountID from the sts:get-caller-identity API call. That call should work in all cases where access permissions allow, but falls back to use accountID referenced by the AWSClient interface.

• Change 2

In locations where ARN in created by concatenation, AccountID is changed to use the GetCallerID helper function described in Change 1. This change occurs in each Read and Update function that touches RDS tags.

[ewbankkit]

It looks like DBInstanceArn, DBParameterGroupArn, DBSubnetGroupArn and DBSecurityGroupArn are now returned from the relevant RDS Create and Describe APIs.
It seems cleaner to use the returned ARN value rather than deriving it.

[bflad]

Hi @potto007 👋 Thanks for submitting this!

Introducing additional API calls per resource can be fairly detrimental to the operation of the provider. It can cause additional failure scenarios and is inefficient in large infrastructures.

If there is a bug with AWSClient.accountid not returning the correct account ID, we should prefer to fix that in the provider initialization process, not downstream in the resources themselves.

As @ewbankkit mentioned though, directly using available ARN attributes for resources is a more preferred solution rather than attempting to derive the ARN using values from the provider initialization process. Its likely this came to be because those attributes did not exist during the original resource implementation.

Do you have the time to switch their implementation of ARN handling? Thanks!

[potto007]

Hi @ewbankkit, @bflad - thanks for looking at this. I was looking at doing it that way, but have a related PR I will be submitting for elasticache. In the case of elasticache, there is no way to get the ARN for a cache cluster. I agree that it is cleaner to use a known value rather than trying to derive it.

Would you prefer if I pull out the helper function and submit it with that PR? I have run across other AWS resources that still require the ARN for tagging, like Lambda - but haven't done the due-diligence to discover whether they have the same limitation as elasticache.

[potto007]

@bflad Yes, I have the time. I'm doing the refactor now. I had some half-written code from last night that did what @ewbankkit is talking about, I just need to polish it up and apply it to each resource_aws_db*.go file.

[bflad]

Would you prefer if I pull out the helper function and submit it with that PR?

If you're referring to the GetCallerID() helper, it likely would not get merged for the reasons mentioned above. Personally, I would be expecting some sort of fix in GetAccountID() or Config.Client() to ensure AWSClient.accountid is properly initialized to the assumed role account ID. At worst, we can get something in fairly quickly to allow configuring the account ID via a provider configuration (the opposite of skip_requesting_account_id).

[potto007]

@bflad - I easily changed the various Read functions for aws/resource_aws_db_*.go using the ARNs from known resources. However, the Update functions are not guaranteed to have that information available, due to the fact that tags can be the only change required, meaning none of the requestUpdate / HasChange branches will be followed within a given Update function.

So, returning to the question of how the accountID should be defined... I was wondering if you knew of cases where aws-provider would expect Profile's AccountID rather than the assumed-role's AccountID? It's quite simple to add logic into GetAccountID to check, and use, the AccountID from config.AssumeRoleARN. However, I'm inclined to access config.AssumeRoleARN from the tagging locations if there is the remote chance of side-effects. Thoughts?

[bflad]

the Update functions are not guaranteed to have that information available

Assuming that d.Set("arn", XXX) has already been called for the resource, the ARN can always be retrieved via d.Get("arn").(string) from the Terraform state. The update function will already have this information available from previous read(s) unless the update function is called immediately after the create function. In those cases, we need to call that d.Set("arn", XXX) during the create function as well, which we normally try to avoid but in some cases the logic is much easier.

I was wondering if you knew of cases where aws-provider would expect Profile's AccountID rather than the assumed-role's AccountID?

Great question. 😄 I am willing to go out on a limb and say its probably not expected to want the account ID of the source account when assuming a cross-account role. Otherwise, the account ID can be retrieved by creating a provider that does not assume the cross-account role.

As far as I know, the behavior of the provider strictly handles the target account (e.g. managing the target account resources, retrieving the target account information from data sources, etc.) in every case except for this one apparently. I think its most valid to update the provider accountid attribute on provider initialization in this situation.

[potto007]

Thanks for giving me your thoughts on this, @bflad. Given your last statement, I think the simplest fix turns out to be the best fix. I reverted everything else I did, and simply moved the "try caller identity" block of code to be first in aws/auth_helpers GetAccountID. I have a test build in progress on our CI, and will run it through some of my failure states.

[potto007]

Yep, this fixed both my RDS tagging issues and my elasticache tagging issues. Would be interesting to see if it fixes the other issues you've identified.

[bflad]

Hi again, @potto007 👋 After thinking about this some more, I have some reservations about changing the ordering of handlers in this logic, which could potentially have some negative impact on folks if they using certain configurations.

For example, this change could start generating sts:GetCallerIdentity failure events in CloudTrail every Terraform run.

Instead of changing any API call ordering, I made an attempt at shortcutting this logic completely if the information is available beforehand: #5177

Can you see if that will work in your environment? It won't help if skip_credentials_validation is enabled, but it shouldn't introduce any breaking changes compared to the existing (master branch) logic.

[potto007]

@bflad that makes sense to me. I was a little nervous about how the order change might impact things. I'll had your PR into a test build over the weekend and see how it works in our environment.

[bflad]

@potto007 fantastic, you are awesome. Please reach out if you need help with anything. Sorry this issue is so nuanced and thank you for everything you have done so far!

[bflad]

This PR should have been superseded by #5177, released in version 1.31.0 of the AWS provider, which uses two methods to try and shortcut the account ID lookup before it reaches this logic:

• Using account ID of the assume role ARN if provided in the provider configuration
• Using account ID found in the sts:GetCallerIdentity call as part of the "credentials validation" portion of the provider initialization (requires that skip_credentials_validation is not enabled, of course 😄 )

Given credentials validation is the default, is recommended for most environments, and provides the same functional behavior of this PR, I'm going to close this one as the underlying problem should be resolved. If its not, we can certainly revisit this. Thanks again for your thoughts and work on this!

[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!