repeat function argument is evaluated too eagerly

Bug #1553306 reported by Zane Bitter
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Heat
Fix Released
High
Zane Bitter

Bug Description

Prior to the fix for bug 1466748, the repeat function did not support using an intrinsic function as a top-level definition of one of the for_each keys, due to type checking of arguments. Specifically this did not work:

   repeat:
     for_each:
       %extnetwork%: {get_param: external_networks}
    template: ...

I believe this was an oversight in the original implementation.

The fix for bug 1466748 fixes this by resolving function references at parse time, so that the get_param intrinsic above resolves to a list before it is typed checked.

Unfortunately this is an incorrect way of handling it, because it works only for functions (like get_param) that are resolvable at parse time. So a user is still unable to use something like:

   repeat:
     for_each:
       %extnetwork%: {get_attr: [some_resource, external_networks]}
    template: ...

but, worse than that, there is a regression because constructs that would previously have worked fine, like:

   repeat:
     for_each:
       %extnetwork%:
        - {get_attr: [resource1, network]}
        - {get_attr: [resource2, network]}
    template: ...

are now broken - not necessarily with an error, but by simply returning the wrong data (get_attr will evaluate to None at parse time in many cases).

The correct solution, and the one consistent with other intrinsic functions, is to delay the resolution of arguments until runtime, but to explicitly allow intrinsic functions as arguments.

Zane Bitter (zaneb)
description: updated
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/288794

Changed in heat:
status: New → In Progress
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/288795

Zane Bitter (zaneb)
Changed in heat:
milestone: none → mitaka-rc1
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to heat (master)

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

commit bf60398c906b3140553976513e9f08ecc44433c8
Author: Zane Bitter <email address hidden>
Date: Fri Mar 4 18:30:28 2016 -0500

    Resolve arguments to 'repeat' function at runtime

    This fixes a regression caused by a7c4b2332f9d1accaa89b51fbc3b09014995983a
    in evaluating the 'for_each' argument too early (at parse time - before the
    result of runtime functions like get_attr are available), while preserving
    the fix for bug 1466748 in allowing intrinsic functions to be used in the
    argument.

    Change-Id: If1e9c754ef372de2f7edd32a9a8247eb271c5871
    Closes-Bug: #1553306
    Related-Bug: #1466748

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

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

commit 5c14291eef0063111c1345bc0b7132356acb2eee
Author: Zane Bitter <email address hidden>
Date: Fri Mar 4 14:20:01 2016 -0500

    Refactor validation of arguments to 'repeat' intrinsic function

    Separate validation of the structure of the function call itself from
    validation of the arguments passed. The former remains in the
    initialisation method, while the latter moves to the validate() method.
    This is consistent with how other intrinsic functions are instantiated,
    and should hopefully lead to less confusion of the kind that resulted in
    bug 1553306.

    Change-Id: Ie20b79c33bded82db3befd3ca25fa76ebed3d417
    Related-Bug: 1553306

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

Fix proposed to branch: stable/liberty
Review: https://review.openstack.org/291324

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

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/291343

Revision history for this message
Thierry Carrez (ttx) wrote : Fix included in openstack/heat 6.0.0.0rc1

This issue was fixed in the openstack/heat 6.0.0.0rc1 release candidate.

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

Reviewed: https://review.openstack.org/291324
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=4d13d27eca036a34ae6acd62b1e39a82cdaebb21
Submitter: Jenkins
Branch: stable/liberty

commit 4d13d27eca036a34ae6acd62b1e39a82cdaebb21
Author: Zane Bitter <email address hidden>
Date: Fri Mar 4 18:30:28 2016 -0500

    Resolve arguments to 'repeat' function at runtime

    This fixes a regression caused by a7c4b2332f9d1accaa89b51fbc3b09014995983a
    in evaluating the 'for_each' argument too early (at parse time - before the
    result of runtime functions like get_attr are available), while preserving
    the fix for bug 1466748 in allowing intrinsic functions to be used in the
    argument.

    Change-Id: If1e9c754ef372de2f7edd32a9a8247eb271c5871
    Closes-Bug: #1553306
    Related-Bug: #1466748
    (cherry picked from commit bf60398c906b3140553976513e9f08ecc44433c8)

tags: added: in-stable-liberty
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on heat (stable/kilo)

Change abandoned by Dave Walker (<email address hidden>) on branch: stable/kilo
Review: https://review.openstack.org/291343
Reason:
stable/kilo closed for 2015.1.4

This release is now pending its final release and no freeze exception has
been seen for this changeset. Therefore, I am now abandoning this change.

If this is not correct, please urgently raise a thread on openstack-dev.

More details at: https://wiki.openstack.org/wiki/StableBranch

Zane Bitter (zaneb)
tags: removed: liberty-backport-potential
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/heat 5.0.2

This issue was fixed in the openstack/heat 5.0.2 release.

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.