Re: [openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Deepak Shetty]

Thanks Mike for the heads up

I fixed it for GlusterFS CI [1]
Post the fix, glusterfs CI jobs are running fine [2] See Jul 10, 10:43 AM
onwards

thanx,
deepak

[1]: https://review.openstack.org/#/c/200399/2
[2]:
https://jenkins07.openstack.org/job/check-tempest-dsvm-full-glusterfs-nv/

On Fri, Jul 10, 2015 at 12:52 AM, Mike Perez  wrote:

> On 16:47 Jun 30, Mike Perez wrote:
> > On 12:24 Jun 26, Matt Riedemann wrote:
> > 
> > > So the question is, is everyone OK with this and ready to make that
> change?
> >
> > Thanks for all your work on this Matt.
> >
> > I'm fine with this. I say bite the bullet and we'll see the CI's surface
> that
> > aren't skipping or failing this test.
> >
> > I will communicate with CI maintainers on the CI list about failures as
> I've
> > been doing, and reference this thread and the meeting discussion.
>
> This landed.
>
> If your Cinder CI is now failing, set
> ATTACH_ENCRYPTED_VOLUME_AVAILABLE=False
> [1] as explained earlier in this thread.
>
> [1] - https://review.openstack.org/#/c/199709/
>
> --
> Mike Perez
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Mike Perez]

On 16:47 Jun 30, Mike Perez wrote:
> On 12:24 Jun 26, Matt Riedemann wrote:
>  
> > So the question is, is everyone OK with this and ready to make that change?
> 
> Thanks for all your work on this Matt.
> 
> I'm fine with this. I say bite the bullet and we'll see the CI's surface that
> aren't skipping or failing this test.
> 
> I will communicate with CI maintainers on the CI list about failures as I've
> been doing, and reference this thread and the meeting discussion.

This landed.

If your Cinder CI is now failing, set ATTACH_ENCRYPTED_VOLUME_AVAILABLE=False
[1] as explained earlier in this thread.

[1] - https://review.openstack.org/#/c/199709/

-- 
Mike Perez

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Deepak Shetty]

Oh, just to be clear, I don't mean to discard what you fixed
My intention was to discuss what would be a better way to fix this in
future thru a feature/blueprint, given there is a consensus

thanx,
deepak

On Thu, Jul 2, 2015 at 8:57 PM, Deepak Shetty  wrote:

>
>
> On Thu, Jul 2, 2015 at 7:05 PM, Matt Riedemann  > wrote:
>
>>
>>
>> On 7/2/2015 4:12 AM, Deepak Shetty wrote:
>>
>>>
>>>
>>> On Wed, Jul 1, 2015 at 5:17 AM, Mike Perez >> > wrote:
>>>
>>> On 12:24 Jun 26, Matt Riedemann wrote:
>>> 
>>> > So the question is, is everyone OK with this and ready to make
>>> that change?
>>>
>>> Thanks for all your work on this Matt.
>>>
>>>
>>> +100, awesome debug, followup and fixing work by Matt
>>>
>>>
>>> I'm fine with this. I say bite the bullet and we'll see the CI's
>>> surface that
>>> aren't skipping or failing this test.
>>>
>>>
>>> Just curious, shouldn't this mean we need to have some way of Cinder
>>> querying Nova
>>> for "do u have this capability" and only then setting the 'encryption'
>>> key in conn_info ?
>>>
>>> Better communication between nova and cinder ?
>>>
>>> thanx,
>>> deepak
>>>
>>>
>>>
>>>
>>> __
>>> OpenStack Development Mailing List (not for usage questions)
>>> Unsubscribe:
>>> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>>>
>> I thought the same about some capability flag in cinder where the volume
>> driver would tell the volume manager if it supported encryption and then
>> the cinder volume manager would use that to tell if a request to create a
>> volume from an encryption type was possible.  But the real problem in our
>> case is the encryption provider support, which is currently the luks and
>> cryptsetup modules in nova.  However, the encryption provider is completely
>> pluggable [1] from what I can tell, the libvirt driver in nova just creates
>> the provider class (assuming it can import it) and calls the methods
>> defined in the VolumeEncryptor abstract base class [2].
>>
>> So whether or not encryption is supported during attach is really up to
>> the encryption provider implementation, the volume driver connector code
>> (now in os-brick), and what the cinder volume driver is providing back to
>> nova during os-initialize_connection.
>>
>
> Yes I understand the issue, hence i said that why not cinder "checks" with
> Nova whether it supports enc for volume-attach , nova returns yes/no and
> based on that cinder accepts/rejects the 'create new enc volume' request.
>
>
>> I guess my point is I don't have a simple solution besides actually
>> failing when we know we can't encrypt the volume during attach - which is
>> at least better than the false positive we have today.
>>
>>
> Definitely what u have proposed/fixed is appreciated. But its a
> workaround, the better way seems to be improving the Nova-Cinder
> communication ?
>
> thanx,
> deepak
>
>
>> [1]
>> http://git.openstack.org/cgit/openstack/nova/tree/nova/volume/encryptors/__init__.py#n47
>> [2]
>> http://git.openstack.org/cgit/openstack/nova/tree/nova/volume/encryptors/base.py#n28
>>
>> --
>>
>> Thanks,
>>
>> Matt Riedemann
>>
>>
>>
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Deepak Shetty]

