Support Backend Delete and Disable Collection Requirements [#602] #637

pattisdr · 2022-06-10T22:20:20Z

❗ Contains migration; verify downrev before merging.

Purpose

Support deleted/disabled collection execution requirements. A lot of this behavior was already true, we're just adding the new concept of a "disabled" collection, and then adding integration tests to confirm the behavior below:

New privacy requests: Deleting a collection will prevent it from being inserted into the graph altogether, while disabling a collection will include it in the graph, but skip running any queries, and just return default values downstream.
In progress privacy requests: If a privacy request is in progress, the graph has already been built and we are in the middle of executing individual tasks. If a collection is disabled or deleted at this point, we will still attempt to access that collection because we are using collection details in memory.
Restarting from failure: A privacy request has failed, and a user is attempting a restart that will try to re-process the failed collection and any remaining collections. Deleting a collection before reprocessing will cause it to be omitted from the new graph, and we won't attempt to connect. Disabling a collection will still cause the collection to be a part of graph, but we will skip it, and just return default values to dependent collections.
Completed privacy requests: Disabling or deleting a collection after the privacy request has completed will have no effect. Execution logs on disabled or deleted collections will remain intact.
Denied privacy requests: Disabling or deleting a collection after the privacy request has was denied will have no effect. Execution logs on disabled or deleted collections will remain intact.

Changes

Adds disabled field to ConnectionConfig (default value of False), as well as disabled_at.
- disabled_at is automatically set if disabled is updated.
- This field can be set through the existing PATCH ConnectionConfig endpoint.
Adds a skipped status to ExecutionLog.
Still make disabled collections part of the graph, but skip them if they are disabled.
- This is accomplished by adding a skip_if_disabled check to the retry decorator which wraps access and erasure requests. If a collection's associated ConnectionConfig is disabled, created a skipped execution log and return empty data to downstream collections. This way, we still get the logs, and the overall graph doesn't have to change if a collection is disabled, but no queries are run for that collection.
Add tests for both deleted and disabled collections for when a collection is deleted/disabled before starting a new request, while a request is in progress, or when we restart a deleted/disabled collection from a failed state.

Checklist

Ticket

Fixes #602

…nlogstatus.skipped. - Allow setting the disabled field in the PATCH ConnectionConfig endpoint - Set disabled_at if disabled is updated - Skip running a collection and return the default value if its associated connectionconfig is disable.

…delete_datastore

…ons on restart.

… deleting a collection while the privacy request is in progress.

…delete_datastore

- Update copy/pasted docstring - Add test for deleting collection and then restarting from failure. - Add new connection config disabled key to api response

… deleted or disabled for completeness.

pattisdr · 2022-06-13T14:58:16Z

@ethyca/docs-authors minor one-line change added to docs about the disabled field for a connectionconfig

pattisdr · 2022-06-13T15:12:04Z

tests/fixtures/integration_fixtures.py

+def integration_mongodb_config(db) -> ConnectionConfig:
+    connection_config = ConnectionConfig(
        key="mongo_example",
        connection_type=ConnectionType.mongodb,
        access=AccessLevel.write,
        secrets=integration_secrets["mongo_example"],
+        name="mongo_example",
    )
+    connection_config.save(db)
+    yield connection_config
+    connection_config.delete(db)


This connectionconfig wasn't being saved to the db previously, which was fine, but now I'd like to be able to persist it to test disabling it mid-privacy request, for example, and I'm changing the fixture scope so updates to disabled are reset for another test.

pattisdr · 2022-06-13T15:12:26Z

tests/integration_tests/test_execution.py

+@pytest.mark.integration
+def test_restart_graph_from_failure(
+    db,
+    policy,
+    example_datasets,
+    integration_postgres_config,
+    integration_mongodb_config,
+    mongo_postgres_dataset_graph,
+) -> None:
+    """Run a failed privacy request and restart from failure"""
+


This test isn't new, I've just renamed the file and added more execution-related tests

pattisdr · 2022-06-13T15:34:56Z

src/fidesops/task/graph_task.py

+                f"ConnectionConfig {connection_config.key} is disabled.",
+            )
+
+    @retry(action_type=ActionType.access, default_return=[])


We used to have this default_return parameter, and it was removed, but I'm restoring it now because it's handy for when a collection is skipped, we just return the "default_return".

conceptualshark · 2022-06-13T17:39:50Z

docs/fidesops/docs/guides/database_connectors.md

+* `disabled` determines whether the ConnectionConfig is active.  If True, we skip running queries for any collection associated with that ConnectionConfig.
+


👍 all good from me - thank you for including!

thanks @conceptualshark!

eastandwestwind

