| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-01-29 | |||
| 15:52:01 | efried | really for completeness | |
| 15:52:31 | stephenfin | Okay. You're aware the warnings are only going to be in the logs though, yeah? | |
| 15:52:37 | stephenfin | So I'm not sure how much feedback you're going to be getting | |
| 15:52:53 | efried | It would be nice to find a way to send them back to the CLI, but yeah, I get it. | |
| 15:53:04 | stephenfin | compared to just trying one by one 'til you find the erroneous extra spec | |
| 15:53:06 | sean-k-mooney | well for errors we can put it in the resonce body | |
| 15:53:20 | stephenfin | sean-k-mooney: right, but not for warnings | |
| 15:53:21 | efried | I think that ^ would be nice for a future microversion, don't need to do it now. | |
| 15:53:22 | sean-k-mooney | for wraning maybe we could alter the respocne to include them too | |
| 15:53:37 | stephenfin | for those we already have a body - the updated flavor (I think) | |
| 15:53:53 | sean-k-mooney | ya it should be the full flavor object | |
| 15:54:05 | sean-k-mooney | but we could extend it from the mircoverion on | |
| 15:54:06 | stephenfin | yup, so no way to get back that warning via the API | |
| 15:54:11 | sean-k-mooney | that said its a nice to have | |
| 15:54:36 | efried | so then yeah, call it 'off' and the warnings are in the logs | |
| 15:54:43 | sean-k-mooney | sure there is we just change the api respoce to also have validtion warning but lets leave that out of scope for now | |
| 15:54:53 | stephenfin | efried: ack | |
| 15:55:22 | efried | (note that we technically wouldn't need 'strict', since that's the default, but again for completeness it would be nice to support it) | |
| 15:55:32 | stephenfin | agreed | |
| 15:55:39 | sean-k-mooney | yep | |
| 15:56:05 | sean-k-mooney | i like the implcit state to be setable explictly even if its the default if you say nothing | |
| 15:56:13 | sean-k-mooney | it makes testing nicer imo | |
| 15:56:33 | sean-k-mooney | and it make documting the behavior simpler since you have a name for each mode | |
| 15:56:55 | efried | could call 'permissive' 'values-only' or something, since 'permissive' isn't particularly descriptive | |
| 15:57:01 | efried | bikeshed away. | |
| 15:57:58 | sean-k-mooney | ya maybe again i just suggeste permissive since selinux in permissve mode doese policy validateion and log any violation but allow whatever the process was doing to continue | |
| 15:58:23 | sean-k-mooney | it seamed like a good analog but not many peopel might make that connection | |
| 15:59:20 | efried | 'ignore-unknown-keys' | |
| 15:59:45 | efried | 'ignore-(well-okay-log-a-warning)-unknown-keys' | |
| 16:00:02 | dansmith | so, | |
| 16:00:10 | dansmith | I had to deal with something and lost track here | |
| 16:00:51 | dansmith | one reason to not allow the client to choose more than strict or not-strict is that if you have permission to create flavors within your tenant, and you can set validation=warn (in the logs), then you can spam the server-side logs with warnings | |
| 16:00:59 | dansmith | which would be considered somewhat of a DoS | |
| 16:02:39 | stephenfin | we're adding a policy for this though, right? | |
| 16:03:10 | dansmith | a policy for what? each of the values of the validation=(on|off|warn) ? | |
| 16:03:38 | stephenfin | <dansmith> stephenfin: but I think it's legit to have a validation=no per-request, but that can return 403 either because of policy or config if it's documented when you add it, no? so later we could turn that off or default it off | |
| 16:03:44 | stephenfin | what did you mean by that? ^ | |
| 16:04:08 | stephenfin | I assumed you meant for the ability to set '?validation=<anything>' | |
| 16:04:49 | dansmith | stephenfin: I meant state that validation=no might be disabled (i.e. always required) in the docs, so that we can allow that to be turned off in config or policy in the future if the op wants to require all flavors be validated | |
| 16:05:05 | dansmith | I'm just arguing against having a client-controlled way to emit WARNING messages in the server-side logs | |
| 16:05:44 | sean-k-mooney | or i guess restircted to a group e.g. admins although it is an admin only api already by default | |
| 16:06:20 | stephenfin | I think WARNING might be a bit much, tbh. INFO seems appropriate if the user has explicitly requested no/limited validation | |
| 16:06:24 | sean-k-mooney | the policy.json seam like a better approch if we were to make it configurable | |
| 16:06:39 | stephenfin | If it was INFO, we'd be okay, right? | |
| 16:07:06 | stephenfin | since no operator will get paged for INFO logs | |
| 16:07:10 | sean-k-mooney | i think dansmith was concerned about ddos risks | |
| 16:07:13 | dansmith | I dunno, I just don't like it in general | |
| 16:07:14 | dansmith | right | |
| 16:07:18 | sean-k-mooney | infor does not really help with hat | |
| 16:07:21 | sean-k-mooney | *that | |
| 16:07:21 | dansmith | but it's not a sticking point | |
| 16:07:25 | dansmith | sean-k-mooney: correct | |
| 16:07:43 | stephenfin | they could already ddos things but just repeatedly hitting the API with working extra specs | |
| 16:07:50 | stephenfin | we log each request iirc | |
| 16:07:56 | stephenfin | *by just | |
| 16:08:05 | stephenfin | that's what API rate limiting is for | |
| 16:08:27 | sean-k-mooney | well i suspect the error could be long if you add a bunch of invalide extra specs so its a larger risk | |
| 16:08:42 | sean-k-mooney | but again its an admin only api by default | |
| 16:08:46 | dansmith | and logging something that is a warning as info doesn't make it info | |
| 16:08:58 | sean-k-mooney | so the request will get rejected by a normal user well before it hit your code | |
| 16:09:00 | dansmith | but anyway, like I say, not a sticking point | |
| 16:09:37 | stephenfin | Is it a warning though? Per above, info does seem appropriate if the user has explicitly requested no/limited validation | |
| 16:09:58 | stephenfin | It's a warning if they didn't, which they'll see in their HTTP 4xx response | |
| 16:10:07 | dansmith | seems like a warning to me :) | |
| 16:10:08 | dansmith | anyway | |
| 16:10:16 | dansmith | we needn't argue about it | |
| 16:10:27 | dansmith | I will just toldjaso if you get CVE paperwork over it :) | |
| 16:10:29 | stephenfin | fine by me :) | |
| 16:16:01 | bauzas | mmm, what the heck is this ? | |
| 16:16:04 | bauzas | stestr: error: unrecognized arguments: --test-path=./nova/tests/functional run test_nova_manage | |
| 16:16:09 | bauzas | holy shit | |
| 16:16:53 | bauzas | I probably need to upgrade stestr by hand | |
| 16:16:59 | bauzas | but I did recreated my venc | |
| 16:17:02 | bauzas | venv* | |
| 16:17:09 | stephenfin | tox -e function --recreate ? | |
| 16:17:17 | stephenfin | *al | |
| 16:18:32 | stephenfin | unless you're using stestr without tox, which is odd | |
| 16:18:57 | stephenfin | seeing as you can do stuff like 'tox -e functional -- -n path_to_functional_test.py::TestClass.test_method' | |
| 16:22:34 | bauzas | stephenfin: that's what I did | |
| 16:22:51 | bauzas | and I ended up with stestr==2.6.0 | |
| 16:23:24 | bauzas | I always asked to run a subset of tests by doing tox -efunc <my_regex> | |
| 16:23:27 | bauzas | and it worked | |
| 16:24:57 | stephenfin | bauzas: That's what I have too. Working fine here | |
| 16:25:17 | bauzas | checking with stestr==2.5.0 | |
| 16:25:38 | bauzas | functional runtests: commands[0] | stestr --test-path=./nova/tests/functional run test_nova_manage | |
| 16:25:42 | bauzas | mmmmm | |
| 16:26:21 | stephenfin | bauzas: '$ tox -e functional -- test_nova_manage' wfm | |
| 16:27:05 | bauzas | I never had to use the positional markers, but whatever, gonna try | |
| 16:27:41 | bauzas | crazy, still same issue | |
| 16:28:07 | stephenfin | want to dump the complete output to paste.o.o | |
| 16:28:19 | stephenfin | it's something simple, I'd suspect | |
| 16:30:06 | bauzas | yeah me too, but can't see the problem | |
| 16:30:22 | bauzas | it's just stestr which doesn't sound to accept --test-path | |
| 16:30:24 | bauzas | http://paste.openstack.org/show/788935/ | |
| 16:31:11 | bauzas | actually, nope | |
| 16:33:59 | bauzas | well, I'm puzzled | |
| 16:34:37 | stephenfin | ah, wait, have you added additional tests for nova-manage? | |
| 16:35:13 | stephenfin | There's a bug with oslo.config whereby the CLI parser is global'ish | |
| 16:35:49 | stephenfin | Yeah, '--remote_debug-port' is a nova-manage option. You're not mocking stuff properly | |
| 16:38:06 | stephenfin | bauzas: mriedem saw the issue pop up in a unit test at https://review.opendev.org/#/c/694806/2/ and it's currently causing an issue with glance and the latest version of cliff | |