389-ds-base

Clone
Source Code
GIT

#50911 Issue 50884 - Health check tool DSEldif check fails
Closed 3 years ago by spichugi. Opened 4 years ago by mreynolds.
mreynolds/389-ds-base issue50884  into  master

file modified
+2 -1 
@@ -150,6 +150,8 @@ 
 

              usercert=dsrc_inst['tls_cert'],
 

              userkey=dsrc_inst['tls_key'],
 

              starttls=dsrc_inst['starttls'], connOnly=True)
 

+     if ds.serverid is not None and ds.serverid.startswith("slapd-"):
 

+         ds.serverid = ds.serverid.replace("slapd-", "", 1)
 

      return ds
 

  

  
@@ -247,7 +249,6 @@ 
 

              if "=" in myattr:
 

                  [attr, val] = myattr.split("=", 1)
 

                  mc.replace(attr, val)
 

-                 print("MARK val: " + val)
 

                  print("Successfully replaced \"{}\"".format(attr))
 

              else:
 

                  raise ValueError("You must specify a value to replace the attribute ({})".format(myattr))

file modified
+17 -6 
@@ -8,7 +8,6 @@ 
 

  

  import sys
 

  import os
 

- import json
 

  import ldap
 

  from lib389.properties import (SER_LDAP_URL, SER_ROOT_DN, SER_LDAPI_ENABLED,
 

                                 SER_LDAPI_SOCKET, SER_LDAPI_AUTOBIND)
 
@@ -94,7 +93,7 @@ 
 

  

      The file should be an ini file, and instance should identify a section.
 

  

-     The ini fileshould have the content:
 

+     The ini file should have the content:
 

  

      [instance]
 

      uri = ldaps://hostname:port
 
@@ -108,13 +107,24 @@ 
 

      starttls = [true, false]
 

      """
 

      config = _read_dsrc(path, log)
 

+     server_id = instance_name
 

  

-     # Does our section exist?
 

-     if not config.has_section(instance_name):
 

-         # If not, return none.
 

-         log.debug("dsrc no such section %s" % instance_name)
 

+     # Do we have an instance name to work with?
 

+     if instance_name is None:
 

+         log.debug("No instance name provided")
 

          return None
 

  

+     # Strip the prefix
 

+     if instance_name.startswith("slapd-"):
 

+         server_id = instance_name = instance_name.replace("slapd-", "", 1)
 

+ 

+     if not config.has_section(instance_name):

I'm assuming this is to handle dsrc with a section of [slapd-localhost] not just [localhost] ? 

+         # instance_name does not have a prefix, but dsrc might, so add it
 

+         instance_name = "slapd-" + instance_name
 

+         if not config.has_section(instance_name):
 

+             log.debug("dsrc no such section: %s" % instance_name)
 

+             return None
 

+ 

      dsrc_inst = {}
 

      dsrc_inst['args'] = {}
 

  
@@ -143,6 +153,7 @@ 
 

      dsrc_inst['pwdfile'] = None
 

      dsrc_inst['prompt'] = False
 

      # Now gather the args
 

+     dsrc_inst['args']['server-id'] = server_id
 

      dsrc_inst['args'][SER_LDAP_URL] = dsrc_inst['uri']
 

      dsrc_inst['args'][SER_ROOT_DN] = dsrc_inst['binddn']
 

      if dsrc_inst['uri'][0:8] == 'ldapi://':

file modified
+3 -4 
@@ -9,7 +9,6 @@ 
 

  

  import copy
 

  import os
 

- import sys
 

  import base64
 

  import time
 

  from struct import pack, unpack
 
@@ -218,12 +217,12 @@ 
 

              endian = "Little Endian"
 

              end = '<'
 

              if flip:
 

-                 end = flipend(end)
 

+                 end = self._flipend(end)
 

          elif pack('>h', 1) == pack('=h',1):
 

              endian = "Big Endian"
 

              end = '>'
 

              if flip:
 

-                 end = flipend(end)
 

+                 end = self._flipend(end)
 

          else:
 

              raise ValueError("Unknown endian, unable to proceed")
 

  
@@ -250,7 +249,7 @@ 
 

          # if the sampled time is more than 20 years off, this is
 

          # probably the wrong endianness
 

          if wrongendian:
 

-             end = flipend(end)
 

+             end = self._flipend(end)
 

              fmtstr = end + base_fmtstr
 

              (rid, sampled_time, local_offset, remote_offset, seq_num) = unpack(fmtstr, nsstate)
 

              tdiff = now-sampled_time

file modified
+2 -2 
@@ -135,7 +135,7 @@ 
 

  

  # RI plugin checks
 

  DSRILE0001 = {
 

-     'dsle': 'DSRLE0001',
 

+     'dsle': 'DSRILE0001',
 

      'severity': 'LOW',
 

      'items' : ['cn=referential integrity postoperation,cn=plugins,cn=config', ],
 

      'detail': """The referential integrity plugin has an asynchronous processing mode.
 
@@ -162,7 +162,7 @@ 
 

  

  # Note - ATTR and BACKEND are replaced by the reporting function
 

  DSRILE0002 = {
 

-     'dsle': 'DSRLE0002',
 

+     'dsle': 'DSRILE0002',
 

      'severity': 'HIGH',
 

      'items' : ['cn=referential integrity postoperation,cn=plugins,cn=config'],
 

      'detail': """The referential integrity plugin is configured to use an attribute (ATTR)

Bug Description:

dsconf healthcheck was failing depending how the server id entered. Using "slapd-INSTANCE" vs "INSTANCE" produced different results.

Fix Description:

Normalize the instance name by always stripping off "slapd-". Also fixes similar issue when ~/.dsrc is used.

Fixed the RI plugin lint report's inconsistent IDs

Fixed issue how flipend was being called for read-nsstate.

relates: https://pagure.io/389-ds-base/issue/50884

If we remove slapd- frominstance_name here then it will not find slapd-localhost section in ~/.dsrc on the next line (it will be looking for localhost)

I think we should make it more flexable and allow user to have both formats in ~/.dsrc. With and without slapd- prefix.

If we remove slapd- frominstance_name here then it will not find slapd-localhost section in ~/.dsrc on the next line (it will be looking for localhost)
I think we should make it more flexable and allow user to have both formats in ~/.dsrc. With and without slapd- prefix.

Here's the issue, .dsrc is basically undocumented. On the wiki it shows you ONLY use instance name (without slapd-). Since there was a single source of truth i just left that as it, but I will see what I can do to make it more robust...

It just looks inconsistent to me... If we allow both slapd-localhost and localhost in CLI but dsrc accepts only localhost.
I think we should change the part of the wiki...

JFTR: This naming prefix, already discussed.... how do you distinguish the slapd-x and slapd-slapd-x, when the tooling is flexible, when user specifies slapd-x? Sure we shouldn't allow creating such an instance but that does not fix the fact that someone already has such an instance (or even created it by other means than our tooling). The looser we go the bigger trouble await us. We should be clear about what is an instance-name and what is a serverid (as in the prefixed one) and not mix those two, preferably ceasing from requiring user to have to think about it (aka, present only the instance-name) unless they have to debug something.

I feel most of this should really be addressed in the docs - not in the code. I will make .dsrc processing more flexible, but I think we just need to release note that you should not use "slapd-" in your instance name, and prevent new instances from using it (have to check if that was done already)

rebased onto 1e3a6c042346cae8db984677892468cc6c26294b
4 years ago

Okay changes made to make dsrc instance name handling more robust, please review...

The change looks to remove 'slapd-' prefix from serverId and instanceName (unless dsrc contains such prefix for instanceName). Looking at src/lib389/lib389/lint.py many messages display examples with 'dsconf slapd-xxx...'. Is it expected ?

The change looks to remove 'slapd-' prefix from serverId and instanceName (unless dsrc contains such prefix for instanceName). Looking at src/lib389/lib389/lint.py many messages display examples with 'dsconf slapd-xxx...'. Is it expected ?

The code now handles all cases where the supplied ID has slapd- or it doesn't, and we now can handle the server IDs in .dsrc which uses slapd- or not.

There is nothing wrong with using slapd-instance from the CLI, but the code needs to properly handle both cases (including how we interact with the .dsrc file).

I agree code should handle both case. My concern is that it looks we are trying to promote instance_name without 'slapd-' prefix but at the same time lint.py (healthcheck ?) will display message with this prefix.

I'm assuming this is to handle dsrc with a section of [slapd-localhost] not just [localhost] ?

I'm assuming this is to handle dsrc with a section of [slapd-localhost] not just [localhost] ?

Correct, this code allows dsrc to use "localhost" or "slapd-localhost"

LGTM. Ack

rebased onto 07a1080
4 years ago

Pull-Request has been merged by mreynolds
4 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/3964

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
None
No Tags
Changes Summary 4
+2 -1
file changed
src/lib389/lib389/cli_base/__init__.py
+17 -6
file changed
src/lib389/lib389/cli_base/dsrc.py
+3 -4
file changed
src/lib389/lib389/dseldif.py
+2 -2
file changed
src/lib389/lib389/lint.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.