Bug 48357 - listener modules created with new listener API always displayed with status=0
listener modules created with new listener API always displayed with status=0
Status: RESOLVED WONTFIX
Product: UCS
Classification: Unclassified
Component: Listener (univention-directory-listener)
UCS 4.3
Other Linux
: P5 normal (vote)
: ---
Assigned To: UCS maintainers
UCS maintainers
:
Depends on:
Blocks: 53824
  Show dependency treegraph
 
Reported: 2018-12-16 08:52 CET by Daniel Tröder
Modified: 2022-03-03 11:37 CET (History)
2 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 3: Simply Wrong: The implementation doesn't match the docu
Who will be affected by this bug?: 3: Will affect average number of installed domains
How will those affected feel about the bug?: 1: Nuisance – not a big deal but noticeable
User Pain: 0.051
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:
troeder: Patch_Available+


Attachments
simple listener module (984 bytes, text/x-python)
2018-12-16 08:52 CET, Daniel Tröder
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Tröder univentionstaff 2018-12-16 08:52:00 CET
Created attachment 9779 [details]
simple listener module

When a listener module created with the new listener API is installed and the u-d-l is restarted the status shown with "univention-directory-listener-ctrl status" is always "0".

The listener did however successfully initialize the module, as can be seen in the logfile.

The status file in /var/lib/univention-directory-listener/handlers/$NAME does contain a "3", but "univention-directory-listener-ctrl status" still shows a "0".

Attached is a simple listener module to reproduce this.
Comment 1 Philipp Hahn univentionstaff 2018-12-16 12:17:05 CET
<http://docs.software-univention.de/developer-reference-4.3.html#listener:handler> specified that "name" must be declared as the top-level symbol in the *Python* module.
This is parsed by the *shell* script "management/univention-directory-listener/src/listener-ctrl", which uses the regular expression
  ^name\s*=\s*['\"]([^'\"]+)['\"]\s*(#.*)?$

Using anything else breaks the status output.
Comment 2 Daniel Tröder univentionstaff 2018-12-18 11:29:34 CET
I created a script that prints the configuration of both new and old API listener modules.

Because the old way is must faster (no Python import required), it is only used as a fall back.

[dtroeder/48357_listener_name 3b8a94241c] Bug #48357: get name of listener modules using new API
Comment 3 Daniel Tröder univentionstaff 2019-08-19 16:08:42 CEST
Alternative:

$ apt install pcre2-utils

$ pcre2grep -M -o3 "ListenerModuleHandler\).*(\n|.)*class Configuration.*(\n|.)*name\s*=\s*[\"'](.+)[\"']" <LISTENERMODULE>

Tested with 
* univention-radius.py
* /usr/share/doc/univention-directory-listener/examples/listener_module_template.py
Comment 4 Ingo Steuwer univentionstaff 2021-05-14 15:42:46 CEST
This issue has been filed against UCS 4.3.

UCS 4.3 is out of maintenance and many UCS components have changed in later releases. Thus, this issue is now being closed.

If this issue still occurs in newer UCS versions, please use "Clone this bug" or reopen it and update the UCS version. In this case please provide detailed information on how this issue is affecting you.
Comment 5 Philipp Hahn univentionstaff 2022-03-02 07:31:58 CET
As an alternative we can just remove "name" or make it optional and default to the basename of the UDL module instead; currently there is a single exception, where they do not match. There's no technical reason to keep "name":

cd /usr/lib/univention-directory-listener/system&&for l in *.py;do echo "${l%.py} $(sed -rne "s/^name\s*=\s*([\"'])(.+)\1\s*(#.*)?$/\2/p" "$l")";done

app_attributes  app_attributes
bind    bind
faillog faillog
gencertificate  gencertificate
hosteddomains   hosteddomains
keytab-member   keytab-member
keytab  keytab
ldap_extension  ldap_extension
ldap_server     ldap_server
license_uuid    license_uuid
nagios-client   nagios-client
nfs-homes       nfs-homes
nfs-shares      nfs-shares
nscd    nscd_update <<<<<<<<<<<<<<<<<<<<
nss     nss
pkgdb-watch     pkgdb-watch
portal_groups   portal_groups
portal_server   portal_server
quota   quota
udm_extension   udm_extension
umc-service-providers   umc-service-providers
univention-admin-diary-backend  univention-admin-diary-backend
univention-saml-groups  univention-saml-groups
univention-saml-idp-config      univention-saml-idp-config
univention-saml-servers univention-saml-servers
univention-saml-simplesamlphp-configuration     univention-saml-simplesamlphp-configuration
well-known-sid-name-mapping     well-known-sid-name-mapping
Comment 6 Daniel Tröder univentionstaff 2022-03-02 15:01:38 CET
Optional (with fallback to $(basename filename)) sounds good to me.
Comment 7 Arvid Requate univentionstaff 2022-03-02 16:15:43 CET
The "name" variable is often used while issuing log messages.
But I agree that can be decided by the specific module.
Comment 8 Daniel Tröder univentionstaff 2022-03-03 08:41:10 CET
(In reply to Arvid Requate from comment #7)
> The "name" variable is often used while issuing log messages.
> But I agree that can be decided by the specific module.

As I understand it, the global variable 'name' is part of the API.
Thus I think the modules should still have such a global variable (and use it for logging).

The listener API is a Python API and the new listener API creates the variable dynamically at runtime.
The univention-directory-listener-ctrl script is shell script that does not understand Python and thus has to workaround that natural limitation → fallback to $(basename filename).
Comment 9 Philipp Hahn univentionstaff 2022-03-03 11:37:40 CET
(In reply to Daniel Tröder from comment #8)
> (In reply to Arvid Requate from comment #7)
> > The "name" variable is often used while issuing log messages.
> > But I agree that can be decided by the specific module.
> 
> As I understand it, the global variable 'name' is part of the API.

But for what purpose?
There is no technical requirement to have it and complicates writing new modules as you have to specify just another obscure value for unknown reason.
Contrary it is very confusing for those modules where "name" != "basename(__file__, ".py")" as then the output of `univention-directory-listsner-ctrl status` uses "name", which currently is not set by the UDL API, which is what this Bug is all about.
If a sensible default can be derived automatically from the file name I'm all for removing that strange requirement.

> Thus I think the modules should still have such a global variable (and use it for logging).

That is completely okay: of the 51 UDL modules in UCS
- only 1 (nscs) explicitly uses a different name
- 14 use "name" for debugging
  base/univention-pam/well-known-sid-name-mapping.py
  mail/univention-mail-dovecot/dovecot-shared-folder.py
  mail/univention-mail-dovecot/dovecot.py
  management/univention-directory-logger/directory_logger.py
  management/univention-directory-manager-modules/listener/udm_extension.py
  management/univention-ldap/listener/ldap_extension.py
  management/univention-management-console/umc-service-providers.py
  management/univention-self-service/listener/selfservice-invitation.py
  services/univention-nfs/nfs-homes.py
  services/univention-nfs/nfs-shares.py
  services/univention-samba/samba-privileges.py
  services/univention-samba/samba-shares.py
  services/univention-samba4/samba-shares.py
  services/univention-samba4/samba4-idmap.py
They could/should use `name = os.path.basename(__file__)` to be even more useful and consistent.
- the other 36 modules never use "name" expect to satisfy the requirement that they must have one.

> The listener API is a Python API and the new listener API creates the variable dynamically at runtime.
> The univention-directory-listener-ctrl script is shell script that does not understand Python and thus has to workaround that natural limitation →
> fallback to $(basename filename).

The following patch is part of my larger queue at <https://git.knut.univention.de/univention/ucs/-/commits/phahn/29420-listener-doc>:
--- management/univention-directory-listener/src/listener-ctrl
+++ management/univention-directory-listener/src/listener-ctrl
@@ -103,6 +103,7 @@ modules () {
                [ -f "$module_file" ] || continue
                rv=
                module_name=$(sed -rne "s/^name\s*=\s*['\"]([^'\"]+)['\"]\s*(#.*)?$/\1/p;T;q" "$module_file") || rv="$FAIL"
+               [ -z "$module_name" ] && module_name=$(basename "$module_file" '.py')
                module_state=$(cat "$STATE_DIR/$module_name" 2>/dev/null) &&
                        [ "$module_state" -eq 3 ] ||
                        rv="$FAIL"

Let's move further discussion to Bug #53824 which is still OPEN.