allocation schema does not set additionalProperties False in all the right places

[Chris Dent]

In microversion 1.12 of placement, a schema for allocations was introduced that required the allocations, project_id and user_id fields. This schema is used for subsequent microversions, copied and manipulated as required.

However, it has a flaw. It does not set additionalProperties: False for the object at which the required fields are set. This means you can add a field and it glides through. This flaw cascades all the way to the reshaper (where I had a test that refused to fail in the expected way).

The diff below demonstrates the problem and a potential fix, but this fix may not be right as it is in the 1.12 microversion and we might want it on microversion 1.30 and beyond, only (which is a pain).

I think we should just fix it, as below, but I'll let others chime in too.

diff --git a/nova/api/openstack/placement/schemas/allocation.py b/nova/api/openstack/placement/schemas/allocation.py
index e149ae3beb..796b7c5d01 100644
--- a/nova/api/openstack/placement/schemas/allocation.py
+++ b/nova/api/openstack/placement/schemas/allocation.py
@@ -113,8 +113,9 @@ ALLOCATION_SCHEMA_V1_12 = {
             "type": "string",
             "minLength": 1,
             "maxLength": 255
- }
+ }
     },
+ "additionalProperties": False,
     "required": [
         "allocations",
         "project_id",
diff --git a/nova/tests/functional/api/openstack/placement/gabbits/allocations-1.28.yaml b/nova/tests/functional/api/openstack/placement/gabbits/allocations-1.28.yaml
index df8fadd66b..04107a996b 100644
--- a/nova/tests/functional/api/openstack/placement/gabbits/allocations-1.28.yaml
+++ b/nova/tests/functional/api/openstack/placement/gabbits/allocations-1.28.yaml
@@ -238,7 +238,9 @@ tests:
       project_id: $ENVIRON['PROJECT_ID']
       user_id: $ENVIRON['USER_ID']
       consumer_generation: null
- status: 204
+ bad_field: moo
+ #status: 204
+ status: 400

 - name: put that allocation to existing consumer
   PUT: /allocations/22222222-2222-2222-2222-222222222222

[Jay Pipes (jaypipes)]

I say just fix it, in 1.12. Pretty sure the API group has been consistent in their guidance about just fixing errors without a microversion change, right?

[Jay Pipes (jaypipes)]

FWIW, I always thought it was kinda weird that additionalProperties wasn't false by default...

[Chris Dent (cdent)]

If, by "API group" you mean API-SIG that's not what the guidance has been. The guidance has been that if a non-500 response changes, that's a new microversion. In this case a request with an alien field that resulted in a 200 or 204 could become a 400 with the proposed fix in place.

With my api-sig hat on, that means we need a microversion.

With my "how did I ever become an adjuticator of microversions, kill me now" hat on, I don't care.

[Ed Leafe (ed-leafe)]

So the scenario is: two deployments, one with this fix, and one without. Identical requests with the additional property are made to both, at the same microversion. One deployment will return a 2xx, and the other will return a 400. Is that what you are talking about?

if it is, that sounds like the classic justification for microversions.

[Tetsuro Nakamura (tetsuro0907)]

+1 on just fix it.

Microversion is good for "older clients with new cloud" and "user discovery of available features between clouds".
In this case I don't think either there are older clients who use this bad_field or this is a new feature.

[OpenStack Infra (hudson-openstack)]

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

[Eric Fried (efried)]

It's definitely wrong to "just fix it".

But do that anyway.

[Matt Riedemann (mriedem)]

Being an admin-only API also comes into play at some level. It's one thing for applications and users of the cloud to break on interop stuff like this, but it's another for admin-only / service level type APIs which we don't expect regular users to be hitting directly. It's a fine line but that's one way to justify not doing a microversion.

https://docs.openstack.org/nova/latest/contributor/microversions.html#when-a-microversion-is-not-needed

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/583907
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=0fc4f95914dec7df68e53514771bf3afe1ee9c4f
Submitter: Zuul
Branch: master

commit 0fc4f95914dec7df68e53514771bf3afe1ee9c4f
Author: Chris Dent <email address hidden>
Date: Thu Jul 19 10:40:58 2018 +0100

    [placement] disallow additional fields in allocations

    Back in microversion 1.12, when the allocations structure was extended
    to allow project_id and user_id on PUT /allocations/{uuid},
    "additionalProperties" was not set in the JSON schema, so it has been
    possible since then to include unused fields in the input. The schema
    was then reused in the creation of subsequent schema for new
    microversions and for new URIs, such as POST /allocations and the
    forthcoming /reshaper.

    This change fixes it by fixing the old microversion. This is the "just
    fix it" option from the discussion on the associated bug. The other
    option is to create a new microversion that corrects the behavior. This
    is more complex than it might initially sound because of the way in
    which the original schema is used to compose new ones.

    Change-Id: Ied464744803864e61a45e03c559760a8a2e2581f
    Closes-Bug: #1782340

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova 18.0.0.0b3 development milestone.