-
Notifications
You must be signed in to change notification settings - Fork 146
Support dropin config files (within .d
directories)
#849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Support dropin config files (within .d
directories)
#849
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #849 +/- ##
==========================================
+ Coverage 84.52% 84.88% +0.36%
==========================================
Files 11 11
Lines 3366 3428 +62
==========================================
+ Hits 2845 2910 +65
+ Misses 521 518 -3
|
682c916
to
e80b867
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a rebase, Python 3.9 support has been removed from main
.
df23c63
to
883bc9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this PR was created based on a suggestion from #847 (comment), but it does add some complexity to the configuration system.
Besides that, testing now has a lot more edge-cases.
At the very least we require some more documentation bits. But I'm not convinced yet that this addition is in the spirit of:
Conservative about features: no features will be accepted without strong and compelling motivation.
Can you provide some insight into where you "really" need this kind of flexibility?
src/west/configuration.py
Outdated
for conf in sorted(dropins_dir.iterdir()): | ||
config.cp.read(dropins_dir / conf, encoding='utf-8') | ||
# finally overwrite with values from higher-prior actual config | ||
config.cp.read(path, encoding='utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you need to read the "main" config
file twice to make sure it has precedence over the config.d/
files. That feels a bit hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be okay if I add some argument to the constructor of the config which allows to create the instance without directly reading the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.cp.read
actually allows passing multiple filenames, why not pass multiple paths to the __init__
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a check at later point that the number of read configs is exactly 1. I can remove this check and rework the for loop and instead pass the list of multiple configs.
Nevertheless: what should be the path attribute of the class in case of multiple configs? This is why I thought that requiring the actual config is the best idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO if you have used multiple paths to load the configuration, it could be the argument for the constructor. It would actually allow the --print-path
to print all used paths. Because now this is a hidden feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I like the idea to use the path of the config as argument for the constructor and within the constructor all according dropin config files are searched and their paths are stored in self.dropins
.
Before I make those adaptions, maybe we can clarify first, if you will accept this feature. See my other comment in the conversation with some explanation what we want to do.
Let's use this conversation for clarification of open questions in order to not spam the issue conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a check at later point that the number of read configs is exactly 1. I can remove this check and rework the for loop and instead pass the list of multiple configs.
Yes please. Also, I think it's better to just add all the code to the __init__
constructor, not to some convenience wrapper.
Nevertheless: what should be the path attribute of the class in case of multiple configs? This is why I thought that requiring the actual config is the best idea.
path
should be the non-.d/
file. If it does not exist, then maybe it should be None? Not sure about that. Just like before, writes to a config file that does not exist yet should create that file, and deletions or appending in a non-existing main file should fail. If there was no test coverage for these then it should be added because the new "drop-ins" are going to make the corresponding logic a bit more complicated.
Writing to .d/
files should not be possible through the CLI, way too complicated. This must be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I make those adaptions, maybe we can clarify first, if you will accept this feature. See my other comment in the conversation with some explanation what we want to do.
For me, it mostly depends on the complexity of the feature and how intuitive it's for users. For example,
Writing to .d/ files should not be possible through the CLI, way too complicated. This must be documented.
What about deletions? An option can come from any of those files, but they can't be deleted if not coming from the main config file? This feels somewhat counter-intuitive.
I'd like some other opinions from @carlescufi or @mbolivar too.
There is a check at later point that the number of read configs is exactly 1. I can remove this check and rework the for loop and instead pass the list of multiple configs.
Yes please. Also, I think it's better to just add all the code to the
__init__
constructor, not to some convenience wrapper.
I agree here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about deletions? An option can come from any of those files, but they can't be deleted
Yes. In this case the user must set the value to the default value explicitly. I think this is very transparent as the user can track in the config files the value of each option. If we allow deleting an option in the config, it might be confusing for users where the different value comes from.
In our company, only a small group of people maintains the build system and tooling and configs (e.g., west). I hope, that it is a common enough use case for others, so that west itself should support it. My first idea was adding a command like An alternative would be to support a dropin directory like The important thing is: the generic configs may change anytime. In this case the new config values should be automatically applied if the users are using the default configs. If they have set some different value (on their own risk), their value shall be used. This scenario is perfectly fitting to the drop-in mechanism. I hope you understand roughly what we do. |
|
||
def print_path(self, args): | ||
config_path = self.config.get_path(args.configfile or LOCAL) | ||
if config_path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could this be None or empty? If it were, something went seriously wrong and it would be better to crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be None
in case of local configs if there is no .west
directory, so if west is run outside any workspace.
This makes sense for this command, e.g. if the user just opens a console in his home directory and runs west config --print-paths
to see which configs are expected.
Would you expect that it fails in this case, or shall it exit 0 and print no path for the local config (as it is now)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user requests the local path while outside any workspace, I would just let the tool crash. This is an obvious misuse of the tool, no need to test for that in the code.
I found a raise MalformedConfig('local configuration file not found')
somewhere else, that's another option.
In any case I would never exit 0, the tool should not succeed.
PS: please do not reply with a question and then hide the thread as "resolved" :-D
src/west/configuration.py
Outdated
for conf in sorted(dropins_dir.iterdir()): | ||
config.cp.read(dropins_dir / conf, encoding='utf-8') | ||
# finally overwrite with values from higher-prior actual config | ||
config.cp.read(path, encoding='utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a check at later point that the number of read configs is exactly 1. I can remove this check and rework the for loop and instead pass the list of multiple configs.
Yes please. Also, I think it's better to just add all the code to the __init__
constructor, not to some convenience wrapper.
Nevertheless: what should be the path attribute of the class in case of multiple configs? This is why I thought that requiring the actual config is the best idea.
path
should be the non-.d/
file. If it does not exist, then maybe it should be None? Not sure about that. Just like before, writes to a config file that does not exist yet should create that file, and deletions or appending in a non-existing main file should fail. If there was no test coverage for these then it should be added because the new "drop-ins" are going to make the corresponding logic a bit more complicated.
Writing to .d/
files should not be possible through the CLI, way too complicated. This must be documented.
Yes, I understand you want something like:
And get configuration updates when pulling the latest version of |
Few initial thoughts:
Let's consider the security impact as well please. I think @pdgendt 's example is a place where I can see how this is different than having an existing global or system config, but it makes me worried. We can assume that a ".d" directory in a home directory and anything in the system locations are controlled well by the user, but the workspace itself is an adversarial environment in my opinion -- that's why we can turn off extensions and why we do not parse extension command files unless the user specifically asks for one.
|
Is the 9f4dcf5 check not enough? If not, can you elaborate on the "threat model" you have in mind?
XDG_CONFIG_HOME sounds like a different feature than this. Are you saying it should come first? |
883bc9b
to
42bc614
Compare
@thorsten-klein The more I think about this, the more I dislike how this creates hidden paths for configuration bits. I was more in favor if updating/importing the configuration as was proposed with #847, which IMO has a clear intent and is a one-off action. One last alternative we did not bring up yet: we take inspiration from |
Exactly. That’s the main drawback with the approach from #847: we cannot easily tell whether a config was intentionally changed by the user, or if it’s simply outdated (or unintentionally modified). Therefore I prefer the dropin solution nowadays.
I’ve considered this as well. But does it really simplify things? Users could even create nested includes, which complicates matters further. Also, how should include order be handled if an
I share the same concern. This approach offers users a lot of flexibility, at the cost of potential intransparency. |
42bc614
to
3572a90
Compare
@marc-hb it's not enough. The threat model is basically the same as extension commands: threat actor wants to insert arbitrary code execution into something that happens when the user runs a west command in a way that is "difficult" to observe. If you look at the way extension commands work, we only read YAML files from each west project to determine what extension command implementations it provides. We do not parse any Python when loading the manifest. If the user runs an extension command, then we parse just the python required to run it, and run it since we assume the user is opting in to running the command. In #849 (comment), @pdgendt shows a west project with a configuration file that I understand is meant to be added to the west.configuration.Configuration object's state when an arbitrary west command, extension or otherwise, is run. Just to illustrate where this could be problematic, consider if the adversary sets In summary I think it is a non-starter to allow the workspace itself to set configuration options. |
@mbolivar I have no doubt The vast majority of users have their workspace in their home directory. If something evil can write to [*] EDIT: users can already do this sort of "dangerous" imports with a single command like |
I'm probably missing something -- when I see this:
I'm guessing you're saying that none of this is intended to create the symlink in config.d to app/shared, and the user has to do it by hand? |
Right, I would go even further and DISCOURAGE users to manually create such symlinks because they create a permanent bridge between a sensitive area (west configs) and updates from a less trusted one (a git repo). Instead, I would have in documentation something like: "after inspecting it very carefully, you make a (static!) copy of some .ini file into Just for the record I think the symlink example came from @pdgendt and not for @thorsten-klein . I think it was a nice explanation shortcut to explain the rationale but with hindsight it may have backfired a little :-) EDIT: I just found @thorsten-klein did mention the symlink idea first. Does not matter. Of course, nothing would stop "unsafe" users to create symlinks like this anyway but there's generally nothing that can be done for people driving over the speed limit without their seat belt. EDIT: and maybe it's OK if they're doing this only in a private and tightly controlled circuit. Does not matter as long as it's not officially recommended. |
I’m not entirely sure where the security concerns come from at this point. The existing Would it perhaps be better to make the drop-in feature explicitly opt-in? UPDATE:
But this adds even more complexity. So maybe we should go with |
c29931f
to
de473e3
Compare
There are (at least?) two important differences that I think concern @mbolivar (and myself now)
Thinking of it, this "dynamic" nature is also bad for reproducibility and other build properties. Imagine You can admittedly symlink
Works for me. |
bab29c2
to
8577c96
Compare
8577c96
to
4b0e0b6
Compare
This feature now requires explicit user activation. To enable it, users must set For example, running This design ensures on the hand hand complete transparency: users can clearly understand that only the I hope this feature is now ready 👍🏻 |
741eff6
to
32103d4
Compare
@thorsten-klein could you please provide an example of tools that already implement something similar to this? I am curious to see how others have done it and whether they've considered the security implications. Thanks! |
.d
directories.d
directories)
I’ve found a few examples of tools that support drop-in configurations:
However, the main difference between [alias]
update= forall -c "rm -rf ~" So IMO, the real security concern is not the drop-in config mechanism itself, but rather the alias feature. Nevertheless, I believe that |
.... today, that we know of, in built-in west Like I tried to explain, that was just:
|
@mbolivar / @carlescufi How do you want to move forward with this PR? Should I do anything else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete review. @thorsten-klein do not rush to address my comments before others agree with the general idea. I do not want you to waste time if the entire concept gets rejected.
In any case thanks for prototyping this, I think it proves this idea can fly.
|
||
def print_path(self, args): | ||
config_path = self.config.get_path(args.configfile or LOCAL) | ||
if config_path: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user requests the local path while outside any workspace, I would just let the tool crash. This is an obvious misuse of the tool, no need to test for that in the code.
I found a raise MalformedConfig('local configuration file not found')
somewhere else, that's another option.
In any case I would never exit 0, the tool should not succeed.
PS: please do not reply with a question and then hide the thread as "resolved" :-D
src/west/configuration.py
Outdated
|
||
def get_path(self, configfile: ConfigFile = ConfigFile.LOCAL): | ||
if configfile == ConfigFile.ALL: | ||
raise RuntimeError(f'{configfile} not allowed for get_path') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise RuntimeError(f'{configfile} not allowed for get_path') | |
raise RuntimeError(f'{configfile} not allowed for --print-path') |
cf = _InternalCF(path) if path else None | ||
if not cf: | ||
return None | ||
if not cf.path and not cf.dropin_paths: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bother creating an object when neither exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. It can be changed to
if not path:
return None
cf = _InternalCF(path)
src/west/configuration.py
Outdated
if self.path: | ||
self.cp.read(self.path, encoding='utf-8') | ||
if self.dropin_paths: | ||
self.dropin_cp.read(self.dropin_paths, encoding='utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain in the class _InternalCF:
"pydoc" why you need to keep these in separate configparser
objects. Because appending is tricky maybe?
At first sight, it looks like merging both here (with the appropriate precedence) would save a ton of code below.
Exerting precedence here would be a good place to let the user know with some INFO log statement.
Either way, the class _InternalCF
becomes a bit more complicated and needs some introduction explaining its data structures.
src/west/configuration.py
Outdated
|
||
def _write(self): | ||
if not self.path: | ||
raise WestNotFound('No config file exists that can be written') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not too difficult, it would be nice to print the missing path in the error message.
Also, if dropins are enabled then it would be nice to print a reminder that writing and appending ignores them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be refactored/simplified again since with current concept a config file must always exist (where the dropin config is actually enabled).
You are right, that a reminder would be nice. You prefer it only in case of writing to a config via command line, or always on reading (e.g. on stderr)?
src/west/configuration.py
Outdated
section, key = _InternalCF.parse_key(option) | ||
|
||
if section not in self.cp: | ||
if (section not in self.cp) or (key not in self.cp[section]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, maybe a reminder that dropins are ignored.
if section not in cp: | ||
cp.add_section(section) | ||
for key, value in contents.items(): | ||
cp[section][key] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind expanding a bit the pydoc for this function? It's pretty cryptic to me, sorry. Or just add some relevant references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I am not sure if this function is still neccessary. I have removed it completely (and its usage in main.py
), and the tests still pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment at the start of the function would indicate that west extension commands could rely on it?
# Internal API for main to use to maintain backwards
# compatibility for existing extensions using the legacy
# function-and-global-state APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Is it possible that there is no test existing for this? I am not sure how I can guarantee that I do not break something here, since I do not know the previous behavior.
So does it make sense that I adapt this function? Will such a function be kept forever or will it be removed at some time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answers to this sort of questions usually lie in git history. git blame
works but I tend to prefer tig blame
. GitHub's blame interface is surprisingly decent and it has direct links to the corresponding pull requests. Of course, this assumes the git history is usable with no too much whitespace pollution :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this? Just spotted by chance:
https://docs.zephyrproject.org/4.2.0/develop/west/west-apis.html#configuration-api
... recommended API to use since west v0.13.
and system), whereby all '.conf' files from each dropin directory are loaded in | ||
alphabetical order. | ||
For example: | ||
.west/config.d/basics.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this falls a bit short and misses at least the fact that writing and appending ignores dropins.
I'm going to be honest here, that I'm not keen on adding these configuration dropins. The configuration mechanism for west developers (and a bit for west users) is a lot more complicated. Future changes need be aware to not break any of this.
Mind you there's already a mechanism for extending functionality: extension commands. You can simply provide west extension commands that are "simple" wrapper commands. |
Thanks for the honest feedback! 👍🏻
I assume we’re not the first ones who’d like to share a company-wide west configuration within a repository, are we? That was just one example that came to mind. For instance, we’d also want to ensure that On the other hand, I also agree that this relatively small use case might not justify adding a large feature like configuration drop-ins. So I am also open for any other solutions for our use case. So, to summarize our use case again: At our company, we’d like to maintain a company-wide west configuration with default values. Without a “drop-in configs” feature, here’s how we tried to approach it: Approach 1: Approach 2: Do you see the challenge we’re facing? What would your suggested (existing) solution? |
As mentioned before in #847,
Dropins are part of the bigger "configuration as code" approach; they enable it. While not exactly it, the flexibility to move files in and out of |
Added new flag `config --print-path` (`-p`) for easy determination of the currently active configuration. The path is printed on console.
32103d4
to
f475a52
Compare
if conf.suffix in ['.conf', '.ini']: | ||
self.dropin_paths.append(self.dropin_dir / conf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why also have .ini
files included? This is not how it's documented above
All files inside a drop-in directory must use `.conf` extension and are
loaded in **alphabetical order**.
if section in self.cp and key in self.cp[section]: | ||
getter = config_getter | ||
elif section in self.dropin_cp and key in self.dropin_cp[section]: | ||
getter = dropin_getter | ||
else: | ||
raise KeyError(option) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a subtle difference here, but one nonetheless. Before we were able to determine a difference between a missing key or an entire missing section, we no longer have that difference.
if section not in cp: | ||
cp.add_section(section) | ||
for key, value in contents.items(): | ||
cp[section][key] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment at the start of the function would indicate that west extension commands could rely on it?
# Internal API for main to use to maintain backwards
# compatibility for existing extensions using the legacy
# function-and-global-state APIs.
f475a52
to
9ad9bdb
Compare
9ad9bdb
to
f624d2f
Compare
Added new flag `config --print-path` (`-p`) for easy determination of the currently active configuration. The path is printed on console. '--print-path' may display default configuration paths for system and global configurations. The local configuration must either exist or be specified via environment variable, since it cannot be determined otherwise.
added config command to list all config files and dropin files which are currently considered by west.
config files in acccording dropin directories are considered automatically. The dropin directory is named as the config itself but with a '.d' suffix. For example for local config the dropin directory is './west/config.d' and for global config it is '~/.westconfig.d' support config dropin directories config files in acccording dropin directories are considered automatically. The dropin directory is named as the config itself but with a '.d' suffix. For example for local config the dropin directory is './west/config.d' and for global config it is '~/.westconfig.d' Boolean option `config.dropins` must be set to `true` so that dropin configs for the according config type are considered. So if the option config.dropins=true in enabled in local config, only local dropin configs are used (but no system or global ones).
f624d2f
to
b9ee7fc
Compare
Successor of #847
Why?
In a big company there are often shared configs for build system and tooling (e.g., west and its configs).
Those generic configs must be applied first and contain some default or fallback values, if the user does not specify any other value in his user config. Remember, that users must always be able to overwrite values from those generic config files within their config.
Solution
The user can enable dropin configs. If enabled with option
config.dropins = true
, all.conf
and.ini
files that are placed inside according dropin folder (according<config>.d
folder next to the config) are applied right before the config.The dropin config folders are:
./west/config.d/*
for local dropin configs~/.westconfig.d/*
for global dropin configetc/westconfig.d/*
//usr/local/etc/westconfig.d/*
/%PROGRAMDATA%\west\config.d/*
for system dropin configAll dropin configs are loaded in alphabetical order before the config file itself is applied.
If the same option/key/value is defined in multiple dropin configs, the last one loaded wins (i.e., later file (e.g.
99.ini
) overrides earlier ones (e.g.01.conf
)).In the end, the config file itself is applied, so if a option's value is defined in the config it always overrides values from dropin configs.
Note: The dropin config mechanism respects the env variables (
WEST_CONFIG_LOCAL
/WEST_CONFIG_SYSTEM
/WEST_CONFIG_GLOBAL
), so that the dropin config folder becomes<WEST_CONFIG_xxx>.d
.Note: The dropin config feature must be enabled per config type (local, global, system) by setting option
config.dropins = true
in the according config. This is important to ensure security. For example: If the option is enabled in global config, only global dropin configs are used (not system or local ones). By this it is avoided that arbitrary dropin configs from any local workspaces are considered. As a result this also means, that dropin configs are never used if the according config file does not exist (since it would have to actively enable this option).A new
west config [--local|--global|--system] --print-path
command is supported. This command prints the path of the currently considered config file.Note: If no local config can be determined, the
--local
command does not output anything (it does NOT fail).A new
west config [--local|--global|--system] --list-paths
command is added, that prints a list of config files that are currently used bywest
commands in this workspace (existing config files and dropin config).