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

connorhu · 2019-08-25T10:50:00Z

Q	A
Branch?	5.0
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#33183
License	MIT
Doc PR	-

This PR adds Instance Profile support to SES API and Http Transport of Mailer component.

Things to do:

changelog. I'm not sure about the version of the changelog entry, maybe 4.4.0.

The token expiry is an unhandled situation. But I'm not sure that the Transport part of the Mailer component can support transport renew.

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/ApiTokenCredential.php

fabpot

Great work! Can you also add a note in the CHANGELOG of the component about the BC break?

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/ApiTokenCredential.php

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/InstanceCredentialProvider.php

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

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/InstanceCredentialProvider.php

fabpot · 2019-08-26T06:14:38Z

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/InstanceCredentialProvider.php

+
+        $instanceMetadataServerURL = $this->getInstanceMetadataServerURL($roleName);
+
+        while (true) {


Why do we try several times?

The amazon sdk does the same. I guess connecting to the 169.254.169.254 IP sometime may temporary fail, so thats why I did keep the same logic.

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/InstanceCredentialProvider.php

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/UsernamePasswordCredential.php

OskarStark · 2019-08-26T06:57:06Z

This feature must target 4.4 branch please

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/ApiTokenCredential.php

fabpot · 2019-08-26T10:25:33Z

@connorhu As noticed by fabbot, you should rebase to get rid of the merge commits. And you should rebase on 4.4 instead of master. The current situation is a bit special as we are working on 2 versions in parallel: 4.4 and 5.0/master.

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/UsernamePasswordCredential.php

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

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

stof · 2019-08-26T10:40:01Z

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

        }

        if ('smtp' === $scheme || 'smtps' === $scheme) {
-            return new SesSmtpTransport($user, $password, $region, $this->dispatcher, $this->logger);
+            return new SesSmtpTransport($credential, $region, $this->dispatcher, $this->logger);


what if an ApiTokenCredential was instantiated and we reach this point ?

@stof I think the supported credential type(s) should be checked by the Transport instance.

@stof SesSmtpTransport only works with UsernamePasswordCredential (as you can see at the first parameter of the constructor). DX would be better if we check the class and throw an exception with a better message than a native type exception.

connorhu · 2019-08-26T10:49:38Z

@fabpot Ok. When I started to work on this I didn't know the right branch because of the complexity of the work (eg BC BREAK)

fabpot · 2019-08-26T11:05:24Z

We can break BC here as the component was marked as experimental in 4.3. That eases things a lot :)

nicolas-grekas · 2019-08-26T11:07:51Z

(this should target branch 4.4)

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

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

src/Symfony/Component/Mailer/Bridge/Amazon/SesRequest.php

mdeboer · 2019-08-26T21:52:52Z

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/InstanceCredentialProvider.php

+
+            $this->client = HttpClient::create();
+        } else {
+            $this->client = $client;


Nitpicking mode on: no need for an else here. Simply do $this->client = $client above where you assign retries. Then check if $this->client is null instead of $client and you can remove the if.

@mdeboer I can remove the else, but you're right.

Sorry yeah, I meant the else hehe 😜 Blame the heatwave..

fabpot · 2019-09-02T16:06:27Z

@connorhu I'm going to rebase on 4.4 this PR.

fabpot · 2019-09-02T16:16:37Z

@connorhu It's a bit more difficult than expected due to the recent changes in the code. Can you have a look? I think the SesRequest class is not the right abstraction. Tell me if you need help.

connorhu · 2019-09-02T17:58:35Z

@fabpot What would be the right abstraction instead of SesRequest? We need to create a bunch of headers from the content and it needs to be a reusable logic because of SesApiTransport and SesHttpTransport. Both of them use this code, but both use a slightly different way.

fabpot · 2019-09-04T06:42:25Z

@connorhu Not sure yet. I let you rebase and I think you will understand what I mean. Some logic in SesRequest are needed in __toString() as well.

connorhu · 2019-09-04T06:48:06Z

@fabpot Ok then. I will do the rebase.

jderusse

The AWS SDK provides many way to authenticate (some of theme are really simple), What's about implementing the most used and chaining theme (in the same way SDK does) ?

I think about AWS config File, official AWS_ENV vars and "assumed role with web identity" (the last one is used when using EKS(K8S) and allocating a ROLE to a ServiceAccount)

$credentialProvider = new ChainCredentialProvider([
  new UserPassProvider($user, $password),
  new EnvVarProvider(),
  new ConfigFileProvider(),
  new RoleProvider(),
  new OidcProvider(),
]);

jderusse · 2020-01-30T13:47:41Z

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/InstanceCredentialProvider.php

+                throw new LogicException(sprintf('You cannot use "%s" as the HttpClient component is not installed. Try running "composer require symfony/http-client".', __CLASS__));
+            }
+
+            $this->client = HttpClient::create();


