Merge lp:~ankatare/juju-ci-tools/juju-aws-add-credential into lp:juju-ci-tools

[ABHAY]

code changes for aws add-credential --in progess

First stage coding is done for review; modified most of the code.

[Christopher Lee (veebers)]

Some overall comments:
  - run "make lint" often, it will catch any of the smaller errors that might creep in (not there is a couple issues with this branch it will catch).

  - Please don't delete your branch and re-propose unless it's really needed (in this case we lose the previous comments from the original PR).

  - Tests are needed for the methods added. In most cases you'll have more joy if you write the tests first, among other things it helps you layout the structure of the code before you start implementing things.

I've had to repeat a couple of comments here, please if you disagree with a comment or have a question about it please comment why that is so we can have a discussion etc. (another reason not to delete the merge proposal).

Please see inline comments regarding code specifics.

review: Needs Fixing

[Christopher Lee (veebers)]

As requested in the previous review, please write tests for the methods added to jujupy and to cover the methods added to assess_add_credentials.py.

Please see inline comments for further review comments.

review: Needs Fixing

[Christopher Lee (veebers)]

Added missing comment.

[Christopher Lee (veebers)]

Added further comment from discussion in our meeting.

[Christopher Lee (veebers)]

This is looking good. Just a couple of minor comments.

Don't forget that there needs to be tests.
Also, don't forget to run 'make lint' before pushing. There is currently 1 lint error in this branch.

review: Needs Fixing

[Christopher Lee (veebers)]

Running the tests I get a fail: http://pastebin.ubuntu.com/23855616/
This is related to test_get_aws_credentials_from_file's call to get_aws_credentials_from_file actually trying to read that file but it doesn't actually exist (it might for you on you machine?).
I've included notes on resolving this inline with the test.

I see that tests for changes to jujupy/client.py are still incoming.

review: Needs Fixing

[viswesuwara nathan (viswesn)]

It's good to have unit testing for
1) assess_aws_credentials,
2) add_credential
3) list_credential.

I also specified couple of comments (see below)

[ABHAY (ankatare)]

> It's good to have unit testing for
> 1) assess_aws_credentials, ( started working on )
> 2) add_credential ( comment incorporated)
> 3) list_credential. ( comment incorporated)
>
> I also specified couple of comments (see below)

review: Approve

[viswesuwara nathan (viswesn)]

Find the review comments specified. Thanks

[ABHAY (ankatare)]

All comments incorporated.

[viswesuwara nathan (viswesn)]

Please find my review comments. And most of my earlier review comments were not addressed. Please address them now and I specified them now too.

[ABHAY (ankatare)]

All Review comments Incorporated Now

[viswesuwara nathan (viswesn)]

looks good.

[Christopher Lee (veebers)]

Your docstrings are using single quotes (') when they should be (") please remedy this. (it might seem pedantic but code keeping within the same coding standard is much easier to read and modify).

Docstrings descriptions and the param statements are intended to state what the intention of the function is and the param statements are there to state what the param is expected to look like (e.g. a list, dict, object of some type) as well as what it's used for (e.g. file object from which to read credential details from.)

Is there a reason why you didn't take my suggestion in the email I sent re: fixing the test_assess_aws_credential (separate out internals into functions to make it easier to test)?
If you had concerns about it that's ok but please state why etc.
Otherwise, the test test_assess_aws_credential is doing _way_ to much to be a useful unit test as it's testing almost everything else within that function (that could be tested interdependently)

Some considerations for some upcoming branches for this code && tests, please think on how you might be able to update this code to handle checking credentials for multiple substrates (GCE etc.) and how those credentials might be passed into the test (i.e. is it a single file with multiple entries in it, or should the test script get run once per substrate, or query the client object that gets created as to what credentials it knows, grab them and wipe the clients memory and proceed from there).
We can discuss a bit further in depth these ideas in one of our upcoming meetings.

review: Needs Fixing

[ABHAY (ankatare)]

Review Comments Incorporated

[lp:~ankatare/juju-ci-tools/juju-aws-add-credential]

1852. By ABHAY on 2017-02-08

code changes to incorporate review comments

1853. By ABHAY on 2017-02-09

small code change as discussed in today's call

1854. By ABHAY on 2017-02-09

code change to add phrase

[Christopher Lee (veebers)]

Getting there, just some small fixes. Also waiting on the test that you have in progress.

review: Needs Fixing

[lp:~ankatare/juju-ci-tools/juju-aws-add-credential]

1855. By ABHAY on 2017-02-13

incorporated review comments

[Christopher Lee (veebers)]

Looking good, just a couple of minor cleanup things left.

Regarding unit test function names, they need to be improved. They need to be more than test_<function name>, the name should describe what the test is intended to confirm.

i.e. test_this_function_raises_when_something_doesnt_match().

This is almost approved. Once it is well sit it here until the boostrap branch of this is done too as that will remove the need for passing in a AWS file and will instead use the credential details the client knows about (as per CI testing.)

review: Approve

[Christopher Lee (veebers)]

Sorry that last comment should have been 'needs fixing' I had approved on the mind. (It's so very close to being approved though ^_^).

review: Needs Fixing

[lp:~ankatare/juju-ci-tools/juju-aws-add-credential]

1856. By ABHAY on 2017-02-14

incorporated review comments

[ABHAY (ankatare)]

Waiting for your Review

[Christopher Lee (veebers)]

LGTM, thanks for that.

review: Approve

Unmerged revisions

1856. By ABHAY on 2017-02-14

incorporated review comments

1855. By ABHAY on 2017-02-13

incorporated review comments

1854. By ABHAY on 2017-02-09

code change to add phrase

1853. By ABHAY on 2017-02-09

small code change as discussed in today's call

1852. By ABHAY on 2017-02-08

code changes to incorporate review comments

1851. By ABHAY on 2017-02-03

code changes for test case assess_aws_credential

1850. By ABHAY on 2017-02-03

minor code change to incorporate review comments

1849. By ABHAY on 2017-02-02

Review comments incorporated

1848. By ABHAY on 2017-02-02

make lint error removed

1847. By ABHAY on 2017-02-02

review comments incorporated

=== added file 'assess_add_credentials.py'
--- assess_add_credentials.py	1970-01-01 00:00:00 +0000
+++ assess_add_credentials.py	2017-02-14 05:01:16 +0000
@@ -0,0 +1,136 @@
+#!/usr/bin/env python
+"""
+ Juju add-credential validation using aws credential yaml file and to
+ verify credentials specified in the yaml file.
+
+ Usage: python assess_add_credentials.py --credential-file=/path/to/yaml_file
+"""
+import yaml
+import csv
+import sys
+import logging
+from argparse import ArgumentParser
+from utility import (
+    add_basic_testing_arguments,
+    JujuAssertionError,
+    temp_yaml_file,
+    )
+from assess_autoload_credentials import (
+    client_from_config,
+    begin_autoload_test,
+    )
+
+log = logging.getLogger("assess_add_credentials")
+
+
+def make_aws_credential_dict(cloud_name, access_key, secret_key):
+    """Returns aws credentials dictionary data structure
+
+    :param cloud_name: cloud name for aws
+    :param access_key: access-key specified in credential file
+    :param secret_key: secret-key specefied in credential file
+    :return: aws credential dictionary
+
+    """
+    return {
+        'credentials': {
+            'aws': {
+                cloud_name: {
+                    'auth-type': 'access-key',
+                    'access-key': access_key,
+                    'secret-key': secret_key,
+                }
+            }
+        }
+    }
+
+
+def verify_aws_add_credentials(
+      client, cloud_name, expected_access_key, expected_secret_key):
+    """Verify that 'cloud_name', credentials are the expected provided values.
+
+    :param client: juju client
+    :param cloud_name: Name of the cloud specified in credential file
+    :param expected_access_key: Expected access-key
+    :param expected_secret_key: Expected secret key
+
+    """
+    actual_credentials_details = yaml.safe_load(
+      client.list_credentials('aws', show_secret=True))
+    actual_credentials = actual_credentials_details[
+      'credentials']['aws']['cloud-credentials'][cloud_name]['details']
+    if expected_access_key != actual_credentials['access-key']:
+        raise JujuAssertionError('AWS access-key mismatch!.')
+    if expected_secret_key != actual_credentials['secret-key']:
+        raise JujuAssertionError('AWS secret-key mismatch!.')
+    log.info("AWS credential validated successfully")
+
+
+def make_aws_cloud_data(credential_file, cloud_name):
+    """Generate aws credentials dict from credential file
+
+    :param credential_file : Expected to be a filepath string.
+
+    """
+    with open(credential_file, 'rt') as acf:
+        access_key, secret_key = get_aws_credentials_from_file(acf)
+    aws_cloud_data = make_aws_credential_dict(
+        cloud_name, access_key, secret_key)
+    return aws_cloud_data, access_key, secret_key
+
+
+def assess_aws_credentials(client_base, credential_file):
+    """Perform juju add-credential using yaml file and verify it.
+
+    :param client_base: juju client
+    :param credential_file: credential_file is expected to be a filepath string
+
+    """
+    cloud_name = 'juju'
+    cloud_details = make_aws_cloud_data(credential_file, cloud_name)
+    cloud_data, access_key, secret_key = cloud_details
+    with temp_yaml_file(cloud_data) as aws_cloud:
+        with begin_autoload_test(client_base) as (client, tmp_scratch_dir):
+            client.add_credential('aws', aws_cloud)
+            verify_aws_add_credentials(
+                client, cloud_name, access_key, secret_key)
+
+
+def get_aws_credentials_from_file(aws_csv_file):
+    """Read credential values from aws credential csv file.
+
+    :param: aws_csv_file: aws_csv_file is expected to be a file-like object.
+
+    """
+    credential_details = {}
+    reader = csv.reader(aws_csv_file, delimiter='=', quotechar='|')
+    for row in reader:
+        credential_details[row[0]] = row[1]
+    return (credential_details['AWSAccessKeyId'],
+            credential_details['AWSSecretKey'])
+
+
+def parse_args(argv):
+    """Parse all arguments."""
+
+    parser = ArgumentParser(
+        description="Testing add-credential command.")
+
+    add_basic_testing_arguments(parser)
+
+    parser.add_argument(
+       '--credential-file', required=True, action='store',
+       help='AWS credential file to be used during bootstrap'
+       )
+    return parser.parse_args(argv)
+
+
+def main(argv=None):
+    args = parse_args(argv)
+    client = client_from_config(args.env, args.juju_bin, False)
+    assess_aws_credentials(client, args.credential_file)
+    return 0
+
+
+if __name__ == '__main__':
+    sys.exit(main())

=== modified file 'jujupy/client.py'
--- jujupy/client.py	2017-02-13 15:14:15 +0000
+++ jujupy/client.py	2017-02-14 05:01:16 +0000
@@ -2674,6 +2674,17 @@
         return self.get_juju_output('list-clouds', '--format',
                                     format, include_e=False)
 
+    def add_credential(self, cloud_name, cloud_file):
+        return self.juju('add-credential',
+                         ("--replace", cloud_name, "-f", cloud_file),
+                         include_e=False)
+
+    def list_credentials(self, cloud_name, show_secret=False, format='json'):
+        args = ('list-credentials', cloud_name, '--format', format)
+        if show_secret:
+            args += ('--show-secrets',)
+        return self.get_juju_output(*args, include_e=False)
+
     def generate_tool(self, source_dir, stream=None):
         args = ('generate-tools', '-d', source_dir)
         if stream is not None:

=== modified file 'jujupy/tests/test_client.py'
--- jujupy/tests/test_client.py	2017-02-13 15:06:00 +0000
+++ jujupy/tests/test_client.py	2017-02-14 05:01:16 +0000
@@ -3291,6 +3291,29 @@
         self.assertEqual(cloned.env.controller.name, controller_name)
         self.assertEqual(fake_client.env.controller.name, 'name')
 
+    def test_add_credential(self):
+        client = ModelClient(JujuData('foo'), None, None)
+        with patch.object(client, 'juju', autospec=True) as mock:
+            client.add_credential('aws', 'credential-file')
+        mock.assert_called_once_with('add-credential', ('--replace', 'aws',
+                                     '-f', 'credential-file'), include_e=False)
+
+    def test_list_credentials(self):
+        env = JujuData('foo')
+        client = ModelClient(env, None, None)
+        with patch.object(client, 'get_juju_output') as mock:
+            client.list_credentials('aws')
+        mock.assert_called_with(
+            'list-credentials', 'aws', '--format', 'json', include_e=False)
+
+    def test_list_credentials_with_show_secret(self):
+        env = JujuData('foo')
+        client = ModelClient(env, None, None)
+        with patch.object(client, 'get_juju_output') as mock:
+            client.list_credentials('aws', 'show_secret')
+        mock.assert_called_with('list-credentials', 'aws', '--format', 'json',
+                                '--show-secrets', include_e=False)
+
     def test_list_clouds(self):
         env = JujuData('foo')
         client = ModelClient(env, None, None)

=== added file 'tests/test_assess_add_credentials.py'
--- tests/test_assess_add_credentials.py	1970-01-01 00:00:00 +0000
+++ tests/test_assess_add_credentials.py	2017-02-14 05:01:16 +0000
@@ -0,0 +1,211 @@
+"""Tests for assess_add_credentials module."""
+
+import assess_add_credentials as aac
+from assess_add_credentials import (
+  make_aws_credential_dict,
+  parse_args,
+  get_aws_credentials_from_file,
+  verify_aws_add_credentials,
+  make_aws_cloud_data,
+  assess_aws_credentials,
+  )
+
+from tests import (
+    TestCase,
+    )
+
+from mock import (
+    Mock,
+    patch,
+    )
+from utility import (
+    JujuAssertionError,
+    )
+import StringIO
+import tempfile
+from textwrap import dedent
+import yaml
+
+
+class TestParseArgs(TestCase):
+
+    def test_common_args(self):
+        credential_file = './rootkey.csv'
+        args = parse_args(["an-env", "/bin/juju", "/tmp/logs",
+                           "an-env-mod", '--credential-file', credential_file])
+        self.assertEqual("an-env", args.env)
+        self.assertEqual("/bin/juju", args.juju_bin)
+        self.assertEqual("/tmp/logs", args.logs)
+        self.assertEqual("an-env-mod", args.temp_env_name)
+        self.assertEqual(False, args.debug)
+        self.assertEqual(credential_file, args.credential_file)
+
+
+class TestawsCredentials(TestCase):
+
+    def test_make_aws_cloud_data(self):
+        exp_access_key = 'KeyId123'
+        exp_secret_key = 'SecretKey321'
+        cloud_name = 'juju'
+        exp_cloud_data = {
+            'credentials': {
+                'aws': {
+                    cloud_name: {
+                        'auth-type': 'access-key',
+                        'access-key': exp_access_key,
+                        'secret-key': exp_secret_key,
+                    }
+                }
+            }
+        }
+        with tempfile.NamedTemporaryFile() as fp:
+            fp.write(dedent("""\
+                   AWSAccessKeyId=KeyId123
+                   AWSSecretKey=SecretKey321
+                     """))
+            fp.flush()
+
+            actual_output = make_aws_cloud_data(fp.name, cloud_name)
+            cloud_data, access_key, secret_key = actual_output
+            actual_cloud_data = cloud_data
+            actual_access_key = access_key
+            actual_secret_key = secret_key
+            self.assertEqual(actual_cloud_data, exp_cloud_data)
+            self.assertEqual(actual_access_key, exp_access_key)
+            self.assertEqual(actual_secret_key, exp_secret_key)
+
+    def test_assess_aws_credential(self):
+        mock_begin_client = Mock()
+        mock_client = Mock()
+        with tempfile.NamedTemporaryFile() as fp:
+            fp.write(dedent("""\
+                    AWSAccessKeyId=KeyId123
+                    AWSSecretKey=SecretKey321
+                      """))
+            fp.flush()
+            with patch.object(
+                    aac, "begin_autoload_test", autospec=True) as m_bat:
+                m_bat.return_value.__enter__.return_value = (
+                    mock_begin_client, 'tmp_dir_path')
+                with patch.object(
+                        aac, 'verify_aws_add_credentials',
+                        autospec=True) as vaac:
+                    assess_aws_credentials(mock_client, fp.name)
+                    vaac.assert_called_once_with(
+                        mock_begin_client, 'juju', 'KeyId123', 'SecretKey321')
+
+    def test_make_aws_credential_dict_returns_dict_with_details(self):
+        cloud_name = 'cloud_name'
+        access_key = 'access_key'
+        secret_key = 'secret_key'
+        actual_output = make_aws_credential_dict(
+          cloud_name, access_key, secret_key)
+        expected_output = {
+         'credentials': {
+            'aws': {
+                cloud_name: {
+                    'auth-type': 'access-key',
+                    'access-key': access_key,
+                    'secret-key': secret_key,
+                  }
+              }
+           }
+        }
+        self.assertDictEqual(actual_output, expected_output)
+
+    def test_get_aws_credentials_from_file(self):
+        expected_aws_access_key = 'TestAccessKey'
+        expected_aws_secret_key = 'TestSecretKey'
+        fake_csv_file_obj = StringIO.StringIO(
+                     dedent("""\
+                 AWSAccessKeyId=TestAccessKey
+                 AWSSecretKey=TestSecretKey
+                 """))
+        actual_aws_access_key, actual_aws_secret_key = \
+            get_aws_credentials_from_file(fake_csv_file_obj)
+        self.assertEqual(actual_aws_access_key, expected_aws_access_key)
+        self.assertEqual(actual_aws_secret_key, expected_aws_secret_key)
+
+    def test_verify_aws_add_credentials_with_correct_values(self):
+        cloud_name = 'cloud_name'
+        access_key = 'test_access_key'
+        secret_key = 'test_secret_key'
+        actual_output = yaml.safe_dump({
+            "credentials": {
+                "aws": {
+                    "cloud-credentials": {
+                        cloud_name: {
+                            "auth-type": "access-key", "details": {
+                                "access-key": "{}".format(access_key),
+                                "secret-key": "{}".format(secret_key)
+                            }
+                        }
+                    }
+                }
+            }
+        })
+        mock_client = Mock()
+        with patch.object(mock_client, "list_credentials",
+                          return_value=actual_output, autospec=True):
+            verify_aws_add_credentials(
+                mock_client, cloud_name, access_key, secret_key)
+        self.assertIn("AWS credential validated successfully",
+                      self.log_stream.getvalue())
+
+    def test_verify_aws_add_credentials_with_access_key_mismatch(self):
+        cloud_name = 'cloud_name'
+        actual_access_key = 'test_access_key'
+        actual_secret_key = 'test_secret_key'
+        expected_access_key = 'access_key'
+        expected_secret_key = 'test_secret_key'
+        actual_output = yaml.safe_dump({
+            "credentials": {
+                "aws": {
+                    "cloud-credentials": {
+                        cloud_name: {
+                            "auth-type": "access-key", "details": {
+                                "access-key": "{}".format(actual_access_key),
+                                "secret-key": "{}".format(actual_secret_key)
+                            }
+                        }
+                    }
+                }
+            }
+        })
+        mock_client = Mock()
+        with patch.object(mock_client, "list_credentials",
+                          return_value=actual_output, autospec=True):
+            with self.assertRaisesRegexp(JujuAssertionError,
+                                         'AWS access-key mismatch!.'):
+                verify_aws_add_credentials(
+                    mock_client, cloud_name, expected_access_key,
+                    expected_secret_key)
+
+    def test_verify_aws_add_credentials_with_secret_key_mismatch(self):
+        cloud_name = 'cloud_name'
+        actual_access_key = 'test_access_key'
+        actual_secret_key = 'test_secret_key'
+        expected_access_key = 'test_access_key'
+        expected_secret_key = 'secret_key'
+        actual_output = yaml.safe_dump({
+            "credentials": {
+                "aws": {
+                    "cloud-credentials": {
+                        cloud_name: {
+                            "auth-type": "access-key", "details": {
+                                "access-key": "{}".format(actual_access_key),
+                                "secret-key": "{}".format(actual_secret_key)
+                            }
+                        }
+                    }
+                }
+            }
+        })
+        mock_client = Mock()
+        with patch.object(mock_client, "list_credentials",
+                          return_value=actual_output, autospec=True):
+            with self.assertRaisesRegexp(JujuAssertionError,
+                                         'AWS secret-key mismatch!.'):
+                verify_aws_add_credentials(
+                    mock_client, cloud_name, expected_access_key,
+                    expected_secret_key)