Francesco Romani 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: Done 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 fixed (actually just renamed) as per comments in config.py 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 fixed as per comments in config.py 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? Done 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: Done 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. Got it. Not changed yet 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 Cons Done 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 will change and update the servlet accordingly 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: Yes, SSL_* are actually token key/cert. Will rename accordingly. Will default to apache-ca.pem for the next upload. 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. Done 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. Done 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... Done 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 comple Sure, waiting. 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
