)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"e307c667dc31cc2f22f39b73473fe8fbf1679bd4","unresolved":false,"context_lines":[{"line_number":10,"context_line":"for (add|remove)_subports, i.e. basically anything other than the"},{"line_number":11,"context_line":"following format:"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"    {\u0027sub_ports\u0027: []}"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"the server will return an HTTPInternalServerError with no details about"},{"line_number":16,"context_line":"why the request failed."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_f5cd77b8","line":13,"updated":"2016-08-18 21:22:03.000000000","message":"when/how is this happening? Try to feed anything bad to a neutron server and see what happens...it\u0027s not going to like it, I don\u0027t think we should pollute the rules and plugin logic with how the request payload is supposed to be validated.","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"d955bcaa9e9fe1948e9f983e0550148fae1dd957","unresolved":false,"context_lines":[{"line_number":10,"context_line":"for (add|remove)_subports, i.e. basically anything other than the"},{"line_number":11,"context_line":"following format:"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"    {\u0027sub_ports\u0027: []}"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"the server will return an HTTPInternalServerError with no details about"},{"line_number":16,"context_line":"why the request failed."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_5039f9d7","line":13,"in_reply_to":"3ac371cc_d5295bf4","updated":"2016-08-18 21:43:55.000000000","message":"You can\u0027t do that because that\u0027s not the format that the API server is supposed to understand and that was wrong in the first place. It\u0027s not one or the other. The 1st form we had is wrong and we should not support both.","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"},{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"3587877937846d536681fc224dd3fda72583e8e6","unresolved":false,"context_lines":[{"line_number":10,"context_line":"for (add|remove)_subports, i.e. basically anything other than the"},{"line_number":11,"context_line":"following format:"},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"    {\u0027sub_ports\u0027: []}"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"the server will return an HTTPInternalServerError with no details about"},{"line_number":16,"context_line":"why the request failed."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_d5295bf4","line":13,"in_reply_to":"3ac371cc_f5cd77b8","updated":"2016-08-18 21:39:44.000000000","message":"If the payload passed is just a list of subports (i.e. the 1st form we had), the trunk service plugin throws a KeyError at [1] and [2]. The generic error returned from the server gives no explanation as to why it failed, when it\u0027s actually a bad subports request body. The same thing goes for sending an empty dict, unnecessary keys, etc.\n\nThis is the same validation that is done automatically by the API when creating a trunk, but currently it\u0027s not done for add|remove_subports. Why shouldn\u0027t we do it?\n\n[1] https://github.com/openstack/neutron/blob/master/neutron/services/trunk/plugin.py#L235\n[2] https://github.com/openstack/neutron/blob/master/neutron/services/trunk/plugin.py#L286","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"6a00f47b8cd9a75da44e2deb2ac232db993f8a74","unresolved":false,"context_lines":[{"line_number":13,"context_line":"    {\u0027sub_ports\u0027: []}"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"the server will return an HTTPInternalServerError with no details about"},{"line_number":16,"context_line":"why the request failed."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This patch improves the request validation to return an InvalidInput"},{"line_number":19,"context_line":"error with a detailed message about the bad request format, similar to"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_d5483b8d","line":16,"updated":"2016-08-18 21:40:37.000000000","message":"try this one for instance:\n\nurl -g -i -X POST http://localhost:9696/v2.0/networks.json -H \"User-Agent: python-neutronclient\" -H \"Content-Type: application/json\" -H \"Accept: application/json\" -H \"X-Auth-Token: $TOKEN\" -d \u0027{\"network\": \"crap\"}\u0027\n\nYou\u0027re gonna get the same failure mode. We should probably deal that more holistically.","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"ed7616429fa06e2412af31a5be380ad827da9bb5","unresolved":false,"context_lines":[{"line_number":13,"context_line":"    {\u0027sub_ports\u0027: []}"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"the server will return an HTTPInternalServerError with no details about"},{"line_number":16,"context_line":"why the request failed."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This patch improves the request validation to return an InvalidInput"},{"line_number":19,"context_line":"error with a detailed message about the bad request format, similar to"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_b0666d27","line":16,"in_reply_to":"3ac371cc_507bb929","updated":"2016-08-18 22:18:52.000000000","message":"The invalid body that is passed over using /trunks/\u003cid\u003e/add_subports would generate the same type of failure to the eyes of the API layer. Now if it were a TypeError, IndexError, etc we could consider translating that, but again why do you think that the answer is sprinkling validation code all over the place? That sounds like a bad strategy to me. Now if you explained to me how exactly you\u0027re hitting this issue, and why you think it\u0027s of paramount importance to fix it just for trunks, we\u0027d save each other the time we can use to focus on something more urgent.\n\nI don\u0027t disagree that is not a clean failure more we\u0027re experiencing, but the fact that we have this affecting virtually 99.99% of everything that Neutron exposed so far, trying to fix it for 0.01% of what\u0027s been just added, tells me that there\u0027s something wrong in the 99.99% and we should not fix it narrowly.","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"},{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"0030a42de691dbc6d79d1e196b8e5ae7816b1ea9","unresolved":false,"context_lines":[{"line_number":13,"context_line":"    {\u0027sub_ports\u0027: []}"},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"the server will return an HTTPInternalServerError with no details about"},{"line_number":16,"context_line":"why the request failed."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This patch improves the request validation to return an InvalidInput"},{"line_number":19,"context_line":"error with a detailed message about the bad request format, similar to"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_507bb929","line":16,"in_reply_to":"3ac371cc_d5483b8d","updated":"2016-08-18 21:58:00.000000000","message":"The same thing happens with \u0027{\"trunk\": \"crap\"}\u0027, which seems like it could also be fixed holistically, but what about requests to things like \"/trunks/\u003cid\u003e/add_subports\"? Even with a holistic fix for base-level resources like /networks.json and /trunks.json, it wouldn\u0027t provide a fix for subresources like that, would it? It seems like we\u0027d have to work it into the service plugin somehow, unless it could be handled in the extension definition somewhere around [1].\n\n[1] https://github.com/openstack/neutron/blob/master/neutron/extensions/trunk.py#L81","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"018a39cbee96fd1beb036d2215305eba65219339","unresolved":false,"context_lines":[{"line_number":19,"context_line":"error with a detailed message about the bad request format, similar to"},{"line_number":20,"context_line":"what the server returns when create_trunk receives a bad request."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Partially-implements: bp/vlan-aware-vms"},{"line_number":23,"context_line":"Change-Id: I5d034661da86686b86ed5400bc6468acc4e1844b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_f5857707","line":22,"updated":"2016-08-18 21:26:44.000000000","message":"Incidentally this may be relevant: https://bugs.launchpad.net/neutron/+bug/1614022","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"2330fee4b67231399fa82343c9e1438189dd6dae","unresolved":false,"context_lines":[{"line_number":19,"context_line":"error with a detailed message about the bad request format, similar to"},{"line_number":20,"context_line":"what the server returns when create_trunk receives a bad request."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Partially-implements: bp/vlan-aware-vms"},{"line_number":23,"context_line":"Change-Id: I5d034661da86686b86ed5400bc6468acc4e1844b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_d09969d2","line":22,"in_reply_to":"3ac371cc_d5bafb70","updated":"2016-08-18 21:47:26.000000000","message":"The root cause is different, yes. The failure mode is the same. The API layer is not handling malformed input. We should fix that. Has this been a burning priority? No, if some uses published API bindings or the CLI. And even if we do want to fix it we should not fix it all over the place, but once and for all. The trick is to understand when an uncaught exception is a result of a malformed input, and that\u0027s probably this has been left alone for this long.","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"},{"author":{"_account_id":16800,"name":"Rawlin Peters","email":"rawlin.peters@hpe.com","username":"rawlin"},"change_message_id":"3587877937846d536681fc224dd3fda72583e8e6","unresolved":false,"context_lines":[{"line_number":19,"context_line":"error with a detailed message about the bad request format, similar to"},{"line_number":20,"context_line":"what the server returns when create_trunk receives a bad request."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Partially-implements: bp/vlan-aware-vms"},{"line_number":23,"context_line":"Change-Id: I5d034661da86686b86ed5400bc6468acc4e1844b"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3ac371cc_d5bafb70","line":22,"in_reply_to":"3ac371cc_f5857707","updated":"2016-08-18 21:39:44.000000000","message":"That looks like a similar error, but I don\u0027t think the fix for that would fix this validation if that\u0027s what you mean.\n\nThe problem is that the same validation that is done for the trunk request body (sub_ports) isn\u0027t done for the add/remove_subports request body. We changed the subports request body to match the trunk request body, but we didn\u0027t give it the same validation.","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"}],"neutron/services/trunk/plugin.py":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"3c46d34fed02466dc0f307c7326e17adf6433a28","unresolved":false,"context_lines":[{"line_number":232,"context_line":"        \"\"\"Add one or more subports to trunk.\"\"\""},{"line_number":233,"context_line":"        # Check for basic validation since the request body here is not"},{"line_number":234,"context_line":"        # automatically validated by the API layer."},{"line_number":235,"context_line":"        subports \u003d subports[\u0027sub_ports\u0027]"},{"line_number":236,"context_line":"        subports_validator \u003d rules.SubPortsValidator("},{"line_number":237,"context_line":"            self._segmentation_types, subports)"},{"line_number":238,"context_line":"        subports \u003d subports_validator.validate(context, basic_validation\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":1,"id":"3ac371cc_95da4316","side":"PARENT","line":235,"updated":"2016-08-18 21:29:12.000000000","message":"you\u0027re undoing what was done in [1], and doing it in a way that I did not want to pursue.\n\n[1] https://review.openstack.org/#/c/345185/2/neutron/services/trunk/plugin.py","commit_id":"793e572617e16e4801cda1ff80fadc949baa96d1"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"e307c667dc31cc2f22f39b73473fe8fbf1679bd4","unresolved":false,"context_lines":[{"line_number":120,"context_line":"        trunk_details[\u0027port_id\u0027] \u003d trunk_validator.validate(context)"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"        subports_validator \u003d rules.SubPortsValidator("},{"line_number":123,"context_line":"            self._segmentation_types, {\u0027sub_ports\u0027: trunk[\u0027sub_ports\u0027]},"},{"line_number":124,"context_line":"            trunk[\u0027port_id\u0027])"},{"line_number":125,"context_line":"        trunk_details[\u0027sub_ports\u0027] \u003d subports_validator.validate(context)"},{"line_number":126,"context_line":"        return trunk_details"}],"source_content_type":"text/x-python","patch_set":1,"id":"3ac371cc_35470f1f","line":123,"updated":"2016-08-18 21:22:03.000000000","message":"When I looked at this in the context of [1], I did not want to pollute this logic with how the body is marshalled and passed over. We should keep it that way.\n\n[1] https://review.openstack.org/#/c/345185/","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"}],"neutron/services/trunk/rules.py":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"e307c667dc31cc2f22f39b73473fe8fbf1679bd4","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        # Perform basic validation on subports, in case subports"},{"line_number":128,"context_line":"        # are not automatically screened by the API layer."},{"line_number":129,"context_line":"        if basic_validation:"},{"line_number":130,"context_line":"            key_spec \u003d {\u0027sub_ports\u0027: {\u0027required\u0027: True,"},{"line_number":131,"context_line":"                                      \u0027type:subports\u0027: None}}"},{"line_number":132,"context_line":"            msg \u003d validators.validate_dict(self.subports_req, key_spec)"},{"line_number":133,"context_line":"            if msg:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3ac371cc_5571eb76","line":130,"updated":"2016-08-18 21:22:03.000000000","message":"nope","commit_id":"ba02f47e9dda5cf0b079ac32c989fb311fe593fa"}]}