| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-01-29 | |||
| 15:46:45 | stephenfin | better. | |
| 15:47:11 | sean-k-mooney | in my head 'stirct' = know keys only, 'permissive' mean allow unkonw keys but warn, off mean dont do validation at all | |
| 15:47:47 | stephenfin | would you ever want to turn it off for *everything*? | |
| 15:47:58 | efried | I'm not married to the names, but I thought three modes: | |
| 15:47:58 | efried | - off: we don't 4xx for anything. We can still run the validators, but just warn if we find something awry. | |
| 15:47:58 | efried | - permissive: only validate values, only for keys we recognize. Ignore unrecognized keys. | |
| 15:47:58 | efried | - strict: above, plus fail on unrecognized keys. | |
| 15:48:01 | sean-k-mooney | its what you will get for the old microversion | |
| 15:48:13 | sean-k-mooney | so i thnk it makes sense to represent that code path as an option | |
| 15:48:36 | stephenfin | efried: That mostly makes sense but I'm wondering what off is useful for | |
| 15:49:07 | sean-k-mooney | stephenfin: off is basicaly whatever behavior you get with the old microverison | |
| 15:49:36 | stephenfin | right, but is that behaviour ever desirable though? | |
| 15:49:38 | sean-k-mooney | so if im using a newer micoverion for the flavor endpoint that add something not in the old one i can still turn off the vlidation | |
| 15:49:42 | efried | stephenfin: We forgot or messed up an in-tree one, and don't want to wait for the fix. Could flip to the prior microversion, but the warnings would give me a way to sort of manually make sure everything else is kosher. | |
| 15:49:46 | sean-k-mooney | stephenfin: yes | |
| 15:50:13 | sean-k-mooney | stephenfin: there was a spec for composable flavor recently | |
| 15:50:16 | stephenfin | efried: wdym by "everything else" | |
| 15:50:28 | efried | the remainder of the extra specs in my request. | |
| 15:50:34 | sean-k-mooney | if that was approved and landed after this feature we might want to use that without validation | |
| 15:50:57 | stephenfin | oh, yes, you can set multiple extra specs at once. duh | |
| 15:50:57 | efried | I try with strict/permissive, I get a bounce on extra spec X, I redrive with 'off' and make sure the only warning I get is on X. | |
| 15:51:04 | sean-k-mooney | efried: im more or less ok with your deffintions by the way your off just does a littel more then i expected | |
| 15:51:13 | efried | we can call it 'warn' | |
| 15:51:22 | stephenfin | efried: vs. just retrying one by one? | |
| 15:51:26 | efried | yes | |
| 15:51:44 | efried | or "I want to test drive this feature" | |
| 15:51:45 | sean-k-mooney | i suggested permissive to mirror selinux but warn would be fine | |
| 15:51:54 | efried | "but still create my flavor" | |
| 15:51:56 | sean-k-mooney | although that implice that strict should be error | |
| 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 | |