Looks great to me @pattisdr ! I especially appreciate the thorough test additions / edits here, as well as clear naming of methods/vars throughout the PR. Thanks!

eastandwestwind · 2022-06-13T16:40:04Z

src/fidesops/migrations/versions/c3472d75c80e_connectionconfig_disabled.py

+    )
+    op.add_column(
+        "connectionconfig",
+        sa.Column("disabled_at", sa.DateTime(timezone=True), nullable=True),


Good foresight to add this disabled_at column

eastandwestwind · 2022-06-13T18:36:23Z

src/fidesops/models/connectionconfig.py

        return super().delete(db=db)
+
+
+@event.listens_for(ConnectionConfig.disabled, "set")


I didn't know about this sqlalchemy feature, awesome!

pattisdr · 2022-06-13T18:55:16Z

thanks for the quick turnaround @eastandwestwind!

* WIP Add disabled and disabled_at to connectionconfig and add executionlogstatus.skipped. - Allow setting the disabled field in the PATCH ConnectionConfig endpoint - Set disabled_at if disabled is updated - Skip running a collection and return the default value if its associated connectionconfig is disable. * Add tests for disabling collections in progress and skipping collections on restart. * Remove unused var. * Add tests for deleting a collection before you make a new request and deleting a collection while the privacy request is in progress. * Move migration. * - Remove unused property - Update copy/pasted docstring - Add test for deleting collection and then restarting from failure. - Add new connection config disabled key to api response * Add missing mocked function. * Update scopes to match. * Add tests that execution logs are untouched if a connection config is deleted or disabled for completeness. * Log request id instead of request object.

pattisdr added 11 commits June 8, 2022 17:17

Merge remote-tracking branch 'ethyca/main' into fidesops_602_disable_…

9c41655

…delete_datastore

Add tests for disabling collections in progress and skipping collecti…

6a50783

…ons on restart.

Remove unused var.

1d91677

Add tests for deleting a collection before you make a new request and…

a12eef1

… deleting a collection while the privacy request is in progress.

Merge remote-tracking branch 'ethyca/main' into fidesops_602_disable_…

b221a78

…delete_datastore

Move migration.

804a4e0

- Remove unused property

d4dcf94

- Update copy/pasted docstring - Add test for deleting collection and then restarting from failure. - Add new connection config disabled key to api response

Add missing mocked function.

2600da6

Update scopes to match.

ecf24f6

Add tests that execution logs are untouched if a connection config is…

ec38f15

… deleted or disabled for completeness.

pattisdr marked this pull request as ready for review June 13, 2022 14:58

seanpreston assigned eastandwestwind Jun 13, 2022

pattisdr commented Jun 13, 2022

View reviewed changes

Log request id instead of request object.

040957a

pattisdr commented Jun 13, 2022

View reviewed changes

conceptualshark reviewed Jun 13, 2022

View reviewed changes

eastandwestwind approved these changes Jun 13, 2022

View reviewed changes

eastandwestwind merged commit 3828b5b into main Jun 13, 2022

eastandwestwind deleted the fidesops_602_disable_delete_datastore branch June 13, 2022 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support Backend Delete and Disable Collection Requirements [#602] #637

Support Backend Delete and Disable Collection Requirements [#602] #637

Uh oh!

pattisdr commented Jun 10, 2022 •

edited

Loading

Uh oh!

pattisdr commented Jun 13, 2022

Uh oh!

pattisdr Jun 13, 2022

Uh oh!

pattisdr Jun 13, 2022

Uh oh!

pattisdr Jun 13, 2022

Uh oh!

conceptualshark Jun 13, 2022

Uh oh!

pattisdr Jun 13, 2022

Uh oh!

eastandwestwind left a comment

Uh oh!

eastandwestwind Jun 13, 2022

Uh oh!

eastandwestwind Jun 13, 2022

Uh oh!

pattisdr commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		* `disabled` determines whether the ConnectionConfig is active. If True, we skip running queries for any collection associated with that ConnectionConfig.

		return super().delete(db=db)


		@event.listens_for(ConnectionConfig.disabled, "set")

Support Backend Delete and Disable Collection Requirements [#602] #637

Support Backend Delete and Disable Collection Requirements [#602] #637

Uh oh!

Conversation

pattisdr commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes

Checklist

Ticket

Uh oh!

pattisdr commented Jun 13, 2022

Uh oh!

pattisdr Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

pattisdr Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

pattisdr Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

conceptualshark Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

pattisdr Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

eastandwestwind left a comment

Choose a reason for hiding this comment

Uh oh!

eastandwestwind Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

eastandwestwind Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

pattisdr commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pattisdr commented Jun 10, 2022 •

edited

Loading