PR#49779: Ticket 49777 - add config subcommand to dsconf - 389-ds-base

389-ds-base

#49779 Ticket 49777 - add config subcommand to dsconf

Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.

mreynolds/389-ds-base ticket49777 into master

Ticket 49777 - add config subcommand to dsconf

Mark Reynolds • 5 years ago

5fe4e21

src/lib389/cli/dsconf

file modified

+3 -1

		`@@ -18,6 +18,7 @@`

		`from lib389 import DirSrv`
		`from lib389._constants import DN_CONFIG, DN_DM`
		`+ from lib389.cli_conf import config as cli_config`
		`from lib389.cli_conf import backend as cli_backend`
		`from lib389.cli_conf import directory_manager as cli_directory_manager`
		`from lib389.cli_conf import plugin as cli_plugin`
		`@@ -71,6 +72,7 @@`

		`subparsers = parser.add_subparsers(help="resources to act upon")`

		`+ cli_config.create_parser(subparsers)`
		`cli_backend.create_parser(subparsers)`
		`cli_directory_manager.create_parsers(subparsers)`
		`cli_schema.create_parser(subparsers)`
		`@@ -122,7 +124,7 @@`
		`if args and args.json:`
		`print(e)`
		`else:`
		`- log.error("Error!: %s" % str(e))`
		`+ log.error("Error: %s" % str(e))`
		`result = False`

		`disconnect_instance(inst)`

src/lib389/lib389/_mapped_object.py

file modified

