Univention Bugzilla – Bug 38938
Define default UCR values in package.univention-config-registry-variables
Last modified: 2021-08-19 11:23:15 CEST
We currently have two commonly used styles of defining defaults for UCR variables: a) explicit: ucr set this/variable?"some default" (e.g. in postinst) b) implicit: configRegistry.get("this/variable", "some default") (in python) Both have advantages and drawbacks: a) good: One can find it easily. b) bad: One can't find it easily. a) bad: Changing defaults on update is a mess. b) good: Changing defaults on update is a breeze. I propose to define the default as part of .univention-config-registry-variables: That would have these advantages: 1. Default values can bee shown as part of the UCR variable documentation and (documentation == true) automatically. 2. Changing defaults on update is a breeze. 3. One can find it easily.
That would be very nice! How would we implement this for variables which have another variable in their defautl value, e.g.: sso_fqdn = configRegistry.get('ucs/server/sso/fqdn', 'ucs-sso.%s' % configRegistry.get('domainname')) Should we allow UCR patterns there? default=ucs-sso.@%@domainname@%@ ? This could introduce recursion. Maybe we shouldn't support this special case.
Definitely this would be bad: "ucs-sso.$(domainname)s", where the format string references are required to be in UCR. >>> print "ucs-sso.$(xeyes)s" % {} ucs-sso.$(xeyes)s Which in turn would lead to code injection: bash# echo "ucs-sso.$(xeyes)s" So that would be a bad idea. @%@ seems to be better. Maybe we can allow simple replacement, but not recursion.
(In reply to Arvid Requate from comment #2) > Definitely this would be bad: "ucs-sso.$(domainname)s", where the format > string references are required to be in UCR. > > >>> print "ucs-sso.$(xeyes)s" % {} > ucs-sso.$(xeyes)s > > Which in turn would lead to code injection: > > bash# echo "ucs-sso.$(xeyes)s" "You get what you deserve - shell is not simple": That is why the correct quoting for UCR is important eval "$(ucr shell)" And you are even safe when doing something like this: ucr set var/with/references='ucs-sso.$(domainname)s' # no "s here! foo="$(ucr get var/with/references)" # "s are not strictly required here echo "$foo" echo $foo # also not vulnerable as "parameter and variable expansion" and "command substitution" are two distinct phases and are NOT recursive. On the other hand when using "eval" or »"« instead of »'« with »ucs set key=val« you are doomed. > @%@ seems to be better. +1 > Maybe we can allow simple replacement, but not recursion. Recursion might not be necessary, but is do-able when required.
(In reply to Philipp Hahn from comment #3) > > Maybe we can allow simple replacement, but not recursion. > > Recursion might not be necessary, but is do-able when required. In theory it can happen: Variable: foo Default: @%@bar@%@ Variable: bar Default: @%@blub@%@ Variable: blub Default: @%@foo@%@ → loop, if all of them are unset.
(In reply to Florian Best from comment #4) > (In reply to Philipp Hahn from comment #3) > > > Maybe we can allow simple replacement, but not recursion. > > > > Recursion might not be necessary, but is do-able when required. > > In theory it can happen: ... I'm not an idiot! But do require that functionality or is one-time evaluation enough?
Sorry, I caused confusion by mixing up shell vs python: We could do this and it would not interfere with shell: * "ucs-sso.%(domainname)s"
(In reply to Arvid Requate from comment #6) > Sorry, I caused confusion by mixing up shell vs python: > > We could do this and it would not interfere with shell: > > * "ucs-sso.%(domainname)s" This might be sub-optimal as '%(...)s' is expanded by Python and will raise tracebacks in Python code, if positional arguments are used or if named attributes are not defined: $ python -c 'print("%(foo)s" % ("bar",))' Traceback (most recent call last): File "<string>", line 1, in <module> TypeError: format requires a mapping $ python -c 'print("%(foo)s" % dict(boom="bar"))' Traceback (most recent call last): File "<string>", line 1, in <module> KeyError: 'foo' Adding recursion (even 1 level only) needs more thinking: Do we *really* need it or is this only a wish-list-feature?
(In reply to Philipp Hahn from comment #7) > Adding recursion (even 1 level only) needs more thinking: Do we *really* > need it or is this only a wish-list-feature? I don't think we need it and we should not implement a difficult logic there. But we need a way to handle the behavior if such definements exists. E.g. should it raise an error, or should it be replaced (e.g. with a empty space) or not at all?
> This might be sub-optimal as '%(...)s' is expanded by Python and will raise tracebacks in Python code, if positional arguments are used or if named attributes are not defined: See Comment 5. That was obviously kind of the point, implying use of "% ucr" everywhere. But that's probably too much work, to adjust all templates to follow this convention.
Created attachment 10161 [details] patch (git:fbest/38938-ucr-default-values) Attached is a patch which evaluates "Default=Value" to the $package.cfg ($package.univention-config-registry-variables) UCR Variable sections. It took me some approaches to implement this, the best approach won: Added a new UCR layer with the lowest priority, it's a file /etc/univention/base-defaults.conf containing all UCR variable default patterns. The base-defaults.conf is written by univention-install-config-registry (i.e. ucr register, ucr unregister) on package installation. The default value allows UCR-Patterns like %@% (and even @!@, maybe we should restrict that?) All interfaces therefor support this, e.g. ucr shell, ucr dump, ucr get, etc. and the Python interface. "ucr info" now also displays the default value (pattern). The UMC module for UCR also displays the default value (pattern). But it cannot add default values. ucr unset currently still prints a message: W: apache2/startsite is still set in scope "default" Should we remove this? There is error handling for recursion-conditions in the default value pattern, when e.g. foo=@%@bar@%@ and bar=@%@foo@%@ just an empty string is returned! As UCR does not use univention-debug I cannot log this situation.
Merged the patch into UCS 5.0: univention-config-registry (15.0.0-5) Bug #38938: UCR variables may now specify default values Bug #38938: show default value in ucr info Bug #38938: evaluate %@% expressions in default values of UCR variables Bug #38938: add layer for default UCR variables univention-management-console-module-ucr (9.0.0-1) Bug #38938: display default value in UMC module changelog-5.0-0.xml Changelog Bug #38938
(In reply to Arvid Requate from comment #0) > We currently have two commonly used styles of defining defaults for UCR > variables: <https://xkcd.com/927/> > + description: _('Pattern of the default value of the UCR variable if it is unset'), TODO: Please re-phrase: At least I don't understand it. BUG: lib/config.c was not updated. [feature/ucs5] c4b3ae9498 fixup! Changelog Bug #38938 [feature/ucs5] cdb6e4996c fixup! Bug #38938: add layer for default UCR variables [feature/ucs5] 3edf7c25dd fixup! Bug #38938: show default value in ucr info
(In reply to Philipp Hahn from comment #12) > BUG: lib/config.c was not updated. Thank you for the early QA! This is going to be harder, I think. The default values may contain @!@ patterns, to replace variables with the content of others. Should we do this in C manually? Should we run "ucr filter" instead, like the lib also does this for "ucr set"?
(In reply to Florian Best from comment #13) > (In reply to Philipp Hahn from comment #12) > > BUG: lib/config.c was not updated. … > Should we run "ucr filter" instead, like the lib also does this for "ucr > set"? I implemented this approach basically, see git:9bffa5c87f6571a9b79458b4449f8af6e0921d84
(In reply to Florian Best from comment #13) > (In reply to Philipp Hahn from comment #12) > > BUG: lib/config.c was not updated. > This is going to be harder, I think. > The default values may contain @!@ patterns, to replace variables with the content of others. > Should we do this in C manually? The C implementation is lacking in many cases and keeping them in-sync is somehow a waste of time. It is only used is a very few cases: $ apt-cache rdepends libunivention-config0 libunivention-config0 Reverse Depends: libunivention-ldb-modules univention-policy-tools univention-directory-listener libunivention-policy0 libunivention-license0 UDL currently uses it to check some settings for *each* transaction. DHCPd uses lib-policy and probably also uses it a lot. As both cases are somehow performance critical I would otherwise say to "fork() and exec(python ucr)", but this is not going to work (i fear). Maybe we should just document that the default layer is not supported by the C implementation at all (or just the feature for recursive substitution). Neither is promising as we now have 3½ different locations for default values. > Should we run "ucr filter" instead, like the lib also does this for "ucr set"? `ucr set` is not called that often, so there the fork()+exec() is okay. But doing it for get() is overkill. My gut feeling is to just not support recursive substitution with C. (In reply to Philipp Hahn from comment #12) > [feature/ucs5] 3edf7c25dd fixup! Bug #38938: show default value in ucr info I has to revert that because my analysis was wrong, because the loop: from univention.config_registry.backend import ConfigRegistry # noqa E402 python/univention/config_registry/__init__.py:37: in <module> from univention.config_registry.frontend import ( # noqa F401 python/univention/config_registry/frontend.py:44: in <module> import univention.config_registry_info as cri python/univention/config_registry_info.py:37: in <module> import univention.config_registry.backend as ucr
(In reply to Philipp Hahn from comment #15) > As both cases are somehow performance critical I would otherwise say to > "fork() and exec(python ucr)", but this is not going to work (i fear). As these services don't use any UCR variables with defaults ATM it wouldn't be a performance problem to use fork(). But then we would need to check that this is never changed. > Maybe we should just document that the default layer is not supported by the > C implementation at all (or just the feature for recursive substitution). > Neither is promising as we now have 3½ different locations for default > values. I think we should support it. > > Should we run "ucr filter" instead, like the lib also does this for "ucr set"? > > `ucr set` is not called that often, so there the fork()+exec() is okay. But > doing it for get() is overkill. > > My gut feeling is to just not support recursive substitution with C. In the menatime I created an implementation which uses `fork() + ucr filter` (git:9bffa5c8). But I dropped this again and replaced it with the evaluation of the @%@-expressions. Maybe you can review this again and help me fixing WIP-issues like maximum recursion, etc.?
The C implementation has been added as well. The description of default values in the UCR UMC module has been adjusted as well. univention-config-registry (15.0.0-4) f618c0e35d99 | Bug #38938 UCR: add default layer 4a21b1dd8cdc | Bug #38938 UCR: UCR variables may now specify default values 3690aebc3604 | Bug #38938 UCR: show default value in ucr info 7f2854c9680d | Bug #38938 UCR: evaluate %@% expressions in default values of UCR variables 2363f423c477 | Bug #38938 UCR: add layer for default UCR variables univention-management-console-module-ucr (9.0.0-1) ca4fa9bdcafb | Bug #38938 UCR: display default value in UMC module changelog-5.0-0.xml 88b42b0558b2 | Changelog Bug #38938
* please also adapt the manual / developer documentation
(In reply to Sönke Schwardt-Krummrich from comment #18) > * please also adapt the manual / developer documentation Done in: 5873e012e226 | Bug #38938: describe default UCR values in developer reference
FIXED: 5873e012e226 → d805f1fb4620 DevDoc OK: 88b42b0558b2 changelog OK: ca4fa9bdcafb UMC ucr TODO: 2363f423c477 7f2854c9680d 3690aebc3604 4a21b1dd8cdc f618c0e35d99
I had to add another commit: Moved the registration of UCR variable defaults before the register actions to ensure templates can use the default values during postinst. univention-config-registry (15.0.6-2) 32fc29989169 | Bug #38938 UCR: register default variable before calling handlers
I had to adjust the registration logic again. The UCR templates weren't committed again if a default value changed. This is now done on register (package installation) and on unregister (package removal). The change causes that the default variables are also logged to /var/log/univention/config-registry.replog. If this is not wished two lines would remove this behavior. univention-config-registry (15.0.7-1) 1fef0aef99fb | fixup! Bug #38938 UCR: fix calling UCR handlers on (un)registration of new default values 9fc27554da45 | Bug #38938 UCR: fix calling UCR handlers on (un)registration of new default values
Philipp added some adjustments, which I reviewed: univention-config-registry (15.0.7-2) 3d50092b3af2 | test[ucr] Test default layer 2ced107e413e | refactor[ucr]: Always use _ConfigRegistry 65d7f45c3b76 | fix[ucr]: Re-implement default layer 7737f790f5d9 | refactor[ucr]: Extract saving code 3efdcc69107d | fix[ucr]: Protect /dev/null ededd2b35e09 | refactor[ucr]: Use generator instead of list 04219827e974 | test[ucr] Test filters bdf096d2f1af | test[ucr] Test default layer ceb765129336 | fix[ucr] Re-load default on update b55bfc47a25e | style[ucr] Fix mypy typing
(In reply to Philipp Hahn from comment #20) > FIXED: 5873e012e226 → d805f1fb4620 DevDoc > OK: 88b42b0558b2 changelog > OK: ca4fa9bdcafb UMC ucr OK: 2363f423c477 python default OK: 7f2854c9680d recursion OK: 3690aebc3604 info OK: ucr info ldap/overlay/memberof OK: 4a21b1dd8cdc ucr/deb/chnagelog OK: f618c0e35d99 C OK: 1fef0aef99fb 9fc27554da45 32fc29989169 register default order [5.0-0] 3d50092b3a test[ucr] Test default layer base/univention-config-registry/tests/test_backend.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) [5.0-0] 2ced107e41 refactor[ucr]: Always use _ConfigRegistry .../python/univention/config_registry/backend.py | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) [5.0-0] 65d7f45c3b fix[ucr]: Re-implement default layer base/univention-config-registry/debian/changelog | 6 ++ .../python/univention/config_registry/backend.py | 94 +++++++++-------------------- base/univention-config-registry/tests/test_backend.py | 1 - 3 files changed, 33 insertions(+), 68 deletions(-) [5.0-0] 7737f790f5 refactor[ucr]: Extract saving code .../python/univention/config_registry/backend.py | 34 +++++++++++++++++------------ 1 file changed, 20 insertions(+), 14 deletions(-) [5.0-0] 3efdcc6910 fix[ucr]: Protect /dev/null base/univention-config-registry/python/univention/config_registry/backend.py | 3 +++ base/univention-config-registry/tests/test_backend.py | 6 ++++++ 2 files changed, 9 insertions(+) [5.0-0] ededd2b35e refactor[ucr]: Use generator instead of list base/univention-config-registry/python/univention/config_registry/backend.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) [5.0-0] 04219827e9 test[ucr] Test filters base/univention-config-registry/debian/ucslint.overrides | 2 ++ .../python/univention/config_registry/filters.py | 2 +- base/univention-config-registry/tests/test_filters.py | 32 +++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) [5.0-0] bdf096d2f1 test[ucr] Test default layer base/univention-config-registry/.coveragerc | 4 + base/univention-config-registry/.coveragerc.pytest | 2 + base/univention-config-registry/debian/control | 2 + base/univention-config-registry/pytest.ini | 13 +++ .../python/univention/config_registry/backend.py | 41 ++++--- .../python/univention/config_registry/frontend.py | 1 + base/univention-config-registry/tests/clib.c | 43 ++++++-- base/univention-config-registry/tests/test_backend.py | 163 ++++++++++++++++++++++++---- 8 files changed, 220 insertions(+), 49 deletions(-) [5.0-0] ceb7651293 fix[ucr] Re-load default on update base/univention-config-registry/python/univention/config_registry/frontend.py | 3 +++ 1 file changed, 3 insertions(+) [5.0-0] b55bfc47a2 style[ucr] Fix mypy typing base/univention-config-registry/python/univention/config_registry/backend.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
UCS 5.0 has been released: https://docs.software-univention.de/release-notes-5.0-0-en.html https://docs.software-univention.de/release-notes-5.0-0-de.html If this error occurs again, please use "Clone This Bug".