Location field is wiped when updating metadata

Bug #911599 reported by justinsb on 2012-01-04
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Glance
High
Jay Pipes
OpenStack Compute (nova)
High
Brian Waldon
Diablo
Undecided
Mark McLoughlin

Bug Description

When I try to set a property on a glance image by POSTing to the OS API at /images/<id>/metadata, the location field is set to Null. I've put the relevant bit of the registry log below.

I've tried looking through the glance code, and I can't see exactly how this is happening. I strongly suspect it's all my fault though, as I was the one that originally filed the bug that said that the location field shouldn't be publicly visible - my guess is that it's being zeroed out somewhere and then set back. Karma :-)

2012-01-04 02:43:20 DEBUG [glance.registry.api.v1.images] Updating image 31cc48af-c5f2-ea38-dcb4-aa0fe111b213 with metadata: {u'status': u'active', u'name': u'image-1325644992887', u'deleted': False, u'checksum': u'3cb82d3c9b570e95deee6f10fca0285f', u'min_ram': u'0', u'disk_format': u'raw', u'id': u'31cc48af-c5f2-ea38-dcb4-aa0fe111b213', u'location': None, u'container_format': u'bare', u'min_disk': u'0', u'is_public': True, u'properties': {u'platformlayerid': u'org.platformlayer.ids.ProjectId [fathomdb]/org.platformlayer.ids.ServiceType [imagefactory]/org.platformlayer.ids.ItemType [diskImageRecipe]/76725505-4652-40db-a70e-1efb634f74fc'}, u'size': 404346423}
2012-01-04 02:43:20 INFO [sqlalchemy.engine.base.Engine] BEGIN (implicit)
2012-01-04 02:43:20 INFO [sqlalchemy.engine.base.Engine] SELECT images.created_at AS images_created_at, images.updated_at AS images_updated_at, images.deleted_at AS images_deleted_at, images.deleted AS images_deleted, images.id AS images_id, images.name AS images_name, images.disk_format AS images_disk_format, images.container_format AS images_container_format, images.size AS images_size, images.status AS images_status, images.is_public AS images_is_public, images.location AS images_location, images.checksum AS images_checksum, images.min_disk AS images_min_disk, images.min_ram AS images_min_ram, images.owner AS images_owner, image_properties_1.created_at AS image_properties_1_created_at, image_properties_1.updated_at AS image_properties_1_updated_at, image_properties_1.deleted_at AS image_properties_1_deleted_at, image_properties_1.deleted AS image_properties_1_deleted, image_properties_1.id AS image_properties_1_id, image_properties_1.image_id AS image_properties_1_image_id, image_properties_1.name AS image_properties_1_name, image_properties_1.value AS image_properties_1_value, image_members_1.created_at AS image_members_1_created_at, image_members_1.updated_at AS image_members_1_updated_at, image_members_1.deleted_at AS image_members_1_deleted_at, image_members_1.deleted AS image_members_1_deleted, image_members_1.id AS image_members_1_id, image_members_1.image_id AS image_members_1_image_id, image_members_1.member AS image_members_1_member, image_members_1.can_share AS image_members_1_can_share
FROM images LEFT OUTER JOIN image_properties AS image_properties_1 ON images.id = image_properties_1.image_id LEFT OUTER JOIN image_members AS image_members_1 ON images.id = image_members_1.image_id
WHERE images.id = ?
2012-01-04 02:43:20 INFO [sqlalchemy.engine.base.Engine] (u'31cc48af-c5f2-ea38-dcb4-aa0fe111b213',)
2012-01-04 02:43:20 DEBUG [sqlalchemy.engine.base.Engine] Col ('images_created_at', 'images_updated_at', 'images_deleted_at', 'images_deleted', 'images_id', 'images_name', 'images_disk_format', 'images_container_format', 'images_size', 'images_status', 'images_is_public', 'images_location', 'images_checksum', 'images_min_disk', 'images_min_ram', 'images_owner', 'image_properties_1_created_at', 'image_properties_1_updated_at', 'image_properties_1_deleted_at', 'image_properties_1_deleted', 'image_properties_1_id', 'image_properties_1_image_id', 'image_properties_1_name', 'image_properties_1_value', 'image_members_1_created_at', 'image_members_1_updated_at', 'image_members_1_deleted_at', 'image_members_1_deleted', 'image_members_1_id', 'image_members_1_image_id', 'image_members_1_member', 'image_members_1_can_share')
2012-01-04 02:43:20 DEBUG [sqlalchemy.engine.base.Engine] Row (u'2012-01-04 02:43:12.977819', u'2012-01-04 02:43:20.102921', None, 0, u'31cc48af-c5f2-ea38-dcb4-aa0fe111b213', u'image-1325644992887', u'raw', u'bare', 404346423, u'active', 1, u'file:///var/lib/glance/images/31cc48af-c5f2-ea38-dcb4-aa0fe111b213', u'3cb82d3c9b570e95deee6f10fca0285f', 0, 0, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None)
2012-01-04 02:43:20 INFO [sqlalchemy.engine.base.Engine] UPDATE images SET updated_at=?, location=?, min_disk=?, min_ram=? WHERE images.id = ?
2012-01-04 02:43:20 INFO [sqlalchemy.engine.base.Engine] ('2012-01-04 02:43:20.423022', None, u'0', u'0', u'31cc48af-c5f2-ea38-dcb4-aa0fe111b213')
2012-01-04 02:43:20 INFO [sqlalchemy.engine.base.Engine] INSERT INTO image_properties (created_at, updated_at, deleted_at, deleted, image_id, name, value) VALUES (?, ?, ?, ?, ?, ?, ?)
2012-01-04 02:43:20 INFO [sqlalchemy.engine.base.Engine] ('2012-01-04 02:43:20.424064', '2012-01-04 02:43:20.424068', None, 0, u'31cc48af-c5f2-ea38-dcb4-aa0fe111b213', u'platformlayerid', u'org.platformlayer.ids.ProjectId [fathomdb]/org.platformlayer.ids.ServiceType [imagefactory]/org.platformlayer.ids.ItemType [diskImageRecipe]/76725505-4652-40db-a70e-1efb634f74fc')

justinsb (justin-fathomdb) wrote :

Aha.... I think the root problem is here, in nova, in nova/image/glance.py

The metadata update fetches all the metadata, merges in the values, and then posts that to glance (which is not the strictly correct in terms of concurrent updates, but anyway...)

The problem is that the fetch returns 'Location' of None.

https://github.com/openstack/nova/blob/master/nova/image/glance.py#L437

def _limit_attributes(image_meta):
    IMAGE_ATTRIBUTES = ['size', 'location', 'disk_format',
                        'container_format', 'checksum', 'id',
                        'name', 'created_at', 'updated_at',
                        'deleted_at', 'deleted', 'status',
                        'min_disk', 'min_ram', 'is_public']
    output = {}
    for attr in IMAGE_ATTRIBUTES:
        output[attr] = image_meta.get(attr)

    output['properties'] = image_meta.get('properties', {})

    return output

That sets ['location'] to None; which then gets passed to the glance registry, which then updates the location to None.

justinsb (justin-fathomdb) wrote :

Removing 'location' from the list above does fix the problem. Is this the right place to fix the problem though? Or should it be fixed in glance itself (is there any legitimate reason to allow the location field to be set externally)?

Brian Waldon (bcwaldon) wrote :

1) We should make sure we don't send properties that aren't explicitly provided
2) We should limit the mutable properties coming through the public api to exactly what is in the spec (disallow location)

Changed in glance:
importance: Undecided → Medium
status: New → Confirmed
Jay Pipes (jaypipes) wrote :

Setting to High, as this results in data loss. Not setting to Critical since there is a workaround.

Changed in glance:
milestone: none → essex-3
importance: Medium → High
justinsb (justin-fathomdb) wrote :

Let me know if you want me to submit my ever-so-complicated patch for nova/image/glance.py :-)

Brian Waldon (bcwaldon) wrote :

Maybe Jay has more of an answer as to whether the Location header should be mutable. If it should be, I think you can just make GlanceImageService a bit smarter and not overwrite it if a value isn't provided. If it shouldn't be mutable, then the patch you indicated above would be fine.

Jay Pipes (jaypipes) wrote :

I actually don't think that the Location field should be settable by a user except on initial registration. After that, I don't think the Location field should be either editable OR readable (for security reasons). Would you both agree with that assessment? If so, I'll assign to myself and get to work with a fix.

justinsb (justin-fathomdb) wrote :

Sounds great to me Jay - thanks!

Jay Pipes (jaypipes) on 2012-01-05
Changed in glance:
assignee: nobody → Jay Pipes (jaypipes)
status: Confirmed → Triaged
Brian Waldon (bcwaldon) wrote :

I buy that. Makes sense to have the same policy as we have for the metadata itself.

Jay Pipes (jaypipes) on 2012-01-11
Changed in glance:
status: Triaged → In Progress
Brian Waldon (bcwaldon) on 2012-01-11
Changed in nova:
importance: Undecided → High
assignee: nobody → Brian Waldon (bcwaldon)
milestone: none → essex-3
status: New → In Progress

Reviewed: https://review.openstack.org/2981
Committed: http://github.com/openstack/nova/commit/3e015b869789c7aeeb90e160ede3b3a7b7921f30
Submitter: Jenkins
Branch: master

commit 3e015b869789c7aeeb90e160ede3b3a7b7921f30
Author: Brian Waldon <email address hidden>
Date: Wed Jan 11 13:27:29 2012 -0800

    Remove 'location' from GlanceImageService

    Glance no longer returns location through its public API, so we
    should not attempt to display it. Addresses bug 911599.

    Change-Id: I3400006eb6ab94095c0c2d2b5dc90cb9b7775a84

Changed in nova:
status: In Progress → Fix Committed
Changed in glance:
assignee: Jay Pipes (jaypipes) → Pete Zaitcev (zaitcev)
Jay Pipes (jaypipes) on 2012-01-12
Changed in glance:
assignee: Pete Zaitcev (zaitcev) → Jay Pipes (jaypipes)
status: In Progress → Fix Committed

Reviewed: https://review.openstack.org/3083
Committed: http://github.com/openstack/nova/commit/1e2f538fb02f7986e4509bf7815617f75327c135
Submitter: Jenkins
Branch: stable/diablo

commit 1e2f538fb02f7986e4509bf7815617f75327c135
Author: Brian Waldon <email address hidden>
Date: Wed Jan 11 13:27:29 2012 -0800

    Remove 'location' from GlanceImageService

    Glance no longer returns location through its public API, so we
    should not attempt to display it. Addresses bug 911599.

    (cherry picked from commit 3e015b869789c7aeeb90e160ede3b3a7b7921f30)

    Change-Id: I3400006eb6ab94095c0c2d2b5dc90cb9b7775a84

tags: added: in-stable-diablo
Changed in glance:
status: Fix Committed → In Progress
Jay Pipes (jaypipes) on 2012-01-23
Changed in glance:
status: In Progress → Fix Committed
Jay Pipes (jaypipes) on 2012-01-23
Changed in glance:
status: Fix Committed → In Progress

Reviewed: https://review.openstack.org/2952
Committed: http://github.com/openstack/glance/commit/0db2cfadf5da692aa6f02abd20018f4aa3d49d2a
Submitter: Jenkins
Branch: master

commit 0db2cfadf5da692aa6f02abd20018f4aa3d49d2a
Author: Jay Pipes <email address hidden>
Date: Tue Jan 10 21:14:26 2012 -0500

    Bug#911599 - Location field wiped on update

    Adds unit test to verify behaviour required:

    * Location field may only be **edited** if the image is
      in a queued state, which indicates that the user issued
      a POST /images with NO location field and NO image data
    * Otherwise, Location field may not be edited and may not
      be read via the API server either.

    Change-Id: I42aba7bc8e2da6ac81b18b564ba096208406f893

Changed in glance:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2012-01-25
Changed in glance:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2012-01-25
Changed in nova:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2012-04-05
Changed in glance:
milestone: essex-3 → 2012.1
Thierry Carrez (ttx) on 2012-04-05
Changed in nova:
milestone: essex-3 → 2012.1
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers