Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Add sessionToken option to SQS transport#45064
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Messenger] Add sessionToken option to SQS transport#45064
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedJan 18, 2022
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
carsonbot commentedJan 18, 2022
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
with minor comment
src/Symfony/Component/Messenger/Bridge/AmazonSqs/Transport/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
jderusse left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
session_token is ephemeral. Having an option for this sounds weird to me.
We already accepted a similar PR for SES (#42982 (review))
Uh oh!
There was an error while loading.Please reload this page.
chalasr commentedJan 19, 2022
@jderusse given you approved, did you mean "good"? |
filkaris commentedJan 19, 2022
I saw what you wrote in the other PR, and it's true that the sdk can use environmental variables instead, and have no configuration in options However I've had cases with multiple credentials where I wanted to control to which account I wanted to connect Environmental variables have fixed names and can only keep one set of credentials, but if you use symfony config you can specify custom variable names eg. |
jderusse commentedJan 19, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@filkaris that's a very good use-case. Thank you for the snippet. 👍 for my side |
a3ae1b2 tod3bce0eComparefabpot commentedJan 19, 2022
Thank you@filkaris. |
Uh oh!
There was an error while loading.Please reload this page.
When trying to authenticate to use an SQS transport, AWS Credentials have 3 "keys"
The last one,
aws_session_tokenis only required for temporary credentials. In those cases though, it must be passed as well otherwise AWS returns a 403 Access Denied.The async-aws library supports this in its configurationhttps://async-aws.com/configuration.html#sessiontoken
This MR essentially makes sure the user can pass this parameter inside yaml options