Use New Fideslang DSR-updated models [#1761] #2009

pattisdr · 2022-12-08T22:05:17Z

❗ Blocked by https://github.com/ethyca/fideslang/pull/95
❗ Contains data migration

Closes #1761

Code Changes

Installed Requirements

Temporarily point fideslang to the unmerged branch here https://github.com/ethyca/fideslang/pull/95 while that PR remains unmerged. Revert before merge.
Temporarily install git in the Dockerfiles prior to installing requirements. Revert before merge.

Dataset yaml files

Updated redis_dataset.yml (which is a dataset describing our redis cache) to not have data_categories at the object level when there are data_categories at the nested field level. This is a constraint introduced from the fidesops-side to keep there from being data category conflicts in nested data structures when we're filtering data on data category before building the DSR package
Updated the dataset yamls (for Database datasets, not saas datasets to have fides_meta.) Fideslang supports both in the request, but any fidesops_meta supplied is converted to fides_meta. We have both in our yaml files right now.
Update the db_dataset.yml which maps our fides database to have a fides_meta instead of a fidesctl_meta field on the ctl_datasets table

Replacing Fidesops Pydantic schemas with new Fideslang ones

Use fideslang FidesKey in favor of FidesOpsKey - FidesKey now allows '>' '<' characters (useful for saas templates) and also bad validation throws a ValueError which is more easily picked up by FastAPI and turned into a ValidationError.
Remove FidesopsDataset in favor of the new Fideslang Dataset which has the important attributes from FidesopsDataset
Rename Dataset to be GraphDataset to not get confused with the Fideslang Dataset.
At the code level - adjust all fidesops_meta variables (at the Dataset, DatasetCollection, and DatasetField levels) to use fides_meta instead. However, leave fidesops_meta keys in the saas-related yaml files for now. If fidesops_meta is supplied, Fideslang converts it to fides_meta for backwards compatibility.
Remove FidesopsDatasetReference in favor of identical Fideslang FidesDatasetReference

Validation

Add new data category validation for the PATCH /datasetconfig endpoints where we validate against data_categories in the database instead of just the static default taxonomy as users could have added other data categories. When running a DSR, the dataset_config.dataset (which is a Fideslang Dataset) is converted into a GraphDataset, and all the GraphDatasets are combined to make a DatasetGraph
Because FidesKey validation errors now throw a FidesValidationError that inherits from ValueError instead of exception, adjust a couple of locations -ctl side where model validation is now throwing a ValidationError.

Database migration

Update the ctl_datasets.fidesctl_meta to be named fides_meta

Steps to Confirm

Same steps in https://github.com/ethyca/fideslang/pull/95. General QA around CTL Datasets and Ops Connections is handy. Loading ops datasets into the db with the old fidesops_meta key and verifying that it is retrieved and serialized with fides_meta is a good check. Main thing, both Ops DatasetConfig.datasets and Ctl ctl_datasets are being serialized with the same model now.

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation Updated:
- documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
- documentation issue created (tag docs-team to complete issue separately)
Issue Requirements are Met
Relevant Follow-Up Issues Created: Ops-side Tests Clear DB Tables: Removes Resources Loaded by Default #2016, https://github.com/ethyca/fideslang/issues/96, Ops-side: Validating Data Categories against DB #2017
Update CHANGELOG.md

Description Of Changes

Update Fides to use updated Fideslang modes (mostly changes on the -ops side) now that Fideslang has been updated so it's base models can support ops code.

We used to have two repositories: fidesctl and fidesops that were both using Fideslang Dataset concepts. Fidesops added more items to subclassed Datasets to support DSRs. We're now moving some of these fields to Fideslang, so we can share the same schemas between both -ctl and -ops usages.

Note to self: The current issue with server startup is related to `fides_meta` being missing from dataset yaml files. The validation error is eaten and isn't surfaced.

# Conflicts: # src/fides/api/main.py

- Replace instances of "fidesops_meta" with fides_meta in the code. The fideslang Dataset validation will take care of converting fidesops_meta to fides_meta. Don't update the yaml files however. - Remove FidesopsDataset in favor of Fideslang Dataset (DSR concepts have been moved to this model) - Remove FidesopsDatasetCollection in favor of Fideslang DatasetCollection (DSR concepts have been moved to this model) - Remove FidesopsDatasetField in favor of Fideslang DatasetField (DSR concepts have been moved to this model) - Remove FidesopsDatasetReference in favor of Fideslang FidesDatasetReference - Remove Fidesops FidesCollectionKey in favor of Fideslang FidesCollectionKey - Adjust Dataset data category validation to validate against data categories saved in the database instead of just the static default taxonomy.

…atasets prevents data categories from being set at object level and nested field level.

…rove that both fidesops_meta and fides_meta fields are currently supported.

…updated to have the important validation from FidesOpsKey.

…set.

# Conflicts: # requirements.txt # src/fides/api/ops/api/v1/endpoints/connection_endpoints.py

…n of fideslang.

…ops_meta.

Fix an instance where Fides Dataset resource was being used instead of Fideslang Dataset. That's why I renamed!

pattisdr · 2022-12-09T01:16:57Z

tests/ops/api/v1/endpoints/test_dataset_endpoints.py

@@ -192,7 +192,7 @@ def test_put_validate_dataset_invalid_length(
        assert response.status_code == 422
        assert (
            json.loads(response.text)["detail"][0]["msg"]
-            == "Illegal length (-1). Only positive non-zero values are allowed."
+            == "ensure this value is greater than 0"


Validation of field length was moved to Fideslang. It was simultaneously updated to use a built-in Pydantic validator instead of our own custom validation to enforce positive non-zero models, so the error message has changed.

pattisdr · 2022-12-09T01:18:21Z

tests/ops/api/v1/endpoints/test_dataset_endpoints.py

@@ -288,7 +288,7 @@ def test_put_validate_dataset_invalid_category(
        )
        assert response.status_code == 422
        details = json.loads(response.text)["detail"]
-        assert ["body", "collections", 0, "fields", 0, "data_categories"] in [
+        assert ["collections", 0, "fields", 0, "data_categories"] in [


Updated validation was added to check data_categories against those in the database, not those in the static default data categories list. To do this, I couldn't use a regular pydantic validator, but needed to do this after the fact, when I could access a database session. So the response has adjusted as the request body itself isn't being validated here.

pattisdr · 2022-12-09T01:31:11Z

My last failing tests are in tests/ops/service/saas_request/test_saas_request_override_factory.py::TestSaasRequestOverrideFactory.

It's odd because that file passes if you run it directly by itself, but if you run nox -s "pytest_ops(unit)" it fails.

EDIT:
This seemed to just be related to the equality comparison. To address, I'm going to switch the equality comparison to be "==" instead of "is" which is the safer way to check this - it checks to be sure the values are equal, rather than where they are stored in memory.

Main branch

(Pdb) signature(f).return_annotation
typing.List[typing.Dict[str, typing.Any]]
(Pdb) signature(f).return_annotation is List[Row]
True
(Pdb) signature(f).return_annotation == List[Row]
True

This branch:

(Pdb) signature(override_function).return_annotation
typing.List[typing.Dict[str, typing.Any]]
(Pdb) signature(override_function).return_annotation is List[Row]
False
(Pdb) signature(override_function).return_annotation == List[Row]
True

…er they are stored at the same address in memory - this is error prone. - Revert change from bad merge.

.fides/db_dataset.yml

pattisdr · 2022-12-09T19:55:09Z

.fides/redis_dataset.yml

-            data_categories:
-            - system.operations
            data_qualifier: aggregated.anonymized.unlinked_pseudonymized.pseudonymized.identified
            fidesops_meta:
              data_type: object # Stores an encrypted representation of the fidesops graph that executes the privacy requests.
            fields:
              - name: <dataset_name>:<collection_name>  # The current collection
-                data_categories:
-                - system.operations


This dataset file reflects what we store in Redis. Now that we're sharing fideslang models between ops and ctl there's validation that doesn't allow data categories at the object field level - they should be on the nested fields instead.

pattisdr · 2022-12-09T19:55:31Z

Dockerfile

@@ -26,6 +26,7 @@ RUN apt-get update && \
    g++ \
    gnupg \
    gcc \
+    git \


This is temporary, will remove when fideslang has been updated.

pattisdr · 2022-12-09T19:56:48Z

data/dataset/bigquery_example_test_dataset.yml

@@ -11,7 +11,7 @@ dataset:
            data_categories: [user.contact.address.street]
          - name: id
            data_categories: [system.operations]
-            fidesops_meta:
+            fides_meta:


I adjusted the fidesops_meta fields in the database datasets, but not the saas datasets to demonstrate both still work. Fideslang dataset schemas can take in both fides_meta and fidesops_meta but they are converted to fides_meta. We also don't want to break backwards compatibility with existing saas datasets.

Good idea. Should/can the saas templates be converted over to use fides_meta in a future PR down the line in the unified fides resources work?

From this comment, it sounded like we wanted to preserve the backwards compatibility of this for now https://github.com/ethyca/fideslang/issues/94#issuecomment-1341175140

pattisdr · 2022-12-09T19:57:48Z

docs/fides/Dockerfile

@@ -5,6 +5,7 @@ RUN apt-get update && \
    g++ \
    gnupg \
    gcc \
+    git \


Temporary. Will remove when fideslang version is bumped.

pattisdr · 2022-12-09T20:02:18Z

src/fides/api/ops/schemas/dataset.py

+    If no data categories in the database, default to using data categories from the default taxonomy.
+    """
+    defined_data_categories: List[FidesKey] = [
+        cat[0] for cat in db.query(DataCategory.fides_key).all()
+    ]
+    if not defined_data_categories:
+        defined_data_categories = list(DefaultTaxonomyDataCategories.__members__.keys())


Adding new validation on data category that looks in the database to see if data categories match ones defined there, instead of matching against the default data categories.

As a backup, if there's nothing in the db, which shouldn't be the case, matching against the static list. Related issue added here: #2016

Would it make sense to add an INFO log saying what DataCategory didn't have any categories in the database? If it's a situation that shouldn't happen it might be worth making a note of it.

pattisdr · 2022-12-09T20:02:47Z

src/fides/api/ops/schemas/dataset.py

-
-class FidesopsMeta(BaseModel):
-    """Fidesops-specific annotations used for query traversal"""
-
-    references: Optional[List[FidesopsDatasetReference]]
-    identity: Optional[str]
-    primary_key: Optional[bool]
-    data_type: Optional[str]
-    """Optionally specify the data type. Fidesops will attempt to cast values to this type when querying."""
-    length: Optional[int]
-    """Optionally specify the allowable field length. Fidesops will not generate values that exceed this size."""
-    return_all_elements: Optional[bool]
-    """Optionally specify to query for the entire array if the array is an entrypoint into the node. Default is False."""
-    read_only: Optional[bool]
-    """Optionally specify if a field is read-only, meaning it can't be updated or deleted."""
-
-    @validator("data_type")
-    def valid_data_type(cls, v: Optional[str]) -> Optional[str]:
-        """Validate that all annotated data categories exist in the taxonomy"""
-        return _valid_data_type(v)
-
-    @validator("length")
-    def valid_length(cls, v: Optional[int]) -> Optional[int]:
-        """Validate that the provided length is valid"""
-        return _valid_data_length(v)
-


Most of these removed concepts have moved to Fideslang.

pattisdr · 2022-12-09T20:04:50Z

src/fides/api/ops/schemas/shared_schemas.py

-class FidesOpsKey(FidesKey):
-    """
-    Overrides fideslang FidesKey validation to throw ValueError
-    """
-
-    @classmethod
-    def validate(cls, value: Optional[str]) -> Optional[str]:
-        """Throws ValueError if val is not a valid FidesKey"""
-        if value == "<instance_fides_key>":
-            # Ignore <instance_fides_key> in saas templates.  This value will be replaced with a
-            # user-specified value.
-            return value
-
-        if value is not None and not cls.regex.match(value):
-            raise ValueError(
-                "FidesKey must only contain alphanumeric characters, '.', '_' or '-'."
-            )
-
-        return value


Getting rid of FidesOpsKey in favor of an updated FidesKey.

I've updated Fides Key to allow ">" "<" characters pending approval. Also the error that FidesKey throws has been updated to be a subclass of a ValueError so can be converted into a pydantic ValidationError.

The only behavior I didn't move to FidesKey was allowing the value argument to be Optional in FidesKey.value. I think on our end we can just use Optional[FidesKey] for this.

pattisdr · 2022-12-09T20:05:46Z

src/fides/api/ops/service/saas_request/saas_request_override_factory.py

@@ -133,7 +133,7 @@ def validate_read_override_function(f: Callable) -> None:
    and that it declares at least 5 parameters.
    """
    sig: Signature = signature(f)
-    if sig.return_annotation is not List[Row]:
+    if sig.return_annotation != List[Row]:


This was causing some failing tests on this branch. I think a '!=' comparison is recommended here anyway. We just want to see if the values are equal, not if they are stored in the same place in memory.

pattisdr · 2022-12-09T20:09:16Z

src/fides/api/ops/graph/config.py

-class Dataset(BaseModel):
+class GraphDataset(BaseModel):


Renamed to not confuse with Fideslang Dataset, which we're using now in "ops" in favor of "FidesopsDataset"

To run a DSR, a Fideslang Dataset (stored in datasetconfig.dataset) is converted into a GraphDataset. Many GraphDatasets make up a DatasetGraph. The DatasetGraph is what we pass to run_access_request or run_erasure_request

…Fides Dataset to be GraphDataset.

pattisdr · 2022-12-09T21:14:34Z

@ethyca/docs-authors This work is changing fidesops_meta across the codebase to be fides_meta instead, although fidesops_meta is still supported for backwards compatibility. I know this attribute is scattered throughout the docs -

src/fides/api/main.py

src/fides/api/ops/service/connectors/saas/connector_registry_service.py

src/fides/api/ops/util/saas_util.py

tests/ops/task/traversal_data.py

TheAndrewJackson · 2022-12-13T02:13:18Z

Does it make sense to merge this into a feature branch instead of directly into main? This epic won't be finished until a few more PRs are merge in. Another fides release may happen before that point and release some of the Unified Resources epic early.

pattisdr · 2022-12-13T19:10:58Z

Hi @TheAndrewJackson thanks for the feature branch suggestion. I'd tried to make this PR standalone but we've discussed some other breaking changes in subsequent PR's. So I think a feature branch makes sense and we can really check to make sure it's been solidly QA'd on the FE and BE before merge.

… GraphDataset rename. - Revert try/except. - Temporarily point fideslang to main branch now that related work has been merged but not yet released.

pattisdr · 2022-12-14T20:21:53Z

Thanks for your review @TheAndrewJackson! Merging into the feature branch -

TheAndrewJackson and others added 11 commits November 22, 2022 09:27

Initial pass at converting project over to new fideslang

5f1327a

Note to self: The current issue with server startup is related to `fides_meta` being missing from dataset yaml files. The validation error is eaten and isn't surfaced.

Merge branch 'main' into ajackson_1761_use_new_fideslang_models

1677721

# Conflicts: # src/fides/api/main.py

Update check_fides_key to catch a ValidationError instead.

ab8a9d5

Remove data_categories at object level. New validation on FidesLang D…

a604e6a

…atasets prevents data categories from being set at object level and nested field level.

Migrate ctl_datasets table to rename fidesctl_meta field to fides_meta.

8117cd7

Rename fidesops_meta to fides_meta in datasets only (not saas) - to p…

39dd611

…rove that both fidesops_meta and fides_meta fields are currently supported.

Get rid of FidesOpsKey in favor of fideslang FidesKey which has been …

182b947

…updated to have the important validation from FidesOpsKey.

Rename Dataset to GraphDataset to avoid confusion with Fideslang Data…

d842448

…set.

Merge branch 'main' into fides_1761_use_new_fideslang_models

a4089ed

# Conflicts: # requirements.txt # src/fides/api/ops/api/v1/endpoints/connection_endpoints.py

Running static checks after merge.

b3d85b4

pattisdr changed the title ~~Use New Fideslang DSR-updated models [#1761]~~ [WIP] Use New Fideslang DSR-updated models [#1761] Dec 8, 2022

pattisdr added 5 commits December 8, 2022 16:43

Update db_dataset to reflect Dataset fidesctl_meta -> fides_meta rename.

543ccf9

Temporarily install git in docs Dockerfile to install a pinned versio…

099be49

…n of fideslang.

Update the other database dataset to have fides_meta instead of fides…

7b06e37

…ops_meta.

Fix FidesKey error messages in tests.

992c1aa

Fix an instance where Fides Dataset resource was being used instead of Fideslang Dataset. That's why I renamed!

Adjust test now that dataset has been updated to have fides_meta.

9655ef5

pattisdr commented Dec 9, 2022

View reviewed changes

pattisdr added the Unified Fides Resources label Dec 9, 2022

pattisdr self-assigned this Dec 9, 2022

pattisdr added the do not merge Please don't merge yet, bad things will happen if you do label Dec 9, 2022

pattisdr mentioned this pull request Dec 9, 2022

Add DSR Concepts to Fideslang [#94] IABTechLab/fideslang#95

Merged

23 tasks

pattisdr added 3 commits December 9, 2022 12:46

Check if return annotation has the same value as List[Row], not wheth…

28c2192

…er they are stored at the same address in memory - this is error prone. - Revert change from bad merge.

Merge main.

ea83275

Update changelog.

e11b22d

pattisdr commented Dec 9, 2022

View reviewed changes

Revert import rename of Fideslang Dataset now that we've renamed the …

f51f640

…Fides Dataset to be GraphDataset.

pattisdr mentioned this pull request Dec 9, 2022

Ops-side: Validating Data Categories against DB #2017

Closed

pattisdr changed the title ~~[WIP] Use New Fideslang DSR-updated models [#1761]~~ Use New Fideslang DSR-updated models [#1761] Dec 9, 2022

pattisdr marked this pull request as ready for review December 9, 2022 20:40

pattisdr requested a review from a team December 9, 2022 20:40

pattisdr assigned seanpreston, ThomasLaPiana and SteveDMurphy and unassigned seanpreston, ThomasLaPiana and SteveDMurphy Dec 9, 2022

pattisdr requested review from seanpreston, SteveDMurphy, ThomasLaPiana and TheAndrewJackson December 9, 2022 21:05