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.

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

feat(sensor): email trigger #2793

Merged
merged 29 commits into from
Sep 17, 2023

Conversation

gokulav137
Copy link
Contributor

@gokulav137 gokulav137 commented Sep 9, 2023

Adding email triggers support using existing email notification service

Resolves #1817

Also added Example and unit tests for email trigger

First time contributor here. Please let me know incase any issues/ changes in design

Email Trigger specification:

EmailTrigger

(Appears on: TriggerTemplate)

EmailTrigger refers to the specification of the email notification trigger.

Field Description
parameters
\[\]TriggerParameter
(Optional)

Parameters is the list of key-value extracted from event’s payload that are applied to the trigger resource.

username
string
(Optional)

Username refers to the username used to connect to the smtp server.

smtpPassword
Kubernetes core/v1.SecretKeySelector
(Optional)

SMTPPassword refers to the Kubernetes secret that holds the smtp password used to connect to smtp server.

host
string

Host refers to the smtp host url to which email is send.

port
int32
(Optional)

Port refers to the smtp server port to which email is send. Defaults to 0.

to
\[\]string
(Optional)

To refers to the email addresses to which the emails are send.

from
string
(Optional)

From refers to the address from which the email is send from.

subject
string
(Optional)

Subject refers to the subject line for the email send.

body
string
(Optional)

Body refers to the body/content of the email send.

Example:

apiVersion: argoproj.io/v1alpha1
kind: Sensor
metadata:
  name: webhook
spec:
  dependencies:
    - name: test-dep
      eventSourceName: webhook
      eventName: example
  triggers:
    - parameters:
        - src:
            dependencyName: test-dep
            dataKey: body.to
          dest: email.to.-1
        - src:
            dependencyName: test-dep
            dataTemplate: "Hi {{.Input.body.name}},\n\n\tHello There.\n\nThanks,\nObi"
          dest: email.body
      template:
        name: email-trigger
        email:
          username: username
          smtpPassword:
            key: password
            name: smtp-secret
          # to:
          #   - [email protected]
          #   - [email protected]
          host: smtp.example.net
          port: 587
          from: [email protected]
          subject: Hello There

Checklist:

dependabot bot and others added 20 commits September 9, 2023 19:19
…goproj#2781)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: gokulav137 <[email protected]>
Signed-off-by: gokulav137 <[email protected]>
    - EmailTrigger use EmailService to send email
    - Execute asserts receiver email is provided
    - Execute asserts receiver emails are valid emails
    - Execute asserts Body cannot be empty
    - Execute asserts Subject cannot be empty

Signed-off-by: gokulav137 <[email protected]>
- Change Port from int to int32
- Fix typo portobuf to protobuf
- Change protobuf type for port from bytes to varint
- Fix typo protobuff to protobuf

Signed-off-by: gokulav137 <[email protected]>
Signed-off-by: gokulav137 <[email protected]>
…ontrollers/sensor/validate.go

Signed-off-by: gokulav137 <[email protected]>
…address

Change is made as notification.emailService doesnot support multiple to
email addresses

Signed-off-by: gokulav137 <[email protected]>
username and password together

Signed-off-by: gokulav137 <[email protected]>
- Add optional comment to EmailTrigger type
- Move validation for To,From,Subject and Body from
	controller validation to trigger execution validation

Signed-off-by: gokulav137 <[email protected]>
EmailTrigger Port default value specified as 0

Signed-off-by: gokulav137 <[email protected]>
- Username can't be empty
- Host can't be empty
- Port should be between 0-65535

Signed-off-by: gokulav137 <[email protected]>
- Test EmailTrigger.FetchResource
- Test EmailTrigger.ApplyResource
- Test EmailTrigger.Execute

Signed-off-by: gokulav137 <[email protected]>
Signed-off-by: gokulav137 <[email protected]>
Signed-off-by: gokulav137 <[email protected]>
@gokulav137 gokulav137 force-pushed the feature-sensor-email-trigger branch from 4e85cd9 to bd49ef9 Compare September 9, 2023 13:52
@gokulav137 gokulav137 marked this pull request as ready for review September 9, 2023 14:14
@gokulav137 gokulav137 marked this pull request as draft September 9, 2023 14:17
@gokulav137 gokulav137 marked this pull request as ready for review September 9, 2023 14:40
Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

Thank for submitting the PR!

Port int32 `json:"port,omitempty" protobuf:"varint,5,opt,name=port"`
// To refers to the email address to which email is send.
// +optional
To string `json:"to,omitempty" protobuf:"bytes,6,opt,name=to"`
Copy link
Member

Choose a reason for hiding this comment

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

Is making it []string 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.

