[GH-1420] Notification Improvements Stage 1 #1748

TejasRGitHub · 2024-12-26T21:21:55Z

Feature or Bugfix

Feature [Enhancements]

Detail

Stage 1 as described in the Gh issue 1420 ( #1420 (comment))

Table on information about when notifications are sent to whom they are sent. I will add this as a part of documentation as well.

Data.all Resource	Notification	Recipient
Share	Share Object Submitted	Dataset owner, steward and share owner ( requestor )
Share	Share Object Approved	Dataset owner, steward and share owner ( requestor )
Share	Share Object Failed	Dataset owner, steward and share owner ( requestor )
Share	Share Object Revoked / Rejected	Dataset owner, steward and share owner ( requestor )
Share	Share verifier finds share is unhealthy	Share requestor team
Share	Share reapplier finds share is unhealthy	Share requestor team
Share	Share reapplier finds share is healthy	Share requestor team
Weekly reminders (Share Objects )	Submitted shares and unhealthy shares	Share requestor team
Weekly reminders (Dataset Objects )	Unhealthy dataset	Dataset owner and stewards team
Weekly reminders (Environment Objects )	Unhealthy environments	Environment owner team
ECS task	Any failures in ECS task	Data.all admin team ( DAAdministrators )
Dataset Table	Dataset table is removed	Dataset owners and stewards and if any shares exist on that table then notify all share object requestor teams
Dataset Table	Dataset table is added	Dataset owners and stewards
Dataset Table	Dataset table which was previously deleted was added back again	Dataset owners and stewards

Tests

Email Sent when something fails in ECS tasks ✅ Sample email -

2. Weekly Reminder email ✅ -

3. Email sent when shares are reapplied ✅ -

4. Email sent when share verifier finds share in unhealthy state ✅

5. Email sent when a glue table is deleted / resynced / added Email sent to dataset owners and dataset stewards when a table is deleted

Email sent to the share owner group when a table is deleted and there are active shares on that table

Email sent incase of share failure -

Relates

notification improvements #1420

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.

noah-paige

part 1 Review - still have a couple more files on Dataset Table notifications and weekly digest

backend/dataall/modules/catalog/tasks/catalog_indexer_task.py

backend/dataall/modules/notifications/services/admin_notifications.py

backend/dataall/modules/notifications/services/ses_email_notification_service.py

backend/dataall/modules/shares_base/services/share_notification_service.py

backend/dataall/modules/shares_base/services/sharing_service.py

backend/dataall/modules/shares_base/services/share_notification_service.py

noah-paige · 2025-01-10T15:33:43Z

backend/dataall/modules/shares_base/db/share_object_repositories.py

+    def find_share_by_dataset_attributes(session, dataset_uri, dataset_owner, groups = None):
+        if groups is None:
+            groups = []


I am guessing there is an edge case where groups could be passed in explicitly as None before and not being set to [] so the query failed?

tbh, I added it because my editor was suggesting an improvement. I think this article does a fine job at explaining why - https://nikos7am.com/posts/mutable-default-arguments/

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py

noah-paige

some additional comments from pending file review

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py

noah-paige · 2025-01-10T17:35:38Z

backend/dataall/modules/s3_datasets/tasks/tables_syncer.py

+                        if table_status_map:
+                            log.info('Sending email notification after dataset table updates were found')
+                            try:
+                                DatasetTableNotifications(dataset=dataset).notify_dataset_table_updates(session=session, table_status_map=table_status_map)
+                            except Exception as e:
+                                error_log = f"Error occurred while sending email to notify about changes to the glue tables for dataset with uri: {dataset.datasetUri} due to: {e}"
+                                task_exceptions.append(error_log)


table_status_map will always be defined so this will always execute right?

also should there be a try catch here or only on the loop over datasets?

table_status_map will always be defined so this will always execute right?

The table_status_map will be empty when there are no newly added tables, no table has been deleted , no table which was deleted previously has been added. And thus when it is empty the if part won't execute.

also should there be a try catch here or only on the loop over datasets?

I have added this try catch explicitly so that that any email sending failures are caught. Also with this , the rest of the code deosn't get affected and I don't want it to get affected

…et policu is removed

TejasRGitHub · 2025-01-15T00:58:01Z

Hi @noah-paige , I have updated the PR. Please let me know if you have any additional code comments. Let me know if you need any clarification / discussion on few of my replies on your comments .

Few additional things which I noticed and added as a part of new updated PR,

Admin group config ( admin_notifications ) in config.json to enable/disable admin email notifications. I am only keeping email admin notifications and not creating UI notifications - as UI notifications will look bloated with a lot of content.
Added try..catch in sharing_services file where missing and reformatted to have the same pattern of try except finally block across the file

noah-paige · 2025-01-16T16:25:36Z

backend/dataall/core/groups/db/constants.py

nit: should we also explicitly import db in the __init__ file? (at dataall/backend/dataall/core/groups/__init__.py) -- think right now it is implicitly imported via api

I wouldn't mind it getting imported implicitly. With a blank init all the group_models and constants file will be imported.

backend/dataall/modules/notifications/services/admin_notifications.py

backend/dataall/modules/notifications/services/ses_email_notification_service.py

backend/dataall/modules/shares_base/services/sharing_service.py

TejasRGitHub · 2025-01-24T16:23:41Z

backend/dataall/modules/shares_base/db/share_object_repositories.py

@@ -42,11 +42,15 @@ def find_share(session, dataset: DatasetBase, env, principal_id, principal_role_
        )

    @staticmethod
-    def find_dataset_shares(session, dataset_uri):
-        return session.query(ShareObject).filter(ShareObject.datasetUri == dataset_uri).all()
+    def find_dataset_shares(session, dataset_uri: str):


Nothing has changed here. Only returning via query.allI() instead of on the same line

TejasRGitHub · 2025-01-24T16:32:07Z

backend/dataall/modules/shares_base/services/share_notification_service.py

@@ -332,30 +437,3 @@ def _create_notification_task(self, subject, msg):
                    log.info(f'Notification type : {share_notification_config_type} is not active')
        else:
            log.info('Notifications are not active')
-


Removing this function from here and moving it to ses_email_notification_service.py

TejasRGitHub · 2025-01-24T16:38:27Z

config.json

@@ -32,7 +32,8 @@
                        "active": false,
                        "persistent_reminders": false,
                        "parameters": {
-                            "group_notifications": true
+                            "group_notifications": true,
+                            "admin_notifications": true


Adding this new parameter to switch on/off admin notifications. This can be later removed and admin notifications can be enabled by default when email notifications are enabled. Keeping this for now, to test and see how many notifications an admin might get with the current code change. There is a chance in which admins can be bombarded with a lot of notifications in which this will serve as a good way to stop creating admin notifications

TejasRGitHub · 2025-01-24T16:39:02Z

deploy/stacks/container.py

@@ -161,7 +161,7 @@ def __init__(
            command=['python3.9', '-m', 'dataall.core.environment.tasks.env_stacks_updater'],
            container_id='container',
            ecr_repository=ecr_repository,
-            environment=self._create_env(),
+            environment=self.env_vars,


Switching to env_vars as those contains the same as self._create_env()

TejasRGitHub · 2025-01-24T16:44:41Z

Hi @petrkalos , @noah-paige , I have made code changes after taking a look at the review comments. Can you please take a look and let me if this PR looks good now

I have re-tested after code changes

Notifications after share is failed ( email + UI ) ✅
Notifications after share is Unhealthy ( email + UI ) ✅
Notifications after share is healthy ( email + UI ) ✅
Weekly digest reminder notifications ( email ) ✅
Notifications to owner ( when table are deleted, added, synced ) and share owners ( if there exists a share on the table ) - ✅
Notifications when ECS or any service fails to Admins ✅

TejasRGitHub added 6 commits December 26, 2024 15:21

Notification Improvements Stage 1, 70%

8750f63

Changes in the principal name

1cd2224

Weekly email task

1728d52

Weekly notifications code and minor changes

daa709e

Glue Table change notifications

84bf9fe

Additional comments and logs plus weekly reminder task

8c861ff

TejasRGitHub marked this pull request as ready for review January 3, 2025 00:27

noah-paige linked an issue Jan 10, 2025 that may be closed by this pull request

notification improvements #1420

Open

noah-paige reviewed Jan 10, 2025

View reviewed changes

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py Outdated Show resolved Hide resolved

noah-paige reviewed Jan 10, 2025

View reviewed changes

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py Outdated Show resolved Hide resolved

noah-paige reviewed Jan 10, 2025

View reviewed changes

TejasRGitHub added 6 commits January 10, 2025 14:01

Syncing changes from local deploy

99ed2ec

Adding new changes

c119c55

Minor correction in the create bucket policy code when access to buck…

f1e4079

…et policu is removed

New changes

870a835

Adding new changes for admin notifications

5b8ec01

Linting update

17158e2

TejasRGitHub added 2 commits January 15, 2025 11:45

Admin notification improvements

fd32987

Adding more refactoring changes

457ce82

noah-paige assigned TejasRGitHub Jan 16, 2025

noah-paige reviewed Jan 16, 2025

View reviewed changes

petrkalos requested changes Jan 17, 2025

View reviewed changes

TejasRGitHub added 6 commits January 21, 2025 14:15

Refactoring and corrections

878082a

Minor changes after testing

401b0de

Adding new changes and corrections

63dd9f1

Minor change and linting

828edf8

Formatting changes

8be4298

Adding improvements after review comments

a408553

TejasRGitHub added 2 commits January 24, 2025 10:12

Small slight changes

8b9b10c

Weekly notification enums

d83d85c

TejasRGitHub commented Jan 24, 2025

View reviewed changes

Few corrections

215ebe4

TejasRGitHub requested review from petrkalos and noah-paige January 24, 2025 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-1420] Notification Improvements Stage 1 #1748

[GH-1420] Notification Improvements Stage 1 #1748

TejasRGitHub commented Dec 26, 2024 •

edited

Loading

noah-paige left a comment

noah-paige Jan 10, 2025

TejasRGitHub Jan 13, 2025

noah-paige left a comment

noah-paige Jan 10, 2025

noah-paige Jan 10, 2025

TejasRGitHub Jan 14, 2025

TejasRGitHub commented Jan 15, 2025

noah-paige Jan 16, 2025

TejasRGitHub Jan 16, 2025

TejasRGitHub Jan 24, 2025

TejasRGitHub Jan 24, 2025

TejasRGitHub Jan 24, 2025

TejasRGitHub Jan 24, 2025

TejasRGitHub commented Jan 24, 2025

[GH-1420] Notification Improvements Stage 1 #1748

Are you sure you want to change the base?

[GH-1420] Notification Improvements Stage 1 #1748

Conversation

TejasRGitHub commented Dec 26, 2024 • edited Loading

Feature or Bugfix

Detail

Tests

Relates

Security

noah-paige left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noah-paige left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejasRGitHub commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejasRGitHub commented Jan 24, 2025

TejasRGitHub commented Dec 26, 2024 •

edited

Loading