Generic dataset module and specific s3_datasets module - part 2 (Create datasets_base and move enums)

[dlpzx]

Feature or Bugfix

⚠️ This PR should be merged after #1250.

• Refactoring

Detail

As explained in the design for #1123 we are trying to implement a generic datasets_base module that can be used by any type of datasets in a generic way.

This PR:

• Creates the skeleton of the datasets_base module consisting of 3 packages: db, api, services. And adds the __init__ file.
• Adds the dependency of s3_datasets to datasets_base in the __init__ file of the s3_datasets module
• Moves datasets_enums to datasets_base

Relates

• Create generic dataset_base and s3_dataset modules from current datasets #1123
• Redshift Data Sharing #955

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]

Tested in AWS:

• Loading of modules works as expected (e.g. we can see the Datasets module)
• All FE views show enums as normal (e.g. DatasetRoles, Topics, classification)
• Edit dataset and add topics works as expected
• Edit dataset confidentiality works as expected
• createEnvironment and createDataset

👀 Because how the loader is configured for CDK and CDK_CLI_EXTENSION load modes it is needed to add the datasets_base explicitly in the config.json file. It is something that was going to happen either way so I added it already.

[noah-paige]

I think ideally we would have dataset_base be automatically activated if any of the child dataset modules (i.e. s3_datasets) is active and not have to expose it here on the config.json

Configuring the dataset_base module as active or not does not really mean anything as it is really the child dataset modules that are meaningful to activate or not

I am not sure how easy it is to do such a thing or if it requires a big shift in the loader logic but if an easy fix

[noah-paige]

I saw your comment that it is required here because of how we load ImportModes of CDK and CDK_CLI_EXTENSION - but can you explain why that is / is there an easy way to resolve?

[dlpzx]

First of all, datasets_base will be added in the config.json at some point (check the full design steps in #1123) because some parameters are relevant for all datasets no only for s3_datasets or redshift_datasets (e.g. confidentiality). That is why I did not give it more thought.

But if you are curious, the issue is a bit hidden, the loader has some methods to check that it initialized the modules correctly. With the current implementation I think that a module cannot have a module interface CDK and not have a CDK_CLI_EXTENSION if it is not a config.json module. I think the problem is not that much on the loader but on the way we are using it in the cdk proxy. I will have a closer look

[noah-paige]

In earlier versions such as v2.4.0 I think we never exposed datasets_base as an module in config.json and we had the same type of __init__.py function where it was only supported by CDK and not CDK_CLI_EXTENSION

Trying to understand why we could do that then but can not now - playing around a bit on how these modules are imported using the loader class

Either way I tested these changes and it looks good so will approve PR but do some more digging here on the side

[noah-paige]

I think I found the issue as to why there is an error when we remove datasets_base from config.json:

• When we load_modules(modes={ImportMode.CDK_CLI_EXTENSION})
  • We first import all modules in_config as part of def _load_modules(...)
  • This would theoretically include s3_datasets and dataset_sharing at this point in time
  • When we import dataset_sharing we have the following line in top level of __init__.py that subsequently imports s3_datasets and datasets_base modules

from dataall.modules.dataset_sharing.db.share_object_repositories import ShareEnvironmentResource

• Then when we _check_loading_correct(...) in loader.py
  • it fails on L252 with ImportError since datasets_base is in sys.modules.keys() but is not in checked_modules_names (since datasets_base is not in config / expected_load)

Previously we never exposed datasets_base or datasets_sharing as configurable modules in config.json so we avoided this problem. Moving the line

from dataall.modules.dataset_sharing.db.share_object_repositories import ShareEnvironmentResource

in backend/dataall/modules/dataset_sharing/__init__.py to within the the def __init__(self): in class SharingApiModuleInterface() resolves this issue on loading modules but may need some additional testing to ensure share APIs still working as expected

[noah-paige]

Doing some digging on the loader class and exposing datasets_base in config.json but tested this PR and changes look good

[dlpzx]

@noah-paige I will implement the import change in another PR, probably with other modules that also have global imports in their init files