+38 -21

		`@@ -163,12 +163,11 @@`

		`def _jsonify(self, fn, args, *kwargs):`
		`# This needs to map all the values to ensure_str`
		`- attrs = fn(args, *kwargs)`
		`+ attrs = fn(use_json=True, args, *kwargs)`
		`str_attrs = {}`
		`for k in attrs:`
		`str_attrs[ensure_str(k)] = ensure_list_str(attrs[k])`
		`-`
		`- response = json.dumps({ "type": "entry", "dn": ensure_str(self._dn), "attrs" : str_attrs });`
		`+ response = json.dumps({"type": "entry", "dn": ensure_str(self._dn), "attrs": str_attrs})`

		`return response`

		`@@ -183,7 +182,7 @@`

		`if (name.endswith('_json')):`
		`int_name = name.replace('_json', '')`
		`- pfunc = partial(self._jsonify, fn=getattr(self, int_name))`
		`+ pfunc = partial(self._jsonify, getattr(self, int_name))`
		`return pfunc`

		`# We make this a property so that we can over-ride dynamically if needed`
		`@@ -326,10 +325,20 @@`
		`:raises: ValueError - if instance is not online`
		`"""`

		`+ if action == ldap.MOD_ADD:`
		`+ action_txt = "ADD"`
		`+ elif action == ldap.MOD_REPLACE:`
		`+ action_txt = "REPLACE"`
		`+ elif action == ldap.MOD_DELETE:`
		`+ action_txt = "DELETE"`
		`+ else:`
		`+ # This should never happen (bug!)`
		`+ action_txt = "UNKNOWN"`
		`+`
		`if value is None or len(value) < 512:`
		`- self._log.debug("%s set(%r, %r)" % (self._dn, key, value))`
		`+ self._log.debug("%s set %s: (%r, %r)" % (self._dn, action_txt, key, value))`
		`else:`
		`- self._log.debug("%s set(%r, value too large)" % (self._dn, key))`
		`+ self._log.debug("%s set %s: (%r, value too large)" % (self._dn, action_txt, key))`
		`if self._instance.state != DIRSRV_STATE_ONLINE:`
		`raise ValueError("Invalid state. Cannot set properties on instance that is not ONLINE.")`

		`@@ -418,7 +427,7 @@`
		`return False`
		`return True`

		`- def get_compare_attrs(self):`
		`+ def get_compare_attrs(self, use_json=False):`
		`"""Get a dictionary having attributes to be compared`
		`i.e. excluding self._compare_exclude`
		`"""`
		`@@ -430,7 +439,7 @@`
		`compare_attrs_dict = {attr:all_attrs_dict[attr] for attr in compare_attrs}`
		`return compare_attrs_dict`

		`- def get_all_attrs(self):`
		`+ def get_all_attrs(self, use_json=False):`
		`"""Get a dictionary having all the attributes of the entry`

		`:returns: Dict with real attributes and operational attributes`
		`@@ -446,7 +455,7 @@`
		`attrs_dict = attrs_entry.data`
		`return attrs_dict`

		`- def get_attrs_vals(self, keys):`
		`+ def get_attrs_vals(self, keys, use_json=False):`
		`self._log.debug("%s get_attrs_vals(%r)" % (self._dn, keys))`
		`if self._instance.state != DIRSRV_STATE_ONLINE:`
		`raise ValueError("Invalid state. Cannot get properties on instance that is not ONLINE")`
		`@@ -454,7 +463,7 @@`
		`entry = self._instance.search_ext_s(self._dn, ldap.SCOPE_BASE, self._object_filter, attrlist=keys, serverctrls=self._server_controls, clientctrls=self._client_controls)[0]`
		`return entry.getValuesSet(keys)`

		`- def get_attr_vals(self, key):`
		`+ def get_attr_vals(self, key, use_json=False):`
		`self._log.debug("%s get_attr_vals(%r)" % (self._dn, key))`
		`# We might need to add a state check for NONE dn.`
		`if self._instance.state != DIRSRV_STATE_ONLINE:`
		`@@ -465,9 +474,16 @@`
		`# It would be good to prevent the entry code intercepting this ....`
		`# We have to do this in this method, because else we ignore the scope base.`
		`entry = self._instance.search_ext_s(self._dn, ldap.SCOPE_BASE, self._object_filter, attrlist=[key], serverctrls=self._server_controls, clientctrls=self._client_controls)[0]`
		`- return entry.getValues(key)`
		`+ vals = entry.getValues(key)`
		`+ if use_json:`
		`+ result = {key: []}`
		`+ for val in vals:`
		`+ result[key].append(val)`
		`+ return result`
		`+ else:`
		`+ return vals`

		`- def get_attr_val(self, key):`
		`+ def get_attr_val(self, key, use_json=False):`
		`self._log.debug("%s getVal(%r)" % (self._dn, key))`
		`# We might need to add a state check for NONE dn.`
		`if self._instance.state != DIRSRV_STATE_ONLINE:`
		`@@ -478,7 +494,7 @@`
		`entry = self._instance.search_ext_s(self._dn, ldap.SCOPE_BASE, self._object_filter, attrlist=[key], serverctrls=self._server_controls, clientctrls=self._client_controls)[0]`
		`return entry.getValue(key)`

		`- def get_attr_val_bytes(self, key):`
		`+ def get_attr_val_bytes(self, key, use_json=False):`
		`"""Get a single attribute value from the entry in bytes type`

		`:param key: An attribute name`
		`@@ -489,7 +505,7 @@`

		`return ensure_bytes(self.get_attr_val(key))`

		`- def get_attr_vals_bytes(self, key):`
		`+ def get_attr_vals_bytes(self, key, use_json=False):`
		`"""Get attribute values from the entry in bytes type`

		`:param key: An attribute name`
		`@@ -500,7 +516,7 @@`

		`return ensure_list_bytes(self.get_attr_vals(key))`

		`- def get_attr_val_utf8(self, key):`
		`+ def get_attr_val_utf8(self, key, use_json=False):`
		`"""Get a single attribute value from the entry in utf8 type`

		`:param key: An attribute name`
		`@@ -511,7 +527,7 @@`

		`return ensure_str(self.get_attr_val(key))`

		`- def get_attr_val_utf8_l(self, key):`
		`+ def get_attr_val_utf8_l(self, key, use_json=False):`
		`"""Get a single attribute value from the entry in utf8 type`

		`:param key: An attribute name`
		`@@ -526,7 +542,7 @@`
		`else:`
		`return None`

		`- def get_attr_vals_utf8(self, key):`
		`+ def get_attr_vals_utf8(self, key, use_json=False):`
		`"""Get attribute values from the entry in utf8 type`

		`:param key: An attribute name`
		`@@ -537,7 +553,7 @@`

		`return ensure_list_str(self.get_attr_vals(key))`

		`- def get_attr_vals_utf8_l(self, key):`
		`+ def get_attr_vals_utf8_l(self, key, use_json=False):`
		`"""Get attribute values from the entry in utf8 type and lowercase`

		`:param key: An attribute name`
		`@@ -548,7 +564,7 @@`

		`return [x.lower() for x in ensure_list_str(self.get_attr_vals(key))]`

		`- def get_attr_val_int(self, key):`
		`+ def get_attr_val_int(self, key, use_json=False):`
		`"""Get a single attribute value from the entry in int type`

		`:param key: An attribute name`
		`@@ -559,7 +575,7 @@`

		`return ensure_int(self.get_attr_val(key))`

		`- def get_attr_vals_int(self, key):`
		`+ def get_attr_vals_int(self, key, use_json=False):`
		`"""Get attribute values from the entry in int type`

		`:param key: An attribute name`
		`@@ -796,7 +812,8 @@`
		`results.append(result)`
		`return results`

		`- # A challenge of this, is how do we manage indexes? They have two naming attribunes....`
		`+`
		`+ # A challenge of this, is how do we manage indexes? They have two naming attributes....`

		`class DSLdapObjects(DSLogging):`
		`"""The object represents the next idea: "Everything is an instance of something`

src/lib389/lib389/cli_base/__init__.py

file modified

+70 -2

		`@@ -9,6 +9,7 @@`
		`import logging`
		`import sys`
		`import json`
		`+ import ldap`

		`from getpass import getpass`
		`from lib389 import DirSrv`
		`@@ -140,14 +141,31 @@`
		`def _generic_get(inst, basedn, log, manager_class, selector, args=None):`
		`mc = manager_class(inst, basedn)`
		`if args and args.json:`
		`- o = mc.get(selector, json=True)`
		`- print(o)`
		`+ o = mc.get(selector, json=True)`
		`+ log.info(o)`
		`else:`
		`o = mc.get(selector)`
		`o_str = o.display()`
		`log.info(o_str)`


		`+ def _generic_get_entry(inst, basedn, log, manager_class, args=None):`
		`+ mc = manager_class(inst, basedn)`
		`+ if args and args.json:`
		`+ log.info(mc.get_all_attrs_json())`
		`+ else:`
		`+ log.info(mc.display())`
		`+`
		`+`
		`+ def _generic_get_attr(inst, basedn, log, manager_class, args=None):`
		`+ mc = manager_class(inst, basedn)`
		`+ for attr in args.attrs:`
		`+ if args and args.json:`
		`+ log.info(mc.get_attr_vals_json(attr))`
		`+ else:`
		`+ log.info(mc.display_attr(attr).rstrip())`
		`+`
		`+`
		`def _generic_get_dn(inst, basedn, log, manager_class, dn, args=None):`
		`mc = manager_class(inst, basedn)`
		`o = mc.get(dn=dn)`
		`@@ -169,6 +187,56 @@`
		`log.info('Successfully deleted %s' % dn)`


		`+ # Attr functions expect attribute values to be "attr=value"`
		`+`
		`+ def _generic_replace_attr(inst, basedn, log, manager_class, args=None):`
		`+ mc = manager_class(inst, basedn)`
		`+ if args and args.attr:`
		`+ for myattr in args.attr:`
		`+ if "=" in myattr:`
		`+ [attr, val] = myattr.split("=", 1)`
		`+ mc.replace(attr, val)`
		`+ log.info("Successfully replaced \"{}\"".format(attr))`
		`+ else:`
		`+ raise ValueError("You must specify a value to replace the attribute ({})".format(myattr))`
		`+ else:`
		`+ # Missing value`
		`+ raise ValueError("Missing attribute to replace")`
		`+`
		`+`
		`+ def _generic_add_attr(inst, basedn, log, manager_class, args=None):`
		`+ mc = manager_class(inst, basedn)`
		`+ if args and args.attr:`
		`+ for myattr in args.attr:`
		`+ if "=" in myattr:`
		`+ [attr, val] = myattr.split("=", 1)`
		`+ mc.add(attr, val)`
		`+ log.info("Successfully added \"{}\"".format(attr))`
		`+ else:`
		`+ raise ValueError("You must specify a value to add for the attribute ({})".format(myattr))`
		`+ else:`
		`+ # Missing value`
		`+ raise ValueError("Missing attribute to add")`
		`+`
		`+`
		`+ def _generic_del_attr(inst, basedn, log, manager_class, args=None):`
		`+ mc = manager_class(inst, basedn)`
		`+ if args and args.attr:`
		`+ for myattr in args.attr:`
		`+ if "=" in myattr:`
		`+ # we have a specific value`
		`+ [attr, val] = myattr.split("=", 1)`
		`+ mc.remove(attr, val)`
		`+ else:`
		`+ # remove all`
		`+ mc.remove_all(myattr)`
		`+ attr = myattr # for logging`
		`+ log.info("Successfully removed \"{}\"".format(attr))`
		`+ else:`
		`+ # Missing value`
		`+ raise ValueError("Missing attribute to delete")`
		`+`
		`+`
		`class LogCapture(logging.Handler):`
		`"""`
		`This useful class is for intercepting logs, and then making assertions about`

