Merge lp:~gary/charms/precise/juju-gui/credentialConfig into lp:~juju-gui/charms/precise/juju-gui/trunk

[Gary Poster]

Adjust formatting

https://codereview.appspot.com/14226043/

[Gary Poster (gary)]

Reviewers: mp+188670_code.launchpad.net,

Message:
Please take a look.

Description:
Add password option to charm

In order to support ecosystems work, add a password option that lets the
charm pre-fill the password in the GUI.

To QA, start up the charm, and verify that you need to enter your
password in the GUI. Do not log in, or if you do, log out afterwards.
Next, juju set juju-gui password=[YOUR ADMIN SECRET] and wait about 20
seconds for the charm to reconfigure itself. Then go to the browser
again and you should not have to log in.

https://code.launchpad.net/~gary/charms/precise/juju-gui/credentialConfig/+merge/188670

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14226043/

Affected files (+333, -16 lines):
   M Makefile
   A [revision details]
   M config.yaml
   M hooks/backend.py
   A hooks/charmhelpers.py
   M hooks/install
   M hooks/utils.py
   M tests/00-setup
   M tests/test_utils.py

[Gary Poster (gary)]

Added comment

https://codereview.appspot.com/14226043/diff/1/hooks/charmhelpers.py
File hooks/charmhelpers.py (right):

https://codereview.appspot.com/14226043/diff/1/hooks/charmhelpers.py#newcode4
hooks/charmhelpers.py:4: # This file is taken from the
python-charmhelpers package. It is possibly no
I added this file because tests wouldn't pass without it even though it
was installed on my system, and for the reasons I describe in the
comment. Please only review the comments I added. The rest is
unnecessary.

https://codereview.appspot.com/14226043/

[Gary Poster (gary)]

*** Submitted:

Add password option to charm

In order to support ecosystems work, add a password option that lets the
charm pre-fill the password in the GUI.

To QA, start up the charm, and verify that you need to enter your
password in the GUI. Do not log in, or if you do, log out afterwards.
Next, juju set juju-gui password=[YOUR ADMIN SECRET] and wait about 20
seconds for the charm to reconfigure itself. Then go to the browser
again and you should not have to log in.

R=
CC=
https://codereview.appspot.com/14226043

https://codereview.appspot.com/14226043/

[Gary Poster (gary)]

On 2013/10/01 19:21:33, benji wrote:
> LGTM

> https://codereview.appspot.com/14226043/diff/1/Makefile
> File Makefile (right):

> https://codereview.appspot.com/14226043/diff/1/Makefile#newcode20
> Makefile:20: libpython-dev
> Most of the other line continuations use a tab and then four spaces.
Mixing
> tabs and spaces isn't the greatest thing in the world, but neither are
the deep,
> deep indents caused by all-tabs-all-the-time. I'm cool either way.

> https://codereview.appspot.com/14226043/diff/1/hooks/install
> File hooks/install (right):

> https://codereview.appspot.com/14226043/diff/1/hooks/install#newcode24
> hooks/install:24: # We need to install the Juju PPA, which we will do
with a
> It looks like this needs to be word wrapped with the line below.

Argh, sorry, I missed that you had requested changes. Fixing.

https://codereview.appspot.com/14226043/

[Gary Poster (gary)]

*** Submitted:

Adjust formatting

R=
CC=
https://codereview.appspot.com/14226043

https://codereview.appspot.com/14226043/

=== modified file 'Makefile'
--- Makefile	2013-09-25 08:58:17 +0000
+++ Makefile	2013-10-01 17:57:29 +0000
@@ -16,7 +16,8 @@
 
 JUJUTEST = juju-test --timeout=120m -v -e "$(JUJU_ENV)" --upload-tools
 VENV = ./tests/.venv
