Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ The types of changes are:

## [Unreleased](https://github.com/ethyca/fidesops/compare/1.5.0...main)

### Added
* 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

### Developer Experience

* Import ordering is now enforced using [isort](https://pycqa.github.io/isort/) in CI [#533](https://github.com/ethyca/fidesops/pull/533)
Expand Down
3 changes: 3 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ reset-db:
server: compose-build
@docker-compose up

server-no-db: compose-build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

useful for local testing

@docker-compose -f docker-compose.no-db.yml up

server-shell: compose-build
@docker-compose run $(IMAGE_NAME) /bin/bash

Expand Down
38 changes: 38 additions & 0 deletions docker-compose.no-db.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
services:
fidesops:
container_name: fidesops
build:
context: .
dockerfile: Dockerfile
expose:
- 8080
healthcheck:
test: [ "CMD", "curl", "-f", "http://0.0.0.0:8080/health" ]
interval: 30s
timeout: 10s
retries: 3
start_period: 1s
ports:
- "8080:8080"
volumes:
- type: bind
source: ./
target: /fidesops
read_only: False
- /fidesops/src/fidesops.egg-info
environment:
- FIDESOPS__DEV_MODE=${FIDESOPS__DEV_MODE}
- FIDESOPS__LOG_PII=${FIDESOPS__LOG_PII}
- FIDESOPS__HOT_RELOAD=${FIDESOPS__HOT_RELOAD}
- FIDESOPS__DATABASE__ENABLED=false
- FIDESOPS__REDIS__ENABLED=false
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Copy link
Contributor Author

@adamsachs adamsachs May 23, 2022

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@adamsachs adamsachs May 23, 2022

Choose a reason for hiding this comment

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

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...

docs:
build:
context: docs/fidesops/
dockerfile: Dockerfile
volumes:
- ./docs/fidesops:/docs
expose:
- 8000
ports:
- "8000:8000"
14 changes: 9 additions & 5 deletions docs/fidesops/docs/guides/configuration_reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Copy link
Contributor Author

@adamsachs adamsachs May 23, 2022

Choose a reason for hiding this comment

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

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

| `PASSWORD` | `FIDESOPS__DATABASE__PASSWORD` | string | apassword | N/A | The password with which to login to the Fidesops application database |
| `PORT` | `FIDESOPS__DATABASE__PORT` | int | 5432 | 5432 | The port at which the Fidesops application database will be accessible |
| `DB` | `FIDESOPS__DATABASE_DB` | string | db | N/A | The name of the database to use in the Fidesops application database |
| `DB` | `FIDESOPS__DATABASE__DB` | string | db | N/A | The name of the database to use in the Fidesops application database |
| `ENABLED` | `FIDESOPS__DATABASE__ENABLED` | bool | True | True | Whether the application database should be enabled. Only set to false for certain narrow uses of the application that do not require a backing application database. |
|---|---|---|---|---|---|
| `HOST` | `FIDESOPS__REDIS__HOST` | string | redis.internal | N/A | The networking address for the Fidesops application Redis cache |
| `PORT` | `FIDESOPS__REDIS__PORT` | int | 6379 | 6379 | The port at which the Fidesops application cache will be accessible |
| `PASSWORD` | `FIDESOPS__REDIS__PASSWORD` | string | anotherpassword | N/A | The password with which to login to the Fidesops application cache |
| `DB_INDEX` | `FIDESOPS__REDIS__DB_INDEX` | int | 0 | 0 | The Fidesops application will use this index in the Redis cache to cache data |
| `DEFAULT_TTL_SECONDS` | `FIDESOPS__REDIS__DEFAULT_TTL_SECONDS` | int | 3600 | 604800 | The number of seconds for which data will live in Redis before automatically expiring |
| `ENABLED` | `FIDESOPS__REDIS__ENABLED` | bool | True | True | Whether the application's redis cache should be enabled. Only set to false for certain narrow uses of the application that do not require a backing redis cache. |
|---|---|---|---|---|---|
| `APP_ENCRYPTION_KEY` | `FIDESOPS__SECURITY__APP_ENCRYPTION_KEY` | string | OLMkv91j8DHiDAULnK5Lxx3kSCov30b3 | N/A | The key used to sign Fidesops API access tokens |
| `CORS_ORIGINS` | `FIDESOPS__SECURITY__CORS_ORIGINS` | List[AnyHttpUrl] | ["https://a-client.com/", "https://another-client.com"/] | N/A | A list of pre-approved addresses of clients allowed to communicate with the Fidesops application server |
Expand All @@ -64,6 +66,7 @@ USER="postgres"
PASSWORD="a-password"
DB="app"
TEST_DB="test"
ENABLED=true

[redis]
HOST="redis"
Expand All @@ -72,6 +75,7 @@ PORT=6379
CHARSET="utf8"
DEFAULT_TTL_SECONDS=3600
DB_INDEX=0
ENABLED=true

[security]
APP_ENCRYPTION_KEY="OLMkv91j8DHiDAULnK5Lxx3kSCov30b3"
Expand All @@ -84,8 +88,8 @@ OAUTH_ROOT_CLIENT_SECRET="fidesopsadminsecret"
TASK_RETRY_COUNT=3
TASK_RETRY_DELAY=20
TASK_RETRY_BACKOFF=2
REQUIRE_MANUAL_REQUEST_APPROVAL=True
MASKING_STRICT=True
REQUIRE_MANUAL_REQUEST_APPROVAL=true
MASKING_STRICT=true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

```

Please note: The configuration is case-sensitive, so the variables must be specified in UPPERCASE.
Expand Down
2 changes: 2 additions & 0 deletions fidesops.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ USER="postgres"
PASSWORD="216f4b49bea5da4f84f05288258471852c3e325cd336821097e1e65ff92b528a"
DB="app"
TEST_DB="test"
ENABLED=true

[redis]
HOST="redis"
Expand All @@ -12,6 +13,7 @@ PORT=6379
CHARSET="utf8"
DEFAULT_TTL_SECONDS=604800
DB_INDEX=0
ENABLED=true

[security]
APP_ENCRYPTION_KEY="OLMkv91j8DHiDAULnK5Lxx3kSCov30b3"
Expand Down
10 changes: 10 additions & 0 deletions src/fidesops/api/deps.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
from typing import Generator

from fidesops.common_exceptions import FunctionalityNotConfigured
from fidesops.core.config import config
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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())

Copy link
Contributor Author

@adamsachs adamsachs May 24, 2022

Choose a reason for hiding this comment

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

@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.

"Application database required, but it is currently disabled! Please update your application configuration to enable integration with an application database."
)
try:
SessionLocal = get_db_session()
db = SessionLocal()
Expand All @@ -16,4 +22,8 @@ def get_db() -> Generator:

def get_cache() -> Generator:
"""Return a connection to our redis cache"""
if not config.redis.ENABLED:
raise FunctionalityNotConfigured(
"Application redis cache required, but it is currently disabled! Please update your application configuration to enable integration with a redis cache."
)
yield get_redis_connection()
21 changes: 21 additions & 0 deletions src/fidesops/api/v1/exception_handlers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from typing import Callable, List

from fastapi import Request
from fastapi.responses import JSONResponse, Response
from starlette.status import HTTP_500_INTERNAL_SERVER_ERROR

from fidesops.common_exceptions import FunctionalityNotConfigured


class ExceptionHandlers:
@staticmethod
def functionality_not_configured_handler(
Copy link
Contributor

Choose a reason for hiding this comment

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

@staticmethod
def functionality_not_configured_handler(
    ...

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

Choose a reason for hiding this comment

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

@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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

)

@classmethod
def get_handlers(cls) -> List[Callable[[Request, Exception], Response]]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return [ExceptionHandlers.functionality_not_configured_handler]
4 changes: 4 additions & 0 deletions src/fidesops/common_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,7 @@ class NoSuchStrategyException(ValueError):

class MissingConfig(Exception):
"""Custom exception for when no valid configuration file is provided."""


class FunctionalityNotConfigured(Exception):
"""Custom exception for when invoked functionality is unavailable due to configuration."""
2 changes: 2 additions & 0 deletions src/fidesops/core/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class DatabaseSettings(FidesSettings):
DB: str
PORT: str = "5432"
TEST_DB: str = "test"
ENABLED: bool = True

SQLALCHEMY_DATABASE_URI: Optional[PostgresDsn] = None
SQLALCHEMY_TEST_DATABASE_URI: Optional[PostgresDsn] = None
Expand Down Expand Up @@ -103,6 +104,7 @@ class RedisSettings(FidesSettings):
DECODE_RESPONSES: bool = True
DEFAULT_TTL_SECONDS: int = 604800
DB_INDEX: int
ENABLED: bool = True

class Config:
env_prefix = "FIDESOPS__REDIS__"
Expand Down
19 changes: 14 additions & 5 deletions src/fidesops/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
from starlette.middleware.cors import CORSMiddleware

from fidesops.api.v1.api import api_router
from fidesops.api.v1.exception_handlers import ExceptionHandlers
from fidesops.api.v1.urn_registry import V1_URL_PREFIX
from fidesops.common_exceptions import FunctionalityNotConfigured
from fidesops.core.config import config
from fidesops.db.database import init_db
from fidesops.tasks.scheduled.scheduler import scheduler
Expand All @@ -29,21 +31,28 @@
)

app.include_router(api_router)
for handler in ExceptionHandlers.get_handlers():
app.add_exception_handler(FunctionalityNotConfigured, handler)


def start_webserver() -> None:
"""Run any pending DB migrations and start the webserver."""
logger.info("****************fidesops****************")
logger.info("Running any pending DB migrations...")
init_db(config.database.SQLALCHEMY_DATABASE_URI)
if config.database.ENABLED:
# don't run db migrations if database is disabled
logger.info("Running any pending DB migrations...")
init_db(config.database.SQLALCHEMY_DATABASE_URI)

scheduler.start()

logger.info("Starting scheduled request intake...")
initiate_scheduled_request_intake()
if config.database.ENABLED:
# don't schedule request intake if database is disabled
logger.info("Starting scheduled request intake...")
initiate_scheduled_request_intake()

logger.info("Starting web server...")
uvicorn.run(
"src.fidesops.main:app",
Copy link
Contributor Author

@adamsachs adamsachs May 23, 2022

Choose a reason for hiding this comment

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

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!)

Copy link
Contributor

@sanders41 sanders41 May 24, 2022

Choose a reason for hiding this comment

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

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

"fidesops.main:app",
host="0.0.0.0",
port=8080,
log_config=None,
Expand Down
28 changes: 28 additions & 0 deletions tests/api/test_deps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
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())
66 changes: 66 additions & 0 deletions tests/api/v1/test_exception_handlers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import json

import pytest
from starlette.testclient import TestClient

from fidesops.api.v1.scope_registry import CLIENT_CREATE
from fidesops.api.v1.urn_registry import CLIENT, HEALTH, PRIVACY_REQUESTS, V1_URL_PREFIX
from fidesops.core import config


@pytest.fixture
def mock_config_db_disabled():
db_enabled = config.config.database.ENABLED
config.config.database.ENABLED = False
yield
config.config.database.ENABLED = db_enabled

@pytest.fixture
def mock_config_redis_disabled():
redis_enabled = config.config.redis.ENABLED
config.config.redis.ENABLED = False
yield
config.config.redis.ENABLED = redis_enabled


class TestExceptionHandlers:
@pytest.mark.usefixtures("mock_config_db_disabled")
def test_db_disabled(self, api_client: TestClient, generate_auth_header):
auth_header = generate_auth_header([CLIENT_CREATE])
# oauth endpoint should not work
expected_response = {"message": "Application database required, but it is currently disabled! Please update your application configuration to enable integration with an application database."}
response = api_client.post(V1_URL_PREFIX + CLIENT, headers=auth_header)
response_body = json.loads(response.text)
assert 500 == response.status_code
assert expected_response == response_body

# health endpoint should still work
expected_response = {"healthy": True}
response = api_client.get(HEALTH)
response_body = json.loads(response.text)
assert 200 == response.status_code
assert expected_response == response_body

@pytest.mark.usefixtures("mock_config_redis_disabled")
def test_redis_disabled(self, api_client: TestClient, generate_auth_header):
auth_header = generate_auth_header([CLIENT_CREATE])
# Privacy requests endpoint should not work
request_body = [
{
"requested_at": "2021-08-30T16:09:37.359Z",
"identity": { "email": "[email protected]" },
"policy_key": "my_separate_policy"
}
]
expected_response = {"message": "Application redis cache required, but it is currently disabled! Please update your application configuration to enable integration with a redis cache."}
response = api_client.post(V1_URL_PREFIX + PRIVACY_REQUESTS, headers=auth_header, json=request_body)
response_body = json.loads(response.text)
assert 500 == response.status_code
assert expected_response == response_body

# health endpoint should still work
expected_response = {"healthy": True}
response = api_client.get(HEALTH)
response_body = json.loads(response.text)
assert 200 == response.status_code
assert expected_response == response_body
3 changes: 2 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ def migrate_test_db() -> None:
"""Apply migrations at beginning and end of testing session"""
logger.debug("Applying migrations...")
assert config.is_test_mode
init_db(config.database.SQLALCHEMY_TEST_DATABASE_URI)
if config.database.ENABLED:
init_db(config.database.SQLALCHEMY_TEST_DATABASE_URI)
logger.debug("Migrations successfully applied")


Expand Down