Create Volume snapshot force parameter is not validated

[Rajalakshmi Ganesan]

Description:

Create Volume snapshot with invalid Force value is returning 200 ok instead of raising Bad Request.

Expected Result:

Should return error code 400. (raise Bad Request)

Actual Result:

Is not raising exception. Returning 200 ok.

LOG:
-------

----------------------------------------------------------------------

rajalakshmi_ganesan@pshys0183~tests:-)>nova --debug volume-snapshot-create 22 --force '~!@#$%^&*()_+'
connect: (10.233.53.165, 8774)
send: 'GET /v1.1/ HTTP/1.1\r\nHost: 10.233.53.165:8774\r\nx-auth-project-id: admin\r\nx-auth-key: testuser\r\naccept-encoding: gzip, deflate\r\nx-auth-user: admin\r\nuser-agent: python-novaclient\r\n\r\n'
reply: 'HTTP/1.1 204 No Content\r\n'
header: Content-Length: 0
header: X-Auth-Token: admin:admin
header: X-Server-Management-Url: http://10.233.53.165:8774/v1.1/admin
header: Content-Type: text/plain; charset=UTF-8
header: Date: Mon, 18 Jun 2012 19:52:37 GMT
send: 'POST /v1.1/admin/os-snapshots HTTP/1.1\r\nHost: 10.233.53.165:8774\r\nContent-Length: 108\r\nx-auth-project-id: admin\r\nx-auth-token: admin:admin\r\ncontent-type: application/json\r\naccept-encoding: gzip, deflate\r\nuser-agent: python-novaclient\r\n\r\n{"snapshot": {"display_name": null, "force": "~!@#$%^&*()_+", "display_description": null, "volume_id": 22}}'
reply: 'HTTP/1.1 200 OK\r\n'
header: X-Compute-Request-Id: req-bc28a045-4308-4c65-819d-2870ebf45adc
header: Content-Type: application/json
header: Content-Length: 165
header: Date: Mon, 18 Jun 2012 19:52:37 GMT
rajalakshmi_ganesan@pshys0183~tests:-)>nova --debug volume-snapshot-create 22 --force 'alphabet1234567890-='
connect: (10.233.53.165, 8774)
send: 'GET /v1.1/ HTTP/1.1\r\nHost: 10.233.53.165:8774\r\nx-auth-project-id: admin\r\nx-auth-key: testuser\r\naccept-encoding: gzip, deflate\r\nx-auth-user: admin\r\nuser-agent: python-novaclient\r\n\r\n'
reply: 'HTTP/1.1 204 No Content\r\n'
header: Content-Length: 0
header: X-Auth-Token: admin:admin
header: X-Server-Management-Url: http://10.233.53.165:8774/v1.1/admin
header: Content-Type: text/plain; charset=UTF-8
header: Date: Mon, 18 Jun 2012 19:53:10 GMT
send: 'POST /v1.1/admin/os-snapshots HTTP/1.1\r\nHost: 10.233.53.165:8774\r\nContent-Length: 115\r\nx-auth-project-id: admin\r\nx-auth-token: admin:admin\r\ncontent-type: application/json\r\naccept-encoding: gzip, deflate\r\nuser-agent: python-novaclient\r\n\r\n{"snapshot": {"display_name": null, "force": "alphabet1234567890-=", "display_description": null, "volume_id": 22}}'
reply: 'HTTP/1.1 200 OK\r\n'
header: X-Compute-Request-Id: req-a004c5de-c53f-4155-8a9e-ebeab21fa63a
header: Content-Type: application/json
header: Content-Length: 165
header: Date: Mon, 18 Jun 2012 19:53:10 GMT

rajalakshmi_ganesan@pshys0183~tests:-( >nova --debug volume-snapshot-create 22 --force ''
connect: (10.233.53.165, 8774)
send: 'GET /v1.1/ HTTP/1.1\r\nHost: 10.233.53.165:8774\r\nx-auth-project-id: admin\r\nx-auth-key: testuser\r\naccept-encoding: gzip, deflate\r\nx-auth-user: admin\r\nuser-agent: python-novaclient\r\n\r\n'
reply: 'HTTP/1.1 204 No Content\r\n'
header: Content-Length: 0
header: X-Auth-Token: admin:admin
header: X-Server-Management-Url: http://10.233.53.165:8774/v1.1/admin
header: Content-Type: text/plain; charset=UTF-8
header: Date: Mon, 18 Jun 2012 19:55:15 GMT
send: 'POST /v1.1/admin/os-snapshots HTTP/1.1\r\nHost: 10.233.53.165:8774\r\nContent-Length: 95\r\nx-auth-project-id: admin\r\nx-auth-token: admin:admin\r\ncontent-type: application/json\r\naccept-encoding: gzip, deflate\r\nuser-agent: python-novaclient\r\n\r\n{"snapshot": {"display_name": null, "force": "", "display_description": null, "volume_id": 22}}'
reply: 'HTTP/1.1 200 OK\r\n'
header: X-Compute-Request-Id: req-b4b202e4-c15a-40be-bfc7-efec2fd2c4c6
header: Content-Type: application/json
header: Content-Length: 165
header: Date: Mon, 18 Jun 2012 19:55:15 GMT

[Mark McLoughlin (markmc)]

Right, all we're doing is:

        force = snapshot.get('force', False)
        if force:
            ...
        else:
            ...

we're not validating that it's a boolean value

utils.bool_from_str() is probably what's needed here

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/12858

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/12860

[clayg (clay-gerrard)]

I think this is a cinderclient bug, only. I think it's kind of a scary thing to reason about how we should translate a clients request when they send us a string where we are expecting a boolean.

From api.openstack.org - example body

{
    "snapshot": {
        "display_name": "snap-001",
        "display_description": "Daily backup",
        "volume_id": "521752a6-acf6-4b2d-bc7a-119f9148cd8c",
        "force": true
     }
}

^ force should never be a string and we shouldn't be treating it as such.*

if body.get('force', False) is True:

might be more "correct" (depending on how the xml de-serializes and xml body)

*But I don't think it would be _better_ to reject the request if it sends us a string, because that would introduce a backwards incompatible change in the api (intended or not, that's how it works today). And we should be pragmatic about avoiding api changes unless it's justifiably necessary.

We should however, IMHO, fix the defacto cinder client to not send garbage as described in the bug.

[Rongze Zhu (zrzhit)]

clayg, If user don't use cinderclient but directly use curl to call REST API, it also can send invalid force parameter.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/12858
Committed: http://github.com/openstack/nova/commit/cce5cf45f738ed95a96a4526253bec01a15277f0
Submitter: Jenkins
Branch: master

commit cce5cf45f738ed95a96a4526253bec01a15277f0
Author: Rongze Zhu <email address hidden>
Date: Wed Sep 12 09:18:53 2012 +0000

    Return 400 if create volume snapshot force parameter is invalid

    Fixes bug #1014689

    * Add is_valid_boolstr function in utils.py
    * Add force parameter check in SnapshotsController.create()
    * Add unittest for invalid force parameter.

    Change-Id: Ie6a07a2ac589da76f52a82b126a6d66e5987edc4

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/12860
Committed: http://github.com/openstack/cinder/commit/c678b045c9839ed30e13fe5e7e655e3cb2e2c3f9
Submitter: Jenkins
Branch: master

commit c678b045c9839ed30e13fe5e7e655e3cb2e2c3f9
Author: Rongze Zhu <email address hidden>
Date: Wed Sep 12 10:18:31 2012 +0000

    Return 400 if create volume snapshot force parameter is invalid

    Fixes bug #1014689

    * Add is_valid_boolstr function in utils.py
    * Add force parameter check in SnapshotsController.create()
    * Add unittest for invalid force parameter.

    Change-Id: I0f64326f33eb4fad1cf384bd825f56f09e935f40

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: milestone-proposed
Review: https://review.openstack.org/13478

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/13478
Committed: http://github.com/openstack/cinder/commit/418ad3ef2dd52200f6d6c26687e64f023c7ba107
Submitter: Jenkins
Branch: milestone-proposed

commit 418ad3ef2dd52200f6d6c26687e64f023c7ba107
Author: Rongze Zhu <email address hidden>
Date: Wed Sep 12 10:18:31 2012 +0000

    Return 400 if create volume snapshot force parameter is invalid

    Fixes bug #1014689

    * Add is_valid_boolstr function in utils.py
    * Add force parameter check in SnapshotsController.create()
    * Add unittest for invalid force parameter.

    Change-Id: I0f64326f33eb4fad1cf384bd825f56f09e935f40
    (cherry picked from commit c678b045c9839ed30e13fe5e7e655e3cb2e2c3f9)

[Seif Lotfy (seif)]

Here is my proposed fix https://review.openstack.org/#/c/38002/

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/38002
Committed: http://github.com/openstack/python-cinderclient/commit/854740c4c793444c955180e7c0c96977e3e06c56
Submitter: Jenkins
Branch: master

commit 854740c4c793444c955180e7c0c96977e3e06c56
Author: Seif Lotfy <email address hidden>
Date: Thu Jul 18 18:01:41 2013 +0000

    Add evaluation of --force parameter when creating snapshots

    Raise BadRequest Exception if value of --force parameter in
    snapshot-create is invalid

    Fixes: bug #1014689

    Change-Id: If4858dc17cedf027112defb935016137727681cc

[Edward Hope-Morley (hopem)]

Why has string been favoured for --force as opposed to a boolean? Using a bool would alleviate the need to guess and parse the string values. Is this just for backwards compatibility reasons?

[John Griffith (john-griffith)]

So yes the client should only do like: "--force" which would set a real boolean = True.

Breaking compat is an issue here though.

That being said, the issue of not handling garbage input is addressed in the cinder API now via bool_from_str(), so that if garbage is passed in it will give an Invalid parameter exception which is correct. I have no problem with doing a check earlier in the client (so long as it's the same as what we implemented in the API [bool_from_str()]).

When we're ready to version bump again I would like to make the --force arguments better but until then I think we need to leave it as is and just fix the issues. Marking as invalid for cinderclient for that reason.