Introduced SMTP notification

[robert-ulbrich-mercedes-benz]

Why are the changes needed?

Currently E-Mail notifications can only be sent using either SendGrid or SES. In many enterprises none of the both services is used, but there is a custom SMTP server available in the company.

What changes were proposed in this pull request?

A new implementation of the Emailer intereface was added. It allows for notifications to be sent via a plain SMTP server. That SMTP server can be configured including authorization.

How was this patch tested?

There are automated unit tests. Also the changes were tested against SMTP interface of SES running a one-binary dev environment. The program was tested solely for our own use cases, which might differ from yours.

Setup process

• Setup dev environment as documented
• Start local kubernetes cluster flytectl demo start --dev
• Modify flyte-single-binary-local.yaml as found below

notifications:
  type: sandbox
  emailer:
    emailServerConfig:
      serviceName: smtp
      smtpServer: email-smtp.eu-central-1.amazonaws.com
      smtpPort: 25
      smtpUsername: <SES-Username>
      smtpPasswordSecretName: smtp_ses_password
    subject: "Notice: Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\"."
    sender: "[email protected]"
    body: >
      Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\". View details at
      <a href=\http://example.com/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}>workflow execution</a>

• Create a file smtp_ses_password in /etc/secrets containing the SES password.
• Compile the single binary make compile
• Start the single-binary POD_NAMESPACE=flyte ./flyte start --config flyte-single-binary-local.yaml
• Create a launchplan

from flytekit import LaunchPlan, WorkflowExecutionPhase
from flytekit.core.notification import Email
from hello import hello_world_wf

hello_world_lp = LaunchPlan.create(
    name="hello_wf_new",
    workflow=hello_world_wf,
    notifications=[
        Email(
            phases=[
                WorkflowExecutionPhase.SUCCEEDED,
                WorkflowExecutionPhase.FAILED,
                WorkflowExecutionPhase.TIMED_OUT,
            ],
            recipients_email=["<your-mail"],
        )
    ],
)

for workflow:

from flytekit import task, workflow

@task
def say_hello() -> str:
    return "Hello, World!"

@workflow
def hello_world_wf() -> str:
    res = say_hello()
    return res

• An email should be sent about the successful execution of the workflow

Screenshots

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

Docs link

Robert Ulbrich[email protected], Mercedes-Benz Tech Innovation GmbH

https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

[codecov]

Codecov Report

Attention: Patch coverage is 48.36066% with 126 lines in your changes missing coverage. Please review.

Project coverage is 36.25%. Comparing base (e67aae0) to head (50ce320).
Report is 163 commits behind head on master.

Files with missing lines	Patch %	Lines
...admin/pkg/async/notifications/mocks/smtp_client.go	32.25%	83 Missing and 22 partials ⚠️
...sync/notifications/implementations/smtp_emailer.go	89.04%	5 Missing and 3 partials ⚠️
flyteadmin/pkg/server/service.go	0.00%	6 Missing ⚠️
flyteadmin/pkg/async/notifications/factory.go	37.50%	5 Missing ⚠️
flyteadmin/pkg/rpc/adminservice/base.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5535      +/-   ##
==========================================
+ Coverage   36.21%   36.25%   +0.03%     
==========================================
  Files        1303     1305       +2     
  Lines      109644   109874     +230     
==========================================
+ Hits        39710    39831     +121     
- Misses      65810    65894      +84     
- Partials     4124     4149      +25     

Flag	Coverage Δ
unittests-datacatalog	51.37% <ø> (ø)
unittests-flyteadmin	55.56% <48.36%> (-0.04%)	⬇️
unittests-flytecopilot	12.17% <ø> (ø)
unittests-flytectl	62.21% <ø> (ø)
unittests-flyteidl	7.12% <ø> (ø)
unittests-flyteplugins	53.35% <ø> (ø)
unittests-flytepropeller	41.75% <ø> (ø)
unittests-flytestdlib	55.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[robert-ulbrich-mercedes-benz]

I am sorry about the bad unit test coverage for the smtp_emailer.go file. Without implementing a simple SMTP server and actually performing an integration test - I have no idea how we can boost the test coverage since I cannot simply mock interactions with the smtp client.

[wild-endeavor]

Just a couple questions. Would you mind please also making sure that the new settings are reachable using the flyte-core Helm chart? An example would be lovely. If not could you add to the chart in this PR, or cut another issue so we don't lose track of it?

Thank you!

[wild-endeavor]

Just to confirm, this is the intended behavior? If admin needs to send a thousand emails, it has to make a new client each time?

[robert-ulbrich-mercedes-benz]

Yes, this is intended behavior. When using the built-in email client of Go, most examples you find work that way. Also with the integration into the Notifications processor this is the simplest approach. Establishing a connection does not seem to be a very complex undertaking when looking at the source code of the email client.

I agree that keeping the connection is more efficient. But it also makes the code more complex, because the connection state needs to be maintained over an instance - so whenever an email is meant to be sent, it first needs to be checked if the connection is still alive.

If you want me to, I can refactor it to only open a connection once to the SMTP server.

[robert-ulbrich-mercedes-benz]

I just checked again the internal implementation of Golang's smtp client: It appears that the client implementation seems to be not multi-thread-safe. Every client instance keeps an internal state between function calls - so when reusing the same client while sending an email for writing another mail can lead to weird behavior.

My assumption is that there is only a single email processor per Flyte Admin instance. That means that there should be no concurrent email processing on any Flyte Admin instance, which makes it safe to reuse an existing Flyste client.

[robert-ulbrich-mercedes-benz]

Just a couple questions. Would you mind please also making sure that the new settings are reachable using the flyte-core Helm chart? An example would be lovely. If not could you add to the chart in this PR, or cut another issue so we don't lose track of it?

Thank you!

The settings can already be set with the current Helm chart. Actually the notification settings with Sendgrid and AWS SES also do not have a good support in the Helm charts yet. Well I guess I did not made it better either, but also did not want to fully update the Helm chart here.

The settings can be put into; workflow_notifications.config.notifications.emailer Helm key for AWS, Sendgrid and also the SMTP server notification settings.

if you want me to update the whole Helm file here, I can do it, but I guess we will then soon lose the scope of the merge request.

[wild-endeavor]

apologies for the delay @robert-ulbrich-mercedes-benz - mind taking a look at the errors in ci here?
much thanks.

[robert-ulbrich-mercedes-benz]

Hi @wild-endeavor,

this PR is becoming a never ending story: After I thought that I finally fixed all linting issues - while I could not run the linter locally , since it always broke down - I tried to run it in different docker container with different Golang versions - it never works.

When running the linter in the CI, it complains about issues in files that I did not touch.

I have no idea how I will ever be able to get a successful build. It is simply annoying. That way new committers are actively kept away :(

[robert-ulbrich-mercedes-benz]

Also now there are suddenly compilation issues in file smtp_emailer.go - despite I was able to compile it locally without any issues. Also the issue it complains about makes no sense at all, since the type implements the interface as it supposed to.

[eapolinario]

@robert-ulbrich-mercedes-benz , sorry for the churn. Can you merge master? I was able to repro the CI errors locally after I did that.

[robert-ulbrich-mercedes-benz]

@robert-ulbrich-mercedes-benz , sorry for the churn. Can you merge master? I was able to repro the CI errors locally after I did that.

I already merged master into the feature branch - I am up to date with master.

[eapolinario]

In a separate environment (like a temp directory), if you setup the repo like so you can repro this locally also:

git init

# Add the remote repository
git remote add origin https://github.com/flyteorg/flyte

# Disable automatic garbage collection
git config --local gc.auto 0

# Fetch the specific commit (with a depth of 1 to minimize data transfer)
git fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +3104ee4d906d535359fb0c2280864159b80ad4af:refs/remotes/pull/5535/merge

# Checkout the fetched commit (detached HEAD state)
git checkout --force refs/remotes/pull/5535/merge

This is essentially what the actions/checkout@v4 gh action does, e.g.: https://github.com/flyteorg/flyte/actions/runs/10792407625/job/29938153742?pr=5535#step:2:62

[eapolinario]

The emailer interface changed a few days ago (in https://github.com/flyteorg/flyte/blame/master/flyteadmin/pkg/async/notifications/interfaces/emailer.go#L12C4-L12C4), after the last time you merged master.

[robert-ulbrich-mercedes-benz]

The emailer interface changed a few days ago (in https://github.com/flyteorg/flyte/blame/master/flyteadmin/pkg/async/notifications/interfaces/emailer.go#L12C4-L12C4), after the last time you merged master.

Hi @eapolinario,

thanks for the hint! Actually Github was playing a trick on me:

Yesterday it showed me that I am synced with the master branch of the original repository. Only this morning it told me, that I am 6 commits behind - those commits were already merged days ago - no idea why those changes were only recognized this morning and not already yesterday :(. So hopefully things will work out now.

[neverett]

Docs changes look good to me, but I'll let others approve the rest of the changes.

[wild-endeavor]

helm test should be getting patched soon.

[welcome]

Congrats on merging your first pull request! 🎉