Making it [] string would better. But argoproj/notifications-engine doesn't support multiple to emails.
This line passes only one to address down.
https://github.com/argoproj/notifications-engine/blob/master/pkg/services/email.go#L84C43-L84C43

}
destination := notifications.Destination{
Service: "email",
Recipient: emailTrigger.To,
Copy link
Member

Choose a reason for hiding this comment

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

How to send to multiple recipients?

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 was thinking along the lines that the user can create a mail group at their end and use that.

But if we need to support this out of the box, I think we have to either:

  • Modify the notifications-engine to support multiple to emails
    maybe something like
to := String.Split(dest.Recipient, ",")
email := smtp.New(smtp.Options{
........
........
}).WithSubject(subject).WithBody(body).To(to[0], to[1:]...)

in https://github.com/argoproj/notifications-engine/blob/master/pkg/services/email.go#L77-L84
This shouldn't break any existing functionality

OR

  • Send multiple email notifications with each of the to addresses. But this isn't ideal.

OR

  • we have to implement a different email service

@whynowy , Would be great to get your opinion on this. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@gokulav137 gokulav137 Sep 13, 2023

Choose a reason for hiding this comment

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

My understanding was that it was sending out multiple emails out with different recipients as mentioned in the second option. As this matches with pattern in other applications like slack where a recipient will be a channel or user.

Hence the suggestion of adding support for comma separated emails for recipients to the email notification service at notification-engine.

But I will take another look at it. Thanks.

Copy link
Contributor Author

@gokulav137 gokulav137 Sep 13, 2023

Choose a reason for hiding this comment

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

Cloned the notifications-engine repo and ran it locally.

Its sending the emails separately. So an email will be sent out for each recipient:

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: another
  annotations:
    notifications.argoproj.io/subscriptions: |
      - trigger: [on-cert-ready]
        destinations:
          - service: email
            recipients: [[email protected], [email protected]]
spec:
  secretName: something
  issuerRef:
    name: letsencrypt-staging
  emailAddresses:
    - [email protected]

I tried adding the comma separated change it worked.

 to := String.Split(dest.Recipient, ",")
 email := smtp.New(smtp.Options{
 ........
 ........
 }).WithSubject(subject).WithBody(body).To(to[0], to[1:]...)

It send an email with multiple to addresses:

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: another
  annotations:
    notifications.argoproj.io/subscriptions: |
      - trigger: [on-cert-ready]
        destinations:
          - service: email
            recipients: ["[email protected],[email protected]", [email protected]]
spec:
  secretName: something
  issuerRef:
    name: letsencrypt-staging
  emailAddresses:
    - [email protected]

Here one email would be sent with only [email protected] as recipient and another with both.

@whynowy what do you think. Should I go with this approach and raise a pull request there. Looks like it will add to notification-engines functionality too.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest:

  1. Make the To a []string;
  2. Do for loop here to send notifications;
  3. If there's a follow up in notification-engine to support multiple recipients, we update it 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.

Cool, sounds like a good plan. Will do the same. Thanks.

@whynowy
Copy link
Member

whynowy commented Sep 12, 2023

Please also add a doc in https://github.com/argoproj/argo-events/tree/master/docs/sensors/triggers. Thanks!

@whynowy whynowy changed the title feature sensor email trigger feat(sensor): email trigger Sep 12, 2023
gokulav137 and others added 8 commits September 16, 2023 09:45
- Modified EmailTrigger.To from string to []string
- Refactored email sending logic to send an email each to all the
	addresses in To

Signed-off-by: gokulav137 <[email protected]>
…omplexity

Removed parameterization to reduce complexity in example

Signed-off-by: gokulav137 <[email protected]>
- Modify types comment for Username and SMTPPassword to be optional
- Remove Username and SMTPPassword requirement check
- Refactor SMTPPassword retrieval from secrets to only when SMTPPassword is
	not nil

Signed-off-by: gokulav137 <[email protected]>
Signed-off-by: gokulav137 <[email protected]>
- Add dynamic to email use case and commented satic to email use case

Signed-off-by: gokulav137 <[email protected]>
Signed-off-by: gokulav137 <[email protected]>
@gokulav137 gokulav137 requested a review from whynowy September 16, 2023 09:10
var errs error
validEmail := regexp.MustCompile(`^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$`)
for _, addr := range emailTrigger.To {
if !validEmail.MatchString(addr) {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we move it to validate.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand validate.go does the pre validations when a sensor is created.

To support dynamic recipient address the validation was moved here. So that validation for to addresses will take place during execution.

Please correct me if I am wrong.

Copy link
Member

Choose a reason for hiding this comment

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

@gokulav137 - you are right, missed that!

@whynowy whynowy merged commit 1d992bc into argoproj:master Sep 17, 2023
joelcomp1 pushed a commit to joelcomp1/argo-events that referenced this pull request Sep 26, 2023
Signed-off-by: gokulav137 <[email protected]>
Signed-off-by: jmillage <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email Trigger
2 participants