Skip to content
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

Support multiple configuration files #829

Merged
merged 1 commit into from Jan 13, 2022
Merged

Conversation

aplanas
Copy link
Contributor

@aplanas aplanas commented Jan 12, 2022

And other refactoring of the config module

@aplanas
Copy link
Contributor Author

aplanas commented Jan 12, 2022

Seems that the config module have extra functions that are helpers, and not related with the configuration file. How would you feel if I extracted those functions from there?

@THS-on
Copy link
Member

THS-on commented Jan 12, 2022

Moving the helpers from config into the right places makes sense to me.

Removing the licensing header needs to be discussed, because we currently this header in every file.

@aplanas
Copy link
Contributor Author

aplanas commented Jan 12, 2022

Removing the licensing header needs to be discussed, because we currently this header in every file.

Dropped this commit. I will create an issue for the discussion.

@THS-on
Copy link
Member

THS-on commented Jan 12, 2022

If KEYLIME_CONFIG is set the config is still merged with the other ones, right?
I would expect it fully overwrite the config.

@aplanas
Copy link
Contributor Author

aplanas commented Jan 12, 2022

If KEYLIME_CONFIG is set the config is still merged with the other ones, right? I would expect it fully overwrite the config.

They are merged, you can see it in the tests (5860d8c#diff-45fa5f8530eb815c0cf606032e587adedb04dc70e59b203535d51c44654cfc4dR52)

This will enable the separation of a system wide configuration (/usr/etc/keylime.conf), an admin overwrite (/etc/keylime.conf) and a user overwrite ($HOME/.config/keylime.conf)

The overwrites can add new key/values and replace previous set key/values.

@THS-on
Copy link
Member

THS-on commented Jan 12, 2022

That changes the current behaviour, but I think this change is a good one and should not break deployments (outside of testing).

It might be nice to have the option to load only one configuration for testing etc.

@mpeters
Copy link
Member

mpeters commented Jan 12, 2022

If KEYLIME_CONFIG is set the config is still merged with the other ones, right? I would expect it fully overwrite the config.

This would be my expectation as well. I like the "stacked" nature of the default config paths, but I would assume that something like KEYLIME_CONFIG would be used for testing or other automated setups that would like to completely control the whole config. But I'm willing to be convinced otherwise.

@aplanas
Copy link
Contributor Author

aplanas commented Jan 12, 2022

That changes the current behaviour, but I think this change is a good one and should not break deployments (outside of testing).

The exposed API is the same, and the changes outside the module are minimal.

It might be nice to have the option to load only one configuration for testing etc.

For tests it is there already. Check test code, is adjusting the list of config files to load for validation.

For a more general case, like for example adding a -c /path/my_keylime.conf new command line parameter, we will need more changes. I plan to do them in a different commit, as I will need this option too.

@aplanas
Copy link
Contributor Author

aplanas commented Jan 12, 2022

This would be my expectation as well. I like the "stacked" nature of the default config paths, but I would assume that something like KEYLIME_CONFIG would be used for testing or other automated setups that would like to completely control the whole config. But I'm willing to be convinced otherwise.

As commented, the new tests added for the config module are already updating the KEYLIME_CONFIGS parameter. The 3rd party tests outside keylime can be adjusted easily, as the API is the same.

Copy link
Member

@THS-on THS-on left a comment

Choose a reason for hiding this comment

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

LGTM. I'm fine with KEYLIME_CONFIG also to be stacking as long we introduce -c /path/to/config as an option to manually overwrite it.

Currently Keylime read the config for all the services from the same
file, in /etc/keylime.conf.

This commit support the load of multiple configuration files, and
merge the contents together.  This will enable the separation of a
system wide configuration file in /usr/etc/keylime.conf, and a admin
owerwrite in /etc/keylime.conf.

Signed-off-by: Alberto Planas <aplanas@suse.com>
@mpeters mpeters merged commit e4ad1f3 into keylime:master Jan 13, 2022
@aplanas aplanas deleted the fix_config branch January 13, 2022 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants