Add support for drop-in configuration directories #813

jrohel · 2023-08-10T08:51:23Z

Introduces a new method Base::load_config() to replace Base::load_config_from_file().

In the original implementation, the Base class constructor loads the default configuration from the distribution configuration file ("/usr/share/dnf5/libdnf.conf"). And later, the user configuration (by default "/etc/dnf/dnf.conf") was loaded by calling the Base::load_config_from_file() method.

New implementation:

The Base class constructor does not load any configuration file.
The application calls Base::load_config() method instead of Base::load_config_from_file().

The order of loading the configuration files by the method Base::load_config():

Creates an alphabetically sorted list of names of all configuration files in the distribution configuration drop-in directory ("/usr/share/dnf5/libdnf.conf.d") and the user configuration drop-in directory ("/etc/dnf/libdnf5.conf.d"). If the file with the same name is present in both drop-in directories, only the file from the user configuration drop-in directory is added to the sorted list. So, the distribution configuration drop-in file can be simply masked by creating a file with the same name in the user configuration drop-in directory.
Retrieves the configuration from the files in the sorted list. Follows the order. The configuration from the next file overrides the previous one - the last option value wins.
Loads the user configuration file (by default "/etc/dnf/dnf.conf"), if exists. If file does not exist and was explicitly requested by the user, the method throws an exeption.

Example configuration:

User configuration files:
/etc/dnf/dnf.conf
/etc/dnf/libdnf5.conf.d/20-user-settings.conf
/etc/dnf/libdnf5.conf.d/60-something.conf
/etc/dnf/libdnf5.conf.d/80-user-settings.conf

Distribution configuration files:
/usr/share/dnf5/libdnf.conf.d/50-something.conf
/usr/share/dnf5/libdnf.conf.d/60-something.conf
/usr/share/dnf5/libdnf.conf.d/90-something.conf

Resulting file loading order by default
("/usr/share/dnf5/libdnf.conf.d/60-something.conf" is skipped, masked by the user file "/etc/dnf/libdnf5.conf.d/60-something.conf"):

/etc/dnf/libdnf5.conf.d/20-user-settings.conf
/usr/share/dnf5/libdnf.conf.d/50-something.conf
/etc/dnf/libdnf5.conf.d/60-something.conf
/etc/dnf/libdnf5.conf.d/80-user-settings.conf
/usr/share/dnf5/libdnf.conf.d/90-something.conf
/etc/dnf/dnf.conf

jrohel · 2023-08-10T09:21:40Z

This solution uses a fixed list of drop-in directories. The file loading order algorithm is inspired by libeconf https://github.com/openSUSE/libeconf.

So it is a competing implementation to PR #741. Competing PR introduces support for including configuration files using a special comment directive in the configuration file.

Note to reviewer:
There is no need to waste time reviewing the implementation. What is important is whether we agree on this support for drop-in configuration directories from the user's point of view. Implementation is now a detail. Subsequently, I will rework it while adding support for "config-manager".

m-blaha · 2023-08-10T09:35:45Z

1. The `Base` class constructor does not load any configuration file.

2. The application calls `Base::load_config()` method instead of `Base::load_config_from_file()`.

Just a question - does it still make sense to have separate base.load_config() and base.setup() methods? Or can we squash the functionalities into only one call?
Or is the intention here to enable additional runtime configuration adjustments for API users?

Conan-Kudo · 2023-08-10T09:45:50Z

/etc/dnf/dnf.conf
/etc/dnf/libdnf5.conf.d/20-user-settings.conf
/usr/share/dnf5/libdnf.conf.d/50-something2.conf
/etc/dnf/libdnf5.conf.d/60-something2.conf
/etc/dnf/libdnf5.conf.d/80-user-settings.conf
/usr/share/dnf5/libdnf.conf.d/90-something2.conf
/etc/dnf/libdnf5.conf.d/99-config-manager.conf

I'm trying to understand this order, does this mean that DNF basically stops reading config if /etc/dnf/dnf.conf exists? My general understanding of how this should work is that /usr configs are read first, then /etc configs afterward will overwrite any set values from /usr. I'm slightly confused if this actually does that.

jrohel · 2023-08-10T10:14:56Z

@Conan-Kudo
The distribution config file /usr/share/dnf5/libdnf.conf is readed only if the user config file /etc/dnf/dnf.conf does not exist.

"libeconf" https://github.com/openSUSE/libeconf does a similar thing.
/usr/vendor/example.suffix is read only if /etc/example.suffix does not exist.

ppisar · 2023-08-10T10:17:37Z

Which configuration among dnf.conf and libdnf5.conf.d take precedence?

jrohel · 2023-08-10T10:19:27Z

@Conan-Kudo
All configuration files from drop-in directories are placed in one alphabetically sorted list by name. If a file with the same name is in both drop-in directories (distribution and user), only the one from the user (/etc/dnf/libdnf5.conf.d/) is used.

jrohel · 2023-08-10T10:24:06Z

@ppisar
As I described. The last value loaded wins (has the highest priority). The order can be seen in the example in the description. "/etc/dnf/dnf.conf" is loaded first and then the files from the "drop-in" directories. So the configuration from the "drop-in" directories has a higher priority than from "/etc/dnf/dnf.conf".

jrohel · 2023-08-10T10:30:05Z

I plan that configuration changes made by config-manager will be stored in /etc/dnf/libdnf5.conf.d/99-config-manager.conf files. That thanks to the prefix 99 they will load last and win.

Conan-Kudo · 2023-08-10T10:31:15Z

That makes sense.

So how will this extend to repo configuration? Because that's the big one that both Fedora and openSUSE would like to solve too.

jrohel · 2023-08-10T11:31:07Z

@Conan-Kudo

So how will this extend to repo configuration? Because that's the big one that both Fedora and openSUSE would like to solve too.

This PR only addresses loading the libdnf configuration. The "[main]" section in the configuration files.

What about repositories configuration? So far I have this idea:

"libdnf5" now loads repositories configuration from /etc/yum.repos.d, /etc/distro.repos.d directories.

We can:
a) add another drop-in directory (for example /etc/dnf/repos-override.d) to override repositories configuration.
b) or load repositories configuration overrides from drop-in configuration files read in this PR (we will also support sections other than "[main]").
c) or load the repositories configuration from the drop-in configuration directories mentioned in this PR, but the files will have a different extension than ".conf" (eg ".repo").

In any case, this would be to support overwriting the configuration of existing repositories. Creating new repositories using these drop-in directories will not be supported.

I am considering glob support for bulk modification of repositories parameters.
E.g. override proxy settings for all repositories whose "id" begins with "fedora":

[fedora*]
proxy=http://xyz.cc

But those are just my ideas. I don't know yet what I will implement or how we will agree in the team.

Conan-Kudo · 2023-08-10T11:49:06Z

So, in openSUSE, we have the following repo dirs: /etc/dnf/repos.d, /etc/yum.repos.d, and /etc/distro.repos.d.

Packaged configs go in the latter two directories, whereas the first path is where user-installed ones go.

I would like to change this to have packaged repository configs move to /usr and leave /etc entirely for user/admin stuff.

This would also include moving the packaged GPG keys to /usr, but that's not going to be hard anyway.

So the idea would be to have /usr/share/dnf5/yum.repos.d for packaged stuff, then /usr/share/dnf5/yum.repos.override.d for packaged overrides (e.g. epel-release turning on CRB in RHEL/CentOS).

For user-controlled things, in addition to the existing directory paths in /etc, having .override.d variants could make sense.

jrohel · 2023-08-10T12:20:05Z

@Conan-Kudo
Changing the repo dirs list is easy. This is given by the configuration value reposdir. And changing the default value in the code is also technically easy - changing the value of the constant. I write "technically" on purpose. I can't change randomly for compatibility. Here it will be more about agreeing what the right directories (paths) are.

