Nova and Glance contain a near-identical signature_utils module

Bug #1528349 reported by Mark McLoughlin on 2015-12-21
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Glance
Low
Dane Fichter
OpenStack Compute (nova)
Wishlist
Dane Fichter

Bug Description

It appears that https://review.openstack.org/256069 took the signature_utils modules from Glance and modified it in fairly superficial ways based on review feedback:

  $ diff -u nova/nova/signature_utils.py glance/glance/common/signature_utils.py | diffstat
  signature_utils.py | 182 ++++++++++++++++++++++++-----------------------------
  1 file changed, 83 insertions(+), 99 deletions(-)

The Oslo project was created to avoid this sort of short-sighted cut-and-pasting. This code should really be in a python library that both Glance and Nova could use directly.

Perhaps the code could be moved to a new library in the Glance project, or a new library in the Oslo project, or into the cryptography library itself?

Louis Taylor (kragniz) wrote :

oslo.signatures, or perhaps just in oslo.utils?

Changed in glance:
status: New → Confirmed
importance: Undecided → Low
Mark McLoughlin (markmc) wrote :

oslo.utils might be ok, but making crytography a dependency for all oslo.utils users is probably undesirable

glance-store?

Daniel Berrange (berrange) wrote :

FYI, the code in Nova may look the same right now, but that's only because it was not following nova best practice. That method needs converting to use Nova's nova.objects.ImageMeta model instead of using anonymous dicts, since Nova's intends to 100% eliminate passing & use of dicts for image properties in favour of an explicit model. So if there is a desire to share the code, it wouldl have to be re-factored so that all the neccessary signature relatsed parameters are explicitly passed in, rather than passing in the image meta dict directly.

Dane Fichter (dane-fichter) wrote :

@Daniel Berrange: If we are moving to an explicit model for image properties, then perhaps the best way to meet that requirement and make this code not specific to Nova is to directly pass all the required image properties to the signature_utils get_verifier method.

I should also add that the Glance and Nova implementations will continue to diverge during the Mitaka cycle, since the Glance implementation has to deprecate an old approach, and the Nova spec will require us to add certificate validation functionality to the Nova implementation.

As compromise to all of the above consideration and with the goal of eliminating duplicate code I propose the following path forward:

1. Amend Nova implementation to take in image properties as individual parameters, not bundled in a dict or ImageMeta object.
2. Remove other Nova-specific aspects of the signature_utils class, but continue implementing the spec and keep the code in Nova for Mitaka.
3. At the beginning of the N development cycle, after Glance no longer has to give deprecation warnings for the outdated approach, merge the signature_utils module into oslo as oslo.signature_utils and remove the duplicate code from Nova and Glance.

Thoughts?

Changed in nova:
status: New → Confirmed
Daniel Berrange (berrange) wrote :

That sounds like a reasonable plan

Dane Fichter (dane-fichter) wrote :

Alright, I'll move forward with this unless there are objections.

Changed in nova:
assignee: nobody → Dane Fichter (dane-fichter)
Andrew Laski (alaski) wrote :

Dane if you are still working on this please assign it back to yourself. But there's been no progress for over 4 months so I'm setting this so others feel free to pick it up.

Changed in nova:
assignee: Dane Fichter (dane-fichter) → nobody
Dane Fichter (dane-fichter) wrote :

Done. Thanks for reminding me. Resolving this is one of my goals for this cycle.

Changed in glance:
assignee: nobody → Dane Fichter (dane-fichter)
Changed in nova:
assignee: nobody → Dane Fichter (dane-fichter)
John Garbutt (johngarbutt) wrote :

We did make creating this library a requirement for the feature. Although we didn't want to block on its creation. Seems we probably should have.

tags: added: glance
Changed in nova:
importance: Undecided → Medium
Dane Fichter (dane-fichter) wrote :

Hi John, we've created the cursive library and will definitely be able to do a release before Nova's code freeze. However, we'd like to add certificate validation (with the approval of Nova core reviewers) before we remove the signature_utils code from Nova. Could you please take a look at:

https://review.openstack.org/#/c/331823/

Matt Riedemann (mriedem) wrote :

There was no spec proposed in Newton for https://review.openstack.org/#/c/331823/ so that's going to have to wait for Ocata.

Changed in nova:
importance: Medium → Wishlist
Daniel Berrange (berrange) wrote :

This isn't a bug - its a feature request to switch to using a new library from Nova. I'm fine with that as a suggestion, but it should be file as a blueprint - probably can be a specless blueprint.

Changed in nova:
status: Confirmed → Invalid
Matt Riedemann (mriedem) wrote :

This is not really a bug. Create a specless blueprint for Ocata to integrate the library into Nova in that release.

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

Changed in nova:
status: Invalid → In Progress

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

Changed in glance:
status: Confirmed → In Progress

Reviewed: https://review.openstack.org/352391
Committed: https://git.openstack.org/cgit/openstack/glance/commit/?id=5afb5d33ed7714aa2684ee9209d05f30655e2c41
Submitter: Jenkins
Branch: master

commit 5afb5d33ed7714aa2684ee9209d05f30655e2c41
Author: Dane Fichter <email address hidden>
Date: Tue Aug 2 23:13:46 2016 -0400

    Use cursive for signature verification

    This change removes the signature_utils module
    from Glance and uses the cursive library, which
    contains an identical module.

    Change-Id: I80fcafa528b87a83b90ed7c0e4c0db9228852bc2
    Depends-On: Ic3ffb6b318dc2ac6c9d3a60bed5198fd4d40e318
    Partial-Bug: #1528349

Change abandoned by Sean Dague (<email address hidden>) on branch: master
Review: https://review.openstack.org/351232
Reason: This review is > 6 weeks without comment, and failed Jenkins the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

Reviewed: https://review.openstack.org/351232
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=d17e701ddbbb6ebcc2a27c4d27053d6e1f87989d
Submitter: Jenkins
Branch: master

commit d17e701ddbbb6ebcc2a27c4d27053d6e1f87989d
Author: dane-fichter <email address hidden>
Date: Tue Mar 14 09:23:44 2017 -0700

    Use cursive for signature verification

    This change removes the signature_utils module
    from Nova and uses the cursive library, which
    contains an identical module.

    Change-Id: I8179282a9d19f829aca0b5bd2775d855b3364c86
    Depends-On: I7e5797661fee258bc0270b5f109704b591633519
    Implements: blueprint signature-code-cleanup
    Partial-Bug: #1528349

Matt Riedemann (mriedem) on 2017-04-10
Changed in nova:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers