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

1974 add default storage used by all dsr policies by default #2438

Merged

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Jan 31, 2023

Closes #1974
Closes #2453

Code Changes

  • new /storage/default endpoints to set default storage configs per storage type

    • PUT /storage/default to upsert default storage configs for a given type ( thetype is specified in body)
    • PUT /storage/default/{storage_type}/secret to upsert secrets for default storage config of a given type
    • GET /storage/default to get all default storage configs (i.e. of all types)
    • GET /storage/active/default to get the currently active default storage config (see below config property related to this)
  • update /config endpoint to allow setting global application config properties via API

    • PATCH /config takes a JSON settings/properties body and has PATCH semantics, i.e. only fields provided are updated
    • a new query param for the existing GET /config endpoint: query param api_set (which defaults to false). If api_set=true, then the API returns a JSON settings/properties body with the global application config properties that have been set via API
    • properties set through the PATCH API are stored as app state an encrypted JSON blob in a single-row db table
    • this is VERY NARROWLY scoped for now - it only allows setting one global config property, viz. the storage.active_default_storage_type that's used to drive which default storage config type should be used in DSR execution
      • there is no reconciliation yet with application config properties set via the "normal" pydantic config (i.e. toml or env var), but the intention is to support that eventually
      • the intention is to open up the endpoint as a general mechanism for setting many (all?) of our application config properties via API, if desired. but that is a larger effort related to the point above, so we're not exposing that functionality for now.
    • if no storage.active_default_storage_type has been set via API, then a placeholder local storage default will be used as the storage config for DSR execution. local storage requires no user input at this point, so it can be safely generated as a placeholder by the system.
  • new scope added for update permission for the above endpoint:

    • SETTINGS_READ = "settings:read"
    • CONFIG_UPDATE = "config:update"
  • updated seed.py and load_examples.py to respect this new framework with keeping similar semantics as previously. specifically:

    • seed.py no longer creates a local storage configuration, and the default access rule that it creates is no longer explicitly linked to any storage configuration/destination. instead, the default access rule will rely on the default storage logic: if no specific user actions are taken, that will be a placeholder local storage config; if the user goes through necessary steps (in the API or UI) to configure a active default s3 storage location, then that will be used
    • load_examples.py will create a default s3 storage destination if AWS secrets are present in the ENV file, and it will set s3 as the active default storage type.
  • migration behavior:

    • all existing storage configurations are updated to have their is_default attribute set to False
    • there is no storage.active_default_storage_type property set on migration, i.e. that app config property will stay unset until explicitly set by a user
    • existing DSR rules are not impacted, they will continue to point at whatever storage destination they'd pointed at previously
      • the default storage functionality added as part of this PR can generally be taken advantage of, but rules will need to be explicitly updated to not point at a specific storage destination

Steps to Confirm

  • run nox -s dev -- ui and submit access privacy requests, ensure they still write to local storage as expected
  • run nox -s test_env and submit access privacy requests, ensure they still write to local storage as expected
    • run with a .env file configured for s3 writing and that should still work as expected
  • run fides deploy up and submit access privacy requests, ensure they still write to local storage as expected
  • run fides by any of the above 3 methods and use the API to:
    1. PUT /storage/default with a body that specifies an s3 storage destination and a bucket of choice
    2. PUT /storage/default/secrets with a body that specifies s3 storage secrets as needed (if applicable)
    3. PATCH /application/settings with a body like this to use s3 as the active default storage location: { "fides.storage.active_default_storage_type" : "s3" }
    4. submit an access privacy request, and it should output to the s3 destination that was configured.

Pre-Merge Checklist

Description Of Changes

Write some things here about the changes and any potential caveats

@adamsachs adamsachs linked an issue Jan 31, 2023 that may be closed by this pull request
@adamsachs adamsachs self-assigned this Jan 31, 2023
@adamsachs adamsachs marked this pull request as ready for review January 31, 2023 02:09
@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Base: 88.44% // Head: 88.63% // Increases project coverage by +0.19% 🎉

Coverage data is based on head (3aea39a) compared to base (5d0fa65).
Patch coverage: 97.36% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2438      +/-   ##
==========================================
+ Coverage   88.44%   88.63%   +0.19%     
==========================================
  Files         328      331       +3     
  Lines       15954    16131     +177     
  Branches     4431     4485      +54     
==========================================
+ Hits        14110    14298     +188     
+ Misses       1689     1676      -13     
- Partials      155      157       +2     
Impacted Files Coverage Δ
src/fides/api/ctl/database/seed.py 100.00% <ø> (+4.54%) ⬆️
...ides/api/ops/api/v1/endpoints/storage_endpoints.py 88.30% <94.02%> (+3.25%) ⬆️
src/fides/api/ops/models/application_config.py 94.28% <94.28%> (ø)
...fides/api/ops/api/v1/endpoints/config_endpoints.py 100.00% <100.00%> (ø)
...fides/api/ops/api/v1/endpoints/policy_endpoints.py 85.40% <100.00%> (+0.07%) ⬆️
src/fides/api/ops/api/v1/scope_registry.py 100.00% <100.00%> (ø)
src/fides/api/ops/api/v1/urn_registry.py 100.00% <100.00%> (ø)
src/fides/api/ops/common_exceptions.py 98.05% <100.00%> (+0.03%) ⬆️
src/fides/api/ops/db/base.py 100.00% <100.00%> (ø)
src/fides/api/ops/models/policy.py 92.57% <100.00%> (+0.10%) ⬆️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adamsachs adamsachs added the do not merge Please don't merge yet, bad things will happen if you do label Feb 6, 2023
@adamsachs
Copy link
Contributor Author

please don't merge yet - making some adjustments based on realizations since i'd last been working on this:

  • we've already got a config_endpoints.py that we should build on for the settings functionality i've put in there
  • we need to ensure we have a data migration for existing storage destinations

@adamsachs adamsachs removed the do not merge Please don't merge yet, bad things will happen if you do label Feb 7, 2023
@adamsachs
Copy link
Contributor Author

code cov is saying 97.36% of the diff is hit but i don't see any codecov comments remaining so unsure what else i can adjust?

Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

Discussed coverage with @adamsachs. We found that the lines that are showing as uncovered are artifacts of previously uncovered lines. All his changes are covered.

@adamsachs
Copy link
Contributor Author

created https://github.com/ethyca/fidesdocs/issues/56 to track docs updates

@adamsachs adamsachs merged commit b069fe5 into main Feb 8, 2023
@adamsachs adamsachs deleted the 1974-add-default-storage-used-by-all-dsr-policies-by-default branch February 8, 2023 19:11
@adamsachs adamsachs mentioned this pull request Feb 22, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants