Alon Bar-Lev has posted comments on this change. Change subject: services, setup: ovirt-vmconsole integration ......................................................................
Patch Set 38: (13 comments) https://gerrit.ovirt.org/#/c/35906/38/packaging/services/ovirt-vmconsole-proxy/ovirt-vmconsole-list.py File packaging/services/ovirt-vmconsole-proxy/ovirt-vmconsole-list.py: Line 22: import sys Line 23: import urllib2 Line 24: import urlparse Line 25: Line 26: import config this should be: from . import config to avoid search in other place of generic name. Line 27: Line 28: from ovirt_engine import configfile, service, ticket Line 29: Line 30: Line 34: def make_ticket_encoder(cfg_file): Line 35: return ticket.TicketEncoder( Line 36: cfg_file.get('SSL_CERTIFICATE'), Line 37: cfg_file.get('SSL_KEY'), Line 38: ) the ticket should use a different certificate than the one that is used for ssl, not sure what is the ssl in this case, you only have the remote. Line 39: Line 40: Line 41: def base_url_from_conf(cfg_file): Line 42: def choose_port_key(cfg_file): Line 51: 'https' if cfg_file.getboolean('ENGINE_HTTPS_ENABLED') Line 52: else 'http'), Line 53: fqdn=cfg_file.get('ENGINE_FQDN'), Line 54: port=cfg_file.get(choose_port_key(cfg_file)) Line 55: ) the setup should set the base url explicitly, we do not care how the engine actually configured all this service should care about is a single base url. Line 56: Line 57: Line 58: def parse_args(): Line 59: parser = argparse.ArgumentParser( Line 96: 'key_fp': args.keyfp, Line 97: 'key_type': args.keytype, Line 98: 'key_content': args.keycontent, Line 99: } Line 100: if args.entity == 'consoles': elif? Line 101: if args.entityid is None: Line 102: raise ValueError('entityid required and not found') Line 103: return { Line 104: 'command': 'available_consoles', Line 156: Line 157: if res.getcode() == HTTP_STATUS_CODE_SUCCESS: Line 158: print(handle_response(res.read())) Line 159: else: Line 160: raise RuntimeError('Engine call failed: code=%d', res.getcode()) exception pattern is: if fail: raise error continue as usual Line 161: Line 162: except Exception as ex: Line 163: logger.error('%s', str(ex)) Line 164: return 1 https://gerrit.ovirt.org/#/c/35906/38/packaging/setup/ovirt_engine_setup/vmconsole_proxy/constants.py File packaging/setup/ovirt_engine_setup/vmconsole_proxy/constants.py: Line 72: ) Line 73: OVIRT_ENGINE_PKIREQUESTSDIR = os.path.join( Line 74: OVIRT_ENGINE_PKIDIR, Line 75: 'requests', Line 76: ) I am unsure, but I think these pki locations should be only at engine side. Line 77: Line 78: OVIRT_ENGINE_PKI_VMCONSOLE_PROXY_HELPER_KEY = os.path.join( Line 79: OVIRT_ENGINE_PKIKEYSDIR, Line 80: '%s.key.nopass' % Const.VMCONSOLE_PROXY_HELPER_PKI_NAME, Line 182: return 'OVESETUP_ENGINE_CONFIG/fqdn' Line 183: Line 184: Line 185: @util.export Line 186: class PKIEnv(object): the bellow vars are not environment keys but constants, please move to Const. Line 187: OVIRT_VMCONSOLE_PROXY_EKU = '1.2.1.1' Line 188: Line 189: OVIRT_VMCONSOLE_ALLOWED_USER = 'ovirt-vmconsole' Line 190: Line 183: Line 184: Line 185: @util.export Line 186: class PKIEnv(object): Line 187: OVIRT_VMCONSOLE_PROXY_EKU = '1.2.1.1' shouldn't this be 1.3.6.1.4.1.2312.13.1.2.1.1 Line 188: Line 189: OVIRT_VMCONSOLE_ALLOWED_USER = 'ovirt-vmconsole' Line 190: Line 191: OVIRT_VMCONSOLE_ALLOWED_GROUP = 'ovirt-vmconsole' https://gerrit.ovirt.org/#/c/35906/38/packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/config.py File packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/config.py: Line 246: 'ENGINE_HTTPS_ENABLED={directFlag}\n' Line 247: 'ENGINE_HTTP_PORT={directHttpPort}\n' Line 248: 'ENGINE_HTTPS_PORT={directHttpsPort}\n' Line 249: 'SSL_CERTIFICATE={certificate}\n' Line 250: 'SSL_KEY={key}\n' all you need is: ENGINE_BASE_URL and create it via setup variables or ask user if this is detached component. I can only guess that SSL_* are actually the token key/certificate. missing CA to trust for ssl connection, this is complex as if we are running on engine it can be defaulted to apache-ca.pem but if not we need to acquire it someone, as it can differ from the engine certificate or be the same. Line 251: ).format( Line 252: fqdn=self.environment[osetupcons.ConfigEnv.FQDN], Line 253: proxyFlag=flag(self.environment[ Line 254: oengcommcons.ConfigEnv.JBOSS_AJP_PORT Line 335: ] Line 336: ) Line 337: Line 338: Line 339: def _existsUserGroup(log, user, group): I think you have such in utils, getUid, getGid. Line 340: try: Line 341: pwd.getpwnam(user) Line 342: except KeyError: Line 343: log.warn(_('User {user} does not exist.'.format(user=user))) Line 353: Line 354: Line 355: def _copyVMConsoleProxyConfig(dialog, conf_file, vmconsole_confdir): Line 356: if os.geteuid() == 0: Line 357: shutil.copy2(conf_file, vmconsole_confdir) please always use transactions. please check for DEVELOPER_MODE env and not os properties. Line 358: else: Line 359: cp_cmd = 'cp -p {conf_file} {vmconsole_confdir}'.format( Line 360: conf_file=conf_file, Line 361: vmconsole_confdir=vmconsole_confdir Line 358: else: Line 359: cp_cmd = 'cp -p {conf_file} {vmconsole_confdir}'.format( Line 360: conf_file=conf_file, Line 361: vmconsole_confdir=vmconsole_confdir Line 362: ) you can remove the cp_cmd temp var... Line 363: Line 364: dialog.note( Line 365: _( Line 366: 'Manual intervention is required, because ' https://gerrit.ovirt.org/#/c/35906/38/packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/pki.py File packaging/setup/plugins/ovirt-engine-setup/vmconsole_proxy/pki.py: letting someone from integration to review this, as mechanism is too complex for me to understand. Line 1: # Line 2: # ovirt-engine-setup -- ovirt engine setup Line 3: # Copyright (C) 2014-2015 Red Hat, Inc. Line 4: # -- 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: 38 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: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@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-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
