The API for volume v3 update/delete encryption type differs from the documentation

Bug #1835186 reported by wangzhiguang
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
cinder-tempest-plugin
In Progress
Critical
Brian Rosmaita
tempest
Fix Released
Medium
Nathan

Bug Description

In the API for volume v3,update/delete encryption type interface is types/{volume_type_id}/encryption/{encryption_id} and types/{volume_type_id}/encryption/{encryption_id}.But,the interface used is /types/%s/encryption/provider by the tempest/lib/services/volume/v3/ path, encryption_types_client.py file.Two things are different。

Revision history for this message
wangzhiguang (wzginspur) wrote :
wangzhiguang (wzginspur)
Changed in tempest:
status: New → Confirmed
status: Confirmed → New
assignee: nobody → wangzhiguang (wzginspur)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to tempest (master)

Fix proposed to branch: master
Review: https://review.opendev.org/669614

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on tempest (master)

Change abandoned by wangzhiguang (<email address hidden>) on branch: master
Review: https://review.opendev.org/669614

Revision history for this message
Doug Schveninger (ds6901) wrote : Re: In the API for volume v3, update/delete encryption type interface is different from code in the encryption_types_client.py file

I was reviewing the bug and wondering if the bug is still valid since the review and patchset being abandon. Is the bug still valid?

Revision history for this message
Martin Kopec (mkopec) wrote :

I'm changing the assignee to Nobody, as there's no activity in the last 6 months.

Anyway, I think the bug is valid. According the documentation [1] urls for DELETE and PUT should look like:
/v3/{project_id}/types/{volume_type_id}/encryption/{encryption_id}
However, the code contains this [2][3]. There's 'provider' instead of the encryption_id. I wonder why it isn't causing any troubles, are the methods even used? I'm gonna check. I'll put the bug to Confirmed meanwhile.

[1] https://docs.openstack.org/api-ref/block-storage/v3/index.html?expanded=update-an-encryption-type-detail#update-an-encryption-type
[2] https://opendev.org/openstack/tempest/src/commit/5785a7d18040ed8e48f7f58ed8bff972f5f25e01/tempest/lib/services/volume/v3/encryption_types_client.py#L85
[3] https://opendev.org/openstack/tempest/src/commit/5785a7d18040ed8e48f7f58ed8bff972f5f25e01/tempest/lib/services/volume/v3/encryption_types_client.py#L74

Changed in tempest:
assignee: wangzhiguang (wzginspur) → nobody
status: New → Confirmed
Martin Kopec (mkopec)
summary: - In the API for volume v3, update/delete encryption type interface is
- different from code in the encryption_types_client.py file
+ The API for volume v3 update/delete encryption type differs from the
+ documentation
Revision history for this message
Ghanshyam Mann (ghanshyammann) wrote :

yes, url used in Tempest is wrong. i cannot find such API in cinder.

This is used in below test and we need to investigate why it is working and not giving error. May be 'provider' string is taken as id by cinder API but at some stage it should return error as this is unknown id.
- https://opendev.org/openstack/tempest/src/commit/acfb494c854819cc4b335ea51d3195d097c3f9cb/tempest/api/volume/admin/test_volume_types.py

Changed in tempest:
importance: Undecided → Medium
Revision history for this message
Lukas Piwowarski (lukas-piwowarski) wrote :

I am not entirely sure but it seems that cinder internally needs only the volume type id when updating or deleting encryption type [1][2][3]. When an encryption type is created via the API it must be connected to a volume type which suggests that volume type id should be enough for identifying the encryption type.

Also when the /v3/{project_id}/types/{volume_type_id}/encryption/{encryption_id} request is send with valid values for {project_id} and {volume_type_id} the request will pass as long as {encryption_id} contains any value (e.g. "abcd") and the volume type has an encryption type.

[1] https://github.com/openstack/cinder/blob/4580c56058a39726dd174b1aefa7b3eacc63904c/cinder/api/contrib/volume_type_encryption.py#L113
[2] https://github.com/openstack/cinder/blob/4580c56058a39726dd174b1aefa7b3eacc63904c/cinder/api/contrib/volume_type_encryption.py#L151
[3] https://github.com/openstack/cinder/blob/e8f03523e8d244fb9c1ecceb091784cc37d63b0d/cinder/db/api.py#L873

Evan Wever (roomist)
Changed in tempest:
assignee: nobody → Evan Wever (roomist)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to tempest (master)

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/tempest/+/924169

Changed in tempest:
assignee: Evan Wever (roomist) → nobody
Nathan (nathanperez)
Changed in tempest:
assignee: nobody → Nathan (nathanperez)
information type: Public → Public Security
Revision history for this message
Jeremy Stanley (fungi) wrote :

If this bug report represents a suspected security vulnerability, please include comments explaining why when switching the type from public to public security. I'm switching it back to a normal bug for now.

information type: Public Security → Public
Revision history for this message
Nathan (nathanperez) wrote :

I changed the “/types/%s/encryption/provider" to “/types/%s/encryption/helloworld” in delete_encryption_type. The function performs correctly (an encryption type is deleted).

The correct documentation to delete an encryption type is /v3/{project_id}/types/{volume_type_id}/encryption/{encryption_id}

https://docs.openstack.org/api-ref/block-storage/v3/#delete-an-encryption-type

I’m not sure why the “provider” is hard coded rather than an {encryption_id} or how it still deletes the encryption type.

Revision history for this message
Rajat Dhasmana (whoami-rajat) wrote :

Hi,

I looked at the tempest and cinder code and here is my analysis:

1. In the update encryption method, we use the encryption ID for notifications[1]
2. In the delete encryption method, we don't use the encryption ID at all[2]

Since only one encryption ID can be associated with a volume type, we allow the encryption type to be deleted via volume type.
But the encryption ID becomes redundant in update and delete methods (apart from the notification use case).

We cannot remove the encryption ID from URL since it will break backward compatibility but we can surely make it optional.
On the tempest side, we receive the encryption ID in the response body when creating the encryption type so we should be passing that to cinder.

So the work items should be:
1. make encryption_id optional in URL for update and delete APIs
2. correct tempest helper methods to pass the encryption ID instead of the 'provider' key

[1] https://github.com/openstack/cinder/blob/master/cinder/api/contrib/volume_type_encryption.py#L114
[2] https://github.com/openstack/cinder/blob/master/cinder/api/contrib/volume_type_encryption.py#L134

Thanks
Rajat Dhasmana

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/tempest/+/935526

Changed in tempest:
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to tempest (master)

Reviewed: https://review.opendev.org/c/openstack/tempest/+/935526
Committed: https://opendev.org/openstack/tempest/commit/5e1a714cfe3ba78c1fb8e60f06f91a864584bec5
Submitter: "Zuul (22348)"
Branch: master

commit 5e1a714cfe3ba78c1fb8e60f06f91a864584bec5
Author: Evan Wever <email address hidden>
Date: Mon Jul 15 11:52:37 2024 -0400

    Fixed encryption type methods to comply with documentation

    This change fixes an optional `encryption_id` parameter
    for updating and deleting encryption types to comply
    with OpenStack documentation.

    `encryption_id` is optional because only one
    `encryption_id` can be associated with a `volume type`,
    therefore, an encryption type is deleted using
    `volume type`.

    `encryption_id` is only used for Cinder
    notifications.

    Closes-bug: #1835186
    Co-Authored-By: Evan Wever <email address hidden>

    Change-Id: Ieca29000b5754373e6250818ccf2b3b6d4ef80e2

Changed in tempest:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder-tempest-plugin (master)
Changed in cinder:
status: New → In Progress
Revision history for this message
Brian Rosmaita (brian-rosmaita) wrote :

Change has broken the cinder gate; needs to be fixed in cinder-tempest-plugin, which is using the old interface for some tests.

affects: cinder → cinder-tempest-plugin
Changed in cinder-tempest-plugin:
importance: Undecided → Critical
assignee: nobody → Brian Rosmaita (brian-rosmaita)
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Bug attachments

Remote bug watches

Bug watches keep track of this bug in other bug trackers.