diff --git a/backend/dataall/modules/dataset_sharing/services/dataset_alarm_service.py b/backend/dataall/modules/dataset_sharing/services/dataset_alarm_service.py index d568dd4d8..0d31f3516 100644 --- a/backend/dataall/modules/dataset_sharing/services/dataset_alarm_service.py +++ b/backend/dataall/modules/dataset_sharing/services/dataset_alarm_service.py @@ -19,11 +19,9 @@ def trigger_table_sharing_failure_alarm( target_environment: Environment, ): log.info('Triggering share failure alarm...') - subject = ( - f'ALARM: DATAALL Table {table.GlueTableName} Sharing Failure Notification' - ) + subject = f'Data.all Share Failure for Table {table.GlueTableName}'[:100] message = f""" - You are receiving this email because your DATAALL {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to share the table {table.GlueTableName} with Lake Formation. + You are receiving this email because your Data.all {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to share the table {table.GlueTableName} with Lake Formation. Alarm Details: - State Change: OK -> ALARM @@ -51,9 +49,9 @@ def trigger_revoke_table_sharing_failure_alarm( target_environment: Environment, ): log.info('Triggering share failure alarm...') - subject = f'ALARM: DATAALL Table {table.GlueTableName} Revoking LF permissions Failure Notification' + subject = f'Data.all Revoke LF Permissions Failure for Table {table.GlueTableName}'[:100] message = f""" - You are receiving this email because your DATAALL {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to revoke Lake Formation permissions for table {table.GlueTableName} with Lake Formation. + You are receiving this email because your Data.all {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to revoke Lake Formation permissions for table {table.GlueTableName} with Lake Formation. Alarm Details: - State Change: OK -> ALARM @@ -76,11 +74,9 @@ def trigger_revoke_table_sharing_failure_alarm( def trigger_dataset_sync_failure_alarm(self, dataset: Dataset, error: str): log.info(f'Triggering dataset {dataset.name} tables sync failure alarm...') - subject = ( - f'ALARM: DATAALL Dataset {dataset.name} Tables Sync Failure Notification' - ) + subject = f'Data.all Dataset Tables Sync Failure for {dataset.name}'[:100] message = f""" -You are receiving this email because your DATAALL {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to synchronize Dataset {dataset.name} tables from AWS Glue to the Search Catalog. +You are receiving this email because your Data.all {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to synchronize Dataset {dataset.name} tables from AWS Glue to the Search Catalog. Alarm Details: - State Change: OK -> ALARM @@ -101,11 +97,9 @@ def trigger_folder_sharing_failure_alarm( target_environment: Environment, ): log.info('Triggering share failure alarm...') - subject = ( - f'ALARM: DATAALL Folder {folder.S3Prefix} Sharing Failure Notification' - ) + subject = f'Data.all Folder Share Failure for {folder.S3Prefix}'[:100] message = f""" -You are receiving this email because your DATAALL {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to share the folder {folder.S3Prefix} with S3 Access Point. +You are receiving this email because your Data.all {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to share the folder {folder.S3Prefix} with S3 Access Point. Alarm Details: - State Change: OK -> ALARM - Reason for State Change: S3 Folder sharing failure @@ -129,11 +123,9 @@ def trigger_revoke_folder_sharing_failure_alarm( target_environment: Environment, ): log.info('Triggering share failure alarm...') - subject = ( - f'ALARM: DATAALL Folder {folder.S3Prefix} Sharing Revoke Failure Notification' - ) + subject = f'Data.all Folder Share Revoke Failure for {folder.S3Prefix}'[:100] message = f""" -You are receiving this email because your DATAALL {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to share the folder {folder.S3Prefix} with S3 Access Point. +You are receiving this email because your Data.all {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to share the folder {folder.S3Prefix} with S3 Access Point. Alarm Details: - State Change: OK -> ALARM - Reason for State Change: S3 Folder sharing Revoke failure @@ -173,11 +165,9 @@ def handle_bucket_sharing_failure(self, bucket: DatasetBucket, target_environment: Environment, alarm_type: str): log.info(f'Triggering {alarm_type} failure alarm...') - subject = ( - f'ALARM: DATAALL S3 Bucket {bucket.S3BucketName} {alarm_type} Failure Notification' - ) + subject = f'Data.all S3 Bucket Failure for {bucket.S3BucketName} {alarm_type}'[:100] message = f""" -You are receiving this email because your DATAALL {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to {alarm_type} the S3 Bucket {bucket.S3BucketName}. +You are receiving this email because your Data.all {self.envname} environment in the {self.region} region has entered the ALARM state, because it failed to {alarm_type} the S3 Bucket {bucket.S3BucketName}. Alarm Details: - State Change: OK -> ALARM - Reason for State Change: S3 Bucket {alarm_type} failure diff --git a/backend/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py b/backend/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py index ae1c02ca4..d090dd6f8 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py +++ b/backend/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py @@ -510,7 +510,7 @@ def handle_share_failure(self, error: Exception) -> None: self.target_folder, self.share, self.target_environment ) - def handle_revoke_failure(self, error: Exception) -> None: + def handle_revoke_failure(self, error: Exception) -> bool: """ Handles share failure by raising an alarm to alarmsTopic Returns @@ -526,6 +526,7 @@ def handle_revoke_failure(self, error: Exception) -> None: DatasetAlarmService().trigger_revoke_folder_sharing_failure_alarm( self.target_folder, self.share, self.target_environment ) + return True @staticmethod def generate_default_kms_decrypt_policy_statement(target_requester_arn): diff --git a/backend/dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py b/backend/dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py index 375c1b94c..924dcd80c 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py +++ b/backend/dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py @@ -444,7 +444,7 @@ def handle_revoke_failure(self, error: Exception) -> bool: f'with target account {self.target_environment.AwsAccountId}/{self.target_environment.region} ' f'due to: {error}' ) - DatasetAlarmService().trigger_revoke_folder_sharing_failure_alarm( + DatasetAlarmService().trigger_revoke_s3_bucket_sharing_failure_alarm( self.target_bucket, self.share, self.target_environment ) return True diff --git a/backend/dataall/modules/dataset_sharing/services/share_processors/lf_process_cross_account_share.py b/backend/dataall/modules/dataset_sharing/services/share_processors/lf_process_cross_account_share.py index 51ba97cc7..950396b9d 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_processors/lf_process_cross_account_share.py +++ b/backend/dataall/modules/dataset_sharing/services/share_processors/lf_process_cross_account_share.py @@ -111,11 +111,14 @@ def process_approved_shares(self) -> bool: shared_item_SM.update_state_single_item(self.session, share_item, new_state) except Exception as e: - self.handle_share_failure(table=table, share_item=share_item, error=e) + # must run first to ensure state transitions to failed new_state = shared_item_SM.run_transition(ShareItemActions.Failure.value) shared_item_SM.update_state_single_item(self.session, share_item, new_state) success = False + # statements which can throw exceptions but are not critical + self.handle_share_failure(table=table, share_item=share_item, error=e) + return success def process_revoked_shares(self) -> bool: @@ -178,9 +181,12 @@ def process_revoked_shares(self) -> bool: revoked_item_SM.update_state_single_item(self.session, share_item, new_state) except Exception as e: - self.handle_revoke_failure(share_item=share_item, table=table, error=e) + # must run first to ensure state transitions to failed new_state = revoked_item_SM.run_transition(ShareItemActions.Failure.value) revoked_item_SM.update_state_single_item(self.session, share_item, new_state) success = False + # statements which can throw exceptions but are not critical + self.handle_revoke_failure(share_item=share_item, table=table, error=e) + return success diff --git a/backend/dataall/modules/dataset_sharing/services/share_processors/lf_process_same_account_share.py b/backend/dataall/modules/dataset_sharing/services/share_processors/lf_process_same_account_share.py index 54df2d900..ac23d5349 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_processors/lf_process_same_account_share.py +++ b/backend/dataall/modules/dataset_sharing/services/share_processors/lf_process_same_account_share.py @@ -95,11 +95,14 @@ def process_approved_shares(self) -> bool: shared_item_SM.update_state_single_item(self.session, share_item, new_state) except Exception as e: - self.handle_share_failure(table, share_item, e) + # must run first to ensure state transitions to failed new_state = shared_item_SM.run_transition(ShareItemActions.Failure.value) shared_item_SM.update_state_single_item(self.session, share_item, new_state) success = False + # statements which can throw exceptions but are not critical + self.handle_share_failure(table, share_item, e) + return success def process_revoked_shares(self) -> bool: @@ -151,9 +154,12 @@ def process_revoked_shares(self) -> bool: revoked_item_SM.update_state_single_item(self.session, share_item, new_state) except Exception as e: - self.handle_revoke_failure(share_item, table, e) + # must run first to ensure state transitions to failed new_state = revoked_item_SM.run_transition(ShareItemActions.Failure.value) revoked_item_SM.update_state_single_item(self.session, share_item, new_state) success = False + # statements which can throw exceptions but are not critical + self.handle_revoke_failure(share_item, table, e) + return success diff --git a/backend/dataall/modules/dataset_sharing/services/share_processors/s3_access_point_process_share.py b/backend/dataall/modules/dataset_sharing/services/share_processors/s3_access_point_process_share.py index da02c5a3c..620c0217e 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_processors/s3_access_point_process_share.py +++ b/backend/dataall/modules/dataset_sharing/services/share_processors/s3_access_point_process_share.py @@ -97,11 +97,14 @@ def process_approved_shares( shared_item_SM.update_state_single_item(session, sharing_item, new_state) except Exception as e: - sharing_folder.handle_share_failure(e) + # must run first to ensure state transitions to failed new_state = shared_item_SM.run_transition(ShareItemActions.Failure.value) shared_item_SM.update_state_single_item(session, sharing_item, new_state) success = False + # statements which can throw exceptions but are not critical + sharing_folder.handle_share_failure(e) + return success @classmethod @@ -160,11 +163,14 @@ def process_revoked_shares( revoked_item_SM.update_state_single_item(session, removing_item, new_state) except Exception as e: - removing_folder.handle_revoke_failure(e) + # must run first to ensure state transitions to failed new_state = revoked_item_SM.run_transition(ShareItemActions.Failure.value) revoked_item_SM.update_state_single_item(session, removing_item, new_state) success = False + # statements which can throw exceptions but are not critical + removing_folder.handle_revoke_failure(e) + return success @classmethod diff --git a/backend/dataall/modules/dataset_sharing/services/share_processors/s3_bucket_process_share.py b/backend/dataall/modules/dataset_sharing/services/share_processors/s3_bucket_process_share.py index 5a43f2b5c..08ee9532b 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_processors/s3_bucket_process_share.py +++ b/backend/dataall/modules/dataset_sharing/services/share_processors/s3_bucket_process_share.py @@ -91,10 +91,14 @@ def process_approved_shares( shared_item_SM.update_state_single_item(session, sharing_item, new_state) except Exception as e: - sharing_bucket.handle_share_failure(e) + # must run first to ensure state transitions to failed new_state = shared_item_SM.run_transition(ShareItemActions.Failure.value) shared_item_SM.update_state_single_item(session, sharing_item, new_state) success = False + + # statements which can throw exceptions but are not critical + sharing_bucket.handle_share_failure(e) + return success @classmethod @@ -161,9 +165,12 @@ def process_revoked_shares( revoked_item_SM.update_state_single_item(session, removing_item, new_state) except Exception as e: - removing_bucket.handle_revoke_failure(e) + # must run first to ensure state transitions to failed new_state = revoked_item_SM.run_transition(ShareItemActions.Failure.value) revoked_item_SM.update_state_single_item(session, removing_item, new_state) success = False + # statements which can throw exceptions but are not critical + removing_bucket.handle_revoke_failure(e) + return success diff --git a/tests/modules/datasets/tasks/test_s3_bucket_share_manager.py b/tests/modules/datasets/tasks/test_s3_bucket_share_manager.py index 01e4cf691..1b1b02678 100644 --- a/tests/modules/datasets/tasks/test_s3_bucket_share_manager.py +++ b/tests/modules/datasets/tasks/test_s3_bucket_share_manager.py @@ -242,7 +242,7 @@ def test_grant_role_bucket_policy_with_no_policy_present( # No Bucket policy. A Default bucket policy should be formed with DataAll-Bucket-ReadOnly, AllowAllToAdmin & RequiredSecureTransport Sids s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = None - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) mocker.patch( "dataall.base.aws.sts.SessionHelper.get_delegation_role_arn", @@ -310,7 +310,7 @@ def test_grant_role_bucket_policy_with_default_complete_policy( s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = json.dumps(bucket_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -356,7 +356,7 @@ def test_grant_role_bucket_policy_with_policy_and_no_allow_owner_sid_and_no_read s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = json.dumps(bucket_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) mocker.patch( "dataall.base.aws.sts.SessionHelper.get_delegation_role_arn", @@ -432,7 +432,7 @@ def test_grant_role_bucket_policy_with_another_read_only_role( s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = json.dumps(bucket_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) mocker.patch( "dataall.base.aws.sts.SessionHelper.get_delegation_role_arn", @@ -690,7 +690,7 @@ def test_grant_dataset_bucket_key_policy_with_complete_policy_present( existing_key_policy = base_kms_key_policy() kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -727,7 +727,7 @@ def test_grant_dataset_bucket_key_policy_with_target_requester_id_absent( existing_key_policy = base_kms_key_policy("OtherTargetRequestorArn") kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) # Mock the S3BucketShareManager with the KMS client with db.scoped_session() as session: @@ -784,7 +784,7 @@ def test_grant_dataset_bucket_key_policy_and_default_bucket_key_policy( kms_client().get_key_id.return_value = "kms-key" kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share3.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share3.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -835,7 +835,7 @@ def test_grant_dataset_bucket_key_policy_with_imported( kms_client().get_key_id.return_value = "kms-key" kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share3.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share3.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -882,7 +882,7 @@ def test_delete_target_role_bucket_policy_with_no_read_only_sid( s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = json.dumps(bucket_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -942,7 +942,7 @@ def test_delete_target_role_bucket_policy_with_multiple_principals_in_policy( s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = json.dumps(bucket_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -1016,7 +1016,7 @@ def test_delete_target_role_bucket_policy_with_one_principal_in_policy( s3_client = mock_s3_client(mocker) s3_client().get_bucket_policy.return_value = json.dumps(bucket_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -1305,7 +1305,7 @@ def test_delete_target_role_bucket_key_policy_with_no_target_requester_id( kms_client().get_key_id.return_value = "kms-key" kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -1346,7 +1346,7 @@ def test_delete_target_role_bucket_key_policy_with_target_requester_id( kms_client().get_key_id.return_value = "kms-key" kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -1393,7 +1393,7 @@ def test_delete_target_role_bucket_key_policy_with_target_requester_id_and_impor kms_client().get_key_id.return_value = "kms-key" kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share3.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share3.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -1440,7 +1440,7 @@ def test_delete_target_role_bucket_key_policy_with_target_requester_id_and_impor kms_client().get_key_id.return_value = "kms-key" kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share3.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share3.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager( @@ -1504,7 +1504,7 @@ def test_delete_target_role_bucket_key_policy_with_multiple_principals_in_policy kms_client().get_key_id.return_value = "kms-key" kms_client().get_key_policy.return_value = json.dumps(existing_key_policy) - iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) + iam_client = mock_iam_client(mocker, target_environment.AwsAccountId, share2.principalIAMRoleName) with db.scoped_session() as session: manager = S3BucketShareManager(