In my description above it was about the introduction of "overrides". That is, configuration files that will not be able to define new repositories, but only modify existing ones.

But we are already running outside the scope of this PR. It handles the main configuration of libdnf (the "[main]" section). No repositories configuration.

jrohel · 2023-08-10T12:31:45Z

@m-blaha

Just a question - does it still make sense to have separate base.load_config() and base.setup() methods? Or can we squash the functionalities into only one call?

I'm not sure. I'd leave it as is for now.

include/libdnf5/base/base.hpp

j-mracek · 2023-08-21T11:29:52Z

libdnf5/base/base.cpp

+
+    // If the name of the configuration file was not specified, the files from the drop-in directories are also loaded.
+    // An alphabetically sorted list of all configuration files from the drop-in directories is created. If there is
+    // a file of the same name in both the user and distribution drop-in directories, only the user file is used.


I am not sure about this logic. If user will specify -C /, then all distribution and /etc overrides will be ignored. May I ask you for more details or some use case when this behavior is beneficial?

This is important to resolve

@j-mracek
-C, --cacheonly
You probably meant -c [config file], --config [config file]. Not yet implemented in dnf5. So -c user_config_file.
What should I do? The user has explicitly specified that they want configuration from this file. Overwriting it with configuration from drop-in directories is probably not what he wants. Therefore, in this case, the configuration from the drop-in directories is not loaded.
A compromise can still be made in this case. Change loading order. Load configuration files from drop-in directories and then from an explicitly user-defined file. But it will be so weird. The user changes the path to the main configuration file, but not the path to the drop-in directories.

j-mracek · 2023-08-21T12:11:24Z

Drop in configuration must be also documented in doc part. It doesn't have to be included in this PR, but at least it must be tracked in a separate issue.

j-mracek · 2023-08-21T15:05:47Z

Notes from discussion:

Requirements

When user specify configuration files, it should be not overridden by drop in configuration

Solutions

Do not load drop in overrides when user specify path to main configuration

allow to be more compatible with e-conf library when main configuration file is loaded first and then overrides
When user will override the default path with the same path, DNF might behave differently, because overrides will be not applied (best, skip_if_unavailable)
It will require to discourage user to use /etc/dnf/dnf.conf because the value migh get overridden by drop in configuration files

Load main configuration file as last

Main config will be the main config containing overrides with higher priority
Overrides will be used only for overrides hard-coded internals, but not for main config
We do not plan to ship any value in /etc/dnf/dnf.conf therefore there will be no difference with a default deployment
Cannot disable drop-in overrides without additional configuration option

Load main configuration files with higher priority

I think this solution (or solution 1) will be required when drop-in paths will be configurable.
Can be used as an upgrade from solution 2 when disablement of drop in overrides will be required

j-mracek · 2023-08-21T15:07:27Z

I would prefer to use Solution 2 - Load main configuration file as last, but there might be some draw back that I overlooked.

j-mracek · 2023-08-21T15:09:18Z

@keszybz May I ask you for your opinion?

ppisar · 2023-08-22T13:06:44Z

I agree with the requirement that a user's configuration has a higher priority / is loaded later than a distribution-wide configuration.
One can have --config-file option to replace /etc/dnf/dnf.conf with a user supplied file and --config-directory option to replace /etc/dnf/libdnf5.conf.d directory with a user supplied directory.
Should we stop loading distribution-wide directory if user passes --config-file? I don't think so. The reason is that "dnf" and "dnf --config-file /etc/dnf/dnf.conf", albeit reading the same files, would behave differently. Then how could a user inhibit loading distribution-wide configuration? I would add another option --no-implicit-config. That would simply disable loading any configuration, both from directories and files, distribution-wide and localhost-specific which were not explicitly pointed with --config-file and --config-directory.

