Juju Charms Collection
storage package

Merge lp://staging/~chris-gondolin/charms/trusty/storage/trunk into lp://staging/charms/storage

  1. Trusty Tahr (14.04)
  2. trunk
  3. Merge into trunk
Proposed by Chris Stratford
Status: Merged
Merged at revision: 39
Proposed branch: lp://staging/~chris-gondolin/charms/trusty/storage/trunk
Merge into: lp://staging/charms/storage
Diff against target: 988 lines (+732/-40)
9 files modified
config.yaml (+23/-8)
files/crypt_mount.sh (+19/-0)
hooks/common_util.py (+72/-20)
hooks/crypt_fs.py (+164/-0)
hooks/storage-provider.d/partition/data-relation-changed (+31/-0)
hooks/test_common_util.py (+23/-12)
hooks/test_crypt_fs.py (+248/-0)
tests/10-crypt-keep-keyfile (+79/-0)
tests/20-crypt-lose-keyfile (+73/-0)
To merge this branch: bzr merge lp://staging/~chris-gondolin/charms/trusty/storage/trunk
Reviewer Review Type Date Requested Status
Matt Bruzek (community) Approve
Cory Johns (community) Needs Fixing
Review via email: mp+234452@code.staging.launchpad.net

Description of the change

Added encrypted filesystem support for block devices.
Introduces two new config variables:

encryption_key_map = "{postgres/0: password, postgres/2: password2}"
store_encryption_keys = yes

If store_encryption_keys is "yes" the passwords will be stored in /root/keyfile--dev-vdc (or vdd, vde, etc. whatever the device name is) and an entry written to /etc/crypttab to allow auto-mounting on reboot.

If store_encryption_keys is anything else, the keyfile will be written temporarily to allow cryptsetup to do its work, then removed, any entries for the device in /etc/crypttab will be removed if present and "defaults,noauto" set in fstab to ensure prevent auto-mounting on reboot (which would fail, waiting for a passphrase).

To post a comment you must log in.
Revision history for this message
Tim Van Steenburgh (tvansteenburgh) wrote :

Hey Chris, nice addition to the storage charm, thanks!

My only quibble with this proposal is that there are no new tests to cover the new functionality. Before giving this a +1 I would like to see some tests to cover the new encryption stuff - maybe some unit test coverage of the crypt_fs module, and a new amulet test to test a deployment scenario.

lp://staging/~chris-gondolin/charms/trusty/storage/trunk updated
41. By Chris Stratford

[chriss] Added a partition provider to cope with physical disk partitions (on MaaS units for example). Added some amulet tests that should cover both the new provider and encryption.

42. By Chris Stratford

[chriss] A bit more logging

43. By Chris Stratford

[chriss] Attempt to get juju test to log useful stuff

44. By Chris Stratford

[chriss] Removed spin_up_delay, as it did not do what I had hoped. Tests *should* work once bug #1378309 is fixed

Revision history for this message
Cory Johns (johnsca) wrote :

Chris,

Thanks for adding the tests for this functionality. The new tests look good, but unfortunately I had some issues running them.

First, I'd just like to point out that Amulet has been updated to fix the issues you are working around and mention in your tests, specifically the issues with the series and local charm not being used properly, and adding relations after deployment not working correctly. So, you can change your d.add('postgresql', 'cs:trusty/postgresql-5') and d.add('storage', '/home/chris/work/charms/trusty/storage') to just d.add('postgresql') and d.add('storage'). Regarding the post-deployment relations, the only thing that you should need to change is to add d.sentry.wait() after adding the relation, to ensure that the system has time to process the change.

That said, with those changes I am still getting failures (http://pastebin.ubuntu.com/8838368/), as well as hook failures (http://pastebin.ubuntu.com/8838352/). This happened on both lxc and aws, on newly bootstrapped environments.

Additionally, `make lint` had some complaints about the new code, just some small quibbles about line length and spacing: http://pastebin.ubuntu.com/8837062/

Finally, some of the existing tests in `make test` are having issues with the new code. It mostly seems like they need some additional mocks (http://pastebin.ubuntu.com/8837040/) or new data values (http://pastebin.ubuntu.com/8837008/), but one in particular seems like it might be an issue in the new code: http://pastebin.ubuntu.com/8837016/

review: Needs Fixing
lp://staging/~chris-gondolin/charms/trusty/storage/trunk updated
45. By Chris Stratford

[chriss] PEP8 fixes. Amulet test fixes now amulet itself is fixed

Revision history for this message
Chris Stratford (chris-gondolin) wrote :

The Amulet tests should now work (d.sentry.wait() after adding the relation doesn't appear to be enough, so there's a sleep in there too now, which should fix the errors you saw)

make lint should also be happy now.

I've got to do some reading up on mocker before fixing the make test problems.

lp://staging/~chris-gondolin/charms/trusty/storage/trunk updated
46. By Chris Stratford

[chriss] Unit test fixes

Revision history for this message
Chris Stratford (chris-gondolin) wrote :

Ok, the unit tests now pass (even the ones that weren't a result of my changes :-)

There still aren't any for the crypt_fs functions or partition storage type additions (yet).

lp://staging/~chris-gondolin/charms/trusty/storage/trunk updated
47. By Chris Stratford

[chriss] Add unit tests for crypt_fs. Minor fixes.

Revision history for this message
Matt Bruzek (mbruzek) wrote :

+1 LGTM

review: Approve
Revision history for this message
Adam Collard (adam-collard) wrote :

This broke the charm :(

http://paste.ubuntu.com/9458517/

There is nothing in the charm that attempts to install cryptsetup-bin package (which installs the cryptsetup script that this MP relies on).

Revision history for this message
Chris Stratford (chris-gondolin) wrote :

Adam, I'm curious to know what system you installed it on to get that error, as it looks like cryptsetup-bin is standard on all recent Ubuntu installs.

I'll fix the bug, but it would be good to know for future reference.

Revision history for this message
David Britton (dpb) wrote :

Hi @Chris --

Adam tested on openstack with official cloud images. Matt tested on lxc. Could be a difference between the images there. :(

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: