12:59:57 #startmeeting nova api 12:59:58 Meeting started Wed Nov 16 12:59:57 2016 UTC and is due to finish in 60 minutes. The chair is alex_xu. Information about MeetBot at http://wiki.debian.org/MeetBot. 12:59:59 Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. 13:00:01 The meeting name has been set to 'nova_api' 13:00:09 who is here today? 13:00:14 o/ 13:00:20 o/ 13:00:48 o/ 13:00:52 let us wait one more minute for johnthetubaguy get a coffee :) 13:01:22 ah, I meant don't wait will be back in a sec 13:01:31 0/ 13:01:38 o/ 13:01:39 hehe 13:01:52 let me whether we have action from previous 13:02:21 no action from previous 13:02:29 #topic priority task 13:02:34 :) 13:02:45 #link https://review.openstack.org/388518 13:03:30 ^ thanks cdent point it out actually we can validate single value with max length of list. So I changed the spec, it makes the json-schema simpler 13:03:45 avoid to use 'oneOf' 13:04:05 but we need use 'array' for all the parameters, but it still looks simper than 'oneOf' 13:04:27 #link https://review.openstack.org/#/c/388518/11/specs/ocata/approved/consistent-query-parameters-validation.rst@96 13:04:39 ^ you guys can find out the example from the link 13:04:40 yeah, just been looking at that, very cunning 13:05:11 so in the conversion of the query string params, if its not a list, we add it as an item of one in a list? 13:05:21 johnthetubaguy: yes 13:05:36 at line 87, there is example 13:05:42 alex_xu: did you mean to delete line 118? 13:05:55 oh, I see, keep the utils 13:05:57 johnthetubaguy: no, still want to keep them 13:06:00 yea 13:06:22 I will update the poc later 13:06:46 alex_xu: you mean that solve the multiple occurrence of same param thing ? 13:06:51 or something else 13:07:10 gmann_: yea 13:07:29 gmann_: for the case name=abc&name=def 13:07:52 alex_xu: but i thought oneOf was only needed for sort_key only 13:08:16 gmann_: with new way, we needn't oneOf anymore 13:09:27 gmann_: for multiple value, it is array. for single value, it is array limit maxItems=1 13:09:29 alex_xu: but still it would not restrict name=abd&name=def right? 13:09:52 alex_xu: ohk, you will parse as list, got it 13:10:00 gmann_: yea, we won't for now. 13:10:09 so the second spec 13:10:25 one sec 13:10:26 #link https://review.openstack.org/393205 13:10:28 oops 13:10:30 johnthetubaguy: go ahead 13:10:42 there was a note about allowing multiple values for a single one 13:10:49 like foo=bar,foo=baz 13:10:59 today we just pick one of them, or something? 13:11:02 alex_xu: so we will be parsing all query params into list and then schema can handle 13:11:16 gmann_: yes 13:11:33 johnthetubaguy: yes 13:11:38 johnthetubaguy: yea json schema just pick one but that same case for request body validation also 13:11:40 I was thinking about this bit: https://review.openstack.org/#/c/388518/11/specs/ocata/approved/consistent-query-parameters-validation.rst@193 13:11:44 we just pick oen of them 13:12:02 oh, do you have an example of that? 13:12:07 yea, that thanks to gmann_ point it out 13:12:41 for example, change-since accept a datefomart 13:12:53 johnthetubaguy: if you run create server tests with 2 name there 1 invalid, still pass 13:13:17 so you could have multiples in the body 13:13:26 I am thinking about keypairs I guess 13:13:29 like this one: 13:13:30 https://git.openstack.org/cgit/openstack/nova/tree/nova/api/openstack/compute/keypairs.py#n122 13:13:50 it seems that would only deal if there was a single type parameter 13:14:06 oops 13:14:09 thats the body 13:14:40 johnthetubaguy: there are multiple value the user put in the url. Then all the values pass into the nova api python code. 13:14:58 The python code only will pick one of them(probably the last one I guess), then validate it 13:15:08 I thinking we have ?user_id=2,user_id=3 13:15:09 yea 13:15:20 is it the routing code that hides that? 13:15:31 maybe other value are invalid, only the last one is valid, the request still can pass the validation 13:15:37 and json schema pick either which is valid 13:15:57 so once we have strict validation, that would be invalid, it has to pass if it works today 13:16:01 so agreed with the approch 13:16:13 But with the new proposal, the json-schema will validate all the input value even the python code only pick the last one 13:16:36 johnthetubaguy: alex_xu i simply ran functional tests with 2 name(1 valid) and API did not complain. json schema picked valid one and ignore invalid one 13:16:41 yes, that seems correct, longer term 13:16:53 alex_xu: 1 sec 13:17:13 I am worried more about the query string params, I thought name was a body param? 13:17:15 alex_xu: i thought it will pick both and make list where json schema will 400 as maxItem is 1 13:17:29 for json, I believe the last one in the document wins 13:17:40 although its a shame we don't call that invalid 13:17:57 johnthetubaguy: it isn't longer term, the json-schema will validate all the values for any request 13:18:13 sorry, I am crossing the streams/wires here 13:18:23 johnthetubaguy: even it can pick first one in case second one is invalid 13:18:45 I am trying to see how the current code accepts multiple and ignores one today, for the keypairs API 13:19:01 just the query strings for now 13:20:31 emm...sorry, I lost the context also :( 13:20:38 alex_xu: last query- so in this spec, if user out multiple value for example for name (name=abc&name-def) it will 400 right. 13:20:49 out->put 13:21:10 as we are making list for each param and passing to json schema 13:21:27 gmann_: do you mean the spec https://review.openstack.org/393205, it won't, just ignore one of them 13:21:43 so lets back up 13:21:54 I think we should focus on the real api, keypairs 13:22:01 thats what the spec is modifying 13:22:08 yea 13:22:13 alex_xu: no this one - https://review.openstack.org/#/c/388518/11/specs/ocata/approved/consistent-query-parameters-validation.rst@193 13:22:17 so I am trying to work out what happens today with the user param 13:23:29 I guess this bit: 13:23:30 https://git.openstack.org/cgit/openstack/nova/tree/nova/api/openstack/compute/keypairs.py#n185 13:23:40 I was going blind but I see it now 13:23:58 gmann_: that spec only describe the new mechanism, whether we return 400, depend the schema 13:24:44 alex_xu: yea 13:24:52 johnthetubaguy: it only take one of them 13:25:03 johnthetubaguy: yea 13:27:13 johnthetubaguy: so.... 13:27:48 in the distant future, when additional_properties = False 13:27:48 we would want user_id=1&user_id=2 to fail 13:27:48 right now we just pick the first one 13:27:48 it feels like we need to preserve that 13:27:48 for the moment 13:27:48 the spec says that, I believe? 13:27:48 I was unclear... 13:27:48 need to keep the same API semantics, so we will not be as strict as we could be about duplicates 13:27:49 now, we have a few options for the validation 13:27:49 1) reject the request with duplicates, long term, we probably want that 13:27:49 2) only validate the element the code will touch 13:28:14 oops, I have network delay 13:28:36 I thought the point of putting this stuff under a microversio was so that we _don't_ need to preserve the old ambiguous behavior? 13:29:15 but duplicates were just being not taken care, should give 400 13:29:21 which is broken as of now 13:29:47 as that was a good example of bad usage 13:30:22 cdent: so step one is don't break what we have, just protect us against bad stuff, step 2 is make it better 13:30:31 at least thats what I have in my head 13:31:16 basically like 2.0 vs 2.1 and the body validation 13:31:19 emm...+1 for don't break what we have now 13:31:45 so you said the validation only takes one of them? 13:31:57 I thought it would get passed the full list of params 13:32:29 johnthetubaguy: with current proposal, it would get passed the full list of params 13:32:38 yep, I am fine with that 13:32:57 alex_xu: and 400 for duplicate case 13:33:12 well, that breaks the existing requests, that would need a new microversion 13:33:27 yea 13:33:38 johnthetubaguy: even that usage was not right 13:34:10 * alex_xu should put a real example in the REST API impact section 13:34:15 alex_xu: but this will return 400 -https://review.openstack.org/#/c/388518/11/specs/ocata/approved/consistent-query-parameters-validation.rst@102 13:34:22 alex_xu: in current spec 13:35:13 so if duplicate value then json object from query param will be like 'name': ['abc', def], 13:35:23 which we should totally do at some point, but is not proposed in the current spec (its mentioned as future work) 13:35:28 gmann_: maybe I didn't clear about that. It is only an example to show how the new mechanism works, it didn't talk about the real would 13:35:56 alex_xu: +1, I think your proposals is good, if I understand it now, but an example would really help 13:36:21 yea that was my assumption but examle seems retricted those 13:37:00 alex_xu: because maxItems in example makes 400 for duplicate 13:37:05 gmann_: I would try to add note clarify that 13:37:28 alex_xu: Thanks 13:37:41 gmann_: emm....forget that one, this spec is about the real world https://review.openstack.org/393205 :) 13:37:58 :) 13:38:54 * johnthetubaguy hmm, I think I am seeing network delays somwhere now 13:39:02 I will upda the spec after the meeting, one note for the example, and put a real world example for the thing in the rest api impact section 13:39:10 s/upda/update/ 13:39:54 so, are we good for the base spec one? let us talk about the second one? 13:41:07 I guess i'm waiting for the network delays now :) 13:41:16 yeah, lets talk second one 13:41:27 #link https://review.openstack.org/393205 13:41:31 yea, sorry got disconnected 13:41:57 for me this looks fine, just version bump needs to be more clear 13:41:59 gmann__: no worries, you didn't miss anything :) 13:42:07 oh :) 13:42:29 alex_xu: johnthetubaguy so that is version bump plan what i stated in comment ? 13:42:38 gmann__: so...no version bump at here 13:43:07 alex_xu: yes, and in future we bump when we will reject invalid filter 13:43:21 yeah, the spec says there is a version bump, I thought we were going to not do that for the moment 13:43:23 gmann__: yea 13:43:40 oops, just found that 13:43:50 that is wrong 13:43:50 yea just ignore for invalid filter and 400 for invalid sort_key 13:44:08 so lets go back to why 13:44:17 existing requests that worked in the past should be accepted 13:44:27 +1 13:44:37 bad requests that were dangerous need to be made "safe" 13:44:46 so the 500 errors can go to 400 errors 13:45:15 the ignore unknown keys seems to be the best approach 13:45:24 and fail with invalid values for know keys 13:45:41 johnthetubaguy: for sort_key also ? 13:45:48 should be the same for both I think 13:46:01 they are both "dangerous" in some sense 13:46:07 johnthetubaguy: i mean if any other key than whitelist for sort_key 13:46:14 johnthetubaguy: yea 13:46:32 totally agree with approach. 13:46:41 sorry, I get what you mean now 13:46:46 the filters are all different keys 13:46:56 but the sort thing is a value 13:47:09 yea 13:47:12 so it will trigger 400 errors 13:47:20 thats annoying, but its consistent 13:47:40 we support multiple sort keys? 13:48:02 yea invalid value gives 400 13:48:03 yes 13:48:13 yes for multiple sort keys 13:48:30 L88 says sort key not sort_key's value :) 13:48:49 "so the 500 errors can go to 400 errors": for the filters, it go to ignore, not 400 13:49:20 or you want that go to 400? 13:49:56 so it gets tricky 13:50:22 we could have a *these are bad* list, and reject those, but ignore unknown values, but that seems like a mess 13:50:44 ah, yea 13:50:44 I think I am OK with filters we don't know, just getting the standard ignore unknown key treatment for now 13:51:35 if user use that bad key, that means the bad request will break their client code? 13:52:25 right, hence the ignore / strip approach 13:52:39 alex_xu: but current code ignore that right? for admin (as we have only non-admin list in code) 13:53:26 operator feedback on the restriction is probably a good idea, but I am not sure what we do if they don't like it 13:53:38 gmann__: I thought the "bad" point to the joined table and column without index 13:53:52 for me too, ignore/strip approach is more better way to go 13:54:34 going back to that, we seem to be back to having a massive list in the spec 13:56:20 ok, so we continue shrik the list 13:56:37 4 mins left for us 13:56:39 well, we need it to match what we have indexed 13:56:44 I believe thats the aim 13:56:55 that might involve adding an index or two, if we really want to keep some 13:57:13 ok 13:57:35 humm 13:58:02 Kevin_Zheng: is the currently all the proposal filter/sort includes index? 13:58:10 no 13:58:16 I am just checking, we are missing loads of them 13:58:21 https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/models.py#L196 13:58:43 note that users filter for deleted=false, hence the compound index 13:59:21 some useful fields missing indexes 13:59:30 like user_id 13:59:52 pretty common to filter by user 13:59:57 it's time to close, we should move back to #openstack-nova 14:00:07 #endmeeting