On Thu, Jul 2, 2015 at 7:05 PM, Matt Riedemann 
wrote:

>
>
> On 7/2/2015 4:12 AM, Deepak Shetty wrote:
>
>>
>>
>> On Wed, Jul 1, 2015 at 5:17 AM, Mike Perez > > wrote:
>>
>> On 12:24 Jun 26, Matt Riedemann wrote:
>> 
>> > So the question is, is everyone OK with this and ready to make that
>> change?
>>
>> Thanks for all your work on this Matt.
>>
>>
>> +100, awesome debug, followup and fixing work by Matt
>>
>>
>> I'm fine with this. I say bite the bullet and we'll see the CI's
>> surface that
>> aren't skipping or failing this test.
>>
>>
>> Just curious, shouldn't this mean we need to have some way of Cinder
>> querying Nova
>> for "do u have this capability" and only then setting the 'encryption'
>> key in conn_info ?
>>
>> Better communication between nova and cinder ?
>>
>> thanx,
>> deepak
>>
>>
>>
>> __
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe:
>> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
> I thought the same about some capability flag in cinder where the volume
> driver would tell the volume manager if it supported encryption and then
> the cinder volume manager would use that to tell if a request to create a
> volume from an encryption type was possible.  But the real problem in our
> case is the encryption provider support, which is currently the luks and
> cryptsetup modules in nova.  However, the encryption provider is completely
> pluggable [1] from what I can tell, the libvirt driver in nova just creates
> the provider class (assuming it can import it) and calls the methods
> defined in the VolumeEncryptor abstract base class [2].
>
> So whether or not encryption is supported during attach is really up to
> the encryption provider implementation, the volume driver connector code
> (now in os-brick), and what the cinder volume driver is providing back to
> nova during os-initialize_connection.
>

Yes I understand the issue, hence i said that why not cinder "checks" with
Nova whether it supports enc for volume-attach , nova returns yes/no and
based on that cinder accepts/rejects the 'create new enc volume' request.


> I guess my point is I don't have a simple solution besides actually
> failing when we know we can't encrypt the volume during attach - which is
> at least better than the false positive we have today.
>
>
Definitely what u have proposed/fixed is appreciated. But its a workaround,
the better way seems to be improving the Nova-Cinder communication ?

thanx,
deepak


> [1]
> http://git.openstack.org/cgit/openstack/nova/tree/nova/volume/encryptors/__init__.py#n47
> [2]
> http://git.openstack.org/cgit/openstack/nova/tree/nova/volume/encryptors/base.py#n28
>
> --
>
> Thanks,
>
> Matt Riedemann
>
>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Matt Riedemann]

