Merge lp:~pfalcon/linaro-aws-tools/linaro-ami-ver-1.5 into lp:linaro-aws-tools

[Paul Sokolovsky]

These changes fix/elaborated number of issues as tracked by linked bugs, specifically:

1. Recovers support for --key/--private-key switch (as present in many utils from ec2-api-tools) to specify user's EC2-regestered SSH key, to make sure that object ownership is properly recorded/given user has access to them for diagnostics.

2. Testsuite was elaborated and restructured to clearly separate unit and integration tests. Only quick-running, side-effects free unit tests were set up to run by default. Docs for running tests were updated.

3. Issues faced during working on lp:1019257 were fixed, with regression tests added, as well as fixes and tests for similar conditions.

[Loïc Minier (lool)]

This looks good implementation wise, but do we really need to bother
with the key passing args? I personally rely on boto's env handling to
use different keys for other scripts; see the "run-boto" script in
~linaro/bin on people.linaro.org for an example. This limits the
complexity of the code and keeps the key handling external.

--
Loïc Minier

[Stevan Radaković (stevanr)]

Looks good to me. I still agree with Loic though, as we had a lot of discussions on the same topic internally.

[Stevan Radaković (stevanr)]

Hey Paul, great work.

README file doesn't contain anything about the 2 new (old) config options nor the instructions for the new form of testing remote executor (EC2_KEY instead of old EC2_PRIVATE_KEY).
I also couldn't run remote executor integration tests with any of my previous keys.

Otherwise, changes look good.

review: Needs Fixing

[Paul Sokolovsky (pfalcon)]

> do we really need to bother with the key passing args?

The problem is that there're many those "keys" in AWS, each serving different purpose (and it's not easy to grasp the whole picture). Let's look at some part of the whole picture:

There're AWS_ACCESS_KEY_ID which is kinda login and AWS_SECRET_ACCESS_KEY which is kinda password for that login - for accessing *AWS* API as the whole. But that's not only credentials AWS uses, specific subservices use additional stuff. For example, EC2 requires SSH public key to be registered (assigned id in the form of "key name"), that key name should be specified when creating an instance, and of course, then user needs corresponding private key to access the instance. Let's do ec2-run-instances --help and read:

[]
     -K, --private-key KEY
          Specify KEY as the private key to use. Defaults to the value of the
          EC2_PRIVATE_KEY environment variable (if set). Overrides the default.
[]
     -k, --key KEYPAIR
          Specifies the key pair to use when launching the instance(s).

So, answering the original questions, if we should bother with all those keys, the good answer is that we should, inasmuch as AWS/EC2 itself does (in the form of its standard utilities in particular). Trying to circumvent AWS/EC2 credential process is bad idea because 1) it may lead to unexpected results (including in data integrity/security domain); 2) for people who know AWS/EC2 credentials semantics, it again will lead to unexpected results.

[lp:~pfalcon/linaro-aws-tools/linaro-ami-ver-1.5]

117. By Paul Sokolovsky on 2012-07-17

Update decsription of commands and options.

[Paul Sokolovsky (pfalcon)]

> README file doesn't contain anything about the 2 new (old) config options

Ok, updated.

[Paul Sokolovsky (pfalcon)]

> nor the instructions for the new form of testing remote executor

Weird, I'm sure I was updating that section of README! Now I see That I did that in wrong branch, let me port that over.

[lp:~pfalcon/linaro-aws-tools/linaro-ami-ver-1.5]

118. By Paul Sokolovsky on 2012-07-17

Mention that boto 2.0+ is required.

Earlier versions had different credentials setting method and may miss
some API calls.

(Initially committed to wrong branch).

119. By Paul Sokolovsky on 2012-07-17

Update Tests section for elaborated unit/integration testsuite.

(Initially committed to wrong branch).

[Paul Sokolovsky (pfalcon)]

> Weird, I'm sure I was updating that section of README! Now I see That I did that in wrong branch, let me port that over.

Here they go. That should make it clear how to run integration part of testsuite. In particular, EC2_KEY doesn't replaces EC2_PRIVATE_KEY, they're used both, essentially, EC2_PRIVATE_KEY is the analog of command-line --private-key option, and in the same vein, EC2_KEY is the analog of --key option.

I run complete testsuite as follows (EC2_PRIVATE_KEY is set in env to point to my priv key of keypair regiseterd as pfalcon@home):

EC2_KEY=pfalcon@home nosetests .

Running just unit tests is as easy as

nosetests

[Stevan Radaković (stevanr)]

> > README file doesn't contain anything about the 2 new (old) config options
>
> Ok, updated.

Ok thanks.
Approve +1.

review: Approve (code)

[Paul Sokolovsky (pfalcon)]

Merged, thanks.

=== modified file 'linaro-ami/README'
--- linaro-ami/README	2012-07-13 10:05:48 +0000
+++ linaro-ami/README	2012-07-17 18:43:18 +0000
@@ -22,18 +22,42 @@
   $ export AWS_ACCESS_KEY_ID=EXAMPLEKEY
   $ export AWS_SECRET_ACCESS_KEY=EXAMPLESECRETKEY
 
-Script Options
---------------
-
-Currently, only one command is recognizable, 'create', which takes one
-additional argument which is the AMI name which should be used from the
-config file (see below).
-
-Script also has one configurable option:
-
---config    Path to the optional configuration file. Default value is
-            linaro-ami.conf
-
+Script Commands And Options
+---------------------------
+
+linaro-ami accepts following commands:
+
+ * create
+
+    Create a custom AMI. This command takes one additional argument which
+    is the AMI section name as defined in the config file (see below).
+
+ * list
+
+    List existing custom AMIs. For each AMI, name, AMI id, description,
+    and some other properties are printed.
+
+
+Script also accepts following options:
+
+--config
+
+    Path to the configuration file. Default value is linaro-ami.conf
+    in the current directory.
+
+--key=KEYPAIR
+
+    Name of the EC2-registered SSH keypair name to use when creating
+    work instances. If not supplied, a temporary key will be created
+    and removed once processing finishes. This option is useful for
+    debugging AMI creation process and to ensure that proper ownership
+    information is recorded in EC2 at all times.
+
+--private-key=FILE
+
+    This option should be supplied if --key was (or, alternatively,
+    EC2_PRIVATE_KEY environment variable should be set) and point to
+    private SSH key from the pair as named by --key.
 
 Configuration File Syntax
 -------------------------

=== modified file 'linaro-ami/linaro-ami'
--- linaro-ami/linaro-ami	2012-06-29 07:58:28 +0000
+++ linaro-ami/linaro-ami	2012-07-17 18:43:18 +0000
@@ -22,24 +22,37 @@
     optparser.add_option(
         "--type", default=DEFAULT_AMI_TYPE,
         help="List images of type (%default, use empty for any)")
+    optparser.add_option(
+        "-K", "--private-key", metavar="FILE",
+        help="Private key file (env:EC2_PRIVATE_KEY)")
+    optparser.add_option(
+        "-k", "--key", metavar="KEY",
+        help="Name of EC2-registered key provided by --private-key")
+
     options, args = optparser.parse_args(sys.argv[1:])
     if len(args) < 1:
         optparser.error("Wrong number of arguments")
 
+    if not options.private_key:
+        options.private_key = os.environ.get("EC2_PRIVATE_KEY")
+
     configuration = ConfigParser()
     configuration.read(options.config)
     aws_controller = AwsController()
     remote_executor = RemoteExecutor()
 
     controller = linaro_ami.LinaroAMI(configuration, aws_controller,
-                                      remote_executor, "ubuntu", options)
+                                      remote_executor,
+                                      "ubuntu", options.private_key,
+                                      options)
     if args[0] == "create":
         if len(args) != 2:
             optparser.error("Usage: %s create <AMI template name from config>"
                             % sys.argv[0])
         if not controller.has_ami(args[1]):
             optparser.error("AMI template '%s' not found" % args[1])
-        controller.command_create(args[1])
+        ami_id = controller.command_create(args[1])
+        print "Created AMI: %s" % ami_id
     elif args[0] == "delete":
         # TODO
         controller.command_delete()

=== modified file 'linaro-ami/linaro-ami.conf'
--- linaro-ami/linaro-ami.conf	2012-07-04 12:01:22 +0000
+++ linaro-ami/linaro-ami.conf	2012-07-17 18:43:18 +0000
@@ -4,7 +4,7 @@
 instance_type = t1.micro
 log_file = linaro-ami.log
 bzr_install = sudo apt-get --assume-yes install bzr
-key_name = ami-test-key
+key_name = linaro-ami-key
 
 [ab-natty-64bit]
 base_ami = ami-87c31aee
@@ -12,6 +12,12 @@
 init_script = node/setup-build-android
 description = Build Slave AMI for android-build.linaro.org based on Natty 64bit
 
+[ci-natty-64bit]
+base_ami = ami-87c31aee
+init_script_repo = lp:~linaro-infrastructure/linaro-ci/lci-build-tools
+init_script = node/setup-natty-node
+description = Build Slave AMI for ci.linaro.org based on Natty 64bit
+
 [ci-oneiric-64bit]
 base_ami = ami-baba68d3
 init_script_repo = lp:~linaro-infrastructure/linaro-ci/lci-build-tools

=== modified file 'linaro-ami/linaro_ami.py'
--- linaro-ami/linaro_ami.py	2012-07-02 15:59:56 +0000
+++ linaro-ami/linaro_ami.py	2012-07-17 18:43:18 +0000
@@ -26,10 +26,12 @@
     """
 
     def __init__(self, config, aws_controller, remote_executor, username,
+                 key_filename=None,
                  options={}, aws_access_key_id=None,
                  aws_secret_access_key=None):
         self.log = logging.getLogger("linaro-ami")
         self.username = username
+        self.key_filename = key_filename
         self.options = options
         self.aws_access_key_id = aws_access_key_id
         self.aws_secret_access_key = aws_secret_access_key
@@ -48,21 +50,27 @@
         based on the parameters provided.
         """
 
-        self.log.info("Command create start.")
+        self.log.info("Starting instance to create AMI...")
         self.aws.connect()
 
         instance = None
         base_ami = self.config.get(ami_name, "base_ami")
         instance_type = self.config.get(ami_name, "instance_type")
         script_repo = self.config.get(ami_name, "init_script_repo")
-        key_name = self.config.get(ami_name, "key_name")
+        if self.options.key:
+            key_autocreate = False
+            key_name = self.options.key
+        else:
+            key_autocreate = True
+            key_name = self.config.get(ami_name, "key_name")
 
         try:
-            self.aws.import_key_pair(key_name, self.executor.get_public_key())
+            if key_autocreate:
+                self.aws.import_key_pair(key_name, self.executor.get_public_key())
             instance = self.aws.run_instance_and_wait(base_ami,
                 key_name, instance_type)
             self.log.info("Started instance: %s", instance.public_dns_name)
-            self.executor.connect(instance.public_dns_name, self.username)
+            self.executor.connect(instance.public_dns_name, self.username, self.key_filename)
             self.log.debug("Connected to instance")
 
             self.log.info("Installing bzr.")
@@ -100,7 +108,8 @@
         except Exception, e:
             logging.error(e.__class__.__name__ + ":" + str(e))
         finally:
-            self.aws.delete_key_pair(key_name)
+            if key_autocreate:
+                self.aws.delete_key_pair(key_name)
             self.executor.close()
             if instance:
                 self.aws.terminate_instance(instance.id)

=== modified file 'linaro-ami/remote_executor.py'
--- linaro-ami/remote_executor.py	2012-07-03 15:24:06 +0000
+++ linaro-ami/remote_executor.py	2012-07-17 18:43:18 +0000
@@ -18,9 +18,12 @@
         # Key used for authentication on the server
         self.key = paramiko.RSAKey.generate(1024)
 
-    def connect(self, address, username):
+    def connect(self, address, username, key_filename=None):
         """Connect to address on port 22 with username and provided RSA key."""
-        self.client.connect(address, username=username, pkey=self.key)
+        if key_filename:
+            self.client.connect(address, username=username, key_filename=key_filename)
+        else:
+            self.client.connect(address, username=username, pkey=self.key)
 
     def execute(self, command):
         """Execute a command remotely, returns output from that command."""
@@ -29,10 +32,21 @@
         # library to execute script remotely and return output.
         transport = self.client.get_transport()
         channel = transport.open_session()
-        output_stream = channel.makefile()
+        channel.set_combine_stderr(True)
         channel.exec_command(command)
+        # Closed remote stdin, so we don't get deadlock
+        # on something trying to read it there.
+        channel.shutdown_write()
+        output = ""
+        while True:
+            # 512 below comes from a minimal buffer a canaonical unix pipe
+            # may have. Of course, that might be not related to paramiko's
+            # channel buffer, but good enoug anyway.
+            data = channel.recv(512)
+            if not data:
+                break
+            output += data
         status = channel.recv_exit_status()
-        output = output_stream.read()
         channel.close()
         # Only need status to know if command has failed actually
         # We don't want continuation if user script failed

=== added file 'linaro-ami/setup.cfg'
--- linaro-ami/setup.cfg	1970-01-01 00:00:00 +0000
+++ linaro-ami/setup.cfg	2012-07-17 18:43:18 +0000
@@ -0,0 +1,3 @@
+[nosetests]
+verbosity=2
+tests=tests/unit/

=== added directory 'linaro-ami/tests/integration'
=== added file 'linaro-ami/tests/integration/__init__.py'
--- linaro-ami/tests/integration/__init__.py	1970-01-01 00:00:00 +0000
+++ linaro-ami/tests/integration/__init__.py	2012-07-17 18:43:18 +0000
@@ -0,0 +1,5 @@
+from . import integration_test_support
+
+
+def teardown():
+    integration_test_support.remove_remote_hosts()

=== added file 'linaro-ami/tests/integration/integration_test_support.py'
--- linaro-ami/tests/integration/integration_test_support.py	1970-01-01 00:00:00 +0000
+++ linaro-ami/tests/integration/integration_test_support.py	2012-07-17 18:43:18 +0000
@@ -0,0 +1,42 @@
+import logging
+import os
+import time
+
+import boto
+
+
+TEST_INSTANCE_NAME = "linaro-ami integration tests"
+
+def get_remote_host():
+    connection = boto.connect_ec2()
+    for reservation in connection.get_all_instances():
+        for i in reservation.instances:
+            if i.tags.get("Name") == TEST_INSTANCE_NAME and i.state == "running":
+                print "Found existing inst"
+                return i.dns_name
+    print "Didn't find existing inst - creating"
+    reservation = connection.run_instances("ami-87c31aee", key_name=os.environ["EC2_KEY"],
+                                instance_type="t1.micro")
+    instance = reservation.instances[0]
+    connection.create_tags([instance.id], {"Name": TEST_INSTANCE_NAME})
+    while instance.state != "running":
+        instance.update()
+        time.sleep(5)
+    # Allow OS to boot and sshd to start
+    time.sleep(20)
+    return instance.dns_name
+
+
+def remove_remote_hosts():
+    connection = boto.connect_ec2()
+    for reservation in connection.get_all_instances():
+        for i in reservation.instances:
+            if i.state == "running":
+                if i.tags.get("Name") == TEST_INSTANCE_NAME \
+                        or i.key_name in ("ami-test-key", "infra-test-key"):
+                    print "Terminating test instance:", i.id
+                    connection.terminate_instances([i.id])
+
+
+if __name__ == "__main__":
+    print get_remote_host()

=== renamed file 'linaro-ami/tests/test_aws_controller.py' => 'linaro-ami/tests/integration/test_aws_controller.py'
=== renamed file 'linaro-ami/tests/test_linaro_ami.py' => 'linaro-ami/tests/integration/test_linaro_ami.py'
--- linaro-ami/tests/test_linaro_ami.py	2012-07-02 15:59:56 +0000
+++ linaro-ami/tests/integration/test_linaro_ami.py	2012-07-17 18:43:18 +0000
@@ -2,12 +2,18 @@
 
 import linaro_ami
 
-from test_support import TestSupport
+from ..test_support import TestSupport
 from ConfigParser import ConfigParser
 from aws_controller import AwsController
 from remote_executor import RemoteExecutor
 
 
+class MockOptions:
+
+    def __getattr__(self, name):
+        return None
+
+
 class TestLinaroAMI:
 
     def __init__(self):
@@ -21,13 +27,13 @@
         remote_executor = RemoteExecutor()
 
         controller = linaro_ami.LinaroAMI(configuration, aws_controller,
-                                          remote_executor, "ubuntu")
+                                          remote_executor, "ubuntu", options=MockOptions())
         ami_id = controller.command_create("dummy-test")
         print ami_id
         all_images = controller.aws.list_images()
         print all_images
         result = filter(lambda i: i.id == ami_id, all_images)
-        assert len(result) == 1
+        assert len(result) == 1, "Actual result: %s" % result
 
     def _test_command_list(self):
         controller = linaro_ami.LinaroAMI("tests/data/linaro-ami.conf",

=== added file 'linaro-ami/tests/integration/test_remote_executor_int.py'
--- linaro-ami/tests/integration/test_remote_executor_int.py	1970-01-01 00:00:00 +0000
+++ linaro-ami/tests/integration/test_remote_executor_int.py	2012-07-17 18:43:18 +0000
@@ -0,0 +1,58 @@
+import os
+import time
+import signal
+import random
+
+import remote_executor
+import paramiko
+from . import integration_test_support
+from remote_executor import RemoteCommandFailedError
+from ..test_support import TestSupport
+
+
+class TestRemoteExecutorIntegration:
+
+    def __init__(self):
+        TestSupport.setup_logging()
+        self.instance_address = integration_test_support.get_remote_host()
+        self.executor = remote_executor.RemoteExecutor()
+
+    def test_stdout_capture(self):
+        self.executor.connect(self.instance_address, "ubuntu", os.environ["EC2_PRIVATE_KEY"])
+        s = "test string " + str(random.random())
+        output = self.executor.execute("echo -n '%s'" % s)
+        assert output == s
+
+    def test_stderr_capture(self):
+        self.executor.connect(self.instance_address, "ubuntu", os.environ["EC2_PRIVATE_KEY"])
+        s = "test string " + str(random.random())
+        output = self.executor.execute("echo -n '%s' 1>&2" % s)
+        assert output == s
+
+    def test_long_stdout(self):
+        self.executor.connect(self.instance_address, "ubuntu", os.environ["EC2_PRIVATE_KEY"])
+        size = 500000
+        signal.alarm(10)
+        output = self.executor.execute("cat /dev/urandom | head -c%d" % size)
+        signal.alarm(0)
+        assert len(output) == size
+
+    def test_long_stderr(self):
+        self.executor.connect(self.instance_address, "ubuntu", os.environ["EC2_PRIVATE_KEY"])
+        size = 50000
+        signal.alarm(10)
+        output = self.executor.execute("cat /dev/urandom | head -c%d 1>&2" % size)
+        signal.alarm(0)
+        assert len(output) == size
+
+    def test_blocked_stdin(self):
+        # Test that we don't block indefinitely on remote command trying to read stdin
+        self.executor.connect(self.instance_address, "ubuntu", os.environ["EC2_PRIVATE_KEY"])
+        # We may hang here, provide (abrupt) way out in such case
+        # TODO: Unfortunately, using custom signal handler doesn't
+        # work here (it's not being called, or maybe calling it
+        # block the py interpreter or something)
+        # signal.signal(signal.SIGALRM, sig_handler)
+        signal.alarm(5)
+        self.executor.execute("cat")
+        signal.alarm(0)

=== added directory 'linaro-ami/tests/unit'
=== added file 'linaro-ami/tests/unit/__init__.py'
=== renamed file 'linaro-ami/tests/test_remote_executor.py' => 'linaro-ami/tests/unit/test_remote_executor.py'
--- linaro-ami/tests/test_remote_executor.py	2012-07-02 15:59:56 +0000
+++ linaro-ami/tests/unit/test_remote_executor.py	2012-07-17 18:43:18 +0000
@@ -1,9 +1,11 @@
 import time
 import signal
 
+from mock import Mock
+
 import remote_executor
 import paramiko
-from . import test_support
+from .. import test_support
 from remote_executor import RemoteCommandFailedError
 
 class MockClient:
@@ -19,29 +21,12 @@
 class MockTransport:
 
     def open_session(self):
-        return MockChannel()
-
-class MockOutputStream:
-
-    def read(self):
-        return "test"
-
-class MockChannel:
-
-    def makefile(self):
-        return MockOutputStream()
-
-    def exec_command(self, command):
-        return
-
-    def get_pty(self):
-        return
-
-    def recv_exit_status(self):
-        return 0
-
-    def close(self):
-        return
+        channel = Mock()
+        # With mock 0.8, the below would be just = ["test", ""]
+        channel.recv.side_effect = lambda sz, buf=["test", ""]: buf.pop(0)
+        channel.recv_exit_status.return_value = 0
+        return channel
+
 
 class TestRemoteExecutor:
 
@@ -57,23 +42,8 @@
     def test_execute(self):
         self.executor.client = MockClient()
         output = self.executor.execute("command")
-        assert output == "test"
+        assert output == "test", "Unexpected result: " + output
 
     def test_get_public_key(self):
         public_key = self.executor.get_public_key()
         assert ' ' in public_key
-
-    def _test_execute_stdin(self):
-        def sig_handler(signum, stack_frame):
-            print "foo"
-            assert False, "Remote execution hanged on stdin-reading command"
-        self.connect()
-        # We may hang here, provide (abrupt) way out in such case
-        # TODO: Unfortunately, using custom signal handler doesn't
-        # work here (it's not being called, or maybe calling it
-        # block the py interpreter or something)
-        # signal.signal(signal.SIGALRM, sig_handler)
-        signal.alarm(5)
-        stdin, stdout, stderr, status = self.executor.execute("cat")
-        signal.alarm(0)
-        assert status == -1, "Received unexpected status: %d" % status