Summary: | Refactor Koha configuration (koha-conf.xml) | ||
---|---|---|---|
Product: | Koha | Reporter: | David Cook <dcook> |
Component: | Architecture, internals, and plumbing | Assignee: | 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
+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, ...) (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. (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. (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. 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. Julian, after reviewing your comments, I used TOML in a small Perl project, and found it very nice to work with. +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. (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 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. (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... 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 (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 (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. 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. 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... (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... 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. (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. 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. (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. 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... (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. (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. |