Properties translations depend on HOT and Cfn template formats

Bug #1620859 reported by Zane Bitter
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Heat
Fix Released
Medium
Peter Razumovsky

Bug Description

The properties translation code is replete with special cases for specific functions in the HOT and CloudFormation template formats:

http://git.openstack.org/cgit/openstack/heat/tree/heat/engine/translation.py?h=stable%2Fmitaka#n157
http://git.openstack.org/cgit/openstack/heat/tree/heat/engine/translation.py?h=stable%2Fmitaka#n159
http://git.openstack.org/cgit/openstack/heat/tree/heat/engine/translation.py?h=stable%2Fmitaka#n244

This makes it impossible for third-party template format plugins to be first-class citizens in Heat, as they are liable to break whenever someone adds a translation rule.

It's also inconceivable that the code is actually correct. It totally fails to take into account such subtleties as the fact that the specific functions it's looking for may in fact be present but buried inside the arguments to other functions.

It's very difficult to determine what the edge cases that this code was designed to catch were, and thus to suggest a better implementation. An obvious one that springs to mind is that checking for dependencies returned by function.dependencies() is a far superior (i.e. actually reliable) way of testing for the presence of {get_resource:} than isinstance. (It will also sweep up {get_attr:} though - but it would be very surprising if we actually wanted those treated differently).

Finally, (and this might be the reason for the edge cases) the whole thing operates by modifying data behind the scenes, which is fairly unsafe. It would be much better if it returned a copy of the data incorporating the modifications.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to heat (master)

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

Changed in heat:
assignee: nobody → Rabi Mishra (rabi)
status: Triaged → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to heat (master)

Reviewed: https://review.openstack.org/366491
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=f2881d407117e829f1723601afd79dab3391744d
Submitter: Jenkins
Branch: master

commit f2881d407117e829f1723601afd79dab3391744d
Author: rabi <email address hidden>
Date: Wed Sep 7 09:08:50 2016 +0530

    Resolve all functions before RESOLVE translation

    As functions can be inside other functions, there is no
    point in checking for specific functions that can be
    template specific. Better to resolve all before
    translating.

    This also adds a functional test to avoid breaking this
    in the future.

    Change-Id: I5f72f7455384b3fd5650bd01e77e64bf485dd178
    Partial-Bug: #1620859

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to heat-specs (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/397070

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to heat (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/420014

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

Related fix proposed to branch: master
Review: https://review.openstack.org/420015

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

Related fix proposed to branch: master
Review: https://review.openstack.org/420016

Changed in heat:
assignee: Rabi Mishra (rabi) → Peter Razumovsky (prazumovsky)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to heat (master)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to heat-specs (master)

Reviewed: https://review.openstack.org/397070
Committed: https://git.openstack.org/cgit/openstack/heat-specs/commit/?id=a1a6d6a15ec8b0fd3d7a2d3e542416212f16b861
Submitter: Jenkins
Branch: master

commit a1a6d6a15ec8b0fd3d7a2d3e542416212f16b861
Author: Peter Razumovsky <email address hidden>
Date: Mon Nov 14 12:12:20 2016 +0300

    Fixing translation mechanism spec

    Spec for discussing and suggesting better way to
    fix and improve current translation mechanism.

    Related-bug: #1620859

    Change-Id: Id66ddc38a3fd8fb135bf49a7c083972fcd74b272

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to heat (master)

Reviewed: https://review.openstack.org/420014
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=bf8e3fd7eaadd008b21dd5b63634d254d629e79d
Submitter: Jenkins
Branch: master

commit bf8e3fd7eaadd008b21dd5b63634d254d629e79d
Author: Peter Razumovsky <email address hidden>
Date: Wed Dec 28 16:09:49 2016 +0400

    Fix using parent_name for Properties

    Currently Properties has arg "parent_name", which
    is used for detailed path in error and allows to
    build path for nested properties schemas. But on
    top parent_name takes resource name as initial,
    which is few incorrect - Properties should raises
    with error about Properties, other info (about in
    what resource this error raised) should be built out of
    Properties module.

    Change-Id: I36e6453a1589c02a3f8cf2c080b38693b23b0f1b
    Related-bug: #1620859

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

Reviewed: https://review.openstack.org/420015
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=74a7c8c38a8d3f802523c1db3d0b28a7470167d0
Submitter: Jenkins
Branch: master

commit 74a7c8c38a8d3f802523c1db3d0b28a7470167d0
Author: Peter Razumovsky <email address hidden>
Date: Wed Dec 28 16:21:43 2016 +0400

    Add full path for Property

    Add full path field for Property class. Now
    properties with sub-schemas should contain properties
    with path, where their parent specified. For example,
    if there is schema:
            "a": type map:
                   schema:
                     "b": type string,
                     "c": type string

    Then property "a" has path == 'a', "b" has path == 'a.b',
    "c" has path == 'a.c'.

    Change-Id: I6d0ba022f34b925d47142669d6076ebd63a2acdc
    Related-bug: #1620859

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

Reviewed: https://review.openstack.org/420016
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=ff2e714332e540aa01dc732e68ffd5fc8b0874c5
Submitter: Jenkins
Branch: master

commit ff2e714332e540aa01dc732e68ffd5fc8b0874c5
Author: Peter Razumovsky <email address hidden>
Date: Fri Dec 30 13:14:28 2016 +0400

    Fix validation in TranslationRule

    Fix validation in TranslationRule class.

    Change-Id: I07662a55f918e4ec9ecb16b466d8f1004f595356
    Related-bug: #1620859

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to heat (master)

Reviewed: https://review.openstack.org/420017
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=ae2db9ff73657dde6f7f3fb479f0a0622da95799
Submitter: Jenkins
Branch: master

commit ae2db9ff73657dde6f7f3fb479f0a0622da95799
Author: Peter Razumovsky <email address hidden>
Date: Fri Dec 30 13:51:27 2016 +0400

    Initial add Translation-in-place mechanism

    Implement base rules storing in Translation mechanism,
    which will translate some property iff it called.

    Rule's value_path/value_name resolved to value and saved to
    rule's value. Each property could have several translation
    rules for it.

    Change-Id: Ia08895bba85d4b217075ad21836ea725eccb44d6
    Closes-bug: #1620859

Changed in heat:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/420018
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=3890695f2384744f3af440f72dbe034575667545
Submitter: Jenkins
Branch: master

commit 3890695f2384744f3af440f72dbe034575667545
Author: Peter Razumovsky <email address hidden>
Date: Fri Dec 30 15:52:24 2016 +0400

    Add translation method for Translation

    Add method translate to Translation - the main part of mechanism.
    It allows to translate properties values by rules and store resolved
    translation, if corresponding flag is enabled.

    "Delete" rule added for deleting existing properties values.

    Change-Id: I87a02bd3dbf349de9f0f2b3d5439e5ddab89caae
    Closes-bug: #1620859

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

Reviewed: https://review.openstack.org/420019
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=e2b22404acf9b0491bec74866ae51dee4286c49e
Submitter: Jenkins
Branch: master

commit e2b22404acf9b0491bec74866ae51dee4286c49e
Author: Peter Razumovsky <email address hidden>
Date: Mon Jan 9 14:49:41 2017 +0400

    Add "Replace" handling for translation

    Add "Replace" rule handling to translation mechanism.

    Change-Id: Ic96996cfcd3fbaf9e966670451bcce33f5d94673
    Closes-bug: #1620859

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

Reviewed: https://review.openstack.org/420020
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=5061db598dce24ad7ef875cfd85827624c0065e2
Submitter: Jenkins
Branch: master

commit 5061db598dce24ad7ef875cfd85827624c0065e2
Author: Peter Razumovsky <email address hidden>
Date: Mon Jan 9 15:59:15 2017 +0400

    Implement "Add" rule for Translation

    "Add" rule allows to merge lists and exists
    only for it.

    Change-Id: Idda890e794d52ee4b38dc88a90a41393c7557a65
    Closes-bug: #1620859

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

Reviewed: https://review.openstack.org/420021
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=6fc095c219fcae8fcab72bf516b3241a4c87715c
Submitter: Jenkins
Branch: master

commit 6fc095c219fcae8fcab72bf516b3241a4c87715c
Author: Peter Razumovsky <email address hidden>
Date: Mon Jan 9 16:53:34 2017 +0400

    Add "Resolve" rule for Translation

    Implement "Resolve" rule for Translation, which
    repeat old style translation mechanism replace rule.

    Change-Id: Ie36eeca24bcfe7509115fbb47fd0b89bf9272863
    Closes-bug: #1620859

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/heat 9.0.0.0b1

This issue was fixed in the openstack/heat 9.0.0.0b1 development milestone.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to heat (master)

Reviewed: https://review.openstack.org/420022
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=e159868d2fd9d54f5715ef85372e849d58fc56d1
Submitter: Jenkins
Branch: master

commit e159868d2fd9d54f5715ef85372e849d58fc56d1
Author: Peter Razumovsky <email address hidden>
Date: Fri Jan 13 17:49:12 2017 +0400

    Enable new translation mechanism

    Disable old translation mechanism and enable new
    one. Integrate it with Properties class and correct
    tests.

    Change-Id: I953a52e9b165d3ea4fb2fc57ceea8083c7f8f30c
    Closes-bug: #1620859

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

Reviewed: https://review.openstack.org/420023
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=da3126a128031f863d027e33bcd8c83bf873818d
Submitter: Jenkins
Branch: master

commit da3126a128031f863d027e33bcd8c83bf873818d
Author: Peter Razumovsky <email address hidden>
Date: Fri Jan 13 18:03:09 2017 +0400

    Add few functional tests for translation

    Add few issue sensitive functional tests for new
    translation mechanism.

    Change-Id: I7b808f314645f752f41dbdf6c0457ea23511bfb7
    Closes-bug: #1620859

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

Reviewed: https://review.openstack.org/420024
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=31639aac3163d7b34c372da1f59cd60406e881d2
Submitter: Jenkins
Branch: master

commit 31639aac3163d7b34c372da1f59cd60406e881d2
Author: Peter Razumovsky <email address hidden>
Date: Fri Jan 13 19:01:01 2017 +0400

    Remove unnecessary old translation mechanism

    Remove useless part of TranslationRule. Now, only Translation
    class implements translation mechanism.

    Change-Id: I303cb935d8750557274d5d97d8ef9a66ccf657ed
    Closes-bug: #1620859

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/heat 9.0.0.0b2

This issue was fixed in the openstack/heat 9.0.0.0b2 development milestone.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

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