As a side note, I think that introducing configuration locations of various priorities is not much helpful: Instead of fighting over a line number in a single configuration file, users and packagers will fight over the highest file name. Therefore I'd like to decrease the amount of various locations to a bare minimum. I understand that configuration directories are easier for populating by third-parties than editing a single file. Therefore I recommend removing a distribution-wide file /usr/share/dnf5/libdnf.conf completely. Regarding /etc/dnf/dnf.conf file, I would discourage user's from using it. The only remaining function of it would be a backward compatibility and a last resort place with guaranteed hightest priority.

ppisar · 2023-08-25T08:04:24Z

I think we will need to implement inhibition of system-wide configuration for DNF5 tests.

jrohel · 2023-08-28T09:59:53Z

Based on discussions in the team, I changed the loading order of the configuration files. I also edited the description accordingly.

j-mracek · 2023-09-01T11:45:13Z

include/libdnf5/base/base.hpp

    /// Call a function that loads the config file, catching errors appropriately
    void with_config_file_path(std::function<void(const std::string &)> func);

-    /// Loads main configuration from file defined by the current configuration.
+    /// @deprecated It will be removed in Fedora 40. It is redundant. It calls `load_config()`.


Proposing to remove It will be removed in Fedora 40, because the removal will have negative consequences on multiple projects.

@j-mracek OK, removed.

Introduces a new method `Base::load_config()` to replace `Base::load_config_from_file()`. In the original implementation, the `Base` class constructor loads the default configuration from the distribution configuration file ("/usr/share/dnf5/libdnf.conf"). And later, the user configuration (by default "/etc/dnf/dnf.conf") was loaded by calling the `Base::load_config_from_file()` method. New implementation: 1. The `Base` class constructor does not load any configuration file. 2. The application calls `Base::load_config()` method instead of `Base::load_config_from_file()`. The order of loading the configuration files by the method `Base::load_config()`: 1. Creates an alphabetically sorted list of names of configuration files in the distribution configuration drop-in directory ("/usr/share/dnf5/libdnf.conf.d") and the user configuration drop-in directory ("/etc/dnf/libdnf5.conf.d"). If the file with the same name is present in both drop-in directories, only the file from the user configuration drop-in directory is added to the sorted list. So, the distribution configuration drop-in file can be simply masked by creating a file with the same name in the user configuration drop-in directory. 2. Retrieves the configuration from the files in the sorted list. Follows the order. The configuration from the next file overrides the previous one - the last option value wins. 3. Loads the user configuration file (by default "/etc/dnf/dnf.conf"), if exists. If file does not exist and was explicitly requested by the user, the method throws an exeption. --------------------------------------------- Example configuration: User configuration files: /etc/dnf/dnf.conf /etc/dnf/libdnf5.conf.d/20-user-settings.conf /etc/dnf/libdnf5.conf.d/60-something.conf /etc/dnf/libdnf5.conf.d/80-user-settings.conf Distribution configuration files: /usr/share/dnf5/libdnf.conf.d/50-something.conf /usr/share/dnf5/libdnf.conf.d/60-something.conf /usr/share/dnf5/libdnf.conf.d/90-something.conf Resulting file loading order by default ("/usr/share/dnf5/libdnf.conf.d/60-something.conf" is skipped, masked by the user file "/etc/dnf/libdnf5.conf.d/60-something.conf"): 1. /etc/dnf/libdnf5.conf.d/20-user-settings.conf 2. /usr/share/dnf5/libdnf.conf.d/50-something.conf 3. /etc/dnf/libdnf5.conf.d/60-something.conf 4. /etc/dnf/libdnf5.conf.d/80-user-settings.conf 5. /usr/share/dnf5/libdnf.conf.d/90-something.conf 6. /etc/dnf/dnf.conf

j-mracek

LGTM

j-mracek · 2023-09-01T13:00:02Z

Failed tests looks like not related to PR.

birdie-github · 2023-09-12T13:41:29Z

