opnsense / core

OPNsense GUI, API and systems backend
https://opnsense.org/
BSD 2-Clause "Simplified" License
3.39k stars 757 forks source link

openvpn: invokeInterfaceRegistration() is not present in OpenVPN ServiceController #7869

Closed Monviech closed 2 months ago

Monviech commented 2 months ago

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

Describe the bug

In OpenVPN, invokeInterfaceRegistration() seems not to be triggered under all circumstances.

It seems to be missing in the ServiceController.

This might need a cleanup (discussed with @fichtner)

git grep invokeInterfaceRegistration
src/opnsense/mvc/app/controllers/OPNsense/Base/ApiMutableServiceControllerBase.php:    protected function invokeInterfaceRegistration()
src/opnsense/mvc/app/controllers/OPNsense/Base/ApiMutableServiceControllerBase.php:            if ($this->invokeInterfaceRegistration()) {
src/opnsense/mvc/app/controllers/OPNsense/IPsec/Api/ServiceController.php:    protected function invokeInterfaceRegistration()

Additionally these could also call the protected function directly:

git grep invoke.registration                                                          
src/opnsense/mvc/app/controllers/OPNsense/Base/ApiMutableServiceControllerBase.php:                $backend->configdRun('interface invoke registration');
src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/GroupController.php:            (new Backend())->configdRun('interface invoke registration');
src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/GroupController.php:            (new Backend())->configdRun('interface invoke registration');
src/opnsense/mvc/app/controllers/OPNsense/Wireguard/Api/ServiceController.php:        $backend->configdRun('interface invoke registration');
src/opnsense/service/conf/actions.d/actions_interface.conf:[invoke.registration]
AdSchellevis commented 2 months ago

I don't think it helps to move the configd event to a separate callout, the only reason the invokeInterfaceRegistration() exists is because it's part of a complete service sequence. In cases where we can implement ApiMutableServiceControllerBase, we can benefit from the full package, but the openvpn case is a bit special and wouldn't benefit a lot from reusing the service controller.

For reference, invokeInterfaceRegistration() usage:

https://github.com/opnsense/core/blob/415b280959b816e755c25e8b1df58462e737c8e1/src/opnsense/mvc/app/controllers/OPNsense/Base/ApiMutableServiceControllerBase.php#L192-L194