Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(core): email relay #2741

Merged
merged 1 commit into from
Oct 26, 2023
Merged

feat(core): email relay #2741

merged 1 commit into from
Oct 26, 2023

Conversation

szkl
Copy link
Contributor

@szkl szkl commented Oct 23, 2023

Description

TBD

Related Issues

Testing

  • Manual

Checklist

  • I have read the CONTRIBUTING guidelines
  • I have tested my code (manually and/or automated if applicable)
  • I have updated the documentation (if necessary)

@szkl szkl marked this pull request as draft October 23, 2023 11:55
@szkl szkl requested review from betimshahini and Cosmin-Parvulescu and removed request for betimshahini October 23, 2023 11:55
@szkl szkl self-assigned this Oct 23, 2023
@szkl szkl force-pushed the feat/core/email-relay branch 3 times, most recently from 4d10d26 to 482ee4f Compare October 23, 2023 14:17
@szkl szkl changed the base branch from main to chore/platform/environment October 23, 2023 14:18
@szkl szkl requested a review from betimshahini October 23, 2023 14:18
@szkl
Copy link
Contributor Author

szkl commented Oct 23, 2023

Initial pull request was #2735

@szkl szkl marked this pull request as ready for review October 23, 2023 14:18
@szkl szkl force-pushed the feat/core/email-relay branch 3 times, most recently from ae2b9b0 to c496524 Compare October 23, 2023 14:26
Cosmin-Parvulescu
Cosmin-Parvulescu reviewed Oct 23, 2023
View reviewed changes
Copy link
Contributor

@Cosmin-Parvulescu Cosmin-Parvulescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to test this, so maybe we can schedule a pairing together tomorrow morning. Left some questions for then.

platform/email/src/types.ts Outdated Show resolved Hide resolved
platform/core/src/relay.ts
}

const recipients = new Array<Address>()
.concat(email.to || [])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle empty to differently? Can we hit this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is better to be ready for any possible case.

platform/core/src/relay.ts Outdated

const response = await fetch(request)
if (!response.ok)
console.error(JSON.stringify(await response.json(), null, 2))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance to hit a scenario where the response is not of type json but rather a text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. MailChannels responds with JSON for errors but it is also possible to have any kind of failure. It should be guarded.

@szkl szkl force-pushed the chore/platform/environment branch from 2e3c1b9 to 6013865 Compare October 23, 2023 19:49
@szkl szkl force-pushed the feat/core/email-relay branch 4 times, most recently from 991901c to 9a49797 Compare October 23, 2023 23:03
@szkl szkl force-pushed the chore/platform/environment branch 3 times, most recently from 9238f6f to 5091c56 Compare October 24, 2023 03:19
@szkl szkl force-pushed the feat/core/email-relay branch from 9a49797 to 02ea3dc Compare October 24, 2023 03:19
@szkl szkl force-pushed the feat/core/email-relay branch from 02ea3dc to 05be9b8 Compare October 24, 2023 03:54
Base automatically changed from chore/platform/environment to main October 24, 2023 08:21
@szkl szkl force-pushed the feat/core/email-relay branch 2 times, most recently from 2da7a83 to 9d15ad5 Compare October 24, 2023 08:54
@Cosmin-Parvulescu
Copy link
Contributor

  • Tried attaching an image on Gmail web client, relayed e-mail lost attachment
  • CC / BCC seems to work OK
  • Tried attaching an XLS on Gmail web client, relayed e-mail lost attachment on Gmail, preserved on @kubelt.com

Cosmin-Parvulescu
Cosmin-Parvulescu reviewed Oct 24, 2023
View reviewed changes
platform/core/.dev.vars.example
Comment on lines +6 to +7
INTERNAL_RELAY_DKIM_DOMAIN = ""
INTERNAL_RELAY_DKIM_SELECTOR = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these ones, DOMAIN & SELECTOR, here? If we declare them in the TOML files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I just put them here as an example for local development if needed.

.github/workflows/main-core.yaml Outdated
Comment on lines 65 to 66
INTERNAL_RELAY_DKIM_DOMAIN
INTERNAL_RELAY_DKIM_SELECTOR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we declare them in the TOML files, are these (DOMAIN & SELECTOR) injected in the GitHub CI pipeline or are they taken from the TOML? Should they be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, not. I am going to check it.

.github/workflows/main-core.yaml Outdated
Comment on lines 99 to 100
INTERNAL_RELAY_DKIM_DOMAIN: ${{ secrets.INTERNAL_RELAY_DKIM_DOMAIN }}
INTERNAL_RELAY_DKIM_SELECTOR: ${{ secrets.INTERNAL_RELAY_DKIM_SELECTOR }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we declare them in the TOML files, are these (DOMAIN & SELECTOR) injected in the GitHub CI pipeline or are they taken from the TOML? Should they be here?

Same for next and release

platform/core/src/index.ts Outdated
const decoder = new TextDecoder()
const reader = message.raw.getReader()
let content = ''
while (true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the while (true) loop, you can simplify the loop condition to exit when done:

let { done, value } = await reader.read();
while (!done) {
  content += decoder.decode(value);
  ({ done, value } = await reader.read());
}

platform/core/src/index.ts
@@ -36,6 +37,17 @@ export default {
},
})
},
async email(message: CloudflareEmailMessage, env: Environment) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a name such as decodeAndRelayEmail be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is called by the worker when an email is received.

platform/core/src/index.ts Outdated
const reader = message.raw.getReader()
let content = ''
while (true) {
const { done, value } = await reader.read()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should failure scenarios be considered? I'm a fan of letting it blow up but not sure what we want to do in case it fails. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I'll lookup what kind of errors can be thrown by this call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't found anything useful.

platform/core/src/relay.ts

import type { Environment } from './types'

export interface CloudflareEmailMessage {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this differ from the interfaces in platform/test/src/index.ts and platform/email/src/types.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a slight difference. I didn't want to couple the two modules just for this interface.

platform/core/src/relay.ts
recipient.address.endsWith(`@${env.INTERNAL_RELAY_DKIM_DOMAIN}`)
)

for (const recipient of recipients) {
Copy link
Contributor

@Cosmin-Parvulescu Cosmin-Parvulescu Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth considering parallelization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, not right away at least. This is only a background process.

@szkl szkl force-pushed the feat/core/email-relay branch from 9d15ad5 to 90da3ad Compare October 25, 2023 21:12
@szkl szkl force-pushed the feat/core/email-relay branch from 90da3ad to 6670435 Compare October 26, 2023 16:03
@szkl szkl merged commit 498aa96 into main Oct 26, 2023
14 checks passed
@szkl szkl deleted the feat/core/email-relay branch October 26, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cosmin-Parvulescu Cosmin-Parvulescu Cosmin-Parvulescu approved these changes

@betimshahini betimshahini Awaiting requested review from betimshahini

Assignees

@szkl szkl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@szkl @Cosmin-Parvulescu