I think the default timeout is 1.0s in official SDK

jderusse · 2020-01-30T13:52:44Z

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

+        } catch (IncompleteDsnException $e) {
+            $role = $dsn->getOption('role');
+
+            if (null === $role) {


role is not needed. In official AWS SDK, the role is fetched from instance metadata. (see InstanceProfileProvider)

fabpot · 2020-05-03T14:21:31Z

Closing in favor of #35992

@Nyholm

This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- [Mailer] Use AsyncAws to handle SES requests | 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](https://github.com/async-aws/aws) 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. Commits ------- 2124387 [Mailer] Use AsyncAws to handle SES requests

carsonbot added Status: Needs Review Feature BC Break labels Aug 25, 2019

OskarStark reviewed Aug 26, 2019

View reviewed changes

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/ApiTokenCredential.php Outdated Show resolved Hide resolved

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/ApiTokenCredential.php Outdated Show resolved Hide resolved

fabpot requested changes Aug 26, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 26, 2019

OskarStark reviewed Aug 26, 2019

View reviewed changes

src/Symfony/Component/Mailer/Bridge/Amazon/Credential/ApiTokenCredential.php Outdated Show resolved Hide resolved

stof requested changes Aug 26, 2019

View reviewed changes

nicolas-grekas added this to the next milestone Aug 26, 2019

yethee reviewed Aug 26, 2019

View reviewed changes

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

mdeboer reviewed Aug 26, 2019

View reviewed changes

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

connorhu force-pushed the master branch from 3c4d586 to ed310e6 Compare August 26, 2019 13:11

connorhu changed the base branch from master to 4.4 August 26, 2019 13:36

connorhu changed the base branch from 4.4 to master August 26, 2019 13:37

mdeboer reviewed Aug 26, 2019

View reviewed changes

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

mdeboer reviewed Aug 26, 2019

View reviewed changes

connorhu changed the title ~~added Instance Profile support to SES Transport of Mailer~~ [Mailer] added Instance Profile support to SES Transport of Mailer Aug 29, 2019

fabpot added the Mailer label Sep 2, 2019

added AWS instance credential provider for SES transport (api and http)

de1e645

fabpot force-pushed the master branch from b99da6d to de1e645 Compare September 2, 2019 16:04

drzraf mentioned this pull request Oct 12, 2019

[Mailer] Add Message-Id to SentMessage when sending an email #33967

Merged

cfoehrdes mentioned this pull request Dec 19, 2019

[MAILER] Let Amazon Mailer use the AWS SDK if available #33183

Closed

jderusse reviewed Jan 30, 2020

View reviewed changes

jderusse mentioned this pull request Mar 6, 2020

[Mailer] Use AsyncAws to handle SES requests #35992

Merged

fabpot closed this May 3, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

connorhu commented Aug 25, 2019 •

edited

Loading

fabpot left a comment

fabpot Aug 26, 2019

connorhu Aug 26, 2019

OskarStark commented Aug 26, 2019

fabpot commented Aug 26, 2019

stof Aug 26, 2019

mdeboer Aug 26, 2019

connorhu Aug 26, 2019

connorhu commented Aug 26, 2019

fabpot commented Aug 26, 2019

nicolas-grekas commented Aug 26, 2019

mdeboer Aug 26, 2019

connorhu Aug 27, 2019

mdeboer Aug 27, 2019

fabpot commented Sep 2, 2019

fabpot commented Sep 2, 2019

connorhu commented Sep 2, 2019

fabpot commented Sep 4, 2019

connorhu commented Sep 4, 2019

jderusse left a comment

jderusse Jan 30, 2020

jderusse Jan 30, 2020

fabpot commented May 3, 2020


		$instanceMetadataServerURL = $this->getInstanceMetadataServerURL($roleName);

		while (true) {

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

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

Conversation

connorhu commented Aug 25, 2019 • edited Loading

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark commented Aug 26, 2019

fabpot commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorhu commented Aug 26, 2019

fabpot commented Aug 26, 2019

nicolas-grekas commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 2, 2019

fabpot commented Sep 2, 2019

connorhu commented Sep 2, 2019

fabpot commented Sep 4, 2019

connorhu commented Sep 4, 2019

jderusse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented May 3, 2020

connorhu commented Aug 25, 2019 •

edited

Loading