src/lib389/lib389/cli_conf/config.py

file added

+60

		`@@ -0,0 +1,60 @@`
		`+ # --- BEGIN COPYRIGHT BLOCK ---`
		`+ # Copyright (C) 2018 Red Hat, Inc.`
		`+ # All rights reserved.`
		`+ #`
		`+ # License: GPL (version 3 or any later version).`
		`+ # See LICENSE for details.`
		`+ # --- END COPYRIGHT BLOCK ---`
		`+`
		`+ from lib389.config import Config`
		`+ from lib389.cli_base import (`
		`+ populate_attr_arguments,`
		`+ _generic_get_entry,`
		`+ _generic_get_attr,`
		`+ _generic_add_attr,`
		`+ _generic_replace_attr,`
		`+ _generic_del_attr,`
		`+ )`
		`+`
		`+`
		`+ def config_get(inst, basedn, log, args):`
		`+ if args and args.attrs:`
		`+ _generic_get_attr(inst, basedn, log.getChild('config_get'), Config, args)`
		`+ else:`
		`+ # Get the entire cn=config entry`
		`+ _generic_get_entry(inst, basedn, log.getChild('config_get'), Config, args)`
		`+ return`
		`+`
		`+`
		`+ def config_add_attr(inst, basedn, log, args):`
		`+ _generic_add_attr(inst, basedn, log.getChild('config_add_attr'), Config, args)`
		`+`
		`+`
		`+ def config_replace_attr(inst, basedn, log, args):`
		`+ _generic_replace_attr(inst, basedn, log.getChild('config_replace_attr'), Config, args)`
		`+`
		`+`
		`+ def config_del_attr(inst, basedn, log, args):`
		`+ _generic_del_attr(inst, basedn, log.getChild('config_del_attr'), Config, args)`
		`+`
		`+`
		`+ def create_parser(subparsers):`
		`+ config_parser = subparsers.add_parser('config', help="Manage server configuration")`
		`+`
		`+ subcommands = config_parser.add_subparsers(help="action")`
		`+`
		`+ get_parser = subcommands.add_parser('get', help='get')`
		`+ get_parser.set_defaults(func=config_get)`
		`+ get_parser.add_argument('attrs', nargs='*', help='Configuration attribute(s) to get')`
		`+`
		`+ add_attr_parser = subcommands.add_parser('add', help='Add attribute value to configuration')`
		`+ add_attr_parser.set_defaults(func=config_add_attr)`
		`+ add_attr_parser.add_argument('attr', nargs='*', help='Configuration attribute to add')`
		`+`
		`+ replace_attr_parser = subcommands.add_parser('replace', help='Replace attribute value in configuration')`
		`+ replace_attr_parser.set_defaults(func=config_replace_attr)`
		`+ replace_attr_parser.add_argument('attr', nargs='*', help='Configuration attribute to replace')`
		`+`
		`+ del_attr_parser = subcommands.add_parser('delete', help='Delete attribute value in configuration')`
		`+ del_attr_parser.set_defaults(func=config_del_attr)`
		`+ del_attr_parser.add_argument('attr', nargs='*', help='Configuration attribute to delete')`

src/lib389/lib389/config.py

file modified

