Skip to content
This repository has been archived by the owner on May 29, 2024. It is now read-only.

Introduce Dynaconf #133

Merged
merged 23 commits into from
Jun 28, 2021
Merged

Introduce Dynaconf #133

merged 23 commits into from
Jun 28, 2021

Conversation

0snap
Copy link
Contributor

@0snap 0snap commented Jun 23, 2021

馃摂 Description

Switch configuration framework from confuse to dynaconf. The PR updates all of Threat Bus, all plugins, all apps, all integration tests and the MISP test utils.

Docs will follow in a separate PR.

馃摑 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/threatbus, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

馃幆 Review Instructions

  • Craft some config files and Interactively check application startup. Test that all required values are present or the validation should complain otherwise.

@0snap 0snap added the enhancement An improvement of existing code label Jun 23, 2021
@0snap 0snap requested a review from lava June 23, 2021 11:49
@0snap
Copy link
Contributor Author

0snap commented Jun 23, 2021

@satta ping: we ditch confuse in favor of the superior config framework dynaconf. This might require re-packaging for Debian.

@satta
Copy link
Collaborator

satta commented Jun 23, 2021

@satta ping: we ditch confuse in favor of the superior config framework dynaconf. This might require re-packaging for Debian.

Thanks, good to know. For buster, yes. Bullseye will have python3-dynaconf (2.2.3-2). Can you do with that version or do you really need 3.X?

@0snap
Copy link
Contributor Author

0snap commented Jun 28, 2021

Hey @satta, I just checked the docs, and we definitely require dynaconf >= 3.1.X for two reasons:

  1. According to their changelog there are breaking changes between v2 and v3, which would affect us. They have deprecated a the usage of a global structure and introduced an object-local settings management. Since we use that (in accordance to the v3 docs), the code is not compatible to dynaconf 2.X.
  2. We use dynaconf Validators to check the user configurations for correctness. Validators are a new feature as of v3.1

@satta
Copy link
Collaborator

satta commented Jun 28, 2021

Got it. This will mean updating and backporting version 3 as well.
This is pretty similar to the other issue we are facing with pika. Looks like I will need to revisit the question of how to handle multiple versions of the same Python module in Debian.

Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

The code changes itself look good to me, but I failed to pass validation when I tried to start threatbus:

$ head -7 config.yaml.example 
logging:
  console: true
  console_verbosity: DEBUG
  file: false
  file_verbosity: DEBUG
  filename: threatbus.log

$ threatbus -c config.yaml.example 
Invalid config: combined validators failed logging.console is required in env main or logging.file is required in env main

$ THREATBUS_LOGGING_CONSOLE=true threatbus -c config.yaml.example 
Invalid config: combined validators failed logging.console is required in env main or logging.file is required in env main

It's not completely clear to me what the issue is.

README.md Show resolved Hide resolved
@0snap
Copy link
Contributor Author

0snap commented Jun 28, 2021

Both things you tried are invalid. You first need to copy the file to something ending in .yaml or .yml, otherwise dynaconf does not pick it up. So when you follow the readme and cp config.yaml.example config.yaml it should work.

Second, as stated in the readme, THREATBUS_LOGGING_CONSOLE=true is invalid and should instead have two underscores.
Here's a working example that would make Threat Bus additionally log to a file:

THREATBUS_LOGGING__FILE=true threatbus -c config.yaml

@lava
Copy link
Member

lava commented Jun 28, 2021

You first need to copy the file to something ending in .yaml or .yml, otherwise dynaconf does not pick it up.

Shouldn't we then add a validator that checks that the -c parameter value ends in .yaml or .yml?

@0snap
Copy link
Contributor Author

0snap commented Jun 28, 2021

Shouldn't we then add a validator that checks that the -c parameter value ends in .yaml or .yml?

While we lose the ability to accept toml or otherwise neat config formats, I agree that we have not yet documented the alternatives a user can pick and hence added a strict check for all application to exit early for any other file extension than yaml or yml.

@0snap 0snap requested a review from lava June 28, 2021 11:07
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

The code looks good; I didn't get to do too much local testing but imho it's fine to merge this now and fix any remaining issues as they come up, since it's a month anyways to the next release.

@0snap 0snap merged commit c7410d0 into master Jun 28, 2021
@0snap 0snap deleted the story/ch26255 branch June 28, 2021 12:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement An improvement of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants