<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [ ] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail -->
kamctl now sources the kamctlrc from only one location if available in this order:
1. same folder as kamctl, 2. {intstall_prefix}/etc/kamailio/, 3. ~/.kamctlrc 4. /etc/kamailio/.
If no kamctlrc is found, it continues just like before. Should it stop?
#### Issue description `kamctl` uses env variables from all the config files above, and retains the env value of the latest config that declares it.
Let's say we have 3 kamctlrc files right now:
``` # first config file (same folder that kamctl resides) # {prefix_install}/sbin/kamctlrc ... SIP_DOMAIN=kamailio.org DBENGINE is commented ... ```
``` # second config file # /etc/kamailio/kamctlrc ... SIP_DOMAIN=kam02.tst.nbg.gilawa.net DBENGINE=MYSQL ... ```
``` # third config file # {prefix_install}/etc/kamailio/kamctlrc ... SIP_DOMAIN is commented DBENGINE=MYSQL ... ```
Running `{install_prefix}/sbin/kamctl` (before changes in order) it prints :
``` Loading config file /home/xenofon/kamailio-source-install/sbin/kamctlrc Loading config file /etc/kamailio/kamctlrc Loading config file /home/xenofon/kamailio-source-install/etc/kamailio//kamctlrc SIP_DOMAIN env var is kam02.tst.nbg.gilawa.net DBENGINE env var is MYSQL ``` suggesting that env variables from multiple sources are gathered and overwritten in cases. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3594
-- Commit Summary --
* kamctl: Source only one kamctlrc
-- File Changes --
M utils/kamctl/kamctl (23)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3594.patch https://github.com/kamailio/kamailio/pull/3594.diff
The scope of loading all configs in cascade is to allow overwriting on specific options with home config, not to have the entire config there. Practically the /etc config is the one with all options and then add a user home config only to change specific options (e.g., have a different mysql user/password in the home config, but all the other options of kamctl to be from /etc/kamailio/kamctlrc). To my knowledge, sourcing many shell (config) files is common, like bash loading /etc/profile and then ~/.profile.
I am fine to have a new option to load only one file, the first found, but current behaviour has to be kept the default.
Many linux software packages uses a different behaviour, if one configuration file is found, its using this and no merge of the values is done. Bash profile loading does it differently indeed, but I would argue that kamctl is more like a tool. But if people depend on this current behaviour, fine.
The main issue that motivated the change is that, if you are executing the Kamailio as non-root user, it will fail to start as it can't read the /etc/kamailio/kamctlrc. This should surely be fixed.
This behaviour is likely there from the beginning, or at least more than 10-15 years old. I do use it, although not very often, to put in home kamctlrc some new queries that can be run via db command, relying on etc file for the rest of the options.
I pushed a commit to check if kamctlrc is readable.
Thank you
Hello @miconda,
Indeed this script goes way back and i am still getting familiar with the codebase. You are suggesting that `/etc/kamailio/kamctl` should be and is the base config file and that is fine.
I see some flows as it right now.
1) What if user has installed kamailio not from official packages but another way in a non standard location. That file is nowhere to be found. Hence, all the other locations will be used to source with order described already (messy i believe). Even `bash` that you have mention, has the base `/etc/profile` and then **one of** `~/.bash_profile`,` ~/.bash_login`,`~/.profile` sourced according to [man bash](https://linux.die.net/man/1/bash ), `invocation section`.
2) The current code has the base of kamctl to be the one residing in the **same folder as kamctl**, ie `{install_prefix}/sbin/kamctlrc`, when `/etc/kamailio/kamctl` should be the base. If right now , I have a config in the same folder that kamctl is (the user config as you say and none of the other two exist, `{install_prefix}/etc/kamilio/kamctlrc` and `~/.kamctlrc`), my options would be overridden by /etc/ one when in fact i placed this to overwrite it. This is due to the search order of kamctl in the script.
Just my observations and thank you for the readable commit!
@xkaraman: usually the applications are designed to work with standard installation from a package or source tree. That's what kamctl tries to look after as well. It is somehow expected that when one does own custom installation may have to tune it properly in many places.
I am not against improving things of about how is done now, I said that your PR breaks existing behaviour, which is common to other pieces of linux software, but also used as it is for many years.
Then, likely a typo of yours, but `/etc/kamailio/kamctlrc` instead of `/etc/kamailio/kamctl` is the expected base config file for kamctl, being the one deployed with package-based installation -- just for sake of clarity.
To your point 1. I somehow responded above, at least partially -- now how many files one app looks for and loads is probably specific to it, I wanted to highlight that sourcing many files is not uncommon. If you want to elaborate more I can do it.
To your 2., can you point where `{install_prefix}/sbin/kamctlrc` is taken as base config?
It is somehow expected that when one does own custom installation may have to tune it properly in many places.
Surely, i understand that. This specific script `kamctl` offers an easy way to accomplish that by searching some standard paths it may find the `kamctlrc`. Maybe we can document, that it searches all the paths and uses all of the config files found and in which order.
I said that your PR breaks existing behaviour
Yes, unfortunately, this breaks the existing behaviour of users and completely understand that. If we can't find a way to preserve the previous behaviour. We can close this altogether.
Then, likely a typo of yours, but `/etc/kamailio/kamctlrc` instead of `/etc/kamailio/kamctl` is the expected base config file for kamctl, being the one deployed with package-based installation -- just for sake of clarity.
True that was a type. I meant `/etc/kamailio/kamctlrc`.
To your 2., can you point where {install_prefix}/sbin/kamctlrc is taken as base config?
I based the above finding on these lines. https://github.com/kamailio/kamailio/blob/18b23187dd89e3b36c0dd95d4796484bef...
The first path that is searched and used (sourced) is the `kamctlrc` found in the same folder as `kamctl` that runs (either in the standard location or not). As you can see in my description of this pr, ``` # Running {install_prefix}/sbin/kamctl (before changes in order) it prints :
Loading config file /home/xenofon/kamailio-source-install/sbin/kamctlrc Loading config file /etc/kamailio/kamctlrc Loading config file /home/xenofon/kamailio-source-install/etc/kamailio//kamctlrc SIP_DOMAIN env var is kam0.XXXX.net DBENGINE env var is MYSQL ```
I missed that part, but indeed is looking for a kamctlrc in the same folder as kamctl. Digging in memory, the actual purpose for it is to allow running/operating kamailio from source directory (just compile, not install), which is convenient when developing. That's also why loadmodule looks not only for `modulename.so` file in `mpath` folders, but also for `modulename/modulename.so`.
If you are in kamailio sources folder, you can run `./utils/kamct/kamctl` and then, as running in a devel environment, the latest `kamctlrc` of the source tree is expected to be the base config. However, the order can be changed, if most of the people find it differently to be more natural. Practically the scope it is to have a base config and a way for each user to amend only specific options, not to have full a config again.
Usually is expected to have the default config as deployed by install (in /etc/ or /usr/local/etc) when running a globally deployed kamctl or by presence in source three when running a devel environment, plus user specific options (e.g., user/password for database) in a user home config.
As we kind of try to move from shell-based kamctl/kamdbctl to kamcli, I looked at it and it does a similar approach, relying on the fact that Python's `configparser.read()` has support to get a list of (config) files:
- https://docs.python.org/3/library/configparser.html#configparser.ConfigParse...
Just to give example of a pretty established API for sourcing many files to config.
Usually is expected to have the default config as deployed by install (in /etc/ or /usr/local/etc) when running a globally deployed kamctl or by presence in source three when running a devel environment, plus user specific options (e.g., user/password for database) in a user home config
I can understand that. I'm not against that of course. Maybe better documentation of how `kamctl` loads its configuration and order would be the best then. ie [(main documentaion](https://www.kamailio.org/wikidocs/tutorials/getting-started/main/#tools) of `kamctl` does not provide any information other than `kamctl` loads its config from `kamctlrc` found in same folder of `kamailio.cfg`). When first read that, I made the assumption that only one file is loaded, but after checking the source code that is not the case.
I may also have missed something in other documentation pages, so please point them to me if you happen to have one.
@xkaraman pushed 1 commit.
4eee8b1ebbbc029ad7bfbaa52cb718fa3d9ded39 Fix source order
I see that the patch introduced a bunch of `echo` lines, which could be good for debugging but can be a pain when using kamctl many times, specially when one wants to divert the output to a file or pipe for further processing (e.g., output of a kamctl rpc command which should be just the jsonrpc output that then can be processed with `jq`).
Either remove them or replace with `mdbg` (an internal function for debug messages).
@xkaraman pushed 1 commit.
6d2315552665d082eddd4bb3fb4ed4b3134640e8 kamctl: Remove echo
Removed them as suggested.
@xkaraman: the PR has conflicts with the master branch, which introduced a while ago checks for access rights of the rc files.
@xkaraman pushed 0 commits.
Closed #3594.
Reopened #3594.
Merged #3594 into master.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Xenofon Karamanos