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

Use the new AWSClient.accountid when generating ARNs #6503

Closed

Conversation

bigkraig
Copy link
Contributor

@bigkraig bigkraig commented May 5, 2016

With the merge of #6385 we are now able to leverage the one-time collection of the AWS account id rather than trying to fetch it from multiple places in the code.

@radeksimko
Copy link
Member

radeksimko commented May 5, 2016

Two notes I have in my mind currently:

  1. Resource-by-resource we'll need to double check if there isn't any way around - i.e. is the IAM API really the only way to compose ARN? In some cases the resource API returns ARN back when raw name is passed into creation/update API call. We may be leverage that instead. I can find examples.
  2. We do need to think about cases where getting account ID may fail - i.e. the meta.accountid string will be empty - the build* functions should in such cases probably return an error and creation/update of such resource should also fail. Hopefully there will be near zero of such cases, but we should expect that to happen.

I will dive deeper into the code later on and do an actual review, I just wanted to share these thoughts here.

@radeksimko
Copy link
Member

@bigkraig I have just checked the API reference of RDS and ElastiCache and I can confirm (unfortunately) that there's no way to get the ARNs from those APIs - i.e. I must admit @stack72's original intention was totally reasonable.

That said I will submit a support ticket to AWS and let them pass the feedback down to RDS & ElastiCache teams. We may be able to get rid of all the logic at some point in the future at least.


The only thing that's preventing this PR from merging are missing conditions for cases when there's no account id provided. The helper function(s) for building ARN(s) should be returning error in such case(s) and that error should cause creation/update/deletion of a resource to fail with that error message - e.g. Failed to construct ARN due to a missing AWS Account ID.

I would consider merging all RDS helper functions into a single one (e.g. func buildRDSARN(identifier, resourceType, accountid, region string) string) + have wrapper functions like buildRDSDbInstanceARN that would be just calling buildRDSARN with the right resourceType per documentation.

Having less functions that generate ARNs would also help when fixing #7309 at some point.

Otherwise this is looking good, thank you for taking the time to do such refactoring. 👍

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Jul 17, 2016
@radeksimko
Copy link
Member

The only thing that's preventing this PR from merging are missing conditions for cases when there's no account id provided.

This was actually implemented & merged in #7151 (available in the next release) - I am therefore closing this PR.

Thanks for the interest and sorry it took me initially so long to get back to you.

@radeksimko radeksimko closed this Aug 7, 2016
@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Aug 7, 2016
@ghost
Copy link

ghost commented Apr 23, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants