[Engine-patches] Change in ovirt-node-plugin-vdsm[master]: engine_page: rewrite register

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: engine_page: rewrite register
......................................................................


Patch Set 6:

(41 comments)

Hi,

First pass.

Thanks,

Alon

....................................................
File src/cert.py
Line 18: # MA  02110-1301, USA.  A copy of the GNU General Public License is
Line 19: # also available at http://www.gnu.org/copyleft/gpl.html.
Line 20: 
Line 21: from M2Crypto import X509
Line 22: from misc import *
do not import *
Line 23: from ovirtnode.ovirtfunctions import ovirt_store_config
Line 24: from ovirtnode.ovirtfunctions import ovirt_safe_delete_config
Line 25: 
Line 26: import logging


Line 22: from misc import *
Line 23: from ovirtnode.ovirtfunctions import ovirt_store_config
Line 24: from ovirtnode.ovirtfunctions import ovirt_safe_delete_config
Line 25: 
Line 26: import logging
please refer to otopi, ovirt-host-deploy, engine-setup for examples for 
conventions.

first standard modules then external modules, then internal modules
Line 27: import datetime
Line 28: import os
Line 29: import re
Line 30: import pwd


Line 35: class Cert(object):
Line 36: 
Line 37:     _LOG_PREFIX = 'ovirt-node-plugin-vdsm.'
Line 38:     _REMOTE_SSH_KEY_FILE = (
Line 39:         '/engine.ssh.key.txt', '/rhevm.ssh.key.txt'
please use convention of:

 xxx = (
     item1,
     item2,
 )

1. each item in own line.

2. always have ',' at the last item.
Line 40:     )
Line 41:     _HTTP_TIMEOUT = 30
Line 42:     _P_ROOT_SSH = pwd.getpwnam('root').pw_dir + '/.ssh'
Line 43:     _P_ROOT_AUTH_KEYS = _P_ROOT_SSH + '/authorized_keys'


Line 38:     _REMOTE_SSH_KEY_FILE = (
Line 39:         '/engine.ssh.key.txt', '/rhevm.ssh.key.txt'
Line 40:     )
Line 41:     _HTTP_TIMEOUT = 30
Line 42:     _P_ROOT_SSH = pwd.getpwnam('root').pw_dir + '/.ssh'
please use os.path.join for construction of paths

please use os.path.expanduser to expand home directory.

Please refer to: otopi::./src/plugins/otopi/network/ssh.py
Line 43:     _P_ROOT_AUTH_KEYS = _P_ROOT_SSH + '/authorized_keys'
Line 44:     _HTTP_SUCCESS_CODE = 200
Line 45: 
Line 46:     # Regular expression used to validate content of SSH public keys:


Line 43:     _P_ROOT_AUTH_KEYS = _P_ROOT_SSH + '/authorized_keys'
Line 44:     _HTTP_SUCCESS_CODE = 200
Line 45: 
Line 46:     # Regular expression used to validate content of SSH public keys:
Line 47:     _SSH_PUBLIC_KEY_RE = re.compile(flags=re.VERBOSE, pattern=r"""
please use convention of:

 xxx = function(
     arg1,
     arg2,
 )
Line 48:       ^
Line 49:       \s*
Line 50:       ssh-(rsa|dss)
Line 51:       \s+


Line 56:     """)
Line 57: 
Line 58:     def __init__(self):
Line 59:         prefix = ''
Line 60:         if not self.__module__.startswith(self._LOG_PREFIX):
this should go into Base object, see otopi::base
Line 61:             prefix = self._LOG_PREFIX
Line 62: 
Line 63:         self.logger = logging.getLogger(prefix + self.__module__)
Line 64:         self.misc = Misc()


Line 60:         if not self.__module__.startswith(self._LOG_PREFIX):
Line 61:             prefix = self._LOG_PREFIX
Line 62: 
Line 63:         self.logger = logging.getLogger(prefix + self.__module__)
Line 64:         self.misc = Misc()
not sure why you need to hold reference for Misc.
Line 65: 
Line 66:     def cert_paths(self):
Line 67:         pkiDir = VDSM_PKI_DIR
Line 68: 


Line 70:         CACERT = pkiDir + "/certs/cacert.pem"
Line 71: 
Line 72:         engineWebCACert = pkiDir + "/certs/engine_web_ca.pem"
Line 73: 
Line 74:         return CACERT, VDSMCERT, engineWebCACert
not sure why you need this functions, you can hold all these as constants.
Line 75: 
Line 76:     def generate_fingerprint(self, path):
Line 77: 
Line 78:         fp = None


Line 72:         engineWebCACert = pkiDir + "/certs/engine_web_ca.pem"
Line 73: 
Line 74:         return CACERT, VDSMCERT, engineWebCACert
Line 75: 
Line 76:     def generate_fingerprint(self, path):
it is not generate, just get...
Line 77: 
Line 78:         fp = None
Line 79: 
Line 80:         with open(path, 'r') as f:


Line 79: 
Line 80:         with open(path, 'r') as f:
Line 81:             pem_file = f.read()
Line 82: 
Line 83:         x509 = X509.load_cert_string(pem_file, X509.FORMAT_PEM)
please try to use kwargs as much as possible.

if you read a file, why not use load_cert?
Line 84:         fp = x509.get_fingerprint('sha1')
Line 85:         fp = ':'.join(fp[pos:pos + 2] for pos in xrange(0, len(fp), 2))
Line 86: 
Line 87:         return fp


Line 81:             pem_file = f.read()
Line 82: 
Line 83:         x509 = X509.load_cert_string(pem_file, X509.FORMAT_PEM)
Line 84:         fp = x509.get_fingerprint('sha1')
Line 85:         fp = ':'.join(fp[pos:pos + 2] for pos in xrange(0, len(fp), 2))
return re.sub(r'(..)', r':\1', x509.get_fingerprint('sha1'))[1:]
Line 86: 
Line 87:         return fp
Line 88: 
Line 89:     def get_http_ca_cert(self, host, port):


Line 90: 
Line 91:         cert_downloaded = False
Line 92: 
Line 93:         try:
Line 94:             cert_pem = ssl.get_server_certificate((host, int(port)))
this gets the end certificate while we need the *CA* certificate.
Line 95:             _, _, engineWebCACert = self.cert_paths()
Line 96: 
Line 97:             with open(engineWebCACert, "w+") as f:
Line 98:                 f.write(cert_pem)


Line 91:         cert_downloaded = False
Line 92: 
Line 93:         try:
Line 94:             cert_pem = ssl.get_server_certificate((host, int(port)))
Line 95:             _, _, engineWebCACert = self.cert_paths()
this function is a copy from legacy and is bad bad bad bad.
Line 96: 
Line 97:             with open(engineWebCACert, "w+") as f:
Line 98:                 f.write(cert_pem)
Line 99: 


Line 98:                 f.write(cert_pem)
Line 99: 
Line 100:             cert_downloaded = True
Line 101:         except Exception, e:
Line 102:             logging.debug("Cannot download CA cert %s", str(e))
why do you swallow exception at functions? user should decide what he needs to 
do in case of exception.
Line 103: 
Line 104:         return cert_downloaded
Line 105: 
Line 106:     def get_authkeys_file(self, IP, port):


Line 111:         data = None
Line 112: 
Line 113:         for key in self._REMOTE_SSH_KEY_FILE:
Line 114:             tmp = self.misc.get_remote_file(
Line 115:                 IP, port, key, timeout=self._HTTP_TIMEOUT,
each parameter at own line, please use kwargs whenever possible.

I do not understand why misc need to have instances, all these utilities are 
just helper within module.
Line 116:                 certPath=engineWebCACert
Line 117:             )
Line 118:             if tmp is not None and \
Line 119:                     self._SSH_PUBLIC_KEY_RE.match(tmp) is not None:


Line 112: 
Line 113:         for key in self._REMOTE_SSH_KEY_FILE:
Line 114:             tmp = self.misc.get_remote_file(
Line 115:                 IP, port, key, timeout=self._HTTP_TIMEOUT,
Line 116:                 certPath=engineWebCACert
comma at last parameter, always.
Line 117:             )
Line 118:             if tmp is not None and \
Line 119:                     self._SSH_PUBLIC_KEY_RE.match(tmp) is not None:
Line 120:                 data = tmp


Line 114:             tmp = self.misc.get_remote_file(
Line 115:                 IP, port, key, timeout=self._HTTP_TIMEOUT,
Line 116:                 certPath=engineWebCACert
Line 117:             )
Line 118:             if tmp is not None and \
should be:

 if (
     tmp is not None and
     self._SSH_PUBLIC_KEY_RE.match(tmp) is not None
 ):
     bla

each condition in own line, no continuation line.
Line 119:                     self._SSH_PUBLIC_KEY_RE.match(tmp) is not None:
Line 120:                 data = tmp
Line 121:                 break
Line 122: 


Line 125:     def add_ssh_key(self, path, strKey):
Line 126:         resKeys = []
Line 127: 
Line 128:         try:
Line 129:             for key in file(path):
always use with statement.

*PLEASE* do not copy code from legacy, it will always be bad.
Line 130:                 if not key.endswith('\n'):  # make sure we have 
complete lines
Line 131:                     key += '\n'
Line 132:                 if key != '\n' and not key.endswith(" 
ovirt-engine\n") or \
Line 133:                         key.startswith("#"):


Line 132:                 if key != '\n' and not key.endswith(" 
ovirt-engine\n") or \
Line 133:                         key.startswith("#"):
Line 134:                     resKeys.append(key)
Line 135:         except IOError:
Line 136:             logging.debug("Failed to read %s", path)
*NEVER* swallow exceptions.
Line 137: 
Line 138:         if not strKey.endswith('\n'):
Line 139:             strKey += '\n'
Line 140: 


Line 140: 
Line 141:         resKeys.append(strKey)
Line 142: 
Line 143:         with open(engineWebCACert, "w") as f:
Line 144:             f.write(''.join(resKeys))
always write into temporary file then rename.

see otopi::./src/plugins/otopi/network/ssh.py

you should also restorecon
Line 145: 
Line 146:     def handle_ssh_key(self, strKey):
Line 147:         """
Line 148:             This functions appends key to the root's authorized_keys 
file.


Line 142: 
Line 143:         with open(engineWebCACert, "w") as f:
Line 144:             f.write(''.join(resKeys))
Line 145: 
Line 146:     def handle_ssh_key(self, strKey):
not sure why you need this function and above function.

one logical block... have a key and put it in authorize keys of root.
Line 147:         """
Line 148:             This functions appends key to the root's authorized_keys 
file.
Line 149:         """
Line 150:         logging.debug('handle_ssh_key start')


Line 153:             try:
Line 154:                 os.mkdir(self._P_ROOT_SSH, 0700)
Line 155:                 self.misc.silent_restorecon(self._P_ROOT_SSH)
Line 156:             except OSError:
Line 157:                 logging.debug("handle_ssh_key: failed to create ssh 
dir!")
and you swallow the exception and continue?!?!
Line 158: 
Line 159:         try:
Line 160:             self.add_ssh_key(self._P_ROOT_AUTH_KEYS, strKey)
Line 161:         except:


Line 161:         except:
Line 162:             logging.debug(
Line 163:                 "handle_ssh_key: failed to write authorized_keys!",
Line 164:                 exc_info=True
Line 165:             )
and how can we continue in this case?
Line 166: 
Line 167:         try:
Line 168:             os.chmod(self._P_ROOT_AUTH_KEYS, 0644)
Line 169:             self.misc.silent_restorecon(self._P_ROOT_AUTH_KEYS)


Line 169:             self.misc.silent_restorecon(self._P_ROOT_AUTH_KEYS)
Line 170:         except:
Line 171:             logging.debug(
Line 172:                 "handle_ssh_key: failed to chmod authorized_keys",
Line 173:                 exc_info=True
again... this is an error, we cannot swallow the exception and continue.
Line 174:             )
Line 175: 
Line 176:         # persist authorized_keys
Line 177:         logging.debug("handle_ssh_key: persist authorized_keys")


Line 188:                 dirName = os.path.dirname(pemFile)
Line 189: 
Line 190:                 bkpCertName = dirName + "/bkp-" + backupTime + '_' + 
certName
Line 191: 
Line 192:                 shutil.copy2(pemFile, bkpCertName)
why not rename?
Line 193:                 st = os.stat(pemFile)
Line 194:                 os.chown(bkpCertName, st.st_uid, st.st_gid)
Line 195:                 ovirt_store_config(bkpCertName)
Line 196: 


Line 194:                 os.chown(bkpCertName, st.st_uid, st.st_gid)
Line 195:                 ovirt_store_config(bkpCertName)
Line 196: 
Line 197:     def node_cleanup_certs(self):
Line 198:         CACERT, VDSMCERT, engineWebCACert = self.cert_paths()
why do you touch artifacts that we do not handle?

vdsm certificates are not handled by the registration, so should not appear in 
this code.
Line 199: 
Line 200:         self._node_backup_certs([CACERT, VDSMCERT, engineWebCACert])
Line 201:         if os.path.exists(engineWebCACert):


....................................................
File src/config.py.in
Line 17: 
Line 18: PACKAGE_NAME = '@PACKAGE_NAME@'
Line 19: PACKAGE_VERSION = '@PACKAGE_VERSION@'
Line 20: ENGINE_NAME = '@ENGINENAME@'
Line 21: ENGINE_URI = 
'/OvirtEngineWeb/register:RHEVManagerWeb/VdsAutoRegistration.aspx'
tuple please
Line 22: REGISTER_TIMEOUT_SEC = 10
Line 23: VDSM_PKI_DIR = '/etc/pki/vdsm'
Line 24: DEFAULT_SSL_PORT = 443


Line 19: PACKAGE_VERSION = '@PACKAGE_VERSION@'
Line 20: ENGINE_NAME = '@ENGINENAME@'
Line 21: ENGINE_URI = 
'/OvirtEngineWeb/register:RHEVManagerWeb/VdsAutoRegistration.aspx'
Line 22: REGISTER_TIMEOUT_SEC = 10
Line 23: VDSM_PKI_DIR = '/etc/pki/vdsm'
we can put our artifacts (web ca certificate) at different location than vdsm. 
use /etc/pki/ovirt-registration or something similar.
Line 24: DEFAULT_SSL_PORT = 443


Line 21: ENGINE_URI = 
'/OvirtEngineWeb/register:RHEVManagerWeb/VdsAutoRegistration.aspx'
Line 22: REGISTER_TIMEOUT_SEC = 10
Line 23: VDSM_PKI_DIR = '/etc/pki/vdsm'
Line 24: DEFAULT_SSL_PORT = 443
Line 25: 
please move all file locations here.


....................................................
File src/engine_page.py
Line 302
Line 303
Line 304
Line 305
Line 306
this should be moved to own module so that the bootargs method can use same 
code.


Line 168:             self.logger.debug("Connecting to engine")
Line 169:             txs += [ActivateVDSM(effective_model["vdsm_cfg.address"],
Line 170:                                  effective_model["vdsm_cfg.port"],
Line 171:                                  effective_model["engine.newreg"]
Line 172:                                  )]
just repeating so I can read this code...

 txs.append(
     ActivateVDSM(
         address=effective_model["vdsm_cfg.address"],
         port=effective_model["vdsm_cfg.port"],
         newreg=effective_model["engine.newreg"],
     )
 )

notice, no drawing the code, always one indent, use kwargs, comma at last 
argument.

now...

I prefer you use the term of legacy registration and not new, as there might be 
new new new method

second, I prefer you use the python convention of (ip, port) tuple.
Line 173: 
Line 174:         if len(txs) > 0:
Line 175:             progress_dialog = 
ui.TransactionProgressDialog("dialog.txs", txs,
Line 176:                                                            self)


Line 197:     LOGGER.debug("Finding port %s:%s with compat %s ssl %s" %
Line 198:                  (engineServer, enginePort, compatPort, sslPort))
Line 199: 
Line 200:     cert = Cert()
Line 201:     cert.node_cleanup_certs()
why?
Line 202: 
Line 203:     # Build port list to try
Line 204:     port_cfgs = [(enginePort, sslPort)]
Line 205:     if compatPort:


Line 314:         self.server = server
Line 315:         self.port = port
Line 316:         self.newreg = newreg
Line 317:         self.misc = Misc()
Line 318:         self.cert = Cert()
not sure why you need to hold reference to these two.
Line 319: 
Line 320:     def cert_validator(self):
Line 321:         cert_path = VDSM().retrieve()["cert_path"]
Line 322:         cert_exists = cert_path and os.path.exists(cert_path)


Line 337:         # pylint: enable-msg=E0611,F0401
Line 338: 
Line 339:         cfg = VDSM().retrieve()
Line 340: 
Line 341:         if self.misc.get_uuid() is None:
will go over this once code is cleaned.
Line 342:             raise RuntimeError("Cannot proceed without uuid request")
Line 343: 
Line 344:         kparams = self.misc.get_ovirtnode_kernel_params()
Line 345: 


....................................................
File src/misc.py
Line 233:         import selinux
Line 234:         try:
Line 235:             return selinux.restorecon(path)
Line 236:         except:
Line 237:             logging.error('restorecon %s failed', path, exc_info=True)
do not use logging, but base class logger, see otopi::Base
Line 238: 
Line 239:     def register_node(self, engine, port):
Line 240: 
Line 241:         (rc, stdout, stderr) = self.execute(


Line 247:             ),
Line 248:             raiseOnError=True
Line 249:         )
Line 250: 
Line 251:         node_address = stdout[0].split(" ")[5]
use regular expression see 
ovirt-host-deploy::./src/plugins/ovirt-host-deploy/vdsm/bridge.py
Line 252:         if node_address == '':
Line 253:             return
Line 254: 
Line 255:         uuid = self.get_uuid()


Line 249:         )
Line 250: 
Line 251:         node_address = stdout[0].split(" ")[5]
Line 252:         if node_address == '':
Line 253:             return
please do not return at middle of functions.

if you do not have route, then this is an *ERROR* throw an exception.
Line 254: 
Line 255:         uuid = self.get_uuid()
Line 256:         host = socket.gethostname()
Line 257:         timeout = REGISTER_TIMEOUT_SEC


Line 268: 
Line 269:             if uuid == "None" and "localhost.localdomain" in 
self.ovirtName:
Line 270:                 logging.warn(
Line 271:                     "WARNING! node with no UUID and no unique 
host-name!"
Line 272:                 )
please generate /etc/vdsm/vdsm.id with uuid
Line 273: 
Line 274:             ret = False
Line 275:             http_res = None
Line 276: 


Line 286:                 http_res = conn.getresponse()
Line 287:             except:
Line 288:                 logging.debug(
Line 289:                     "register_node failed in HTTPS. Retrying using 
HTTP."
Line 290:                 )
please remove this code, and read the design, there is no fallback to http.
Line 291:                 try:
Line 292:                     conn = None
Line 293:                     conn = httplib.HTTPConnection(engine + ":" + port)
Line 294:                     conn.request("GET", http_uri)


Line 310:                 ret = True
Line 311:                 break
Line 312: 
Line 313:         if conn is not None:
Line 314:             conn.close()
you should use with statement.
Line 315: 
Line 316:         socket.setdefaulttimeout(system_timeout)
Line 317:         logging.debug("register_node end. return %s", ret)
Line 318: 


Line 317:         logging.debug("register_node end. return %s", ret)
Line 318: 
Line 319:         return ret
Line 320: 
Line 321:     def get_remote_file(self, IP, port, fileName, timeout=None, 
certPath=None):
ok, this is way too much... please stop copy from legacy and think how to write 
it from scratch but properly... exception handling, with statement, proper 
logging, no fallback to http, etc.
Line 322:         logging.debug(
Line 323:             "get_remote_file start. IP = %s port = %s fileName = 
\"%s\"" %
Line 324:             (IP, port, fileName)
Line 325:         )


-- 
To view, visit http://gerrit.ovirt.org/17682
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I38f3b800c445f8dbb0fa0e89d128cea1e3407798
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-node-plugin-vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsl...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com>
Gerrit-Reviewer: Joey Boggs <jbo...@redhat.com>
Gerrit-Reviewer: Michael Burns <mbu...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches