Enable cert updates #50

sevenmachines · 2015-03-24T13:51:06Z

When we change the certificate in our cloud formation config we want a way to update the certificates on instances, and to set these certificates on any https load balancers on the stack.
This patch creates a fab_task update_certs() to carry out both of these tasks, first removing/adding the certificates using IAM, then using a simple ELB class to set_ssl_certificates() on any https listeners on the stacks load_balancers

ashb · 2015-03-25T13:38:43Z

bootstrap_cfn/elb.py

+                # necessarily a problem
+                logging.info("ELB::set_ssl_certificates: "
+                             "No load balancers found in stack, "
+                             "no certificate updates needed")


This probably be a warning or an error - the user has requested something to be updated that couldn't be. This also means we should likely return a non-0 exit code.

I was going to throw an exception here to allow the caller to deal with there being no elbs or ignoring, exiting seems a little strict?

ashb · 2015-03-25T13:54:38Z

Can you also rebase this into a small number of commits? In this case probably just 1.

ashb · 2015-04-08T14:42:18Z

@niallcreech Travis is reporting that the tests are failing

FAIL: test_ssl_delete (tests.test.CfnTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/ministryofjustice/bootstrap-cfn/tests/test.py", line 246, in test_ssl_delete
    self.assertTrue(x)
AssertionError: False is not true

Did you change the API here perhaps?

sevenmachines · 2015-04-08T14:49:56Z

Yep, I'm on it. its not the API, delete checks to see if the cert exists. I'm setting up the test to mock the call to list_server_certs.

coveralls · 2015-04-08T16:16:02Z

Coverage decreased (-3.3%) to 46.13% when pulling a15ee7f on enable_cert_updates into e4e41a2 on master.

sevenmachines · 2015-04-08T16:17:33Z

Update test_ssl_delete test case to reflect new 'check cert exists' flow for IAM class

Now expanding tests to cover update_certs fab_task

sevenmachines · 2015-04-09T15:46:16Z

Need to fill out the test cases then hopefully done

coveralls · 2015-04-10T12:12:12Z

Coverage decreased (-2.68%) to 50.58% when pulling d604429 on enable_cert_updates into 78707b7 on master.

sevenmachines · 2015-04-10T12:17:40Z

Added more test cases, hopefully ready for review

mattmb · 2015-04-16T12:38:26Z

bootstrap_cfn/fab_tasks.py

+    Also handle settings the certificates on ELB's
+    """
+    if verbose:
+        logging.basicConfig(level=logging.INFO)


Rather than setting a flag called verbose here, it would be better to just use the logger with different levels. Like logging.info when you want it to appear for everyone and logging.debug when you want it only if the verbosity is increased. Then we can set the level globally in the fabfile maybe?

sevenmachines · 2015-04-17T14:44:34Z

Dropped verbose arg from fab task
elb: Made load_balance search more pythonistic
iam: Made exception catching more specific to KeyError
iam: Move to pythonic mid-method returns rather than collecting
results and returning on method exit
iam: Combine cert_exists and force checks on if
iam: combine some dictionary calls to compact code
iam: Fix remote and local cert data comparison
iam: Compact update_ssl_certificates checks on force and exists

sevenmachines · 2015-04-17T14:49:51Z

Not sure about some of this 'forcing' logic

mattmb · 2015-04-20T10:48:51Z

bootstrap_cfn/elb.py

+            load_balancer_resources = self.cfn.get_stack_load_balancers(stack_name)
+            found_load_balancer_names = [lb.physical_resource_id for lb in load_balancer_resources]
+            # Use load balancer names to filter gettingload balancer details
+            if len(found_load_balancer_names) > 0:


think we might want load_balancers=[] here or in the else block because if this is not true the next if statement will fail (if I'm reading it right).

mattmb · 2015-04-20T11:27:45Z

@niallcreech and I have just had a good chat about this.

Two things we thought of:

Force may not be necessary. Use cases are my repo has the correct cert, I run the tool, nothing changes. My repo has a new cert, I run the tool, the cert gets updated.
Multiple ELBs with different certs. We think it's probably safe the way it works at the moment because the cert wont be updated, the ARN wont change etc. But might be nicer to track what's changing (rather than an updated count (maybe a dictionary of what's updated)

stuartornum · 2015-04-21T09:06:06Z

This is a massive PR, I have no idea where to start

sevenmachines · 2015-04-21T09:49:29Z

Mostly covered with Matt yesterday, I’ve got a couple of small changes to make but unfortunately I’ve no internet until the afternoon. Press on with any bootstrap work you’re doing, I’ll fix this up around it once you’re finished

sevenmachines · 2015-04-21T16:03:27Z

Reworked IAM force options to remove them and have sane logic
Return list of updated elb's when we run update_ssl's
Updated tests to reflect new force and list return logic

sevenmachines · 2015-04-21T16:04:04Z

Will need to wait until trim_the_fat has been merged and see how rebasey this will be before moving to review

mattmb · 2015-04-21T16:16:15Z

Cool, thanks @niallcreech I've got my fingers crossed with the git gods that it won't be too conflicty 😄

Created a fab task to update certificates on a stack. Checks are carried out to see if the current local and remote certificates are the same, if not the certificates are uploaded and and load balancers with certificates in the stack will be updated fab_tasks: - Create a fab task update_certs to update the ssl certificates from the config - Small delay between updating certificates on instances and load balancer to avoid setting the load balancer cert when ARN is available but the certificate is not properly registered iam: - Added getting an arn from a certificate name to the IAM class. - Simplify getting an arn for certs using boto get_server_certificate - Handle exceptions when trying to delete/upload a certificate - Simplify upload ssl logic to error on trying to update a missing certificate elb: - Create an ELB class to handle interaction with the stacks load balancers. - Throw an exception if we try to set certs on elbs when none are defined in the config cloudformation: - Added utility method to get a list of load balancers for the stack test_iam: - Added test cases to cover noew iam things

mattmb · 2015-05-01T13:49:00Z

I'd say this looks good now. Anyone else want to comment before I merge?

Enable cert updates

ashb reviewed Mar 25, 2015
View reviewed changes

ashb added the in progress label Apr 7, 2015

sevenmachines self-assigned this Apr 8, 2015

sevenmachines force-pushed the enable_cert_updates branch 2 times, most recently from 5eab969 to 1ee7124 Compare April 8, 2015 14:33

sevenmachines changed the title ~~Enable cert updates~~ Enable cert updates (WIP) Apr 8, 2015

sevenmachines force-pushed the enable_cert_updates branch from 1ee7124 to a15ee7f Compare April 8, 2015 16:14

sevenmachines force-pushed the enable_cert_updates branch 3 times, most recently from 3f096af to 9e63a10 Compare April 9, 2015 15:39

sevenmachines force-pushed the enable_cert_updates branch from 9e63a10 to d604429 Compare April 10, 2015 12:11

sevenmachines changed the title ~~Enable cert updates (WIP)~~ Enable cert updates Apr 10, 2015

sevenmachines removed their assignment Apr 10, 2015

sevenmachines added ready and removed ready labels Apr 10, 2015

ashb added ready for review and removed in progress labels Apr 13, 2015

mattmb reviewed Apr 16, 2015
View reviewed changes

sevenmachines force-pushed the enable_cert_updates branch from 8960c4c to 54f1243 Compare April 17, 2015 14:49

mattmb reviewed Apr 20, 2015
View reviewed changes

sevenmachines closed this Apr 21, 2015

sevenmachines removed the ready for review label Apr 21, 2015

sevenmachines reopened this Apr 21, 2015

sevenmachines added the ready for review label Apr 21, 2015

mattmb added in progress and removed ready for review labels Apr 21, 2015

mookin assigned sevenmachines Apr 21, 2015

sevenmachines force-pushed the enable_cert_updates branch 2 times, most recently from 0ef9337 to ebe625b Compare April 21, 2015 16:02

sevenmachines force-pushed the enable_cert_updates branch from ebe625b to 70de16a Compare April 30, 2015 10:53

sevenmachines added ready for review and removed in progress labels Apr 30, 2015

sevenmachines removed their assignment May 2, 2015

stuartornum added a commit that referenced this pull request May 5, 2015

Merge pull request #50 from ministryofjustice/enable_cert_updates

7cfdad7

Enable cert updates

stuartornum merged commit 7cfdad7 into master May 5, 2015

stuartornum removed the ready for review label May 5, 2015

stuartornum deleted the enable_cert_updates branch May 5, 2015 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable cert updates #50

Enable cert updates #50

sevenmachines commented Mar 24, 2015

ashb Mar 25, 2015

sevenmachines Apr 8, 2015

ashb commented Mar 25, 2015

ashb commented Apr 8, 2015

sevenmachines commented Apr 8, 2015

coveralls commented Apr 8, 2015

sevenmachines commented Apr 8, 2015

sevenmachines commented Apr 9, 2015

coveralls commented Apr 10, 2015

sevenmachines commented Apr 10, 2015

mattmb Apr 16, 2015

sevenmachines commented Apr 17, 2015

sevenmachines commented Apr 17, 2015

mattmb Apr 20, 2015

mattmb commented Apr 20, 2015

stuartornum commented Apr 21, 2015

sevenmachines commented Apr 21, 2015

sevenmachines commented Apr 21, 2015

sevenmachines commented Apr 21, 2015

mattmb commented Apr 21, 2015

mattmb commented May 1, 2015

Enable cert updates #50

Enable cert updates #50

Conversation

sevenmachines commented Mar 24, 2015

ashb Mar 25, 2015

Choose a reason for hiding this comment

sevenmachines Apr 8, 2015

Choose a reason for hiding this comment

ashb commented Mar 25, 2015

ashb commented Apr 8, 2015

sevenmachines commented Apr 8, 2015

coveralls commented Apr 8, 2015

sevenmachines commented Apr 8, 2015

sevenmachines commented Apr 9, 2015

coveralls commented Apr 10, 2015

sevenmachines commented Apr 10, 2015

mattmb Apr 16, 2015

Choose a reason for hiding this comment

sevenmachines commented Apr 17, 2015

sevenmachines commented Apr 17, 2015

mattmb Apr 20, 2015

Choose a reason for hiding this comment

mattmb commented Apr 20, 2015

stuartornum commented Apr 21, 2015

sevenmachines commented Apr 21, 2015

sevenmachines commented Apr 21, 2015

sevenmachines commented Apr 21, 2015

mattmb commented Apr 21, 2015

mattmb commented May 1, 2015