Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1241] Restore default DSR policies #1426

Merged
merged 20 commits into from
Oct 14, 2022
Merged

Conversation

seanpreston
Copy link
Contributor

@seanpreston seanpreston commented Oct 13, 2022

Closes #1241

Code Changes

  • This PR adds a method load_default_policies to be run within init_db
  • load_default_policies creates two DSR execution policies equivalent to those in Fidesops

Steps to Confirm

  • nox -s teardown -- volumes
  • nox -s dev
  • docker compose run fides /bin/bash, then:
pip install ipython
ipython

and in the shell prompt (this can be copied and pasted)

# All table models must be loaded in to assuage any dependency issues
from fideslib.db.base_class import Base
from fideslib.models.audit_log import AuditLog
from fideslib.models.client import ClientDetail
from fideslib.models.fides_user import FidesUser
from fideslib.models.fides_user_permissions import FidesUserPermissions
from fides.api.ops.models.authentication_request import AuthenticationRequest
from fides.api.ops.models.connectionconfig import ConnectionConfig
from fides.api.ops.models.datasetconfig import DatasetConfig
from fides.api.ops.models.email import EmailConfig
from fides.api.ops.models.manual_webhook import AccessManualWebhook
from fides.api.ops.models.policy import Policy, Rule, RuleTarget
from fides.api.ops.models.privacy_request import PrivacyRequest
from fides.api.ops.models.storage import StorageConfig

# Create a database session
from fides.api.ctl.database.session import sync_session
db = sync_session()

# Do some assertions on data that should have been loaded in during `nox -s dev`
assert len(Policy.all(db=db)) == 2
assert len(Rule.all(db=db)) == 2
assert len(RuleTarget.all(db=db)) == 62

if no AssertionErrors or otherwise are thrown, this script has worked

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
  • Update CHANGELOG.md

@seanpreston seanpreston changed the title Sp 1241 default policies [Draft] [#1241] Restore default DSR policies Oct 13, 2022
@seanpreston seanpreston changed the title [Draft] [#1241] Restore default DSR policies [#1241] Restore default DSR policies Oct 13, 2022
@seanpreston seanpreston marked this pull request as ready for review October 13, 2022 01:54
@seanpreston seanpreston requested a review from a team October 13, 2022 01:54
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but with several nits.

src/fides/api/ctl/database/database.py Outdated Show resolved Hide resolved
src/fides/api/ctl/database/database.py Outdated Show resolved Hide resolved
src/fides/api/ctl/database/database.py Outdated Show resolved Hide resolved
src/fides/api/ctl/database/database.py Outdated Show resolved Hide resolved
src/fides/api/ctl/database/database.py Outdated Show resolved Hide resolved
@ThomasLaPiana ThomasLaPiana requested a review from a team October 13, 2022 08:42
@ThomasLaPiana
Copy link
Contributor

@seanpreston when I run the server, I'm seeing this error:

2022-10-13 10:05:44.698 [ERROR] (database:configure_db:101): Unable to configure database: fides.api.ops.common_exceptions.PolicyValidationError: Policy rules are invalid, action conflict in erasure rules detected for categories user.name and user

@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented Oct 13, 2022

also it appears the database reset aren't working either

Edit: turns it out was related. I commented out the "erasure rules" setup code chunk and it works just fine now, including database resets. This is the offending section:

    log.info("Creating: Ops Data Category Erasure Rules...")
    for target in default_data_categories:
        data = {
            "data_category": target,
            "rule_id": erasure_rule.id,
        }
        compound_key = to_snake_case(RuleTarget.get_compound_key(data=data))
        data["key"] = compound_key
        try:
            RuleTarget.create(
                db=db_session,
                data=data,
            )
        except KeyOrNameAlreadyExists:
            # This rule target already exists against the Policy
            pass

@ThomasLaPiana
Copy link
Contributor

looking at previous tests runs, I definitely broke this. I think I know what I did but will confirm and fix

@ThomasLaPiana
Copy link
Contributor

server issues sorted! now fixing up the tests

@ThomasLaPiana
Copy link
Contributor

@seanpreston can you give this a review now? classic switcheroo

@ThomasLaPiana
Copy link
Contributor

ThomasLaPiana commented Oct 13, 2022

I'm seeing intermittent issues here when resetting the database 🤔 this needs further investigation and should not be merged until it is fixed

This is where it's hanging:

2022-10-13 13:16:41.291 [INFO] (main:log_request:262): Request received | {'method': 'GET', 'status_code': 200, 'handler_time': '513.392ms', 'path': '/health'}
2022-10-13 13:17:03.135 [INFO] (database:reset_db:64): Resetting database...

I have a sneaking suspicion that the database drop is colliding with another connection

@ThomasLaPiana
Copy link
Contributor

ok, I've added some changes in that should make this more safe, and it wasn't triggered during the CI run but I'm also not 100% if its been fixed. Will warn the engineering team and keep an eye on it

Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed some changes here and added some comments, but otherwise LGTM 👍

clients/privacy-center/config/config.json Show resolved Hide resolved
noxfiles/docker_nox.py Show resolved Hide resolved
src/fides/api/ctl/database/seed.py Show resolved Hide resolved
@ThomasLaPiana
Copy link
Contributor

I noticed the hanging test is still happening intermittently, so I added a timeout for all tests of 15 minutes

@ThomasLaPiana ThomasLaPiana merged commit bd9fda8 into main Oct 14, 2022
@ThomasLaPiana ThomasLaPiana deleted the sp-1241-default-policies branch October 14, 2022 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new "default" DSR policies for Unified Fides
3 participants