runners; Add accountId to findAmiID #6744
Conversation
This should resolve issues where accountId is needed for findAmiID Signed-off-by: Eli Uriegas <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
facebook-github-bot
added
the
CLA Signed
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts
Fixed
Show fixed
Hide fixed
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts
Fixed
Show fixed
Hide fixed
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts
Fixed
Show fixed
Hide fixed
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
Dismissed
Show dismissed
Hide dismissed
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
Fixed
Show fixed
Hide fixed
Signed-off-by: Eli Uriegas <[email protected]>
Signed-off-by: Eli Uriegas <[email protected]>
Signed-off-by: Eli Uriegas <[email protected]>
| export async function findAmiID(metrics: Metrics, region: string, filter: string, owners = 'amazon'): Promise<string> { | ||
| const ec2 = new EC2({ region: region }); | ||
| const accountId = await getCurrentAccountId(); | ||
| const allOwners = [accountId, owners]; |
There was a problem hiding this comment.
These would be a good set of owners to include in the list:
| // Helper function to get the current AWS account ID | ||
| async function getCurrentAccountId(): Promise<string> { | ||
| const sts = new STS(); | ||
| const identity = await sts.getCallerIdentity().promise(); |
There was a problem hiding this comment.
Please double check to see if the lambdas have access to this identity before checking in the code. But I suspect my comment below includes all the identities that would be needed
There was a problem hiding this comment.
Claude seemed to think that we already did
| export async function findAmiID(metrics: Metrics, region: string, filter: string, owners = 'amazon'): Promise<string> { | ||
| const ec2 = new EC2({ region: region }); | ||
| const accountId = await getCurrentAccountId(); | ||
| const allOwners = [accountId, owners]; |
There was a problem hiding this comment.
Also, note that the function definition has "owners" passed in here as an optional parameter that no caller actually uses.
Consider changing that optional parameter's type to a list of strings and moving this owner identification code to a higher state of being 😇
| const sts = new STS(); | ||
| const identity = await sts.getCallerIdentity().promise(); | ||
| if (!identity.Account) { | ||
| throw new Error('Unable to retrieve AWS account ID'); |
There was a problem hiding this comment.
Instead of failing with error, should we just log the error and fallback to Amazon only images ?
|
What is our test plan on deploying this ? Is there a way to test it in canary only ? |
Signed-off-by: Eli Uriegas <[email protected]>
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
Dismissed
Show dismissed
Hide dismissed
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
Dismissed
Show dismissed
Hide dismissed
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
Dismissed
Show dismissed
Hide dismissed
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
Dismissed
Show dismissed
Hide dismissed
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
Dismissed
Show dismissed
Hide dismissed
| const ec2 = new EC2({ region: region }); | ||
| const accountId = await getCurrentAccountId(); | ||
| const lfAccountId = '391835788720'; |
There was a problem hiding this comment.
why is lf account hardcoded? Seems like a bug, something that would possibly not work in all cases (ali deployed in LF, using an image from Meta)
|
One note, maybe you want to specify the acc number somehow if you don't want to use amazon by default. Searching on all accounts simultaneously can be problematic if multiple accounts have a image with the same name. It is not obvious that you will use the one from both accounts with the later date. And it can be confusing in some cases and a bug hard to detect. |
This should resolve issues where accountId is needed for findAmiID
Resolves #6688