Fix AWS get_price action to use assumed role credentials#2104
Fix AWS get_price action to use assumed role credentials#2104ravikiranvm wants to merge 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the AWS get_price action to use resolved/assumed-role AWS credentials (via getCredentialsFromAuth) when querying the Pricing API, aligning runtime behavior with how credentials are already handled in the action’s property option loaders.
Changes:
- Resolve AWS credentials from
context.authbefore callinggetPriceListWithCache. - Update the unit test to assert
getPriceListWithCacheis called with resolved credentials rather than the raw auth object.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/blocks/aws/src/lib/actions/get-price-action.ts | Fetches credentials from auth and passes them into pricing calls. |
| packages/blocks/aws/test/get-price-action.test.ts | Updates expectation to match the new credential-passing behavior. |
Comments suppressed due to low confidence (1)
packages/blocks/aws/src/lib/actions/get-price-action.ts:157
getPriceListWithCacheis an async function; calling it withoutawaitinside thistryblock means promise rejections won't be caught by the surroundingtry/catch, so the action may throw an unwrapped error instead of the intended "An error occurred while fetching prices" message. Await the call (orreturn await ...) so errors are handled consistently.
const priceList = getPriceListWithCache(
credentials,
service.ServiceCode!,
filters,
PRICING_REGION,
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Shouldn't we show account selection in the UI? In this case, you're not displaying the list of accounts in the block properties.
|
| }); | ||
| const credentials = account?.['accounts'] | ||
| ? await getCredentialsForAccount(context.auth, account['accounts']) | ||
| : await getCredentialsFromAuth(context.auth); |
There was a problem hiding this comment.
This is a fallback for those existing flows which doesn't have the Account prop; so it fallbacks to the original behaviour
| options: async ({ auth }: any) => { | ||
| if (!auth) { | ||
| options: async ({ auth, account }: any) => { | ||
| if (!auth || !account) { |
There was a problem hiding this comment.
|| !account) this assertion doesnt do anything, account itself is a dynamic property, so its not visible but its also not undefined, this will never be true.
Even if it were to work, it would be wrong, as it would mean accounts without roles can not be used in this action
i think you can just remove the check entirely, if its not set we will get the error of no credentials found with account, and anyways account is a required property when shown
| if (!auth || !service) { | ||
| refreshers: ['auth', 'account', 'account.accounts', 'service'], | ||
| props: async ({ auth, account, service }, { input }) => { | ||
| if (!auth || !account || !service) { |
There was a problem hiding this comment.
same here about the account
Fixes OPS-3871.
Additional Notes
We were passing raw auth from context in get_price runtime instead of resolving credentials first.
When a connection is configured with only assumeRoleArn (no access key/secret), the Pricing call could fall back to host/default credentials under implicit-role mode. That caused pricing action to run under an unintended principal and fail with permission errors.