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

Permissions module reload_delta is not scoped to individual tables. #3318

Closed
whosgonna opened this issue Dec 28, 2022 · 7 comments
Closed

Permissions module reload_delta is not scoped to individual tables. #3318

whosgonna opened this issue Dec 28, 2022 · 7 comments

Comments

@whosgonna
Copy link
Contributor

Description

The permissions module has two reload RPC commands, permissions.addressReload which reloads the address table, and permissions.trustedReload, which reloads the trusted table.

The module parameter reload_delta exists to prevent concurrent reloading of a single table:

3.25. reload_delta (int)

The number of seconds that have to be waited before executing a new RPC reload. By default there is a rate limiting of maximum one reload in five seconds.

If set to 0, no rate limit is configured. Note carefully: use this configuration only in tests environments because executing two RPC reloads of the same table at the same time can cause to kamailio to crash.

If one of the reload rpc commands is issues (i.e. permissions.addressReload), then the OTHER reload command (permissions.trustedReload) will be blocked, despite acting against two separate tables.

Troubleshooting

Reproduced the issue.

Reproduction

(DB config for permissions module omitted for brevity)

loadmodule "rtimer.so"
loadmodule "permissions.so"
loadmodule "jsonrpcs"

modparamx("rtimer", "timer", "name=permissions_reload;mode=0;interval=300")
modparam("rtimer", "exec", "timer=permissions_reload;route=RELOAD_PERMISSIONS")


route[RELOAD_PERMISSIONS] {
    xnotice("Reloading trusted records\n");
    jsonrpc_exec('{"jsonrpc": "2.0", "method": "permissions.trustedReload", "id": 1}');
    xalert("jsonrpc response code: $jsonrpl(code) - the body is: $jsonrpl(body)\n");

    xnotice("Reloading addresses\n");
    jsonrpc_exec('{"jsonrpc": "2.0", "method": "permissions.addressReload", "id": 1}');
    xalert("jsonrpc response code: $jsonrpl(code) - the body is: $jsonrpl(body)\n");

}

Debugging Data

The above config yields output like this:

2(8) NOTICE: <script>: Reloading trusted records
2(8) ALERT: <script>: jsonrpc response code: 200 - the body is: {
    "jsonrpc":      "2.0",
    "result":       "Reload OK",
    "id":   1
}
2(8) NOTICE: <script>: Reloading addresses
2(8) ERROR: permissions [rpc.c:42]: rpc_check_reload(): ongoing reload
2(8) ALERT: <script>: jsonrpc response code: 500 - the body is: {
    "jsonrpc":      "2.0",
    "error":        {
        "code": 500,
        "message":      "ongoing reload"
    },
    "id":   1
}

Possible Solutions

Ideally each reload would be tracked separately. This could possibly involve separate reload_delta parameters for each table (although it seems excessive to change the interface).

It's also possible that this behavior is by design, in which case, clarification of the documentation for the reload_delta parameter would be nice.

Additional Information

Problem occurs in kamailio 5.6.2. Likely within all version since the reload_delta parameter was added.

  • Operating System:

Tested in Alpine, but OS seems irrelevant.

@abalashov
Copy link
Contributor

abalashov commented Dec 28, 2022

You would be correct in that rpc_trusted_reload() and rpc_address_reload() call the same rpc_check_reload() function, which uses a single timer. So, calling a reload on either of those would reset the timer.

But why do you want to reload them separately? Does it matter?

Also:

The module parameter reload_delta exists to prevent concurrent reloading of a single table:``

That's not accurate. That's not what the documentation says its purpose is.

@whosgonna
Copy link
Contributor Author

I'm calling them separately because there's no function to call them together. ;)

@henningw
Copy link
Contributor

The reload_delta functionality was added because it was observed that excessive reloads (e.g. every few seconds) could cause issues under load, as mentioned in the docs. A workaround for your cfg is to add two individual timers for both reload commands, but this might cause some small drift between the two timer executions. Regarding changing the implementation or extending the documentation, a pull request could be of course created.

@abalashov
Copy link
Contributor

Ah, okay. Personally, it seems to me the real problem is in the documentation's claim that reload_delta creates separation between "RPC reloads", vs. no clear conception of what an "RPC reload" is. In thinking about it generally, I would expect such a reload to encompass both tables.

@miconda
Copy link
Member

miconda commented Jan 5, 2023

Probably the developer thought only one of the tables are used in a Kamailio instance, they kind of overlap a lot in purpose and functionality. Personally I use only address table, I don't think I ever initiated a Kamailio deployment using the trusted table.

Anyhow, because this kind of functionality seems to be needed in other places, I added support in core for rate-limiting the execution of RPC commands to specific time interval that can be set via the new core parameter rpc_exec_delta. It is not done automatically for all commands, only for those that are exported with RPC_EXEC_DELTA flag in the C code. For now I updated the exports of RPC reload commands for tls and permissions module.

The core value is checked first, being done when the rpc command is searched to be executed, so if it is lower than per module restriction (like in permissions), it hits first.

The rate limiting is per command, but the time interval value is global, which should be (good) enough for now, imo. If more flexibility would be needed, one more thing that can be added is to make rpc_exec_delta updatable via rpc as well.

The new functionality is available only in devel version, it is not a candidate to be backported because it touches many places.

Testing and feedback is appreciated. I think this issue can be closed if no other comments/proposals come in soon.

@whosgonna
Copy link
Contributor Author

Thank you. I'll try to create a testing situation around this.

@miconda
Copy link
Member

miconda commented Jan 6, 2023

OK, open a new issue if any problem is found.

@miconda miconda closed this as completed Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants