Bug 26790

Summary: Refactor Koha configuration (koha-conf.xml)
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: Bugs List <koha-bugs>
Status: REOPENED --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: julian.maurice, m.de.rooy, magnus, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28278
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14162
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:

Description David Cook 2020-10-22 22:30:28 UTC
I propose moving /yazgfs/config out of koha-conf.xml and into koha-conf.yml (or even several different *-conf.yml files).

The majority of the /yazgfs child nodes are actually configuration values for zebrasrv (ie Zebra). We should respect /yazgfs as Zebra configuration and leave it alone.

I also propose we move from XML to YAML, as it's more human-friendly and it's easier to parse into the data structure that we need. XML::Simple, which is an obsolete module according to its creator, is currently used to parse koha-conf.xml and it's behaviour can be unpredictable. We should ideally eliminate our usage of it.
Comment 1 Julian Maurice 2020-10-23 09:55:26 UTC
+1 for moving Koha config out of zebra config file
+1 for not using XML

But -1 for using YAML. I know we already use YAML for everything in Koha, but YAML has a lot of disadvantages (for instance https://www.arp242.net/yaml-config.html, or https://hitchdev.com/strictyaml/why/implicit-typing-removed/)
One concrete problem we had in Koha recently was that some MARC field tags starting with 0 were parsed as octal numbers. (040 becomes 32 but 099 remains 99)
I think that this bug might be a good opportunity to consider alternatives (HJSON, JSON5, TOML, ...)
Comment 2 David Cook 2020-10-25 23:55:07 UTC
(In reply to Julian Maurice from comment #1)
> +1 for moving Koha config out of zebra config file
> +1 for not using XML
> 
> But -1 for using YAML. I know we already use YAML
> for everything in Koha, but YAML has a lot of disadvantages

I'm not finding those links particularly persuasive. 

Using Perl's YAML module, I wasn't able to reproduce a single issue from those links. Plus many of the proposed issues should be easily avoidable anyway. 

> One concrete problem we had in Koha recently was that some MARC field tags
> starting with 0 were parsed as octal numbers. (040 becomes 32 but 099
> remains 99)

Can you point me to a bug report for that one? That sounds very interesting. I couldn't reproduce that problem based on the description.

> I think that this bug might be a good opportunity to consider alternatives
> (HJSON, JSON5, TOML, ...)

I've never heard of HJSON or JSON5, but I have used TOML a couple times. It was easy to use, and the documentation is good.

It looks like libtoml-perl is available in the Debian repositories, so that would be OK. 

I'm not particularly wedded to YAML. I'd be fine with TOML.
Comment 3 Julian Maurice 2020-10-26 08:50:25 UTC
(In reply to David Cook from comment #2)
> (In reply to Julian Maurice from comment #1)
> > +1 for moving Koha config out of zebra config file
> > +1 for not using XML
> > 
> > But -1 for using YAML. I know we already use YAML
> > for everything in Koha, but YAML has a lot of disadvantages
> 
> I'm not finding those links particularly persuasive. 
> 
> Using Perl's YAML module, I wasn't able to reproduce a single issue from
> those links. Plus many of the proposed issues should be easily avoidable
> anyway.
> 
> > One concrete problem we had in Koha recently was that some MARC field tags
> > starting with 0 were parsed as octal numbers. (040 becomes 32 but 099
> > remains 99)
> 
> Can you point me to a bug report for that one? That sounds very interesting.
> I couldn't reproduce that problem based on the description.

Can't remember the exact details and can't reproduce either... it was about the ES mappings file, MARC fields being changed when exporting/re-importing...

But the real problem IMO is that YAML is implementation dependent.
For instance, this document:

foo: 040

can be interpreted as { "foo": "040" } or { "foo": 32 } depending on how you read it. (Try this https://onlineyamltools.com/convert-yaml-to-json)

Even in Perl where implicit typing seems to be disabled by default, it can be turned on anywhere in the code ($YAML::Syck::ImplicitTyping = 1), so you can never be sure of what will happen.
Comment 4 David Cook 2020-10-26 22:11:18 UTC
(In reply to Julian Maurice from comment #3)
> Can't remember the exact details and can't reproduce either... it was about
> the ES mappings file, MARC fields being changed when
> exporting/re-importing...
 
Hmm maybe someone else will surface it later.

> But the real problem IMO is that YAML is implementation dependent.

That's a good point.

> For instance, this document:
> foo: 040
> 
> can be interpreted as { "foo": "040" } or { "foo": 32 } depending on how you
> read it. (Try this https://onlineyamltools.com/convert-yaml-to-json)

Thanks for that link. That is interesting.

However, it seems lazy to write it as 'foo: 040'. Surely it would be safer to explicitly write it as "foo": "040", since we know that they are strings. (That being said, since Perl is weakly typed, I wonder if Perl would try to convert that internally into a number...)

> Even in Perl where implicit typing seems to be disabled by default, it can
> be turned on anywhere in the code ($YAML::Syck::ImplicitTyping = 1), so you
> can never be sure of what will happen.

In theory, we should know, but I reckon that Koha is big enough that it can be easy to lose track of how the YAML has been set up across the board.
Comment 5 David Cook 2020-10-26 22:11:48 UTC
I have other priorities/interests at the moment, but why don't we go with TOML for this refactoring? There's no harm in trying it out.
Comment 6 David Cook 2020-10-28 02:16:23 UTC
Julian, after reviewing your comments, I used TOML in a small Perl project, and found it very nice to work with.
Comment 7 Martin Renvoize 2021-05-04 12:47:26 UTC
+1 for moving Koha config out of zebra config file
+1 for not using XML

As for the TOML vs YAML debate, I don't really have a horse in the race.  The YAML support in Perl seems to have reached a suitable level of maturity so I feel it's a fairly safe bet really.. but I can understand some of the issues Julian presents.  TOML is certainly a bit simpler in scope, and we shouldn't really be using the full scope of YAML in our configs anyway really so I'd suggest we shouldn't really be falling into many of the traps it presents.. How mature is the TOML support in perl... I note David looked at the TOML module (which is pure perl) and hasn't been updated since 2016.. does that mean it's perfect or it's just not maintained?  There's also a recent TOML::XS module on cpan.. but will this get packaged and is it likely to be around for a while and maintained.

Either way.. I think the splitting out of Zebra config from the config file is a great first step.
Comment 8 Julian Maurice 2021-07-01 12:47:24 UTC
(In reply to Martin Renvoize from comment #7)
> How mature is the TOML support in perl... I note David looked
> at the TOML module (which is pure perl) and hasn't been updated since 2016..
> does that mean it's perfect or it's just not maintained?  There's also a
> recent TOML::XS module on cpan.. but will this get packaged and is it likely
> to be around for a while and maintained.
This looks problematic :/
Maybe it's safer to stick with YAML for now.

> Either way.. I think the splitting out of Zebra config from the config file
> is a great first step.
+1
Comment 9 Tomás Cohen Arazi 2021-11-29 11:17:20 UTC
My idea with bug 14162 was to move this into a file that would be manipulated using a CLI tool, or the UI, and we can maintain  with our packaging tools.

The main problem I faced, and raise here: it would be great to have a way to split the configuration with includes, for example: the ldap config could be a separate file.
Comment 10 David Cook 2021-11-30 06:20:04 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> My idea with bug 14162 was to move this into a file that would be
> manipulated using a CLI tool, or the UI, and we can maintain  with our
> packaging tools.
 
I find this idea attractive.

> The main problem I faced, and raise here: it would be great to have a way to
> split the configuration with includes, for example: the ldap config could be
> a separate file.

That's a good point. I was envisioning configuration broken up to multiple files like that as well, although I figured the key in the main configuration file would be something like "ldap_config_file": "/etc/koha/sites/SITE/ldap_config.yml".

I suppose an alternative might be something like... /etc/koha/sites/SITE/conf.d/ldap_config.yml and LDAP code looks for a file called "ldap_config.yml" in a configuration directory pointed to by the main /etc/koha/sites/SITE/koha-conf.yml file?

I suppose in both scenarios you need some kind of mechanism for loading the config (either at start-up or on demand...)

Food for thought...
Comment 11 Julian Maurice 2021-11-30 08:57:56 UTC
Yet another alternative would be to replace the main configuration file by environment variables.
This is the approach recommended by https://12factor.net/config and can make working with containers (docker/k8s/...) easier.

Of course not everything can be put inside env vars, so it would probably be needed to have env vars that point to files. For instance LDAP_CONFIG_FILE=/etc/koha/sites/SITE/conf.d/ldap_config.yml

Another advantage: no need to worry about the main config file format, or an include mechanism

My 2 cents
Comment 12 Tomás Cohen Arazi 2021-11-30 09:14:36 UTC
(In reply to Julian Maurice from comment #11)
> Yet another alternative would be to replace the main configuration file by
> environment variables.
> This is the approach recommended by https://12factor.net/config and can make
> working with containers (docker/k8s/...) easier.
> 
> Of course not everything can be put inside env vars, so it would probably be
> needed to have env vars that point to files. For instance
> LDAP_CONFIG_FILE=/etc/koha/sites/SITE/conf.d/ldap_config.yml
> 
> Another advantage: no need to worry about the main config file format, or an
> include mechanism

My vote would be:
- Use separate files with expected names (config.yaml, shibboleth.yaml, etc)
- No includes
- Have C4::Context->config rebuild the 'old structure we already have, and deal with a different new handling on a separate bug (Koha::Config singleton implementation?)
- Have a way for C4::Context->config to override any configuration if an env variable exists. e.g. KOHA_DATABASE_NAME should override the relevant entry.

My two cents as well
Comment 13 David Cook 2021-11-30 22:49:47 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> My vote would be:
> - Use separate files with expected names (config.yaml, shibboleth.yaml, etc)
> - No includes
> - Have C4::Context->config rebuild the 'old structure we already have, and
> deal with a different new handling on a separate bug (Koha::Config singleton
> implementation?)
> - Have a way for C4::Context->config to override any configuration if an env
> variable exists. e.g. KOHA_DATABASE_NAME should override the relevant entry.
> 
> My two cents as well

I was thinking something like this as well. 

The other day I was setting up a Dockerized program on AWS ECS using Fargate, and I used a combination of file-based configuration with environmental variables passed by the orchestrator (ie AWS ECS) to override particular relevant entries. 

Using Fargate was very cost effective (the bill is $0 since it's a very short lived program) but it did make it more challenging in terms of providing customized configuration files, since you don't have any control of the Docker host. The de facto ECS approach there seemed to be to have an additional custom container for pulling in configuration files from external sources and then mounting them into to shared volume with the application container.
Comment 14 David Cook 2021-11-30 22:53:12 UTC
I've only used k8s once to play around on my desktop, so I don't know it very well, but I think it has more robust methods than AWS ECS for deploying configuration files and managing secrets. 

But overall I think developing in a way that makes it easier to use containers is a great idea.
Comment 15 David Cook 2021-12-01 22:31:38 UTC
Btw, just sharing an experience using the https://metacpan.org/pod/TOML library. 

It takes a TOML Boolean false and converts it into a Perl string that says "false".

I'm regretting using TOML and that library on a little program of mine. Glad that I didn't invest much in it...
Comment 16 Julian Maurice 2021-12-02 08:10:11 UTC
(In reply to David Cook from comment #15)
> Btw, just sharing an experience using the https://metacpan.org/pod/TOML
> library. 
> 
> It takes a TOML Boolean false and converts it into a Perl string that says
> "false".

Thanks for sharing.

The TOML module seems to be the worst implementation available. No release since 2017, only supports v0.4 of TOML spec, and of course this boolean horror :)

TOML::Tiny and TOML::XS seems to be better alternatives, both supporting the latest version of the spec, and their latest releases were this year, in august.
So, Perl support for TOML has improved since comment 7. And the latest version of TOML::Tiny is packaged in Debian testing, which is promising.
Now, is TOML suited for Koha configuration ? That's another question...
Comment 17 David Cook 2021-12-09 03:40:25 UTC
I was thinking more today about how this all could work and I'm thinking something like this:

my $mq_crud_conf = Koha::Config->relative_file('mq_crud.yml');

The "relative" here means relative to $ENV{'KOHA_CONF'} as the configuration directory.

Now I'm pondering whether we build in L1 and L2 caching into that method or if we have multiple different methods:

Koha::Config->relative_file()
Koha::Config->relative_file_cached()

It might be a good idea to have multiple methods because sometimes you don't need to waste time setting up the cache. (For instance, when uploading a plugin, you might only need "relative_file", but when calling a plugin over X records in a row you'd definitely want to use "relative_file_cached".)

But curious what other people think.
Comment 18 David Cook 2021-12-09 03:55:44 UTC
(In reply to David Cook from comment #17)
> 
> my $mq_crud_conf = Koha::Config->relative_file('mq_crud.yml');
> 

Oh but this wouldn't work with Tomas's ideas, which were very good.

(In reply to Tomás Cohen Arazi from comment #12)
> My vote would be:
> - Use separate files with expected names (config.yaml, shibboleth.yaml, etc)
> - No includes
> - Have C4::Context->config rebuild the 'old structure we already have, and
> deal with a different new handling on a separate bug (Koha::Config singleton
> implementation?)
> - Have a way for C4::Context->config to override any configuration if an env
> variable exists. e.g. KOHA_DATABASE_NAME should override the relevant entry.

What about using AUTOLOAD?

my $mq_crud = Koha::Config->mq_crud();

If the "mq_crud" doesn't exist in an internal data structure, it could then do the Koha::Config->relative_file('mq_crud.yml') or looks for KOHA_MQ_CRUD in an environmental variable to load the configuration from file and then saves it. (This wouldn't use Koha::Cache at all and would require a process restart to refresh configuration like with koha-conf.xml.)

The only downside of that would be that you couldn't use an env var override for lower level config... like Koha::Config->mq_crud()->destination;

Although you probably could do something clever... where Koha::Config->mq_crud() returns a Koha::Config::Entry object which could use AUTOLOAD or have dynamically created methods during object creation time. 

There are options... which of course makes it tougher to choose.
Comment 19 David Cook 2022-12-06 06:23:09 UTC
I don't think we'll get consensus on this one, so closing for now.

Locally, I've started to create other configuration files to try to keep customizations out of koha-conf.xml. So far so good.
Comment 20 David Cook 2023-08-09 06:23:37 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> My vote would be:
> - Use separate files with expected names (config.yaml, shibboleth.yaml, etc)
> - No includes
> - Have C4::Context->config rebuild the 'old structure we already have, and
> deal with a different new handling on a separate bug (Koha::Config singleton
> implementation?)
> - Have a way for C4::Context->config to override any configuration if an env
> variable exists. e.g. KOHA_DATABASE_NAME should override the relevant entry.
> 
> My two cents as well

These are still good ideas. 

I think we could do them with some very small changes to Koha/Config.pm.
Comment 21 David Cook 2023-08-09 06:36:06 UTC
Tried to use Koha::Config->read_from_file($file) to create a data structure I wanted using XML but couldn't do it. Going to be much nicer to use YAML...
Comment 22 Marcel de Rooy 2024-04-23 13:37:43 UTC
(In reply to David Cook from comment #0)
> I propose moving /yazgfs/config out of koha-conf.xml and into koha-conf.yml
> (or even several different *-conf.yml files).
> 
> The majority of the /yazgfs child nodes are actually configuration values
> for zebrasrv (ie Zebra). We should respect /yazgfs as Zebra configuration
> and leave it alone.
> 
> I also propose we move from XML to YAML, as it's more human-friendly and
> it's easier to parse into the data structure that we need. XML::Simple,
> which is an obsolete module according to its creator, is currently used to
> parse koha-conf.xml and it's behaviour can be unpredictable. We should
> ideally eliminate our usage of it.

Does this report include thinking about how we could secure more sensitive data in our koha-conf file? Like DB password, encryption key, etc. Note e.g. discussion on encryption keys in koha-conf on 34976.
Comment 23 David Cook 2024-04-24 00:04:39 UTC
(In reply to Marcel de Rooy from comment #22)
> Does this report include thinking about how we could secure more sensitive
> data in our koha-conf file? Like DB password, encryption key, etc. Note e.g.
> discussion on encryption keys in koha-conf on 34976.

I hadn't thought of that specifically, but there's no reason we couldn't think about that too.

I think that sensitive data in koha-conf.xml is fairly secure at the moment. Only root and the Koha instance user can read koha-conf.xml (at least with the Debian packages). 

But are you thinking of something more like "docker secret"? Or AWS Secrets Manager/HashiCorp Vault? 

Something where the secrets are encrypted at rest?

I think the tough part with Koha is there are so many moving pieces. Lots of daemons, lots of cronjobs. All which need access to the secret/sensitive data, which gets more complicated if you have to deal with secret keepers running in a separate process, which need security of their own too. 

--

It could be interesting to build multiple ways to get secret/sensitive data. 

With "docker secret", maybe some way to say KOHA_CONF=/run/secrets/koha-conf.yml. Symlinks might also be workable here.

I think secret keepers would be harder, but not impossible. 

--

Overall, this is probably a good place to be thinking about these things.