-SYSDEPS = build-essential bzr libapt-pkg-dev python-pip python-virtualenv xvfb
+SYSDEPS = build-essential bzr libapt-pkg-dev python-pip python-virtualenv xvfb \
+		libpython-dev
 
 all: setup
 

=== modified file 'config.yaml'
--- config.yaml	2013-09-12 19:58:07 +0000
+++ config.yaml	2013-10-01 17:57:29 +0000
@@ -117,6 +117,11 @@
       Do not set unless you understand and accept the risks.
     type: boolean
     default: true
+  password:
+    description: |
+      If given, the password to use for the environment to immediately
+      connect.  Do not set unless you understand and accept the risks.
+    type: string
   sandbox:
     description: |
       Run using an in-memory sandbox rather than a real (or even improv) Juju

=== modified file 'hooks/backend.py'
--- hooks/backend.py	2013-09-12 19:58:07 +0000
+++ hooks/backend.py	2013-10-01 17:57:29 +0000
@@ -148,7 +148,8 @@
             build_dir, secure=config['secure'], sandbox=config['sandbox'],
             ga_key=config['ga-key'],
             default_viewmode=config['default-viewmode'],
-            show_get_juju_button=config['show-get-juju-button'])
+            show_get_juju_button=config['show-get-juju-button'],
+            password=config.get('password'))
         # Expose the service.
         charmhelpers.open_port(80)
         charmhelpers.open_port(443)