Based on discussions in the team, I changed the loading order of the configuration files. I also edited the description accordingly.

Is this now the status quo?

Resulting file loading order by default
("/usr/share/dnf5/libdnf.conf.d/60-something.conf" is skipped,
masked by the user file "/etc/dnf/libdnf5.conf.d/60-something.conf"):

    /etc/dnf/libdnf5.conf.d/20-user-settings.conf
    /usr/share/dnf5/libdnf.conf.d/50-something.conf
    /etc/dnf/libdnf5.conf.d/60-something.conf
    /etc/dnf/libdnf5.conf.d/80-user-settings.conf
    /usr/share/dnf5/libdnf.conf.d/90-something.conf
    /etc/dnf/dnf.conf

If files are loaded in this exact order, then dnf.conf settings will override whatever the user has specified, IOW dnf.conf settings cannot be overridden.

j-mracek · 2023-09-15T14:20:45Z

Based on discussions in the team, I changed the loading order of the configuration files. I also edited the description accordingly.

Is this now the status quo?
Resulting file loading order by default
("/usr/share/dnf5/libdnf.conf.d/60-something.conf" is skipped,
masked by the user file "/etc/dnf/libdnf5.conf.d/60-something.conf"):

    /etc/dnf/libdnf5.conf.d/20-user-settings.conf
    /usr/share/dnf5/libdnf.conf.d/50-something.conf
    /etc/dnf/libdnf5.conf.d/60-something.conf
    /etc/dnf/libdnf5.conf.d/80-user-settings.conf
    /usr/share/dnf5/libdnf.conf.d/90-something.conf
    /etc/dnf/dnf.conf
If files are loaded in this exact order, then dnf.conf settings will override whatever the user has specified, IOW dnf.conf settings cannot be overridden.

DNF5 component will ship only empty /etc/dnf/dnf.conf therefore there is no requirement to override a content of the configuration file. The overrides will be used to overrides hard-coded configuration of the library.

jrohel added the ready for review label Aug 10, 2023

jrohel mentioned this pull request Aug 10, 2023

config: Add support for included configuration files #741

Closed

jrohel mentioned this pull request Aug 15, 2023

Add support for repos configuration overrides #820

Merged

jrohel force-pushed the feature/config_dropin_dirs branch from 6b6781b to aa5640c Compare August 16, 2023 06:22

j-mracek self-assigned this Aug 21, 2023

j-mracek reviewed Aug 21, 2023

View reviewed changes

include/libdnf5/base/base.hpp Show resolved Hide resolved

j-mracek reviewed Aug 21, 2023

View reviewed changes

jrohel force-pushed the feature/config_dropin_dirs branch from aa5640c to 9dcccde Compare August 21, 2023 11:54

jrohel force-pushed the feature/config_dropin_dirs branch from 9dcccde to f75e3d5 Compare August 28, 2023 09:51

jrohel force-pushed the feature/config_dropin_dirs branch from f75e3d5 to d62f669 Compare August 29, 2023 12:07

pkratoch mentioned this pull request Aug 31, 2023

Add tests for drop-in configuration directories rpm-software-management/ci-dnf-stack#1373

Merged

j-mracek reviewed Sep 1, 2023

View reviewed changes

jrohel added 3 commits September 1, 2023 14:07

Add drop-in configuration directories to package

04eacfe

Base: Remove unused configuration loading methods

2f39d84

jrohel force-pushed the feature/config_dropin_dirs branch from d62f669 to 2f39d84 Compare September 1, 2023 12:08

j-mracek approved these changes Sep 1, 2023

View reviewed changes

j-mracek added this pull request to the merge queue Sep 1, 2023

Merged via the queue into rpm-software-management:main with commit 4658312 Sep 1, 2023

Conan-Kudo mentioned this pull request Sep 11, 2023

Document new repository and configuration paths #883

Closed

birdie-github mentioned this pull request Sep 12, 2023

