Generic shares_base module and specific s3_datasets_shares module - part 3 (share processor and manager interfaces) #1298

dlpzx · 2024-05-23T09:37:15Z

Feature or Bugfix

Feature
Refactoring

Detail

As explained in the design for #1123 and #1283 we are trying to implement generic datasets_base and shares_base modules that can be used by any type of datasets and by any type of shareable object in a generic way.

In this PR:

Move ECS handler for sharing tasks to shares_base
Move sharing tasks to shares_base
Move DataSharingService to shares_base and rename it as SharingService. Make SharingService generic and remove any reference to specific items. In this process:
- attach and delete table and folder permissions are moved into the specific share processor. delete permissions are processed item by item and moved into the shareItemService
- Some methods are copied from ShareObjectRepository in s3_datasets_shares to shares_base. They have not been removed from s3_datasets_shares, instead there is a TODO marking those methods that have been copied. The migration and clean-up of shareObjectRepository will be done in a following PR
- Need to introduce DatasetBaseRepository.get_dataset_by_uri(session, share.datasetUri) to avoid future circular dependencies: shares_base depends only on datasets_base.
- Clean-up and consolidate methods: remove updates of the share items outside of state machine transactions. Only re-used methods and dataset lock handling in share_manager --> TODO: dataset_lock manager should be its own service outside of sharing, but this is out of the scope of this PR.
- Add updates of share_item statuses in except clauses for more robust sharing
Introduce ShareProcessor and ShareManager interfaces and use them in the SharingService: instead of the processor inheriting the manager class, the processor uses the ShareProcessor interface and constructs a manager when needed.
Introduce new load ImportMode SHARES_TASK and register ShareProcessors in s3_datasets_share (backend/dataall/modules/s3_datasets_shares/__init__.py)

See full detail of SharingService design in #1283

Next steps/Open questions

For failures I think we should rollback whatever actions where performed. For example, if we are sharing a table and it failed in one step, it should revert all the steps executed before. @petrkalos @SofiaSazonova @noah-paige what do you think?

Relates

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.

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

# Conflicts: # backend/dataall/modules/s3_datasets_shares/services/share_object_service.py

dlpzx · 2024-05-28T15:02:00Z

Testing locally:

Approve shares

First we will test that sharing works. After approve the share task is correctly handled. And items are processed as usual. We will test success cases. For folders and tables, we also need to verify UI permissions (GET_TABLE, GET_FOLDER) are attached

S3 bucket share
S3 access points share (1 folder) - everything completed, DA permissions attached
S3 access points share (multiple folders)
Glue table share (1 table)
Glue table share (multiple tables)
S3 bucket share + S3 access points share + Glue table all together

Then unsuccessful cases:

Approve S3 bucket share + S3 access points share + Glue table all together - we delete the principal IAM role very fast after approving the share and then when we share the items end up in Share_Failed

Mix of success/failures:

For a malformed KMS policy folders and bucket sharing failed and tables succeeded. Items do not end up in limbo states and do not make other types of sharing fail.

Then cases with the dataset_lock:

lock the dataset in the database and process a new share request. Let it wait until it times out and verify that all items end in Share_Failed

Revoke shares

After revoke the share task is correctly handled. And items are processed as usual. We will test success cases. For folders and tables, we also need to verify UI permissions (GET_TABLE, GET_FOLDER) are removed

S3 bucket share
S3 access points share (1 folder) - permissions deleted
S3 access points share (multiple folders)
Glue table share (1 table)
Glue table share (multiple tables)
S3 bucket share + S3 access points share + Glue table all together

Then unsuccessful cases:

Revoke S3 bucket share + S3 access points share + Glue table all together - we delete the principal IAM role very fast after revoking the share and then when we share the items end up in Revoke_Failed --->There is an error but is unrelated to this PR: it is impossible to delete the role fast enough, so before deletion it already goes into the processors/manager that are not ensuring that the role is needed for revoke for access points share

Then cases with the dataset_lock:

lock the dataset in the database and process a new share request. Let it wait until it timesout and verify that all items end in Revoke_Failed

Verify shares

Verify after sharing - all good, datetime updated
Delete principal IAM role and verify - all items updated as unhealthy
Introduce error in table share and verify all items - table as unhealthy, other items healthy
On top of the above Introduce error in bucket share and verify - table and bucket as unhealthy
Introduce error in folder share and verify folder only - folder as unhealthy

Re-apply shares

from the previous hit reapply - all 3 types of items are re-shared

Testing in AWS

Needed as we are changing the ECS tasks and the handlers. We just need tot est that the handler triggers the ECS tasks and that the processors are loaded:

CICD completes and ECS tasks still exist
Request share (verify, approve or revoke) ----> all good! Checked the logs, all the different shareItems are verified

Found issues:

the clean-up of malformed KMS policies has some issues. I deleted an IAM role and re-created it the policy then cannot go out of the malformed state
principal IAM role checks inside the revoke managers/processors (explained above)

dlpzx · 2024-05-29T16:03:58Z

backend/dataall/modules/s3_datasets_shares/services/share_object_service.py

@@ -537,73 +538,6 @@ def _validate_group_membership(session, share_object_group, environment_uri):
                message=f'Team: {share_object_group} is not a member of the environment {environment_uri}',
            )

-    @staticmethod


This is moved to the share_item_service because now it applies to each item, not to the whole share

dlpzx · 2024-05-29T16:05:50Z

...d/dataall/modules/s3_datasets_shares/services/share_processors/glue_table_share_processor.py

I tried having github rename and edit the file backend/dataall/modules/s3_datasets_shares/services/share_processors/lakeformation_process_share.py but it is ignoring it. The same for the other processors. The inside logic has barely being touched, just the construction

dlpzx · 2024-05-29T16:08:01Z

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

+            source_env_group,
+            env_group,
+        )
+


get_share_data_items_by_type is new, it replaces get_share_data_items called in the data_sharing_service

(shared_tables, shared_folders, shared_buckets) = ShareObjectRepository.get_share_data_items( session, share_uri, ShareItemStatus.Share_Approved.value )

dlpzx · 2024-05-29T16:08:50Z

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

+    def list_all_active_share_objects(session) -> [ShareObject]:
+        return session.query(ShareObject).filter(ShareObject.deleted.is_(None)).all()
+
+    @staticmethod


This one has suffered some modifications. We can now send a message

dlpzx · 2024-05-29T16:09:38Z

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

@@ -241,13 +238,6 @@ def __init__(self, state):
                    ]
                },
            ),


This transition is unnecessary if we make a failure more flexible

dlpzx · 2024-05-29T16:10:11Z

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

@@ -4,7 +4,6 @@
 class ShareableType(GraphQLEnumMapper):
    Table = 'DatasetTable'
    StorageLocation = 'DatasetStorageLocation'
-    View = 'View'


remove unused. I know it could have waited for another PR, but it was very very small

dlpzx · 2024-05-29T16:11:12Z

tests/modules/datasets/test_share.py