+9 -9

		`@@ -31,24 +31,24 @@`
		`- set access and error logging`
		`- get and set "cn=config" attributes`
		`"""`
		`- def __init__(self, conn):`
		`+ def __init__(self, conn, dn=None):`
		`"""@param conn - a DirSrv instance """`
		`super(Config, self).__init__(instance=conn)`
		`self._dn = DN_CONFIG`
		`# self._instance = conn`
		`# self.log = conn.log`
		`config_compare_exclude = [`
		`- 'nsslapd-ldapifilepath',`
		`- 'nsslapd-accesslog',`
		`- 'nsslapd-auditfaillog',`
		`+ 'nsslapd-ldapifilepath',`
		`+ 'nsslapd-accesslog',`
		`+ 'nsslapd-auditfaillog',`
		`'nsslapd-ldifdir',`
		`- 'nsslapd-errorlog',`
		`- 'nsslapd-instancedir',`
		`- 'nsslapd-lockdir',`
		`+ 'nsslapd-errorlog',`
		`+ 'nsslapd-instancedir',`
		`+ 'nsslapd-lockdir',`
		`'nsslapd-bakdir',`
		`- 'nsslapd-schemadir',`
		`+ 'nsslapd-schemadir',`
		`'nsslapd-auditlog',`
		`- 'nsslapd-rootpw',`
		`+ 'nsslapd-rootpw',`
		`'nsslapd-workingdir',`
		`'nsslapd-certdir'`
		`]`

mreynolds commented 5 years ago

Description: Added a "config" subcomand to dsconf for getting and
setting cn=config attributes. Also fixed JSON issues.

https://pagure.io/389-ds-base/issue/49777

Reviewed by: ?

spichugi commented on line 79 of src/lib389/lib389/cli_base/__init__.py 5 years ago

I think we should also allow a user to delete attribute without specifying the exact attribute value.
Something like:

dsconf -v -D "cn=Directory manager" ldap://localhost:389 config delete passwordMaxAge

spichugi commented 5 years ago

Okay, everything works but I have couple suggestions. :)

The debug now is misleading, it doesn't mention the 'action'
So we have something like:

# dsconf -v -D "cn=Directory manager" ldap://localhost:389 config delete passwordMaxAge=1231231
......
INFO: open(): bound as cn=Directory manager
DEBUG: cn=config set('passwordMaxAge', '1231231')
INFO: Success!
INFO: Command successful.
DEBUG: dsconf is brought to you by the letter H and the number 25.

I think we should mention the 'action' in the DEBUG: cn=config set('passwordMaxAge', '1231231') line. You can find the code here:
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/_mapped_object.py#_313

Another thing, I think, we can use wrappers (self.add, self.replace, etc.) instead of 'set()' calls. You can find them here:
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/_mapped_object.py#_235

If we want to delete all attribute values, we can use remove_all.
And if the value was specified - we use remove.

What do you think?

mreynolds commented 5 years ago

The PR needs a little work, but this is how it "should" work:

dsconf -v -D "cn=Directory manager" ldap://localhost:389 config delete passwordMaxAge=1231231

--> deletes specific value

dsconf -v -D "cn=Directory manager" ldap://localhost:389 config delete passwordMaxAge

--> deletes all passwordMaxAge attributes. This almost works, but it expects a value. So I need to change it

mreynolds commented 5 years ago

Changes made, please review.

Note - I also add more validation to the "set" operations

rebased onto 39cac9c018136c46c6be4b3899b68d1bcca5c25f

5 years ago

spichugi commented 5 years ago

Looks good to me! Ack

rebased onto 5fe4e21

5 years ago

Pull-Request has been merged by mreynolds

5 years ago

spichugi commented 3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/2838

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago

Metadata

Assignee

None

Tags

No Tags

Changes Summary 5

+3 -1

file changed

src/lib389/cli/dsconf

+38 -21

file changed

src/lib389/lib389/_mapped_object.py

+70 -2

file changed

src/lib389/lib389/cli_base/__init__.py

+60

file added

src/lib389/lib389/cli_conf/config.py

+9 -9

file changed

src/lib389/lib389/config.py

389-ds-base

Source Code

#49779 Ticket 49777 - add config subcommand to dsconf Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds. mreynolds/389-ds-base ticket49777 into master

Metadata

Changes Summary 5

#49779 Ticket 49777 - add config subcommand to dsconf

Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.

mreynolds/389-ds-base ticket49777 into master