environment variables to disable app db and cache. some related cleanup #550

adamsachs · 2022-05-23T19:58:23Z

Purpose

Provide config options to allow fidesops to start cleanly without db and cache integration. Used for narrow use cases like standalone masking engine.

Changes

Added FIDESOPS__DATABASE__ENABLED and FIDESOPS__REDIS__ENABLED configuration variables to allow fidesops to run cleanly in a "stateless" mode without any database or redis cache integration
added exception handlers and associated support to provide clean error handling when endpoints that require db or cache integration are invoked when they're configured to be disabled
minor update in main.py's uvicorn invocation to consistently start app even if init_db isn't called
documentation updates for above config options; cleanup some typos in existing documentation ( @ethyca/docs-authors review please!)

Below are some snippets of various API responses when FIDESOPS__DATABASE__ENABLED and FIDESOPS__REDIS__ENABLED are both false:

/privacy-request endpoint requires redis cache, so a 500 is returned with appropriate error message:

→ curl --location -i --request POST 'http://0.0.0.0:8080/api/v1/privacy-request' \
--header 'Authorization: Bearer ...' \
--header 'Content-Type: application/json' \
--data-raw '[
    {
        "requested_at": "2021-08-30T16:09:37.359Z",
        "identity": {"email": "[email protected]"},
        "policy_key": "my_separate_policy"
    }
]'
HTTP/1.1 500 Internal Server Error
date: Mon, 23 May 2022 15:35:04 GMT
server: uvicorn
content-length: 164
content-type: application/json

{"message":"Application redis cache required, but it is currently disabled! Please update your application configuration to enable integration with a redis cache."}

/oauth/token endpoint requires app db , so a 500 is returned with appropriate error message:

→ curl --location -i --request POST 'http://0.0.0.0:8080/api/v1/oauth/token' \
--form 'client_id="<some_admin_id>"' \
--form 'client_secret="<some_admin_secret>"'
HTTP/1.1 500 Internal Server Error
date: Mon, 23 May 2022 15:35:13 GMT
server: uvicorn
content-length: 171
content-type: application/json

{"message":"Application database required, but it is currently disabled! Please update your application configuration to enable integration with an application database."

/masking/mask endpoint does not require db or redis cache, so it still responds successfully:

→ curl --location -i --request PUT 'http://0.0.0.0:8080/api/v1/masking/mask' \
--header 'Content-Type: application/json' \
--data-raw ' {
    "values": ["[email protected]", "my name is adam", "my name is john"],
    "masking_strategy": {
        "strategy": "hash",
        "configuration": {

        }
    }
}
'
HTTP/1.1 200 OK
date: Mon, 23 May 2022 15:35:42 GMT
server: uvicorn
content-length: 285
content-type: application/json

{"plain":["[email protected]","my name is adam","my name is john"],"masked_values":["f02ea12b1ec5ff17e91caedd895556b2ae604df0d8c54a9b73ff95dcff772d91","c78a804a926d46dde3f2939a71c797de565c05ad3842b5a5b757d79c91a8653d","01b46035b3a4c3a7414a0c481e67d1e6a4c1ca43846f5ed3e5c66f7792b09ee9"]

/health endpoint does not require db or redis cache, so it still responds successfully:

→ curl -i --request GET 'http://0.0.0.0:8080/health'
HTTP/1.1 200 OK
date: Mon, 23 May 2022 15:44:00 GMT
server: uvicorn
content-length: 16
content-type: application/json

{"healthy":true}

Checklist

Update CHANGELOG.md file in appropriate sections
Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
If docs updated (select one):
- 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)
Good unit test/integration test coverage
This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #545

adamsachs · 2022-05-23T19:59:39Z

Makefile

@@ -43,6 +43,9 @@ reset-db:
 server: compose-build
 	@docker-compose up

+server-no-db: compose-build


useful for local testing

adamsachs · 2022-05-23T20:02:12Z

docs/fidesops/docs/guides/configuration_reference.md

@@ -30,16 +30,18 @@ The `fidesops.toml` file should specify the following variables:
 | TOML Variable | ENV Variable | Type | Example | Default | Description |
 |---|---|---|---|---|---|
 | `SERVER` | `FIDESOPS__DATABASE__SERVER` | string | postgres.internal | N/A | The networking address for the Fideops Postgres database server |
-| `USER` | `FIDESOPS__DATABASE_USER` | string | postgres | N/A | The database user with which to login to the Fidesops application database |
-| `PASSWORD` | `FIDESOPS__DATABASE_PASSWORD` | string | apassword | N/A | The password with which to login to the Fidesops application database |
+| `USER` | `FIDESOPS__DATABASE__USER` | string | postgres | N/A | The database user with which to login to the Fidesops application database |


these were existing doc typos as far as i could tell -- missing the double underscore

adamsachs · 2022-05-23T20:04:21Z

docs/fidesops/docs/guides/configuration_reference.md

-REQUIRE_MANUAL_REQUEST_APPROVAL=True
-MASKING_STRICT=True
+REQUIRE_MANUAL_REQUEST_APPROVAL=true
+MASKING_STRICT=true


updated doc reference because camel-cased booleans in my .toml weren't working -- pretty sure lowercase is needed

adamsachs · 2022-05-23T20:05:46Z

src/fidesops/api/v1/exception_handlers.py

+        )
+
+    @classmethod
+    def get_handlers(cls) -> List[Callable[[Request, Exception], Response]]:


wanted to provide a starting point if more exception handlers would be needed over time...perhaps there's a more elegant/dynamic way of doing this, having to list out the methods seems a bit cumbersome

conceptualshark

👍 docs changes look great to me, thanks for catching them!

adamsachs · 2022-05-23T20:14:47Z

src/fidesops/main.py

    uvicorn.run(
-        "src.fidesops.main:app",


when init_db() wasn't run, uvicorn wasn't finding the fidesops module anymore when this was the run command. i think i was able to pin it down to the fact that within init_db(), we run src/migrations/env.py, which happens to add .. to the sys.path -- and only because of that the src.fidesops path is picked up correctly. to me this seems like an unrecognized and unintentional side-effect -- init_db() should not be needed to get the base app running, i don't think -- if it is intended, then we need to do it in a much less obfuscated way.

as far as i could tell, making this switch here doesn't impact existing behavior, and it also allows the app to run if init_db() is not called. i don't know of any negative consequences (though i could certainly be missing something!)

Your explanation here makes sense to me and everything seems to work fine with your change.

NevilleS · 2022-05-23T20:34:14Z

docker-compose.no-db.yml

+      - FIDESOPS__DATABASE__ENABLED=false
+      - FIDESOPS__REDIS__ENABLED=false


Commenting here with some docker compose trivia... you can avoid this separate no-db.yml by exposing this ENV variable on the main file like so:

- FIDESOPS__DATABASE__ENABLED =${FIDESOPS__DATABASE__ENABLED:-true}

That will default to true, or take the value of the ENV var provided to the docker compose invocation.

Then you just need to set the value of the ENV variable in the Makefile to adjust that behaviour, like:

server-no-db: export FIDESOPS__DATABASE__ENABLED=false server-no-db: ... <continue existing target here>

I do this exact pattern in fidesdemo, see:

https://github.com/ethyca/fidesdemo/blob/main/docker-compose.yml#L68

https://github.com/ethyca/fidesdemo/blob/main/Makefile#L189

certainly more concise for the ENV var piece of it, but the no-db.yml compose also doesn't bootstrap the db and redis (and docs) services, to more closely simulate the "standalone" deployment. as far as i know (and based on our conversation on friday), i don't think there's a way to have branching logic in docker-compose based on env var? but i can dig on that if you think it's possible, because i definitely do not love having the additional no-db.yml compose file.

also open to other suggestions to accomplish the above (i.e. setting env var and also not starting up db and redis services)

Ah yes, that's true... it's Hard™️ to make docker compose files dynamic and support different run configurations.

TIL this exists: https://docs.docker.com/compose/profiles/

oh - that looks pretty nice. i can take a shot at that, seems a lot cleaner. thanks for pointing me to that!

hmm. because fidesops depends on db and redis, i don't think this is going to work for us, without some significant refactor. as far as i can tell, there's no way to express conditional dependencies, and i feel like the no-db use case is enough of an edge case for now that i am reluctant to remove those dependencies...

sanders41 · 2022-05-23T21:01:49Z

src/fidesops/api/v1/exception_handlers.py

+
+
+class ExceptionHandlers:
+    def functionality_not_configured_handler(


@staticmethod def functionality_not_configured_handler( ...

sanders41 · 2022-05-24T02:10:07Z

src/fidesops/api/deps.py

 from fidesops.db.session import get_db_session
 from fidesops.util.cache import get_cache as get_redis_connection


 def get_db() -> Generator:
    """Return our database session"""
+    if not config.database.ENABLED:
+        raise FunctionalityNotConfigured(


Let's add a tests for this and the get_cache. I'm thinking add a new file at tests/api/test_deps.py unless you think there is a better place to put it.

import pytest from fidesops.api.deps import get_cache, get_db from fidesops.common_exceptions import FunctionalityNotConfigured from fidesops.core import config @pytest.fixture def mock_config(): db_enabled = config.config.database.ENABLED redis_enabled = config.config.redis.ENABLED config.config.database.ENABLED = False config.config.redis.ENABLED = False yield config.config.database.ENABLED = db_enabled config.config.redis.ENABLED = redis_enabled @pytest.mark.usefixtures("mock_config") def test_get_cache_not_enabled(): with pytest.raises(FunctionalityNotConfigured): next(get_cache()) @pytest.mark.usefixtures("mock_config") def test_get_db_not_enabled(): with pytest.raises(FunctionalityNotConfigured): next(get_db())

@sanders41 thank you for writing the test up! i ran a bit further with this and also added in a test on the endpoint layer to make sure the exception handlers work as expected - let me know what you think.

…tion handlers. make exception handler a static method.

adamsachs · 2022-05-24T13:41:41Z

src/fidesops/api/v1/exception_handlers.py

+        request: Request, exc: FunctionalityNotConfigured
+    ) -> JSONResponse:
+        return JSONResponse(
+            status_code=HTTP_500_INTERNAL_SERVER_ERROR, content={"message": str(exc)}


@sanders41 while you're here - any thoughts on 500 as the error code here? i considered other options but anything else i could think of seemed like it could be misinterpreted, whereas i feel like a generic 500 with a clear error message probably has the best chance of being well understood. but i'd like a second opinion on this!

500 make sense to me here. The other option I could see making sense is 503, but 503 I think would imply things are down temporarily and will come back up which won't be the case so that could be confusing.

sanders41

Looks good to me. I like the new tests!

…up (#550) * environment variables to disable app db and cache. some related cleanup * update changelog.md * add tests db and redis disabled configs. add tests for endpoint exception handlers. make exception handler a static method. Co-authored-by: Adam Sachs <[email protected]>

Adam Sachs added 2 commits May 23, 2022 15:45

environment variables to disable app db and cache. some related cleanup

a3e3d07

update changelog.md

d390687

adamsachs linked an issue May 23, 2022 that may be closed by this pull request

environment var to disable fidesops db integration for certain use cases #545

Closed

adamsachs commented May 23, 2022

View reviewed changes

conceptualshark reviewed May 23, 2022

View reviewed changes

adamsachs commented May 23, 2022

View reviewed changes

NevilleS reviewed May 23, 2022

View reviewed changes

adamsachs requested a review from sanders41 May 23, 2022 21:39

adamsachs self-assigned this May 23, 2022

sanders41 suggested changes May 24, 2022

View reviewed changes

add tests db and redis disabled configs. add tests for endpoint excep…

4810006

…tion handlers. make exception handler a static method.

adamsachs commented May 24, 2022

View reviewed changes

sanders41 approved these changes May 24, 2022

View reviewed changes

adamsachs merged commit 43770d2 into main May 24, 2022

adamsachs deleted the 545-environment-var-to-disable-fidesops-db-integration-for-certain-use-cases branch May 24, 2022 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

environment variables to disable app db and cache. some related cleanup #550

environment variables to disable app db and cache. some related cleanup #550

adamsachs commented May 23, 2022 •

edited

Loading

adamsachs May 23, 2022

adamsachs May 23, 2022 •

edited

Loading

adamsachs May 23, 2022

adamsachs May 23, 2022

conceptualshark left a comment

adamsachs May 23, 2022 •

edited

Loading

sanders41 May 24, 2022 •

edited

Loading

NevilleS May 23, 2022

adamsachs May 23, 2022 •

edited

Loading

NevilleS May 23, 2022

NevilleS May 23, 2022

adamsachs May 23, 2022

adamsachs May 23, 2022 •

edited

Loading

sanders41 May 23, 2022

sanders41 May 24, 2022

adamsachs May 24, 2022 •

edited

Loading

adamsachs May 24, 2022

sanders41 May 24, 2022

sanders41 left a comment

		- FIDESOPS__DATABASE__ENABLED=false
		- FIDESOPS__REDIS__ENABLED=false



		class ExceptionHandlers:
		def functionality_not_configured_handler(

environment variables to disable app db and cache. some related cleanup #550

environment variables to disable app db and cache. some related cleanup #550

Conversation

adamsachs commented May 23, 2022 • edited Loading

Purpose

Changes

Checklist

Ticket

Choose a reason for hiding this comment

adamsachs May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conceptualshark left a comment

Choose a reason for hiding this comment

adamsachs May 23, 2022 • edited Loading

Choose a reason for hiding this comment

sanders41 May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsachs May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsachs May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsachs May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanders41 left a comment

Choose a reason for hiding this comment

adamsachs commented May 23, 2022 •

edited

Loading

adamsachs May 23, 2022 •

edited

Loading

adamsachs May 23, 2022 •

edited

Loading

sanders41 May 24, 2022 •

edited

Loading

adamsachs May 23, 2022 •

edited

Loading

adamsachs May 23, 2022 •

edited

Loading

adamsachs May 24, 2022 •

edited

Loading