@@ -1498,21 +1498,6 @@ def test_verify_items_share_request(db, client, user2, group2, share3_processed,
    assert status == ShareItemHealthStatus.PendingVerify.value




Removed because this function does not exist anymore, but we will introduce more tests once the refactoring is done

Implement ShareProcessor and manager interfaces

cf2c7e9

petrkalos reviewed May 23, 2024

View reviewed changes

backend/dataall/modules/shares_base/services/sharing_service.py Outdated Show resolved Hide resolved

petrkalos reviewed May 23, 2024

View reviewed changes

backend/dataall/modules/shares_base/services/sharing_service.py Show resolved Hide resolved

dlpzx mentioned this pull request May 23, 2024

Create generic shares_base and s3_datasets_shares modules from current dataset_sharing #1283

Closed

dlpzx added 3 commits May 23, 2024 17:03

Move ECS handler and tasks into shares_base

8e98624

Finish basic layout of share processors and registry

55bc4c0

Simplify design - initialize managers in processors

5be0eba

dlpzx force-pushed the feat/generic-dataset-sharing-3 branch from 2c78c13 to d92479d Compare May 28, 2024 08:56

Cleaning up PR

5b7c1d0

dlpzx force-pushed the feat/generic-dataset-sharing-3 branch from d92479d to 5b7c1d0 Compare May 28, 2024 11:55

Merge branch 'refs/heads/main' into feat/generic-dataset-sharing-3

d176047

# Conflicts: # backend/dataall/modules/s3_datasets_shares/services/share_object_service.py

dlpzx added 7 commits May 29, 2024 09:14

Linting

3a69f4d

simplifying sharing_service

8ff96e6

Fix permissions granting sequence

23cfbf3

Fixing small issues

b2aaa05

Better error handling of share items with multiple share processors

4498770

Fix issues with sqlalchemy session in update

6e6f44c

Revert rename share_object_state_machines.py

ef08ef1

dlpzx commented May 29, 2024

View reviewed changes

dlpzx requested a review from petrkalos May 29, 2024 16:11

dlpzx marked this pull request as ready for review May 29, 2024 16:11

Re-organize loading of modules

a7940f1

dlpzx force-pushed the feat/generic-dataset-sharing-3 branch from d2ffa77 to a7940f1 Compare June 4, 2024 07:35

Re-organize loading of modules

6ddc74f

petrkalos approved these changes Jun 4, 2024

View reviewed changes

dlpzx merged commit e20e750 into main Jun 4, 2024
10 checks passed

dlpzx deleted the feat/generic-dataset-sharing-3 branch June 6, 2024 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic shares_base module and specific s3_datasets_shares module - part 3 (share processor and manager interfaces) #1298

Generic shares_base module and specific s3_datasets_shares module - part 3 (share processor and manager interfaces) #1298

dlpzx commented May 23, 2024 •

edited

Loading

dlpzx commented May 28, 2024 •

edited

Loading

dlpzx May 29, 2024

dlpzx May 29, 2024

dlpzx May 29, 2024

dlpzx May 29, 2024

dlpzx May 29, 2024

dlpzx May 29, 2024

dlpzx May 29, 2024

@@ @@ -241,13 +238,6 @@ def __init__(self, state): @@
                                   ]
                               },
                           ),

		@@ -1498,21 +1498,6 @@ def test_verify_items_share_request(db, client, user2, group2, share3_processed,
		assert status == ShareItemHealthStatus.PendingVerify.value

Generic shares_base module and specific s3_datasets_shares module - part 3 (share processor and manager interfaces) #1298

Generic shares_base module and specific s3_datasets_shares module - part 3 (share processor and manager interfaces) #1298

Conversation

dlpzx commented May 23, 2024 • edited Loading

Feature or Bugfix

Detail

Next steps/Open questions

Relates

Security

dlpzx commented May 28, 2024 • edited Loading

Testing locally:

Approve shares

Revoke shares

Verify shares

Re-apply shares

Testing in AWS

Found issues:

dlpzx May 29, 2024

Choose a reason for hiding this comment

dlpzx May 29, 2024

Choose a reason for hiding this comment

dlpzx May 29, 2024

Choose a reason for hiding this comment

dlpzx May 29, 2024

Choose a reason for hiding this comment

dlpzx May 29, 2024

Choose a reason for hiding this comment

dlpzx May 29, 2024

Choose a reason for hiding this comment

dlpzx May 29, 2024

Choose a reason for hiding this comment

dlpzx commented May 23, 2024 •

edited

Loading

dlpzx commented May 28, 2024 •

edited

Loading