=== added file 'hooks/charmhelpers.py'
--- hooks/charmhelpers.py	1970-01-01 00:00:00 +0000
+++ hooks/charmhelpers.py	2013-10-01 17:57:29 +0000
@@ -0,0 +1,288 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# This file is taken from the python-charmhelpers package.  It is possibly no
+# longer maintained there, now that a new charmhelpers package has replaced
+# the one from which this was taken, so the file has been copied here.  We
+# intend to transition to the newer charmhelpers library at some point in the
+# future.
+
+# IMPORTANT: Do not modify this file to add or change functionality.  If you
+# really feel the need to do so, first convert our code to the charmhelpers
+# library, and modify it instead (or modify the helpers or utils module here,
+# as appropriate).
+
+"""Helper functions for writing Juju charms in Python."""
+
+__metaclass__ = type
+__all__ = ['get_config',
+           'log',
+           'log_entry',
+           'log_exit',
+           'relation_get',
+           'relation_set',
+           'relation_ids',
+           'relation_list',
+           'config_get',
+           'unit_get',
+           'open_port',
+           'close_port',
+           'service_control',
+           'unit_info',
+           'wait_for_machine',
+           'wait_for_page_contents',
+           'wait_for_relation',
+           'wait_for_unit',
+           ]
+
+from collections import namedtuple
+import json
+import operator
+from shelltoolbox import (
+    command,
+    script_name,
+    run,
+)
+import tempfile
+import time
+import urllib2
+import yaml
+from subprocess import CalledProcessError
+
+
+SLEEP_AMOUNT = 0.1
+Env = namedtuple('Env', 'uid gid home')
+# We create a juju_status Command here because it makes testing much,
+# much easier.
+juju_status = lambda: command('juju')('status')
+
+
+def log(message, juju_log=command('juju-log')):
+    return juju_log('--', message)
+
+
+def log_entry():
+    log("--> Entering {}".format(script_name()))
+
+
+def log_exit():
+    log("<-- Exiting {}".format(script_name()))
+
+
+def get_config():
+    _config_get = command('config-get', '--format=json')
+    return json.loads(_config_get())
+
+
+def relation_get(attribute=None, unit=None, rid=None):
+    cmd = command('relation-get')
+    if attribute is None and unit is None and rid is None:
+        return cmd().strip()
+    _args = []
+    if rid:
+        _args.append('-r')
+        _args.append(rid)
+    if attribute is not None:
+        _args.append(attribute)
+    if unit:
+        _args.append(unit)
+    return cmd(*_args).strip()
+
+
+def relation_set(**kwargs):
+    cmd = command('relation-set')
+    args = ['{}={}'.format(k, v) for k, v in kwargs.items()]
+    cmd(*args)
+
+
+def relation_ids(relation_name):
+    cmd = command('relation-ids')
+    args = [relation_name]
+    return cmd(*args).split()
+
+
+def relation_list(rid=None):
+    cmd = command('relation-list')
+    args = []
+    if rid:
+        args.append('-r')
+        args.append(rid)
+    return cmd(*args).split()
+
+
+def config_get(attribute):
+    cmd = command('config-get')
+    args = [attribute]
+    return cmd(*args).strip()
+
+
+def unit_get(attribute):
+    cmd = command('unit-get')
+    args = [attribute]
+    return cmd(*args).strip()
+
+
+def open_port(port, protocol="TCP"):
+    cmd = command('open-port')
+    args = ['{}/{}'.format(port, protocol)]
+    cmd(*args)
+
+
+def close_port(port, protocol="TCP"):
+    cmd = command('close-port')
+    args = ['{}/{}'.format(port, protocol)]
+    cmd(*args)
+
+START = "start"
+RESTART = "restart"
+STOP = "stop"
+RELOAD = "reload"
+
+
+def service_control(service_name, action):
+    cmd = command('service')
+    args = [service_name, action]
+    try:
+        if action == RESTART:
+            try:
+                cmd(*args)
+            except CalledProcessError:
+                service_control(service_name, START)
+        else:
+            cmd(*args)
+    except CalledProcessError:
+        log("Failed to perform {} on service {}".format(action, service_name))
+
+
+def configure_source(update=False):
+    source = config_get('source')
+    if ((source.startswith('ppa:') or
+         source.startswith('cloud:') or
+         source.startswith('http:'))):
+        run('add-apt-repository', source)
+    if source.startswith("http:"):
+        run('apt-key', 'import', config_get('key'))
+    if update:
+        run('apt-get', 'update')
+
+
+def make_charm_config_file(charm_config):
+    charm_config_file = tempfile.NamedTemporaryFile()
+    charm_config_file.write(yaml.dump(charm_config))
+    charm_config_file.flush()
+    # The NamedTemporaryFile instance is returned instead of just the name
+    # because we want to take advantage of garbage collection-triggered
+    # deletion of the temp file when it goes out of scope in the caller.
+    return charm_config_file
+
+
+def unit_info(service_name, item_name, data=None, unit=None):
+    if data is None:
+        data = yaml.safe_load(juju_status())
+    service = data['services'].get(service_name)
+    if service is None:
+        # XXX 2012-02-08 gmb:
+        #     This allows us to cope with the race condition that we
+        #     have between deploying a service and having it come up in
+        #     `juju status`. We could probably do with cleaning it up so
+        #     that it fails a bit more noisily after a while.
+        return ''
+    units = service['units']
+    if unit is not None:
+        item = units[unit][item_name]
+    else:
+        # It might seem odd to sort the units here, but we do it to
+        # ensure that when no unit is specified, the first unit for the
+        # service (or at least the one with the lowest number) is the
+        # one whose data gets returned.
+        sorted_unit_names = sorted(units.keys())
+        item = units[sorted_unit_names[0]][item_name]
+    return item
+
+
+def get_machine_data():
+    return yaml.safe_load(juju_status())['machines']
+
+
+def wait_for_machine(num_machines=1, timeout=300):
+    """Wait `timeout` seconds for `num_machines` machines to come up.
+
+    This wait_for... function can be called by other wait_for functions
+    whose timeouts might be too short in situations where only a bare
+    Juju setup has been bootstrapped.
+
+    :return: A tuple of (num_machines, time_taken). This is used for
+             testing.
+    """
+    # You may think this is a hack, and you'd be right. The easiest way
+    # to tell what environment we're working in (LXC vs EC2) is to check
+    # the dns-name of the first machine. If it's localhost we're in LXC
+    # and we can just return here.
+    if get_machine_data()[0]['dns-name'] == 'localhost':
+        return 1, 0
+    start_time = time.time()
+    while True:
+        # Drop the first machine, since it's the Zookeeper and that's
+        # not a machine that we need to wait for. This will only work
+        # for EC2 environments, which is why we return early above if
+        # we're in LXC.
+        machine_data = get_machine_data()
+        non_zookeeper_machines = [
+            machine_data[key] for key in machine_data.keys()[1:]]
+        if len(non_zookeeper_machines) >= num_machines:
+            all_machines_running = True
+            for machine in non_zookeeper_machines:
+                if machine.get('instance-state') != 'running':
+                    all_machines_running = False
+                    break
+            if all_machines_running:
+                break
+        if time.time() - start_time >= timeout:
+            raise RuntimeError('timeout waiting for service to start')
+        time.sleep(SLEEP_AMOUNT)
+    return num_machines, time.time() - start_time
+
+
+def wait_for_unit(service_name, timeout=480):
+    """Wait `timeout` seconds for a given service name to come up."""
+    wait_for_machine(num_machines=1)
+    start_time = time.time()
+    while True:
+        state = unit_info(service_name, 'agent-state')
+        if 'error' in state or state == 'started':
+            break
+        if time.time() - start_time >= timeout:
+            raise RuntimeError('timeout waiting for service to start')
+        time.sleep(SLEEP_AMOUNT)
+    if state != 'started':
+        raise RuntimeError('unit did not start, agent-state: ' + state)
+
+
+def wait_for_relation(service_name, relation_name, timeout=120):
+    """Wait `timeout` seconds for a given relation to come up."""
+    start_time = time.time()
+    while True:
+        relation = unit_info(service_name, 'relations').get(relation_name)
+        if relation is not None and relation['state'] == 'up':
+            break
+        if time.time() - start_time >= timeout:
+            raise RuntimeError('timeout waiting for relation to be up')
+        time.sleep(SLEEP_AMOUNT)
+
+
+def wait_for_page_contents(url, contents, timeout=120, validate=None):
+    if validate is None:
+        validate = operator.contains
+    start_time = time.time()
+    while True:
+        try:
+            stream = urllib2.urlopen(url)
+        except (urllib2.HTTPError, urllib2.URLError):
+            pass
+        else:
+            page = stream.read()
+            if validate(page, contents):
+                return page
+        if time.time() - start_time >= timeout:
+            raise RuntimeError('timeout waiting for contents of ' + url)
+        time.sleep(SLEEP_AMOUNT)

=== modified file 'hooks/install'
--- hooks/install	2013-08-06 09:56:52 +0000
+++ hooks/install	2013-10-01 17:57:29 +0000
@@ -21,10 +21,7 @@
 import json
 import os
 
-# If the user's environment has "juju-origin: ppa" set, they will
-# automatically have access to python-charmhelpers and python-shelltoolbox.
-# However, we want to support environments that use the non-PPA environment as
-# well.  To do so, we need to install the Juju PPA, which we will do with a
+# We need to install the Juju PPA, which we will do with a
 # couple of functions that are actually maintained in python-shelltoolbox.
 import bootstrap_utils
 
@@ -39,8 +36,8 @@
 # Python dependencies must be installed here so that the charm can import and
 # use required libraries.
 PYTHON_DEPENDENCIES = (
-    'python-apt', 'python-charmhelpers', 'python-launchpadlib',
-    'python-shelltoolbox', 'python-tempita',
+    'python-apt', 'python-launchpadlib', 'python-shelltoolbox',
+    'python-tempita',
 )
 bootstrap_utils.run(*(('apt-get', 'install', '-y') + PYTHON_DEPENDENCIES))
 

=== modified file 'hooks/utils.py'
--- hooks/utils.py	2013-09-23 07:33:58 +0000
+++ hooks/utils.py	2013-10-01 17:57:29 +0000
@@ -406,15 +406,15 @@
         console_enabled, login_help, readonly, in_staging, charmworld_url,
         build_dir, secure=True, sandbox=False,
         default_viewmode='sidebar', show_get_juju_button=False,
