Merge lp:~frankban/juju-quickstart/jenv-env into lp:juju-quickstart

[Francesco Banconi]

Use both credentials to connect to Juju.

Use both the user name and password when connecting
to the Juju WebSocket API.

Update the jenv models to reflect what we expect to
find in the jenv file.

QA:
- use quickstart as usual
  (.venv/bin/python juju-quickstart ...);
- the user is now printed to stdout;
- the environment and the GUI log in correctly,
  in the case the environment is bootstrapped by
  quickstart or already there.

https://codereview.appspot.com/190060043/

[Francesco Banconi (frankban)]

Reviewers: mp+244772_code.launchpad.net,

Message:
Please take a look.

Description:
Use both credentials to connect to Juju.

Use both the user name and password when connecting
to the Juju WebSocket API.

Update the jenv models to reflect what we expect to
find in the jenv file.

QA:
- use quickstart as usual
   (.venv/bin/python juju-quickstart ...);
- the user is now printed to stdout;
- the environment and the GUI log in correctly,
   in the case the environment is bootstrapped by
   quickstart or already there.

https://code.launchpad.net/~frankban/juju-quickstart/jenv-env/+merge/244772

(do not edit description out of merge proposal)

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

Affected files (+173, -44 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/models/jenv.py
   M quickstart/tests/models/test_jenv.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_manage.py

[Jay R. Wren (evarlast)]

On 2014/12/15 17:51:50, frankban wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/190060043/

[Richard Harding (rharding)]

LGTM no qa. Love it when things are as straight forward as this seems.

https://codereview.appspot.com/190060043/

[Brad Crittenden (bac)]

QA - OK

I was surprised to see the username printed as 'user-bac' (because I
hadn't noticed that part of the code).

Why do we have the user- prefix? Is that a juju-gui thing?

I don't think we should make that user visible, i.e. don't print 'user-'
when showing the credentials being used. The prefix does not appear in
the .jenv file or 'juju user list'.

https://codereview.appspot.com/190060043/

[Francesco Banconi (frankban)]

On 2014/12/15 19:54:50, bac wrote:
> QA - OK

> I was surprised to see the username printed as 'user-bac' (because I
hadn't
> noticed that part of the code).

> Why do we have the user- prefix? Is that a juju-gui thing?

> I don't think we should make that user visible, i.e. don't print
'user-' when
> showing the credentials being used. The prefix does not appear in the
.jenv file
> or 'juju user list'.

Hi Brad,

the use prefix is required when loggin in to the Juju API. But I agree
with you:
hiding it in the UX could be a good idea, especially because I checked
with
the GUI trunk and we already do that in the login form.
I'll merge this and propose a follow up branch to hide the prefix.
Thank you for the QA and thank you all for the reviews!

https://codereview.appspot.com/190060043/

[Francesco Banconi (frankban)]

*** Submitted:

Use both credentials to connect to Juju.

Use both the user name and password when connecting
to the Juju WebSocket API.

Update the jenv models to reflect what we expect to
find in the jenv file.

QA:
- use quickstart as usual
   (.venv/bin/python juju-quickstart ...);
- the user is now printed to stdout;
- the environment and the GUI log in correctly,
   in the case the environment is bootstrapped by
   quickstart or already there.

R=jay.wren, rharding, bac
CC=
https://codereview.appspot.com/190060043

https://codereview.appspot.com/190060043/

=== modified file 'quickstart/app.py'
--- quickstart/app.py	2014-11-12 16:40:54 +0000
+++ quickstart/app.py	2014-12-15 17:51:32 +0000
@@ -293,16 +293,16 @@
         raise ProgramExit(msg)
 
 
-def get_admin_secret(env_name):
-    """Return the Juju admin secret for the given environment name.
+def get_credentials(env_name):
+    """Return the Juju credentials for the given environment name.
 
-    Parse the jenv file to retrieve the admin secret.
-    Raise a ProgramExit if the admin secret cannot be retrieved.
+    Parse the jenv file to retrieve the environment user name and password.
+    Raise a ProgramExit if the credentials cannot be retrieved.
     """
     try:
-        return jenv.get_value(env_name, 'bootstrap-config', 'admin-secret')
+        return jenv.get_credentials(env_name)
     except ValueError as err:
-        msg = b'cannot retrieve environment admin-secret: {}'.format(err)
+        msg = b'cannot retrieve environment credentials: {}'.format(err)
         raise ProgramExit(msg)
 
 
@@ -323,7 +323,7 @@
     return 'wss://{}'.format(api_address)
 
 
-def connect(api_url, admin_secret):
+def connect(api_url, username, password):
     """Connect to the Juju API server and log in using the given credentials.
 
     Return a connected and authenticated Juju Environment instance.
@@ -346,7 +346,7 @@
         else:
             break
     try:
-        env.login(admin_secret)
+        env.login(password, user=username)
     except jujuclient.EnvError as err:
         msg = 'unable to log in to the Juju API server on {}: {}'
         raise ProgramExit(msg.format(api_url, err.message))
 
=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2014-11-12 08:48:35 +0000
+++ quickstart/manage.py	2014-12-15 17:51:32 +0000
@@ -542,10 +542,11 @@
         api_url = app.get_api_url(options.env_name, juju_command)
 
     # Retrieve the admin-secret for the current environment.
-    admin_secret = app.get_admin_secret(options.env_name)
+    print('retrieving the Juju environment credentials')
+    username, password = app.get_credentials(options.env_name)
 
     print('connecting to {}'.format(api_url))
-    env = app.connect(api_url, admin_secret)
+    env = app.connect(api_url, username, password)
 
     # Inspect the environment and deploy the charm if required.
     charm_url, machine, service_data, unit_data = app.check_environment(
@@ -559,10 +560,11 @@
     address = app.watch(env, unit_name)
     env.close()
     url = 'https://{}'.format(address)
-    print('\nJuju GUI URL: {}\npassword: {}\n'.format(url, admin_secret))
+    print('\nJuju GUI URL: {}\nusername: {}\npassword: {}\n'.format(
+        url, username, password))
     gui_api_url = 'wss://{}:443/ws'.format(address)
     print('connecting to the Juju GUI server')
-    gui_env = app.connect(gui_api_url, admin_secret)
+    gui_env = app.connect(gui_api_url, username, password)
 
     # Handle bundle deployment.
     if options.bundle is not None:
 
=== modified file 'quickstart/models/jenv.py'
--- quickstart/models/jenv.py	2014-11-11 13:21:57 +0000
+++ quickstart/models/jenv.py	2014-12-15 17:51:32 +0000
@@ -22,6 +22,7 @@
 
 from __future__ import unicode_literals
 
+import logging
 import os
 
 from quickstart import (
@@ -30,6 +31,12 @@
 )
 
 
+# Define the constant tag used by Juju for user entities.
+JUJU_USER_TAG = 'user'
+# Define the default Juju user when an environment is initially bootstrapped.
+JUJU_DEFAULT_USER = 'admin'
+
+
 def exists(env_name):
     """Report whether the jenv generated file exists for the given env_name."""
     jenv_path = _get_jenv_path(env_name)
@@ -52,13 +59,73 @@
     """
     jenv_path = _get_jenv_path(env_name)
     data = serializers.yaml_load_from_path(jenv_path)
+    try:
+        return _get_value_from_yaml(data, *args)
+    except ValueError as err:
+        raise ValueError(
+            b'cannot read {}: {}'.format(jenv_path.encode('utf-8'), err))
+
+
+def get_credentials(env_name):
+    """Return the Juju environment credentials stored in the jenv file.
+
+    The credentials are returned as a tuple (username, password).
+
+    Raise a ValueError if:
+        - the environment file is not found;
+        - the environment file contents are not parsable by YAML;
+        - the credentials cannot be found.
+    """
+    jenv_path = _get_jenv_path(env_name)
+    data = serializers.yaml_load_from_path(jenv_path)
+
+    # Retrieve the user name.
+    try:
+        user = _get_value_from_yaml(data, 'user')
+    except ValueError as err:
+        # This is probably an old version of Juju not supporting multiple
+        # users. Return the default user name.
+        logging.warn('cannot retrieve the user name from {}: {}'.format(
+            jenv_path, bytes(err).decode('utf-8')))
+        user = JUJU_DEFAULT_USER
+
+    # Retrieve the password.
+    try:
+        password = _get_value_from_yaml(data, 'password')
+    except ValueError as err:
+        # This is probably an old version of Juju not supporting multiple
+        # users. Fall back to the admin secret.
+        msg = b'cannot retrieve the password from {}: '.format(
+            jenv_path.encode('utf-8'))
+        logging.debug(msg + bytes(err))
+        try:
+            password = _get_value_from_yaml(
+                data, 'bootstrap-config', 'admin-secret')
+        except ValueError as err:
+            raise ValueError(msg + bytes(err))
+
+    return '{}-{}'.format(JUJU_USER_TAG, user), password
+
+
+def _get_value_from_yaml(data, *args):
+    """Read and return a value from the given YAML decoded data.
+
+    Return the value corresponding to the specified args sequence.
+    For instance, if args is ('bootstrap-config', 'admin-secret'), the value
+    associated with the "admin-secret" key included on the "bootstrap-config"
+    section is returned.
+
+    Raise a ValueError if:
+        - the YAML contents are not properly structured;
+        - one the keys in args is not found.
+    """
     section = 'root'
     for key in args:
         try:
             data = data[key]
         except (KeyError, TypeError):
-            msg = ('invalid YAML contents in {}: ''{} key not found in the {} '
-                   'section'.format(jenv_path, key, section))
+            msg = ('invalid YAML contents: {} key not found in the {} section'
+                   ''.format(key, section))
             raise ValueError(msg.encode('utf-8'))
         section = key
     return data
 
=== modified file 'quickstart/tests/models/test_jenv.py'
--- quickstart/tests/models/test_jenv.py	2014-11-11 13:21:57 +0000
+++ quickstart/tests/models/test_jenv.py	2014-12-15 17:51:32 +0000
@@ -87,8 +87,8 @@
         # A ValueError is raised if the specified section cannot be found.
         with self.make_jenv('local', yaml.safe_dump(self.jenv_data)) as path:
             expected_error = (
-                'invalid YAML contents in {}: no-such key not found in the '
-                'root section'.format(path))
+                'cannot read {}: invalid YAML contents: no-such key not found '
+                'in the root section'.format(path))
             with self.assert_value_error(expected_error):
                 jenv.get_value('local', 'no-such')
 
@@ -96,8 +96,8 @@
         # A ValueError is raised if the specified subsection cannot be found.
         with self.make_jenv('local', yaml.safe_dump(self.jenv_data)) as path:
             expected_error = (
-                'invalid YAML contents in {}: series key not found in the '
-                'bootstrap-config section'.format(path))
+                'cannot read {}: invalid YAML contents: series key not found '
+                'in the bootstrap-config section'.format(path))
             with self.assert_value_error(expected_error):
                 jenv.get_value('local', 'bootstrap-config', 'series')
 
@@ -108,3 +108,56 @@
             with self.assertRaises(ValueError) as context_manager:
                 jenv.get_value('ec2')
         self.assertIn(expected_error, bytes(context_manager.exception))
+
+
+class TestGetCredentials(
+        helpers.JenvFileTestsMixin, helpers.ValueErrorTestsMixin,
+        unittest.TestCase):
+
+    def test_valid_credentials(self):
+        # The user name and password are correctly returned.
+        data = {'user': 'jean-luc', 'password': 'Secret!'}
+        with self.make_jenv('local', yaml.safe_dump(data)):
+            username, password = jenv.get_credentials('local')
+        self.assertEqual('user-jean-luc', username)
+        self.assertEqual('Secret!', password)
+
+    def test_default_user(self):
+        # The default user name is returned if it's not possible to retrieve it
+        # otherwise.
+        data = {'password': 'Secret!'}
+        with self.make_jenv('local', yaml.safe_dump(data)) as path:
+            expected_logs = [
+                'cannot retrieve the user name from {}: '
+                'invalid YAML contents: '
+                'user key not found in the root section'.format(path)]
+            with helpers.assert_logs(expected_logs, 'warn'):
+                username, password = jenv.get_credentials('local')
+        self.assertEqual('user-admin', username)
+        self.assertEqual('Secret!', password)
+
+    def test_using_admin_secret(self):
+        # If the password is not found in the jenv file, the admin secret is
+        # returned if present.
+        data = {
+            'user': 'who',
+            'bootstrap-config': {'admin-secret': 'Admin!'},
+        }
+        with self.make_jenv('local', yaml.safe_dump(data)) as path:
+            expected_logs = [
+                'cannot retrieve the password from {}: '
+                'invalid YAML contents: '
+                'password key not found in the root section'.format(path)]
+            with helpers.assert_logs(expected_logs, 'debug'):
+                username, password = jenv.get_credentials('local')
+        self.assertEqual('Admin!', password)
+
+    def test_password_not_found(self):
+        # A ValueError is raised if the password cannot be found anywhere.
+        with self.make_jenv('local', yaml.safe_dump({})) as path:
+            expected_error = (
+                'cannot retrieve the password from {}: '
+                'invalid YAML contents: bootstrap-config key '
+                'not found in the root section'.format(path))
+            with self.assert_value_error(expected_error):
+                jenv.get_credentials('local')
 
=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2014-11-12 12:21:09 +0000
+++ quickstart/tests/test_app.py	2014-12-15 17:51:32 +0000
@@ -469,7 +469,7 @@
         with self.make_jenv('ec2', '') as path:
             logs = [
                 'cannot retrieve the Juju API URL: '
-                'invalid YAML contents in {}: '
+                'cannot read {}: invalid YAML contents: '
                 'state-servers key not found in the root section'.format(path)
             ]
             with helpers.assert_logs(logs, level='warn'):
@@ -713,31 +713,32 @@
         # A ProgramExit is raised if the environment type cannot be retrieved.
         with self.make_jenv('aws', '') as path:
             expected_error = (
-                'cannot retrieve environment type: invalid YAML '
-                'contents in {}: bootstrap-config key not found in the root '
+                'cannot retrieve environment type: cannot read {}: invalid '
+                'YAML contents: bootstrap-config key not found in the root '
                 'section'.format(path))
             with self.assert_program_exit(expected_error):
                 app.get_env_type('aws')
 
 
-class TestGetAdminSecret(
+class TestGetCredentials(
         helpers.JenvFileTestsMixin, ProgramExitTestsMixin, unittest.TestCase):
 
     def test_success(self):
-        # The admin secret is successfully retrieved.
+        # The user name and password are successfully retrieved.
         with self.make_jenv('ec2', yaml.safe_dump(self.jenv_data)):
-            admin_secret = app.get_admin_secret('ec2')
-        self.assertEqual('Secret!', admin_secret)
+            username, password = app.get_credentials('ec2')
+        self.assertEqual('user-admin', username)
+        self.assertEqual('Secret!', password)
 
     def test_error(self):
-        # A ProgramExit is raised if the admin secret cannot be retrieved.
+        # A ProgramExit is raised if the credentials cannot be retrieved.
         with self.make_jenv('ec2', '') as path:
             expected_error = (
-                'cannot retrieve environment admin-secret: invalid YAML '
-                'contents in {}: bootstrap-config key not found in the root '
-                'section'.format(path))
+                'cannot retrieve environment credentials: cannot retrieve the '
+                'password from {}: invalid YAML contents: bootstrap-config '
+                'key not found in the root section'.format(path))
             with self.assert_program_exit(expected_error):
-                app.get_admin_secret('ec2')
+                app.get_credentials('ec2')
 
 
 class TestGetApiUrl(
@@ -768,16 +769,18 @@
 
 class TestConnect(ProgramExitTestsMixin, unittest.TestCase):
 
-    admin_secret = 'Secret!'
+    username = 'MyUser'
+    password = 'Secret!'
     api_url = 'wss://api.example.com:17070'
 
     def test_connection_established(self):
         # The connection is done and the Environment instance is returned.
         with mock.patch('quickstart.juju.connect') as mock_connect:
-            env = app.connect(self.api_url, self.admin_secret)
+            env = app.connect(self.api_url, self.username, self.password)
         mock_connect.assert_called_once_with(self.api_url)
         mock_env = mock_connect()
-        mock_env.login.assert_called_once_with(self.admin_secret)
+        mock_env.login.assert_called_once_with(
+            self.password, user=self.username)
         self.assertEqual(mock_env, env)
 
     @mock.patch('time.sleep')
@@ -788,7 +791,7 @@
         expected = 'unable to connect to the Juju API server on {}: bad wolf'
         with mock.patch('quickstart.juju.connect', mock_connect):
             with self.assert_program_exit(expected.format(self.api_url)):
-                app.connect(self.api_url, self.admin_secret)
+                app.connect(self.api_url, self.username, self.password)
         mock_connect.assert_called_with(self.api_url)
         self.assertEqual(30, mock_connect.call_count)
         mock_sleep.assert_called_with(1)
@@ -805,10 +808,11 @@
         mock_connect = mock.Mock(
             side_effect=[ValueError('bad wolf'), mock_env])
         with mock.patch('quickstart.juju.connect', mock_connect):
-            env = app.connect(self.api_url, self.admin_secret)
+            env = app.connect(self.api_url, self.username, self.password)
         mock_connect.assert_called_with(self.api_url)
         self.assertEqual(2, mock_connect.call_count)
-        mock_env.login.assert_called_once_with(self.admin_secret)
+        mock_env.login.assert_called_once_with(
+            self.password, user=self.username)
         self.assertEqual(mock_env, env)
         mock_sleep.assert_called_once_with(1)
         expected = 'unable to connect to the Juju API server on {}: bad wolf'
@@ -822,9 +826,10 @@
             mock_login = mock_connect().login
             mock_login.side_effect = self.make_env_error('bad wolf')
             with self.assert_program_exit(expected.format(self.api_url)):
-                app.connect(self.api_url, self.admin_secret)
+                app.connect(self.api_url, self.username, self.password)
         mock_connect.assert_called_with(self.api_url)
-        mock_login.assert_called_once_with(self.admin_secret)
+        mock_login.assert_called_once_with(
+            self.password, user=self.username)
 
     def test_other_errors(self):
         # Any other errors occurred during the log in process are not trapped.
@@ -833,7 +838,7 @@
             mock_login = mock_connect().login
             mock_login.side_effect = error
             with self.assertRaises(ValueError) as context_manager:
-                app.connect(self.api_url, self.admin_secret)
+                app.connect(self.api_url, self.username, self.password)
         self.assertIs(error, context_manager.exception)
 
 
 
=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py	2014-11-12 14:10:26 +0000
+++ quickstart/tests/test_manage.py	2014-12-15 17:51:32 +0000
@@ -787,8 +787,8 @@
             'status': 'trusty',
             # The API URL must be retrieved (the environment was not ready).
             'get_api_url': 'wss://1.2.3.4:17070',
-            # Retrieve the admin secret.
-            'get_admin_secret': 'Secret!',
+            # Retrieve the environment credentials.
+            'get_credentials': ('MyUser', 'Secret!'),
             # Connect to the Juju Environment API endpoint.
             'connect': env,
             # The environment is then checked.
@@ -838,11 +838,11 @@
             options.env_name, self.juju_command)
         mock_app.get_api_url.assert_called_once_with(
             options.env_name, self.juju_command)
-        mock_app.get_admin_secret.assert_called_once_with(options.env_name)
+        mock_app.get_credentials.assert_called_once_with(options.env_name)
         mock_app.connect.assert_has_calls([
-            mock.call('wss://1.2.3.4:17070', 'Secret!'),
+            mock.call('wss://1.2.3.4:17070', 'MyUser', 'Secret!'),
             mock.call().close(),
-            mock.call('wss://1.2.3.4:443/ws', 'Secret!'),
+            mock.call('wss://1.2.3.4:443/ws', 'MyUser', 'Secret!'),
             mock.call().close(),
         ])
         mock_app.check_environment.assert_called_once_with(