Issue1248: Persistent Email Reminders

[anushka-singh]

Feature or Bugfix

• Feature

Detail

• When a share request is initiated and remains pending for an extended period, dataset producers will receive automated email reminders at predefined intervals. These reminders will prompt producers to either approve or extend the share request, thereby preventing delays in accessing datasets.

Attaching screenshots for emails:

• Email will be sent every Monday at 9am UTC. Schedule can be changed in cron expression in container.py

Relates

• Introduce Persistent Email Reminders for Producers of Datasets #1248

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

• Does this PR introduce or modify any input fields or queries - this includes
  fetching data from storage outside the application (e.g. a database, an S3 bucket)?
  • Is the input sanitized?
  • What precautions are you taking before deserializing the data you consume?
  • Is injection prevented by parametrizing queries?
  • Have you ensured no eval or similar functions are used?
• Does this PR introduce any functionality or component that requires authorization?
  • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  • Are you logging failed auth attempts?
• Are you using or adding any cryptographic features?
  • Do you use a standard proven implementations?
  • Are the used keys controlled by the customer? Where are they stored?
• Are you introducing any new policies/roles/users?
  • Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[dlpzx]

I think we need to review the design a bit because this pattern is new to data.all:

1. an scheduled ECS task gets triggered, reads some metadata from RDS, executes some light business logic and queues Worker tasks
2. the Worker Lambda sends the emails

At first sight it seems like we can simplify the workflow and run everything from ECS. Instead of creating Worker tasks (lines 235-249) we could directly call

SESEmailNotificationService.send_email_task(
                subject, message, recipient_groups_list, recipient_email_list
            )

[dlpzx]

@noah-paige @petrkalos @SofiaSazonova I would like your opinion on this one :)

[noah-paige]

I think the only benefit of sending task to queue for worker lambda is if the worker lambda has code bundled in its image that is not there for the ECS container. However this is not the case and both images should have all the backend code at their disposal for whatever data.all task

While separating out all of our backend code into a more microservice aligned design could be useful - it is not in scope for this PR and requires much more design consideration and careful thought

TLDR - I think let's just keep it all in ECS :)

[anushka-singh]

I actually had discussed the design with Noah before implementing it and we considered both options:

Option 1: Create a new ECS task that finds pending shares, creates tasks for them, and queues them up, then uses the existing worker Lambda to send emails.

Option 2: Use the send_email_task directly, as you suggested.

I chose Option 1 because I thought having a queue would be beneficial for managing a high volume of outgoing emails. It helps to queue up tasks and avoid potential clashes.

I am open to hearing what others have to say too.

[dlpzx]

I am going to vote for option 1: If we were implementing this from scratch, I would go for a queue + Lambda to process the emails. The part that I do not like form the current architecture is that we are using an ECS task to do very minor things. The new pattern that I would like to see is: scheduled Lambda ---> SQS queue ---> Worker Lambda. But that is out of scope for this PR. We can approve the current scheduled ECS ---> SQS queue ---> Worker Lambda and think about scheduled ECS tasks that could be handled in Lambda in a separate PR

[noah-paige]

+1 after some more thought I think we maybe should just keep all compute for email reminders in ECS for now

I think it should only require a small change to L238 of share_notification_service to not use Worker.queue() and instead directly call

SESEmailNotificationService.send_email_task(
     subject, message, recipient_groups_list, recipient_email_list
 )

I think it could be worthwhile to keep Task creation nonetheless for auditability

[anushka-singh]

@noah-paige Added

SESEmailNotificationService.send_email_task(
     subject, message, recipient_groups_list, recipient_email_list
 )

directly to share_notification_service

[dlpzx]

typo: I voted for option 1, but I am not strong opinionated.

[dlpzx]

Left some comments and changes needed

[anushka-singh]

@dlpzx @noah-paige
Addressed all the comments. Please take a look again.