/etc/dnf/{dnf.d|conf.d}/*.conf overrides support rpm-software-management/dnf#1914

Closed

jrohel mentioned this pull request Nov 7, 2023

DNF config-manager Plugin #405

Closed

3 tasks

Conan-Kudo mentioned this pull request Dec 17, 2023

Tracking upstream projects that do not support hermetic-usr for configuration uapi-group/specifications#76

Open

37 tasks

keszybz mentioned this pull request Dec 17, 2023

RFE: allow listing protected packages under /usr #1107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for drop-in configuration directories #813

Add support for drop-in configuration directories #813

jrohel commented Aug 10, 2023 •

edited

Loading

jrohel commented Aug 10, 2023

m-blaha commented Aug 10, 2023

Conan-Kudo commented Aug 10, 2023

jrohel commented Aug 10, 2023

ppisar commented Aug 10, 2023

jrohel commented Aug 10, 2023 •

edited

Loading

jrohel commented Aug 10, 2023

jrohel commented Aug 10, 2023 •

edited

Loading

Conan-Kudo commented Aug 10, 2023

jrohel commented Aug 10, 2023 •

edited

Loading

Conan-Kudo commented Aug 10, 2023 •

edited

Loading

jrohel commented Aug 10, 2023

jrohel commented Aug 10, 2023

j-mracek Aug 21, 2023

j-mracek Aug 21, 2023

jrohel Aug 21, 2023

j-mracek commented Aug 21, 2023

j-mracek commented Aug 21, 2023

j-mracek commented Aug 21, 2023

j-mracek commented Aug 21, 2023

ppisar commented Aug 22, 2023

ppisar commented Aug 25, 2023

jrohel commented Aug 28, 2023

j-mracek Sep 1, 2023

jrohel Sep 1, 2023

j-mracek left a comment

j-mracek commented Sep 1, 2023

birdie-github commented Sep 12, 2023

j-mracek commented Sep 15, 2023

Add support for drop-in configuration directories #813

Add support for drop-in configuration directories #813

Conversation

jrohel commented Aug 10, 2023 • edited Loading

Example configuration:

jrohel commented Aug 10, 2023

m-blaha commented Aug 10, 2023

Conan-Kudo commented Aug 10, 2023

jrohel commented Aug 10, 2023

ppisar commented Aug 10, 2023

jrohel commented Aug 10, 2023 • edited Loading

jrohel commented Aug 10, 2023

jrohel commented Aug 10, 2023 • edited Loading

Conan-Kudo commented Aug 10, 2023

jrohel commented Aug 10, 2023 • edited Loading

What about repositories configuration? So far I have this idea:

Conan-Kudo commented Aug 10, 2023 • edited Loading

jrohel commented Aug 10, 2023

jrohel commented Aug 10, 2023

j-mracek Aug 21, 2023

Choose a reason for hiding this comment

j-mracek Aug 21, 2023

Choose a reason for hiding this comment

jrohel Aug 21, 2023

Choose a reason for hiding this comment

j-mracek commented Aug 21, 2023

j-mracek commented Aug 21, 2023

Requirements

Solutions

j-mracek commented Aug 21, 2023

j-mracek commented Aug 21, 2023

ppisar commented Aug 22, 2023

ppisar commented Aug 25, 2023

jrohel commented Aug 28, 2023

j-mracek Sep 1, 2023

Choose a reason for hiding this comment

jrohel Sep 1, 2023

Choose a reason for hiding this comment

j-mracek left a comment

Choose a reason for hiding this comment

j-mracek commented Sep 1, 2023

birdie-github commented Sep 12, 2023

j-mracek commented Sep 15, 2023

jrohel commented Aug 10, 2023 •

edited

Loading

jrohel commented Aug 10, 2023 •

edited

Loading

jrohel commented Aug 10, 2023 •

edited

Loading

jrohel commented Aug 10, 2023 •

edited

Loading

Conan-Kudo commented Aug 10, 2023 •

edited

Loading