On 7/2/2015 4:12 AM, Deepak Shetty wrote:



On Wed, Jul 1, 2015 at 5:17 AM, Mike Perez mailto:thin...@gmail.com>> wrote:

On 12:24 Jun 26, Matt Riedemann wrote:

> So the question is, is everyone OK with this and ready to make that 
change?

Thanks for all your work on this Matt.


+100, awesome debug, followup and fixing work by Matt


I'm fine with this. I say bite the bullet and we'll see the CI's
surface that
aren't skipping or failing this test.


Just curious, shouldn't this mean we need to have some way of Cinder
querying Nova
for "do u have this capability" and only then setting the 'encryption'
key in conn_info ?

Better communication between nova and cinder ?

thanx,
deepak



__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



I thought the same about some capability flag in cinder where the volume 
driver would tell the volume manager if it supported encryption and then 
the cinder volume manager would use that to tell if a request to create 
a volume from an encryption type was possible.  But the real problem in 
our case is the encryption provider support, which is currently the luks 
and cryptsetup modules in nova.  However, the encryption provider is 
completely pluggable [1] from what I can tell, the libvirt driver in 
nova just creates the provider class (assuming it can import it) and 
calls the methods defined in the VolumeEncryptor abstract base class [2].


So whether or not encryption is supported during attach is really up to 
the encryption provider implementation, the volume driver connector code 
(now in os-brick), and what the cinder volume driver is providing back 
to nova during os-initialize_connection.


I guess my point is I don't have a simple solution besides actually 
failing when we know we can't encrypt the volume during attach - which 
is at least better than the false positive we have today.


[1] 
http://git.openstack.org/cgit/openstack/nova/tree/nova/volume/encryptors/__init__.py#n47
[2] 
http://git.openstack.org/cgit/openstack/nova/tree/nova/volume/encryptors/base.py#n28


--

Thanks,

Matt Riedemann


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Deepak Shetty]

On Wed, Jul 1, 2015 at 5:17 AM, Mike Perez  wrote:

> On 12:24 Jun 26, Matt Riedemann wrote:
> 
> > So the question is, is everyone OK with this and ready to make that
> change?
>
> Thanks for all your work on this Matt.
>

+100, awesome debug, followup and fixing work by Matt


>
> I'm fine with this. I say bite the bullet and we'll see the CI's surface
> that
> aren't skipping or failing this test.
>

Just curious, shouldn't this mean we need to have some way of Cinder
querying Nova
for "do u have this capability" and only then setting the 'encryption' key
in conn_info ?

Better communication between nova and cinder ?

thanx,
deepak
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Matt Riedemann]

On 6/30/2015 6:47 PM, Mike Perez wrote:

On 12:24 Jun 26, Matt Riedemann wrote:


So the question is, is everyone OK with this and ready to make that change?


Thanks for all your work on this Matt.

I'm fine with this. I say bite the bullet and we'll see the CI's surface that
aren't skipping or failing this test.

I will communicate with CI maintainers on the CI list about failures as I've
been doing, and reference this thread and the meeting discussion.



Just a status report on this since there are a lot of moving parts:

1. The tempest change merged: https://review.openstack.org/#/c/193831/

2. The devstack change is approved: https://review.openstack.org/#/c/193834/

3. The d-g change is still under review: 
https://review.openstack.org/#/c/193835/


4. Tom Barron opened a couple of nova bugs for issues found by third 
party CI on the cinder change:


https://bugs.launchpad.net/nova/+bug/1470142

https://bugs.launchpad.net/nova/+bug/1470562

I have patches up in nova for both of those bugs.

--

Thanks,

Matt Riedemann


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Mike Perez]

On 12:24 Jun 26, Matt Riedemann wrote:
 
> So the question is, is everyone OK with this and ready to make that change?

Thanks for all your work on this Matt.

I'm fine with this. I say bite the bullet and we'll see the CI's surface that
aren't skipping or failing this test.

I will communicate with CI maintainers on the CI list about failures as I've
been doing, and reference this thread and the meeting discussion.

-- 
Mike Perez

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

[openstack-dev] [nova][cinder][qa] encrypted volumes tests don't actually test encrypted volumes for most backends

[Matt Riedemann]

Tempest has the TestEncryptedCinderVolumes scenario test [1] which 
creates an encrypted volume type, creates a volume from that volume 
type, boots a server instance and then attaches/detaches the 'encrypted' 
volume to/from the server instance.


This works fine in the integrated gate because LVM is used as the 
backend and the encryption providers used in the test are implemented in 
nova to work with the iscsi libvirt volume driver - it sets the 
'device_path' key in the connection_info['data'] dict that the 
encryption provider code is checking.


The calls to the encryption providers in nova during volume attached are 
based on whether or not the 'encrypted' key is set in 
connection_info['data'] returned from the os-initialize_connection 
cinder API.  In the case of iscsi and several other volume drivers in 
cinder this key is set to True if the volume's 'encryption_key_id' field 
is set in the volume object.


It was noticed that the encrypted volume tests were passing the ceph job 
even though the libvirt volume driver in nova wasn't setting the 
device_path key, so it's not actually doing encryption on the attached 
volume - but the test wasn't failing, so it's a big false positive.


Upon further inspection, it is passing because it isn't doing anything, 
and it isn't doing anything because the rbd volume driver in cinder 
isn't setting the 'encrypted' key in connection_info['data'] in it's 
initialize_connection() method.


So we got to this cinder change [2] which originally was just setting 
the encrypted key for the rbd volume driver until it was pointed out 
that we should set that key globally in the volume manager if the volume 
driver isn't setting it, so that's what the latest version of the change 
does.


The check-tempest-dsvm-full-ceph job is passing on that change because 
of a series of dependent changes [3].  Basically, a config option is 
needed in tempest to tell it whether or not to run the 
TestEncryptedCinderVolumes tests. This defaults to True for backwards 
compatibility. Then there is a devstack change to set the flag in 
tempest.conf based on an environment variable to devstack. Then there is 
a change to devstack-gate to set that flag to False for the Ceph job.
Finally, the cinder change depends on the devstack-gate change so 
everything is in order and it doesn't blow up after marking the rbd 
volume connection as encrypted - which would fail if we didn't skip the 
test.


Now the issue is going to be, there are lots of other volume drivers in 
cinder that are going to be getting this encrypted key set to True which 
is going to blow up without the support in nova for encrypting the 
volume during attach.


The glusterfs and sheepdog jobs are failing in that patch for different 
reasons actually, but we expect third party CI to fail if they don't 
configure tempest by setting TEMPEST_ATTACH_ENCRYPTED_VOLUME=False in 
their devstack run.


So the question is, is everyone OK with this and ready to make that change?

An alternative to avoid the explosion is when nova detects that it 
should use an encryption provider but the 'device_path' key isn't set in 
connection_info, it could use the noop encryption provider and just 
ignore it, but that's putting our heads in the sand and the test is 
passing with a false positive - you're not actually getting encrypted 
attached volumes to your server instances which is the point of the test.


I'll get this on the cinder meeting agenda for next week for discussion 
before the cinder change is approved, unless we come up with other 
alternatives, like a 'supports_encryption' capability flag in cinder 
(something like that) which could tell the cinder API during a request 
to create a volume from an encrypted type that the volume driver doesn't 
support it and the request fails with a 400.  That'd be an API change 
but might be acceptable given the API is pretty much broken today already.


[1] 
http://git.openstack.org/cgit/openstack/tempest/tree/tempest/scenario/test_encrypted_cinder_volumes.py

[2] https://review.openstack.org/#/c/193673/
[3] 
https://review.openstack.org/#/q/status:open+branch:master+topic:bug/1463525,n,z


--

Thanks,

Matt Riedemann


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev