Improve configuration variable troubleshooting with DEBUG logs, more helpful errors, and unit tests #579

NevilleS · 2022-05-30T23:32:41Z

Purpose

After watching @RobertKeyser run into some mysterious errors setting up the fidesops configuration values recently, I thought I'd make a few QOL improvements to the debug-ability of our config.

Changes

Changed the error message for FIDESOPS__SECURITY__APP_ENCRYPTION_KEY to output the number of characters to aid troubleshooting
Added FIDESOPS__SECURITY__LOG_LEVEL configuration variable to allow controlling the log level #579
Added DEBUG logs at startup to view all configuration values #579
Added unit tests for FidesopsConfig loading and tests for APP_ENCRYPTION_KEY validation

Checklist

Update CHANGELOG.md file
- Merge in main so the most recent CHANGELOG.md file is being appended to
- Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
- Add a link to this PR at the end of the description with the PR number as the text. example: #1
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)
- n/a documentation issue created (tag docs-team to complete issue separately)
Good unit test/integration test coverage
n/a 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

n/a

Adds the expected & actual char length for the config var to the error message, to aid in troubleshooting

NevilleS · 2022-05-30T23:33:50Z

src/fidesops/core/config.py

@@ -214,6 +243,14 @@ class Config:  # pylint: disable=C0115
        f'Startup configuration: pii logging = {os.getenv("FIDESOPS__LOG_PII") == "True"}'
    )

+    def log_all_config_values(self) -> None:


This gets called during start_webserver to provide some helpful DEBUG logs. I find it's often difficult to figure out if my ENV variables or configuration file is loading as expected, so this aids in that

this will be super helpful, thanks!

NevilleS · 2022-05-31T00:08:09Z

src/fidesops/core/config.py

@@ -266,15 +302,15 @@ def get_config() -> FidesopsConfig:
    """
    try:
        return FidesopsConfig.parse_obj(load_toml("fidesops.toml"))
-    except (FileNotFoundError, ValidationError) as e:


Catching the ValidationError here was odd; it basically made it try loading the config twice:

once from the file, which if invalid threw an error

then once again from defaults, which may throw an error

This is surprising, because if you have an invalid config it fails twice, and usually with different error messages, which is misleading.

By removing this catch, the validation error simply crashes startup and throws a more relevant warning log and stacktrace.

good catch, I'll actually have to update this on my fideslog integration branch

Ideally let's get this merged to main so you can just rebase 👍

NevilleS · 2022-05-31T00:09:49Z

src/fidesops/main.py

+        logger.warning(
+            "WARNING: log level is DEBUG, so sensitive or personal data may be logged. "
+            "Set FIDESOPS__SECURITY__LOG_LEVEL to INFO or higher in production."
+        )


I'm increasingly of the opinion we should remove the LOG_PII variable, and just use this single LOG_LEVEL instead. Having DEBUG vs INFO logs is a standard config option most apps support, so it'll be more intuitive to use this.

NOTE: I really wanted to DEBUG log all the ENV vars at startup too, but by the time this method runs the FidesopsConfig has already been loaded (and potentially crashed), so it's too late. Since we're using the config to set the log level simultaneously, it's awkward to use DEBUG logs to see the ENV vars... kind of a chicken & egg situation.

NevilleS · 2022-05-31T00:12:33Z

tests/core/test_config.py

+from fidesops.core.config import get_config
+
+
+def test_config_from_default() -> None:


These first three tests (load from default, load from path, load from ENV) didn't have any tests yet so I added some basic ones

eastandwestwind

Looking good @NevilleS, just wanted additional clarify on the fidesops.toml you've added, due to the fact that I'm working on testing new env variables for fideslog that may need to be added to this toml, too.

src/fidesops/core/config.py

eastandwestwind · 2022-05-31T17:10:19Z

src/fidesops/core/config.py

@@ -266,15 +302,15 @@ def get_config() -> FidesopsConfig:
    """
    try:
        return FidesopsConfig.parse_obj(load_toml("fidesops.toml"))
-    except (FileNotFoundError, ValidationError) as e:


good catch, I'll actually have to update this on my fideslog integration branch

eastandwestwind · 2022-05-31T17:18:38Z

src/fidesops/core/config.py

@@ -214,6 +243,14 @@ class Config:  # pylint: disable=C0115
        f'Startup configuration: pii logging = {os.getenv("FIDESOPS__LOG_PII") == "True"}'
    )

+    def log_all_config_values(self) -> None:


this will be super helpful, thanks!

data/config/fidesops.toml

NevilleS · 2022-06-01T16:18:03Z

@eastandwestwind I pushed the update here, LMK if there's anything else you'd recommend. Thanks!

…helpful errors, and unit tests (#579) * Improve error message for APP_ENCRYPTION_KEY Adds the expected & actual char length for the config var to the error message, to aid in troubleshooting * Update tests with black formatting * Add tests for loading config vars * Only attempt to parse config vars once * Add ability to specify LOG_LEVEL via the config * Log all config vars during startup at DEBUG level * Update CHANGELOG.md with new features * Update configuration docs * Add comment to test fidesops.toml * Use string interpolation instead of f-strings for log statements

NevilleS added 9 commits May 27, 2022 16:13

Improve error message for APP_ENCRYPTION_KEY

b8fa0d8

Adds the expected & actual char length for the config var to the error message, to aid in troubleshooting

Update tests with black formatting

6f1c970

Add tests for loading config vars

153d4f0

Only attempt to parse config vars once

3274a22

Add ability to specify LOG_LEVEL via the config

1721fd2

Log all config vars during startup at DEBUG level

755154c

Update CHANGELOG.md with new features

89b7fb8

Update configuration docs

da8ada6

Add comment to test fidesops.toml

3233775

NevilleS added the run unsafe ci checks Triggers running of unsafe CI checks label May 30, 2022

NevilleS commented May 31, 2022

View reviewed changes

seanpreston assigned eastandwestwind May 31, 2022

eastandwestwind reviewed May 31, 2022

View reviewed changes

Use string interpolation instead of f-strings for log statements

3aba69d

Merge branch 'main' into ns-improve-encryption-key-error-message

299442b

eastandwestwind approved these changes Jun 1, 2022

View reviewed changes

eastandwestwind merged commit 345f7b5 into main Jun 1, 2022

eastandwestwind deleted the ns-improve-encryption-key-error-message branch June 1, 2022 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve configuration variable troubleshooting with DEBUG logs, more helpful errors, and unit tests #579

Improve configuration variable troubleshooting with DEBUG logs, more helpful errors, and unit tests #579

NevilleS commented May 30, 2022

NevilleS May 30, 2022

eastandwestwind May 31, 2022

NevilleS May 31, 2022

NevilleS May 31, 2022

eastandwestwind May 31, 2022

NevilleS Jun 1, 2022

NevilleS May 31, 2022

NevilleS May 31, 2022

NevilleS May 31, 2022

eastandwestwind left a comment

eastandwestwind May 31, 2022

eastandwestwind May 31, 2022

NevilleS commented Jun 1, 2022

		from fidesops.core.config import get_config


		def test_config_from_default() -> None:

Improve configuration variable troubleshooting with DEBUG logs, more helpful errors, and unit tests #579

Improve configuration variable troubleshooting with DEBUG logs, more helpful errors, and unit tests #579

Conversation

NevilleS commented May 30, 2022

Purpose

Changes

Checklist

Ticket

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

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

Choose a reason for hiding this comment

eastandwestwind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NevilleS commented Jun 1, 2022