Skip to content

Improve python plugin configuration#1605

Merged
BareosBot merged 10 commits intobareos:masterfrom
arogge:dev/arogge/master/better-plugin-config
Dec 11, 2023
Merged

Improve python plugin configuration#1605
BareosBot merged 10 commits intobareos:masterfrom
arogge:dev/arogge/master/better-plugin-config

Conversation

@arogge
Copy link
Member

@arogge arogge commented Nov 27, 2023

add support for configuration files

by setting one or both of the options defaults_file or overrides_file you can now tell every plugin using the baseclass to read a configuration file.
While defaults_file will add the values before the plugin definition (i.e. plugin defintion will override the values from that file), using overrides_files will add the values after defaults and plugin definition (i.e. will override defaults and plugin definition).

All paths are also treated relative to the FD's configuration directory to make life easier.

add support for encoded configuration values

if you need to encode a value to make it unreadable (e.g. an api-key), you can now do that. If you - for example - want to set option password to an encoded value, you instead set the option password#enc to the encoded value and the baseclass will take care of decoding the value for the plugin.

TODO

  • provide a script to actually encode values
  • package everything correctly
  • add/update documentation

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
    Check backport line
  • Required backport PRs have been created
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
Tests
  • Decision taken that a test is required (if not, then remove this paragraph)
  • The choice of the type of test (unit test or systemtest) is reasonable
  • Testname matches exactly what is being tested
  • On a fail, output of the test leads quickly to the origin of the fault

@arogge arogge force-pushed the dev/arogge/master/better-plugin-config branch 2 times, most recently from bc02124 to 08c9c75 Compare November 27, 2023 12:09
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

This is pretty cool, but i do have some questions.

  • Is the idea that each plugin overwrites config_path so that each of them has a unique config file ? If not, maybe the core should just load the file into a string and pass that to python so that we do not have to bother with the path stuff inside python.

  • If each plugin is supposed to share the same config file, should we use the sections to tell which option belongs to which plugin ? I.e.

[Base] # shared by every plugin
...
[vmware] # only for vmware plugins
...
[my-plugin]
...

This would obviously only work if the plugin knows its name.

@arogge
Copy link
Member Author

arogge commented Nov 28, 2023

This is pretty cool, but i do have some questions.

* Is the idea that each plugin overwrites `config_path` so that each of them has a unique config file ?  If not, maybe the core should just load the file into a string and pass that to python so that we do not have to bother with the path stuff inside python.

No, that's not the idea.

* If each plugin is supposed to share the same config file, should we use the sections to tell which option belongs to which plugin ? I.e.

We're not sharing the configuration files. Retrieving the config-path from core is just so you can configure with relative paths.

* What do you think of using https://docs.python.org/3/library/configparser.html for parsing ? Looks like it has a similar goal and its a standard library (?).  That way we can say that we follow a/the standard.

Thought about it (especially as it is used in the VMWare plugin), but the mapping code would be more complicated than the "parser" we have for the configuration files now.
Also, I didn't want to use a different approach than the plugindef parser, so you don't get strange effects when moving a setting from plugindef to configuration file or back.

@arogge arogge force-pushed the dev/arogge/master/better-plugin-config branch from 08c9c75 to 3c8832a Compare November 28, 2023 17:21
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

I added some additional comments

@arogge arogge force-pushed the dev/arogge/master/better-plugin-config branch from 7af197f to c8728dc Compare December 5, 2023 13:47
@arogge arogge marked this pull request as ready for review December 5, 2023 13:48
@sebsura sebsura self-assigned this Dec 5, 2023
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

I added some suggestions. I am also not sure if we package the new script on windows at all. Since the windows version does have access to the new plugin option, it should probably also have access to this functionality.

@arogge arogge requested a review from sebsura December 8, 2023 15:43
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

Looks great. I added one small comment; after that is taken care of ill approve this!

This patch adds bVarUsedConfig that provides the path to the
configuration that the fd uses. This will contain something like
`/etc/bareos/bareos-fd.d/*/*.conf`.
@arogge arogge force-pushed the dev/arogge/master/better-plugin-config branch from fafc739 to 5a86437 Compare December 11, 2023 09:23
@arogge arogge mentioned this pull request Dec 11, 2023
14 tasks
@arogge arogge force-pushed the dev/arogge/master/better-plugin-config branch from 824877d to 5a86437 Compare December 11, 2023 14:22
this patch extends BareosFdPluginBaseclass to provide a standard way to
work with plugin configuration files.

When the plugin options parser encounters one of the options
defaults_file or overrides_file, it will treat the values as a path to
an ini-style configuration file and will try to read it. The resulting
options are added with precedence overrides_file, plugin definition,
defaults_file.
The code will consume the defaults_file and overrides_file options, so
the plugin will not see them later.
by adding a transform (currently only "enc") to an option name, the
python baseclass will now decode the value.
i.e. if you configure `password#enc=E+*g/GAhM4` you will end up with
the option password containing the value "password".
the dictionaries in the module were previously filled with arbitrary
numbers that matched what we had in the enums.
With this patch we now export the enum value itself, so no mismatches
can happen.
The patch also converts the filetype defines into an enum.
We now encode with b85encode instead of a85encode. The major difference
is the set of characters used to represent the result. While a85encode
will use a colon (:) that is also used as an option separator in the
plugindef, b85encode will not.
As a result values encoded with b85encode will be simpler to use.
@arogge arogge force-pushed the dev/arogge/master/better-plugin-config branch from 5a86437 to 62a3b07 Compare December 11, 2023 21:07
@BareosBot BareosBot merged commit a77b053 into bareos:master Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants