HTTP error codes are all kinds of broke

[Zane Bitter]

The "fix" for the (non-) bug 1447194 ("Invalid exceptions raised in service.py") changed the perfectly valid exceptions raised in service.py to completely invalid ones.

Creating a new stack or updating an existing stack with a typo in one of the resource types in the template now results in a 404 error indicating that the stack doesn't exist.

Creating or updating a stack with a malformatted resource type (e.g. an empty string, or another type like an integer) in the template may now result in a 500 error.

In both cases we should obviously be returning 400, since the input is wrong.

The only improvement is that when querying the API for a resource schema for a template resource defined in the global environment, the patch now apparently unintentionally but arguably correctly returns 500 instead of 400. (This seems better since the problem is in the provider template installed on the server, not in any data the user has sent.)

The offending patch has only just been backported to Kilo and has not appeared in a release yet. Since the case of an invalid provider template in the global environment is pretty rare, and IMHO does not rise to the level that would justify changing API return codes in a stable branch, I propose to just revert the patch.

For master I'll come up with a fix that incorporates the improvement, and makes it explicit instead of implicit, and backport that to Liberty.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/248327
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=5ed06a9b780a4058fb0dcae253e16e55dd6c3912
Submitter: Jenkins
Branch: stable/kilo

commit 5ed06a9b780a4058fb0dcae253e16e55dd6c3912
Author: Zane Bitter <email address hidden>
Date: Fri Nov 20 22:00:59 2015 -0500

    Revert "Fix wrong type of exception raised"

    This reverts commit c5bb4d2ab83820f5ee2ac3fc6e8d9210e98b26ef.

    Every part of this patch was wrong, except that it did (by accident)
    start returning 500 errors instead of 400 errors in the case where a
    template in the global environment was not found, which is actually the
    operator's fault and not the user's. That's not really a serious enough
    problem to justify changing it on a stable branch though, so this is a
    straight revert. Luckily the patch hasn't made it into a release yet.

    Change-Id: Ia3154c7c41bb69cfa2ee36eb6d48fcd339900d60
    Closes-Bug: #1518458
    Related-Bug: #1447194

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/249324
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=775b862cf04d1a55069bd07b82f2bbcd2361c140
Submitter: Jenkins
Branch: stable/liberty

commit 775b862cf04d1a55069bd07b82f2bbcd2361c140
Author: Zane Bitter <email address hidden>
Date: Fri Nov 20 21:27:13 2015 -0500

    Fix HTTP error codes due to invalid templates

    This reverts commit 9a4d719ea1a6ca63f527b5c78308fd288058d42b and creates a
    new exception type, InvalidGlobalResource, that is raised when a
    non-existent template is referenced in the global environment and which
    results in an HTTP 500 error.

    This fixes the cases where we could return 404 or 500 errors as a result of
    template validation failures, when we should return a 400 error.

    Change-Id: I3c7b3182957448bb13514696cc2e12f5aaddd253
    Closes-Bug: #1518458
    Related-Bug: #1447194
    (cherry picked from commit 517f91ce27299a2a1e4551a8310e3a1ce1151b09)

[Thierry Carrez (ttx)]

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

[OpenStack Infra (hudson-openstack)]

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

commit 9935c38e5c2eab7bc1c151e4046405e150e84d7e
Author: Zane Bitter <email address hidden>
Date: Mon Nov 30 18:08:56 2015 -0500

    Raise EntityNotFound in get_resource_info()

    Rather than return None when doing get_resource_info() on resource types
    that don't exist, raise an EntityNotFound exception. We can then catch that
    and turn it into StackValidationFailed where appropriate, but we don't make
    assumptions about where the code is being called from at the point where we
    raise the exception.

    Change-Id: Idf6f837befea8ccb10fdaf9196490da3dd5be1fb
    Related-Bug: #1447194
    Related-Bug: #1518458

[OpenStack Infra (hudson-openstack)]

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

commit 06a713c4456203cd561f16721dc8ac3bcbb37a39
Author: Zane Bitter <email address hidden>
Date: Tue Nov 24 12:29:38 2015 -0500

    Add a separate get_class_to_instantiate() method to Environment

    We were previously using get_class for two different purposes - to get a
    resource plugin on which we could perform introspection to obtain the
    properties and attributes schema, and to get a resource plugin we could
    instantiate to create a Resource object. These are both the same except in
    the case of a TemplateResource, where having two different use cases for
    the same piece of code was adding considerable extra complexity. Combining
    the use cases in this way also made the error handling confusing (leading
    to bug 1518458).

    This change separates out the two cases.

    Change-Id: I3bde081cd4537810c8c6a0948ab447c3fb7ca9bc
    Related-Bug: #1447194
    Related-Bug: #1518458
    Related-Bug: #1508115

[OpenStack Infra (hudson-openstack)]

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

commit 9a3c3c0577c12aa02c6571ae908e7ea64d5008b4
Author: Zane Bitter <email address hidden>
Date: Tue Nov 24 12:29:38 2015 -0500

    Get rid of the Resource.resource_class() method

    Now that we have get_class_to_instantiate() and it always returns
    TemplateResource (instead of a custom subclass) for template resources, we
    can just compare the types of resources directly.

    Change-Id: I96275c3ead3ea3565e55b99a8c5a9948eaa15b78
    Related-Bug: #1508115
    Related-Bug: #1447194
    Related-Bug: #1518458

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/liberty
Review: https://review.openstack.org/269691

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/kilo
Review: https://review.openstack.org/269692

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/269691
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=87116e262329e5cd309a0f1e35ffefd06080022c
Submitter: Jenkins
Branch: stable/liberty

commit 87116e262329e5cd309a0f1e35ffefd06080022c
Author: Zane Bitter <email address hidden>
Date: Tue Nov 24 12:29:38 2015 -0500

    Load template files only from their known source

    Modify get_class to ensure that user-defined resources cannot result in
    reads from the local filesystem. Only resources defined by the operator
    in the global environment should read local files.

    To make this work, this patch also adds a separate
    get_class_to_instantiate() method to the Environment.

    We were previously using get_class for two different purposes - to get a
    resource plugin on which we could perform introspection to obtain the
    properties and attributes schema, and to get a resource plugin we could
    instantiate to create a Resource object. These are both the same except in
    the case of a TemplateResource, where having two different use cases for
    the same piece of code was adding considerable extra complexity. Combining
    the use cases in this way also made the error handling confusing (leading
    to bug 1518458).

    This change separates out the two cases.

    Change-Id: I845e7d23c73242a4a4c9c40599690ab705c75caa
    Closes-Bug: #1496277
    Related-Bug: #1447194
    Related-Bug: #1518458
    Related-Bug: #1508115
    (cherry picked from commit 06a713c4456203cd561f16721dc8ac3bcbb37a3
                           and 26e6d5f6d776c1027c4f27058767952a58d15e25)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/269692
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=fa19a617a79fd1cb0d892bb8ea87c4b9f6398c34
Submitter: Jenkins
Branch: stable/kilo

commit fa19a617a79fd1cb0d892bb8ea87c4b9f6398c34
Author: Zane Bitter <email address hidden>
Date: Tue Nov 24 12:29:38 2015 -0500

    Load template files only from their known source

    Modify get_class to ensure that user-defined resources cannot result in
    reads from the local filesystem. Only resources defined by the operator
    in the global environment should read local files.

    To make this work, this patch also adds a separate
    get_class_to_instantiate() method to the Environment.

    We were previously using get_class for two different purposes - to get a
    resource plugin on which we could perform introspection to obtain the
    properties and attributes schema, and to get a resource plugin we could
    instantiate to create a Resource object. These are both the same except in
    the case of a TemplateResource, where having two different use cases for
    the same piece of code was adding considerable extra complexity. Combining
    the use cases in this way also made the error handling confusing (leading
    to bug 1518458).

    This change separates out the two cases.

    Change-Id: I845e7d23c73242a4a4c9c40599690ab705c75caa
    Closes-Bug: #1496277
    Related-Bug: #1447194
    Related-Bug: #1518458
    Related-Bug: #1508115
    (cherry picked from commit 06a713c4456203cd561f16721dc8ac3bcbb37a3
                           and 26e6d5f6d776c1027c4f27058767952a58d15e25)

[Doug Hellmann (doug-hellmann)]

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

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/liberty
Review: https://review.openstack.org/285661

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/285661
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=645810eb4038bea334b7c996e49a4ed4f7379290
Submitter: Jenkins
Branch: stable/liberty

commit 645810eb4038bea334b7c996e49a4ed4f7379290
Author: Rakesh H S <email address hidden>
Date: Tue Mar 1 12:08:42 2016 +0530

    Raise ResourceTypeNotFound in get_resource_info()

    Rather than return None when doing get_resource_info() on resource types
    that don't exist, raise a ResourceTypeNotFound exception. We can then catch
    that and turn it into StackValidationFailed where appropriate, but we don't
    make assumptions about where the code is being called from at the point where
    we raise the exception.

    Change-Id: Idf6f837befea8ccb10fdaf9196490da3dd5be1fb
    Related-Bug: #1447194
    Related-Bug: #1518458
    Closes-Bug: #1550179