Barbican key manager: missing key blocks volume deletion

[Johannes Grassler]

Summary
=======

If a key for an encrypted volume no longer exists, cinder.keymgr.barbican will block volume deletion due to an uncaught NotFound exception here:

https://github.com/openstack/cinder/blob/stable/mitaka/cinder/keymgr/barbican.py#L335

This error condition should be handled more gracefully: a 404 upon an attempt to delete can simply be ignored with a warning in the log, since all it signifies is that the task is already done.

Impact
======

This has been observed in stable/mitaka. With the Barbican key manager gone as of Newton, this should not be an issue in Newton, unless Castellan based key management makes the same mistake.

Steps to repoduce
=================

1) Create an encrypted Cinder volume using the Barbican key manager
2) Delete its encryption key using `openstack secret delete`
3) Attempting to delete the volume will yield the error shown in the attached excerpt from the Cinder API log (stacktrace.log)

[Johannes Grassler (jgr-launchpad)]

Note: this problem will also crop up when one tries to delete an encrypted volume created using cinder.keymgr.conf_key_mgr (that volume will have an invalid key_id of `00000000-0000-0000-0000-000000000000`).

[Johannes Grassler (jgr-launchpad)]

Here's a quick and dirty patch for stable/mitaka by the way. I won't get around to doing this properly until at least a week after the summit, but feel free to use this patch in the meantime.

[Johannes Grassler (jgr-launchpad)]

Also, somebody trying to get encrypted volumes working on Newton described a very similar failure mode on #openstack-barbican today, so this might affect Newton as well.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/mitaka
Review: https://review.openstack.org/388061

[Sayali Lunkad (sayalilunkad)]

Fix for castellan: https://review.openstack.org/#/c/388079/

[Kaitlin Farr (kaitlin-farr)]

Related bug report for why the uuid of all-zeroes was being requested in the first place: https://bugs.launchpad.net/cinder/+bug/1621109

It should be fixed in master, but the backport for stable/newton is still outstanding: https://review.openstack.org/#/c/376992/

I agree that the volume deletion should not be blocked if the key does not exist, but I think it is proper logic for Castellan to throw an error if the user requests to delete a key with an invalid uuid.

Looking at the cinder code [1], it seems that the exception thrown by the key manager is actually caught correctly, but the *actual* error that is blocking volume deletion is thrown when the warning message is formatted.

***You should be able to resolve this bug if you change e.msg to e.message [2].***

1. https://github.com/openstack/cinder/blob/d4cde0393d1d84a58d9a247c236d303cf24d66d8/cinder/volume/api.py#L453-L458

2. https://github.com/openstack/cinder/blob/d4cde0393d1d84a58d9a247c236d303cf24d66d8/cinder/volume/api.py#L458

[Kaitlin Farr (kaitlin-farr)]

On closer inspection, e.message won't work either. The whole log statement should get reworked because it doesn't seem to be doing what the author intended at all.

[Sayali Lunkad (sayalilunkad)]

Looking at the existing code I don't see any specific handling for checking the validity of the UUID in the error handling. And that should probably be done at an earlier stage. I think this problem should be tackled on the key_mgr side and not in the cinder/volume/api.py because this is related to key not present/invalid. Also since we do print the UUID it is easier to debug if it is the valid one although it might be better to add that check as well, but I think that can be another patch.

[Kaitlin Farr (kaitlin-farr)]

The error from the attached stacktrace that's causing the volume deletion failure is this:

2016-10-07 09:08:28.433 13444 ERROR cinder.api.middleware.fault "volume: %s."), e.msg, resource=volume)
2016-10-07 09:08:28.433 13444 ERROR cinder.api.middleware.fault AttributeError: 'HTTPClientError' object has no attribute 'msg'

It caught the exception thrown by the key manager correctly, but a new exception is thrown because the log statement is trying to access an attribute that doesn't exist.

This bug can be resolved if that log statement is reworked with no changes to the key manager code.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Mike Perez (<email address hidden>) on branch: stable/mitaka
Review: https://review.openstack.org/388061

[Kaitlin Farr (kaitlin-farr)]

Reviewed: https://review.openstack.org/455749
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=6e2d8fd0664d468228d78ceea0e96e10d560be6a
Submitter: Jenkins
Branch: master

commit 6e2d8fd0664d468228d78ceea0e96e10d560be6a
Author: Eric Harney <email address hidden>
Date: Tue Apr 11 11:25:09 2017 -0400

    Fix encryption key deletion error handling on volume delete

    e.msg is only present for CinderExceptions, not all exception
    types.

    Closes-Bug: #1681874

    Change-Id: I02601ae2a9ba95ab7145b86e24a094272f4cfbf6

[Kaitlin Farr (kaitlin-farr)]

There was a duplicate bug report opened: https://bugs.launchpad.net/cinder/+bug/1681874

[OpenStack Infra (hudson-openstack)]

Change abandoned by Davanum Srinivas (dims) (<email address hidden>) on branch: master
Review: https://review.openstack.org/388079
Reason: Looks like this was fixed on the cinder side