Alon Bar-Lev has posted comments on this change. Change subject: services, setup: ovirt-vmconsole integration ......................................................................
Patch Set 6: (17 comments) maybe I missed that. but where do you issue the ssh certificates? 0600 ovirt-vmconsole proxy-ssh_host_rsa 0644 root proxy-ssh_host_rsa-cert.pub principal:fqdn 0600 ovirt-vmconsole proxy-ssh_user_rsa 0644 root proxy-ssh_user_rsa-cert.pub principal:ovirt-vmconsole-proxy https://gerrit.ovirt.org/#/c/35906/6/packaging/services/ovirt-vmconsole-proxy/ovirt-vmconsole-list-consoles.in File packaging/services/ovirt-vmconsole-proxy/ovirt-vmconsole-list-consoles.in: we do not write template for python scripts. if you want such template, please have single config.py.in and import it within scripts. from . import config please always have .py suffix for python scripts. Line 1: #!/usr/bin/env python Line 2: # Line 3: # Copyright 2015 Red Hat Line 4: # Line 1: #!/usr/bin/env python /usr/local/bin/python Line 2: # Line 3: # Copyright 2015 Red Hat Line 4: # Line 5: # Licensed under the Apache License, Version 2.0 (the "License"); Line 39: config_sources = [ Line 40: VMPROXY_CONF Line 41: ] Line 42: config_sources.extend(glob.glob(os.path.join( Line 43: '%s.d' % VMPROXY_CONF, '*.conf'))) why do you need the config_sources temp variable? return configfile.ConfigFile( [VMPROXY_CONF] + glob.glob(os.path.join('%s.d' % VMPROXY_CONF, '*.conf')) ) Line 44: Line 45: return configfile.ConfigFile(config_sources) Line 46: Line 47: Line 48: def parse_args(): Line 49: try: Line 50: return str(int(sys.argv[1])), sys.argv[2] Line 51: except (ValueError, IndexError): Line 52: raise RuntimeError("usage: %s version entityid" % sys.argv[0]) this is ugly, please use standard argparse. Line 53: Line 54: Line 55: def main(): Line 56: service.setupLogger() Line 71: 'user_id': entity_id, Line 72: })) Line 73: Line 74: url = 'http://%s/ovirt-engine/services/vmconsole-proxy/' % ( Line 75: config.get('ENGINE_URL')) 1. url should be as it means, url. 2. default should be http://localhost/xxx 3. not sure why you need the url temp variable. Line 76: Line 77: res = urllib.urlopen(url, req) Line 78: Line 79: print(res.read()) Line 75: config.get('ENGINE_URL')) Line 76: Line 77: res = urllib.urlopen(url, req) Line 78: Line 79: print(res.read()) you must check for http status code Line 80: Line 81: except Exception as ex: Line 82: logger.error(str(ex)) Line 83: return 1 Line 78: Line 79: print(res.read()) Line 80: Line 81: except Exception as ex: Line 82: logger.error(str(ex)) should be: logger.error('%s', ex) so you do not break things if exception contains '%' Line 83: return 1 Line 84: Line 85: return 0 Line 86: https://gerrit.ovirt.org/#/c/35906/6/packaging/services/ovirt-vmconsole-proxy/ovirt-vmconsole-list-keys.in File packaging/services/ovirt-vmconsole-proxy/ovirt-vmconsole-list-keys.in: same comments. no reason to re-write when you can reuse. I think you can use either: 1. two main scripts that calls single logic, remove duplicates. 2. one script that based on arguments provide this or that. 3. one script and symlink to provide this or that based on argv[0] Line 1: #!/usr/bin/env python Line 2: # Line 3: # Copyright 2015 Red Hat Line 4: # https://gerrit.ovirt.org/#/c/35906/6/packaging/setup/ovirt_engine_setup/engine/constants.py File packaging/setup/ovirt_engine_setup/engine/constants.py: Line 179: ) Line 180: OVIRT_ENGINE_PKI_LOCAL_VMCONSOLE_PROXY_KEY = os.path.join( Line 181: OVIRT_ENGINE_PKIKEYSDIR, Line 182: 'vmconsole-proxy.key.nopass', Line 183: ) if you have vmconsole own constants why do we need changes in the global config? Line 184: OVIRT_ENGINE_PKI_REPORTS_KEY = os.path.join( Line 185: OVIRT_ENGINE_PKIKEYSDIR, Line 186: 'reports.key.nopass', Line 187: ) https://gerrit.ovirt.org/#/c/35906/6/packaging/setup/ovirt_engine_setup/vmconsole_proxy/constants.py File packaging/setup/ovirt_engine_setup/vmconsole_proxy/constants.py: I just assume this is clone of websocket proxy module, will let integration review that. Line 1: # Line 2: # ovirt-engine-setup -- ovirt engine setup Line 3: # Copyright (C) 2014-2015 Red Hat, Inc. Line 4: # https://gerrit.ovirt.org/#/c/35906/6/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/pki/ca.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/pki/ca.py: Line 352: ), Line 353: ( Line 354: oenginecons.FileLocations. Line 355: OVIRT_ENGINE_PKI_LOCAL_VMCONSOLE_PROXY_STORE Line 356: ), please make sure you enroll this with proper eku, so we can relay on eku when validating. Line 357: ( Line 358: oenginecons.FileLocations. Line 359: OVIRT_ENGINE_PKI_LOCAL_WEBSOCKET_PROXY_CERT Line 360: ), https://gerrit.ovirt.org/#/c/35906/6/packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/config.py File packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/config.py: Line 218: ovmpcons.FileLocations. Line 219: OVIRT_ENGINE_VMCONSOLE_PROXY_CONFIG_SETUP Line 220: ), Line 221: content=( Line 222: "ENGINE_URL={engine_fqdn}\n" this should be url or if ENGINE_FQDN then also have: ENGINE_URL="${ENGINE_PROTOCOL}://${ENGINE_FQDN}:${ENGINE_PORT}/ovirt-engine" the protocol and the port should be gotten out of the user and if automatically out of what engine actually produces, so it will work in devenv. Line 223: "SSL_CERTIFICATE={certificate}\n" Line 224: "SSL_KEY={key}\n" Line 225: "CERT_FOR_DATA_VERIFICATION={engine_cert}\n" Line 226: ).format( Line 221: content=( Line 222: "ENGINE_URL={engine_fqdn}\n" Line 223: "SSL_CERTIFICATE={certificate}\n" Line 224: "SSL_KEY={key}\n" Line 225: "CERT_FOR_DATA_VERIFICATION={engine_cert}\n" this^ is not required Line 226: ).format( Line 227: engine_fqdn=self.environment[ Line 228: ovmpcons.EngineConfigEnv.ENGINE_FQDN Line 229: ], https://gerrit.ovirt.org/#/c/35906/6/packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/pki.py File packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/pki.py: Line 95: ) Line 96: ) Line 97: Line 98: if not engine_vmp_pki_found: Line 99: self._enrolldata = remote_engine.EnrollCert( this is one ugly function, will let integration review that. Line 100: remote_engine=self.environment[ Line 101: osetupcons.CoreEnv.REMOTE_ENGINE Line 102: ], Line 103: engine_fqdn=self.environment[ Line 119: OVIRT_ENGINE_PKIREQUESTSDIR, Line 120: engine_pki_certs_dir=ovmpcons.FileLocations. Line 121: OVIRT_ENGINE_PKICERTSDIR, Line 122: key_size=self.environment[ovmpcons.ConfigEnv.KEY_SIZE], Line 123: url="http://www.ovirt.org/Features/Serial_Console"; ? Line 124: ) Line 125: self._enrolldata.enroll_cert() Line 126: Line 127: self._need_eng_cert = not os.path.exists( Line 147: 'pki-resource?resource=engine-certificate&' Line 148: 'format=X509-PEM'.format( Line 149: engine_fqdn=remote_engine_host Line 150: ) Line 151: ) you do not need engine certificate Line 152: ) as urlObj: Line 153: engine_ca_cert = urlObj.read() Line 154: if engine_ca_cert: Line 155: self._engine_cert = engine_ca_cert Line 179: ) Line 180: def _misc_pki(self): Line 181: self._enrolldata.add_to_transaction( Line 182: uninstall_group_name='ca_pki_vmp', Line 183: uninstall_group_desc='VMP PKI keys', what is vmp? vmconsole? Line 184: ) Line 185: uninstall_files = [] Line 186: self.environment[ Line 187: osetupcons.CoreEnv.REGISTER_UNINSTALL_GROUPS -- To view, visit https://gerrit.ovirt.org/35906 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I034ef8e6d10da5dc93eda61e0c5c518ca13a5a28 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
