[Mailer] Use AsyncAws to handle SES requests #35992

jderusse · 2020-03-06T22:25:15Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	#33183, #35468 and #35037
License	MIT
Doc PR	TODO

alternative to #33326

This PR replace the native code to call AWS SES by the new AsyncAws project maintained by @Nyholm and me.

This removes complexity of signing request, and adds new features likes:

authentication via .aws/config.ini, Instance profile, WebIdentity (K8S service account)
usesignature V4 (the one recommanded by the Official SDK )
fully compatible with API (uses the official AWS SDK interface contract to generate classes)

Because it's based on symfony/http-client, it's fully integrable with Symfony application.

jderusse · 2020-03-06T22:25:35Z

src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiTransportTest.php

-                new SesApiTransport('ACCESS_KEY', 'SECRET_KEY'),
-                'ses+api://ACCESS_KEY@email.eu-west-1.amazonaws.com',
+                new SesApiTransport(new SesClient(Configuration::create(['accessKeyId' => 'ACCESS_KEY', 'accessKeySecret' => 'SECRET_KEY']))),
+                'ses+api://ACCESS_KEY@us-east-1',


Is it a BC Break?

src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiTransportTest.php

jderusse · 2020-03-06T22:28:05Z

src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesApiTransport.php

        if ($email->getAttachments()) {
-            return [
-                'Action' => 'SendRawEmail',
-                'RawMessage.Data' => base64_encode($email->toString()),


this is probably the issue of #35037

src/Symfony/Component/Mailer/Bridge/Amazon/composer.json

jderusse · 2020-03-06T22:31:52Z

src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesApiTransport.php

+            $simpleMessage->setBody(new Body([
+                'Text' => [
+                    'Data' => $email->getTextBody(),
+                    'Charset' => $email->getTextCharset(),


this were not provided in current version

chalasr · 2020-03-07T12:41:06Z

IMHO AsyncAWS should also be a soft dependency when using the HTTP transport.
I think we would better preserve the current implementation and wire the AsyncAWS one when it is installed, since it is not a core dependency.
Installing it by default could then be handled in a flex recipe.

Nyholm · 2020-03-12T12:00:22Z

Thank you for the PR and thank you @chalasr for the review. I agree with you. AsyncAws should be a soft dependency. Currently it is only installed in require-dev section. If you are trying to use features that require the external library you get an exception. See https://github.com/symfony/symfony/pull/35992/files#diff-4e7b1bdc9093065fae5fff42cdde5d8cR42

chalasr · 2020-03-13T00:52:18Z

@Nyholm My point is: we have an HTTP implementation that works out of the box currently. Requiring AsyncAWS to be available for using the HTTP transport implicitly makes it an hard dependency, even if it's not in the require section, since you now get an exception instead of something that works immediately.

Considering the pros + the fact that AsyncAWS is maintained by two core members, I'm OK for requiring it with a meaningful exception.
But still, throwing in 5.1 is a BC break. IMHO we should deprecate the current implementation first, suggesting to install the missing package as it will be required in 6.0, and automatically wire the AsyncAWS-based implementation when it is installed.

Nyholm · 2020-03-14T10:47:39Z

Thank you. I understand the point you are trying to make.

we have an HTTP implementation that works out of the box currently.

Yes, But it has some bugs and it does not support all authentication mechanism available for AWS.
Of course, bugs can be fixed and authentication can be implemented.
We created AsyncAws to be able to abstract authentication away from this bridge. (And possible other parts of the symfony organization that may use AWS). We are also happy to extend the maintainer team with more Symfony core members if there is a wish for that.

But still, throwing in 5.1 is a BC break.

True, but this component and this bridge is still marked as experimental.

Side note: Most code in Async AWS is generated so the maintenance is kept to a minimum.

Nyholm · 2020-03-14T16:31:08Z

True, but this component and this bridge is still marked as experimental.

Oh no.. @chalasr pinged my privately and very politely pointed out that I was wrong. This component is not experimental.

jderusse · 2020-03-14T18:28:05Z

Thanks you for your review @chalasr. I agreed with you, The current HttpClient implementation should stay for BC.

But as @Nyholm pointed it, this implementation have several opened bugs. One of them is the way the request is signed. It currently use the version 2 which is not supported by all regions (https://docs.aws.amazon.com/general/latest/gr/signature-version-2.html). And maintaing a new implementation, is not trivial (see async-aws or AWS SDK)

I suggest to use the AsyncAws Adapter when installed package is installed otherwise to fallback to HttpClient Adapter (as suggested by @chalasr), AND to deprecate the HttpClient Adapter in 5.1 to remove the complexity from Symfony.

WDYT?

nicolas-grekas · 2020-03-14T18:29:42Z

Since this is experimental, we can break BC and cut maintenance efforts.

chalasr · 2020-03-15T00:00:12Z

@nicolas-grekas Actually, this is not experimental :) #35992 (comment)
@jderusse Sounds like a plan!

jderusse · 2020-03-15T22:47:09Z

PR updated according our discussion.

src/Symfony/Component/Mailer/Bridge/Amazon/CHANGELOG.md

composer.json

chalasr · 2020-03-17T08:58:23Z

Tests seem broken

UPGRADE-6.0.md

src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesTransportFactory.php

chalasr · 2020-04-12T12:57:48Z

Rebase needed

connorhu · 2020-04-20T07:14:14Z

@jderusse Since this is an alternative solution, can I close the #33326 pull request?

Nyholm · 2020-04-24T12:35:25Z

@connorhu Probably, but let's wait to see if this PR is accepted. If this PR is rejected then #33326 might be something that should be considered.

Nyholm

If this is ready for merge we should release 1.0.0 of async-aws/ses.

@jderusse Could you verify the failing tests?

chalasr

CI failures are unrelated. 👍

fabpot · 2020-05-03T14:23:52Z

Thank you @jderusse.

…ication (jderusse) This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- [AmazonSqsMessenger] Use AsyncAws to handle SQS communication | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | / | License | MIT | Doc PR | / Similar to #35992 this PR use AsyncAws to handle Sqs messages sent/receive It move complexity of authentication/streaming outside Symfony while keeping HttpClient integration. Commits ------- 7c4888e [AmazonSqsMessenger] Use AsyncAws to handle SQS communication

carsonbot added Status: Needs Review Feature Deprecation labels Mar 6, 2020

jderusse commented Mar 6, 2020

View reviewed changes

jderusse force-pushed the ses branch from b632f00 to bca6e20 Compare March 6, 2020 22:33

chalasr changed the title ~~Use AsyncAws to handle SES requests~~ [Mailer] Use AsyncAws to handle SES requests Mar 7, 2020

chalasr added the Mailer label Mar 7, 2020

chalasr added this to the next milestone Mar 7, 2020

jderusse force-pushed the ses branch 2 times, most recently from 06828b9 to 0da08e2 Compare March 15, 2020 22:44

jderusse mentioned this pull request Mar 16, 2020

[AmazonSqsMessenger] Use AsyncAws to handle SQS communication #36094

Merged

fabpot requested changes Mar 16, 2020

View reviewed changes

src/Symfony/Component/Mailer/Bridge/Amazon/CHANGELOG.md Outdated Show resolved Hide resolved

composer.json Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 16, 2020

jderusse force-pushed the ses branch from 0da08e2 to b9a1428 Compare March 16, 2020 16:50

jderusse force-pushed the ses branch 3 times, most recently from e298f62 to 55fa287 Compare March 18, 2020 22:48

stof reviewed Mar 19, 2020

View reviewed changes

UPGRADE-6.0.md Outdated Show resolved Hide resolved

src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesTransportFactory.php Outdated Show resolved Hide resolved

jderusse mentioned this pull request Apr 12, 2020

Add Async Aws php-enqueue/enqueue-dev#1046

Closed

1 task

jderusse force-pushed the ses branch from 82b5e7c to 845dd52 Compare April 12, 2020 13:34

Nyholm mentioned this pull request Apr 12, 2020

Package ideas async-aws/aws#201

Closed

10 tasks

jderusse force-pushed the ses branch from 845dd52 to d282198 Compare April 15, 2020 19:47

Nyholm approved these changes Apr 24, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Apr 24, 2020

jderusse force-pushed the ses branch 2 times, most recently from 7bcb68d to 9e9c6c7 Compare April 24, 2020 21:13

chalasr approved these changes May 3, 2020

View reviewed changes

fabpot approved these changes May 3, 2020

View reviewed changes

fabpot mentioned this pull request May 3, 2020

[Mailer] added Instance Profile support to SES Transport of Mailer #33326

Closed

[Mailer] Use AsyncAws to handle SES requests

2124387

fabpot force-pushed the ses branch from cf63418 to 2124387 Compare May 3, 2020 14:23

fabpot mentioned this pull request May 3, 2020

[Mailer] Use AsyncAws to handle SES requests symfony/symfony-docs#13611

Closed

fabpot merged commit 669b7f1 into symfony:master May 3, 2020

jderusse deleted the ses branch May 3, 2020 15:31

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

fabpot mentioned this pull request May 5, 2020

Release v5.1.0-BETA1 #36713

Merged

nandogameiro mentioned this pull request Jun 26, 2020

Issue with Amazon-Mailer package repman-io/repman#206

Closed

fabpot mentioned this pull request Aug 11, 2020

[Mailer] fixed destination override by envelope in amazon api transport #35042

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mailer] Use AsyncAws to handle SES requests #35992

[Mailer] Use AsyncAws to handle SES requests #35992

jderusse commented Mar 6, 2020 •

edited

Loading

jderusse Mar 6, 2020

jderusse Mar 6, 2020

jderusse Mar 6, 2020

chalasr commented Mar 7, 2020

Nyholm commented Mar 12, 2020

chalasr commented Mar 13, 2020

Nyholm commented Mar 14, 2020

Nyholm commented Mar 14, 2020

jderusse commented Mar 14, 2020

nicolas-grekas commented Mar 14, 2020

chalasr commented Mar 15, 2020

jderusse commented Mar 15, 2020

chalasr commented Mar 17, 2020

chalasr commented Apr 12, 2020

connorhu commented Apr 20, 2020

Nyholm commented Apr 24, 2020

Nyholm left a comment •

edited

Loading

chalasr left a comment

fabpot commented May 3, 2020

[Mailer] Use AsyncAws to handle SES requests #35992

[Mailer] Use AsyncAws to handle SES requests #35992

Conversation

jderusse commented Mar 6, 2020 • edited Loading

jderusse Mar 6, 2020

Choose a reason for hiding this comment

jderusse Mar 6, 2020

Choose a reason for hiding this comment

jderusse Mar 6, 2020

Choose a reason for hiding this comment

chalasr commented Mar 7, 2020

Nyholm commented Mar 12, 2020

chalasr commented Mar 13, 2020

Nyholm commented Mar 14, 2020

Nyholm commented Mar 14, 2020

jderusse commented Mar 14, 2020

nicolas-grekas commented Mar 14, 2020

chalasr commented Mar 15, 2020

jderusse commented Mar 15, 2020

chalasr commented Mar 17, 2020

chalasr commented Apr 12, 2020

connorhu commented Apr 20, 2020

Nyholm commented Apr 24, 2020

Nyholm left a comment • edited Loading

Choose a reason for hiding this comment

chalasr left a comment

Choose a reason for hiding this comment

fabpot commented May 3, 2020

jderusse commented Mar 6, 2020 •

edited

Loading

Nyholm left a comment •

edited

Loading