-        config_js_path=None, ga_key=''):
+        config_js_path=None, ga_key='', password=None):
     """Generate the GUI configuration file."""
     log('Generating the Juju GUI configuration file.')
     is_legacy_juju = legacy_juju()
-    user, password = None, None
-    if (is_legacy_juju and in_staging) or sandbox:
-        user, password = 'admin', 'admin'
-    else:
-        user, password = None, None
+    user = 'admin' if is_legacy_juju else 'user-admin'
+    # Normalize empty string passwords to None.
+    password = password if password else None
+    if password is None and ((is_legacy_juju and in_staging) or sandbox):
+        password = 'admin'
     api_backend = 'python' if is_legacy_juju else 'go'
     if secure:
         protocol = 'wss'
@@ -678,7 +678,7 @@
     release_dir = os.path.join(BASE_DIR, 'release')
     cmd_log(run('rm', '-rf', release_dir))
     os.mkdir(release_dir)
-    uncompress = command('tar', '-x', '-z', '-C', release_dir, '-f')
+    uncompress = command('tar', '-x', '-a', '-C', release_dir, '-f')
     cmd_log(uncompress(release_tarball))
     # Link the Juju GUI dir to the contents of the release tarball.
     cmd_log(run('ln', '-sf', first_path_in_dir(release_dir), JUJU_GUI_DIR))

=== modified file 'tests/00-setup'
--- tests/00-setup	2013-06-11 14:04:04 +0000
+++ tests/00-setup	2013-10-01 17:57:29 +0000
@@ -27,7 +27,8 @@
     if [ ! -f "$ACTIVATE" -o "$REQUIREMENTS" -nt "$ACTIVATE" ]; then
         virtualenv --distribute $VENV
         . $VENV/bin/activate && \
-            yes w | pip install --use-mirrors -r $REQUIREMENTS
+            yes w | env BZR_PLUGIN_PATH='-user' \
+            pip install --use-mirrors -r $REQUIREMENTS
         touch $VENV/bin/activate
     fi
 }

=== added symlink 'tests/charmhelpers.py'
=== target is u'../hooks/charmhelpers.py'
=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2013-09-12 19:58:07 +0000
+++ tests/test_utils.py	2013-10-01 17:57:29 +0000
@@ -791,6 +791,28 @@
         self.assertIn("socket_url: 'ws://", js_conf)
         self.assertIn('socket_protocol: "ws"', js_conf)
 
+    @mock.patch('utils.legacy_juju')
+    def test_write_gui_config_default_python_password(self, mock_legacy_juju):
+        mock_legacy_juju.return_value = True
+        write_gui_config(
+            False, 'This is login help.', True, True, self.charmworld_url,
+            self.build_dir, config_js_path='config',
+            password='kumquat')
+        js_conf = self.files['config']
+        self.assertIn('user: "admin"', js_conf)
+        self.assertIn('password: "kumquat"', js_conf)
+
+    @mock.patch('utils.legacy_juju')
+    def test_write_gui_config_default_go_password(self, mock_legacy_juju):
+        mock_legacy_juju.return_value = False
+        write_gui_config(
+            False, 'This is login help.', True, True, self.charmworld_url,
+            self.build_dir, config_js_path='config',
+            password='kumquat')
+        js_conf = self.files['config']
+        self.assertIn('user: "user-admin"', js_conf)
+        self.assertIn('password: "kumquat"', js_conf)
+
     def test_setup_haproxy_config_insecure(self):
         setup_haproxy_config(self.ssl_cert_path, secure=False)
         # The insecure approach eliminates the https redirect.