[#1393] Update Fidesops config with sane defaults where necessary #1395

seanpreston · 2022-09-27T22:18:11Z

Purpose

This PR addresses some shortcomings in the Fidesops config highlighted by the recent removal of the fidesops.toml from the Docker build. I've not tried to set defaults for any config vars within the [database], [redis], or [security] subsections, since they're either handled separately, or setting a default is an anti-pattern.

This Fideslib PR also aims to clarify which part of config. loading any error has occurred in.

Changes

Only populate analytics_id if analytics_opt_out is either None or False — this still preserves the behaviour of an explicit opt-out.
Sets port to 8080
Sets task_retry_ vars to 0 — this means by default nodes in the Fidesops identity graph traversal will not attempt to retry if they fail.

Checklist

Ticket

Fixes #1393

NevilleS

Thanks - few notes from my end

NevilleS · 2022-09-27T23:22:28Z

src/fidesops/ops/core/config.py

-    task_retry_count: int
-    task_retry_delay: int  # In seconds
-    task_retry_backoff: int
+    # By default Fidesops will not retry graph nodes


Why not? Our previous defaults for this (set in the TOML) did have nonzero retries, and I think that's helpful behaviour!

I think not retrying at all is a sane default given the context of:

this var. is also used in the CI tests (ultimately we'll overload this) and causes runtimes to increase significantly

the reporting of what's happening during the retries is opaque (again, we will fix this in a subsequent release)

it's blanket applied across all datastores, regardless of any specifics — a more sane implementation would allow for datastore level overrides

It can always be overloaded

OK, I'll trust you to roll this into future improvements so that our default retry logic is reasonable. I'd argue that we should disable it in CI if we want (we can override this ourselves!) but understand if the retries are surprising right now. We can improve that in our error reporting work

src/fidesops/ops/core/config.py

NevilleS · 2022-09-27T23:24:48Z

src/fidesops/ops/core/config.py

+        FidesopsNotificationSettings
+    ] = FidesopsNotificationSettings()
+
+    port: int = 8080  # Run the webserver on port 8080 by default


This variable can't be set via an ENV option right now, since there isn't a env var prefix set for the top-level config object

NevilleS · 2022-09-27T23:26:26Z

src/fidesops/ops/core/config.py

-    notifications: FidesopsNotificationSettings
-
-    port: int
+    execution: Optional[ExecutionSettings] = ExecutionSettings()


There's a (required!) config variable missing from this schema: fidesops.security.drp_jwt_secret. Can you add it to the schema and ensure it's optional?

It's in the Fideslib config already, so we shouldn't specify it twice

Ah, I see! I'd like it to have a default though, can't we make that optional?

Good spot, have made this update to Fideslib

Now that this is optional jwt_key: str = config.security.drp_jwt_secret in src/fidesops/ops/api/v1/endpoints/drp_endpoints.py has the wrong type. Interestingly it already handles the None case.

NevilleS

One more suggestion: can you bump the fideslib requirement in this PR and ensure the related PR lands? So that this all comes together into the ops codebase and closing the related issue.

seanpreston · 2022-09-28T13:54:21Z

requirements.txt

@@ -5,7 +5,6 @@ celery[pytest]==5.2.7
 click==8.1.3
 cryptography~=3.4.8
 dask==2022.8.0
-emails


This is unused, so I'm removing it here as part of this housekeeping.

seanpreston · 2022-09-28T13:56:32Z

src/fidesops/ops/core/config.py

-        return v or cls.generate_and_store_client_id()
+        if not v and not values.get("analytics_opt_out"):
+            v = cls.generate_and_store_client_id()
+        return v


cc @eastandwestwind — this should hopefully fix the client_id generation issue once and for all

seanpreston · 2022-09-28T14:02:53Z

src/fidesops/ops/core/config.py

+        FidesopsNotificationSettings
+    ] = FidesopsNotificationSettings()
+
+    port: int = int(


I'd be open to other ways of doing this than casting to int(...) though it seemed practical for now.

My main gripe is that this could be confusing for users as the env vars are expected to be strings, where port must then be an integer. It's technically possible to run Fidesops with FIDESOPS__PORT="iamastring" and the error message would be along the lines of ValueError: invalid literal for int() with base 10: 'iamastring'.

The idea here being the user gets a more informative error message?

src/fidesops/ops/core/config.py

sanders41 · 2022-09-28T15:49:03Z

src/fidesops/ops/core/config.py

-    notifications: FidesopsNotificationSettings
-
-    port: int
+    execution: Optional[ExecutionSettings] = ExecutionSettings()


Now that this is optional jwt_key: str = config.security.drp_jwt_secret in src/fidesops/ops/api/v1/endpoints/drp_endpoints.py has the wrong type. Interestingly it already handles the None case.

sanders41 · 2022-09-28T16:31:30Z

src/fidesops/ops/core/config.py

+        FidesopsNotificationSettings
+    ] = FidesopsNotificationSettings()
+
+    port: int = int(


The idea here being the user gets a more informative error message?

Changes made

sanders41 · 2022-09-29T01:22:50Z

@seanpreston I re-ran my isolated container and everything worked well.

After these updates jwt_key: str = config.security.drp_jwt_secret in src/fidesops/ops/api/v1/endpoints/drp_endpoints.py has the wrong type now. It needs to either be updated to jwt_key: Optional[str] or the type could be removed and use type inference, either way would work. Other than that everything looks good now.

seanpreston · 2022-09-29T13:26:53Z

After these updates jwt_key: str = config.security.drp_jwt_secret in src/fidesops/ops/api/v1/endpoints/drp_endpoints.py has the wrong type now

I updated that already, you might just need to refresh the diff. Will merge away.

ThomasLaPiana · 2022-09-29T16:54:15Z

src/fidesops/ops/core/config.py

-    task_retry_backoff: int
+    # By default Fidesops will not retry graph nodes
+    task_retry_count: int = 0
+    task_retry_delay: int = 0  # In seconds


a nit here, but # In seconds doesn't help end-users. If we want to clarify it we should update the variable name to contain the unit, i.e. task_retry_delay_seconds

You don't think all users are going to read the source code first 😄 ?

Making this change would be a breaking change, task_retry_delay was in previous releases it just gets a default value now. So the question now is it worth it to make a breaking change or could we add something to the docs and that be enough?

Sean Preston added 4 commits September 27, 2022 18:11

add sane defaults

59f7316

make subsections of config with complete defaults optional

7224262

lowercase database.enabled, set defaults for optional configs

3082c97

update return type

59de0df

seanpreston assigned sanders41 Sep 27, 2022

seanpreston added the run unsafe ci checks Triggers running of unsafe CI checks label Sep 27, 2022

updates changelog

52fcd9a

NevilleS suggested changes Sep 27, 2022

View reviewed changes

Sean Preston added 2 commits September 27, 2022 21:18

make PORT an env var

7cdd87e

cast env var to int

7d07c22

NevilleS previously requested changes Sep 28, 2022

View reviewed changes

remove unnecessary unpinned dependency

ed40eda

seanpreston commented Sep 28, 2022

View reviewed changes

bump fideslib version

c6ca84b

seanpreston commented Sep 28, 2022

View reviewed changes

Sean Preston added 2 commits September 28, 2022 11:35

bump fideslib to 3.1.4

66adb6e

add defaults for the non optional config subclasses

ff5008b

sanders41 reviewed Sep 28, 2022

View reviewed changes

Sean Preston added 3 commits September 28, 2022 18:07

set empty dict to default for config subclasses that require some fields

591047e

use .get() in assemble URL for correct error message, correct comment

0d3c124

update jwt_key type annotation

60371f9

sanders41 approved these changes Sep 29, 2022

View reviewed changes

sanders41 merged commit effa4cb into main Sep 29, 2022

sanders41 deleted the sp-1393-config branch September 29, 2022 13:30

ThomasLaPiana reviewed Sep 29, 2022

View reviewed changes

seanpreston mentioned this pull request Oct 4, 2022

Run Fides from a single command ethyca/fides#1210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1393] Update Fidesops config with sane defaults where necessary #1395

[#1393] Update Fidesops config with sane defaults where necessary #1395

seanpreston commented Sep 27, 2022 •

edited

Loading

NevilleS left a comment

NevilleS Sep 27, 2022

seanpreston Sep 28, 2022

NevilleS Sep 28, 2022

NevilleS Sep 27, 2022

seanpreston Sep 28, 2022

NevilleS Sep 27, 2022

seanpreston Sep 28, 2022

NevilleS Sep 28, 2022

seanpreston Sep 28, 2022

sanders41 Sep 28, 2022

NevilleS left a comment

seanpreston Sep 28, 2022 •

edited

Loading

seanpreston Sep 28, 2022

seanpreston Sep 28, 2022

sanders41 Sep 28, 2022

sanders41 Sep 28, 2022

sanders41 Sep 28, 2022

sanders41 commented Sep 29, 2022

seanpreston commented Sep 29, 2022

ThomasLaPiana Sep 29, 2022

sanders41 Sep 29, 2022 •

edited

Loading

[#1393] Update Fidesops config with sane defaults where necessary #1395

[#1393] Update Fidesops config with sane defaults where necessary #1395

Conversation

seanpreston commented Sep 27, 2022 • edited Loading

Purpose

Changes

Checklist

Ticket

NevilleS left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NevilleS left a comment

Choose a reason for hiding this comment

seanpreston Sep 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanders41 commented Sep 29, 2022

seanpreston commented Sep 29, 2022

Choose a reason for hiding this comment

sanders41 Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

seanpreston commented Sep 27, 2022 •

edited

Loading

seanpreston Sep 28, 2022 •

edited

Loading

sanders41 Sep 29, 2022 •

edited

Loading