| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-05-05 | |||
| 13:55:01 | rpodolyaka | dansmith: I haven't seen the service user spec :( this seems to be a simple fix - we use it in downstream mitaka | |
| 13:55:12 | rpodolyaka | *easy to backport | |
| 13:55:26 | dansmith | rpodolyaka: yeah, this seems like just a bad assumption made in the when-to-raise code | |
| 13:56:49 | rpodolyaka | yeah, there was two different conditions in two different places | |
| 13:57:00 | rpodolyaka | hopefully `use_service_credentials` now covers it properly in https://review.openstack.org/#/c/422696/3/nova/network/neutronv2/api.py | |
| 13:58:55 | dansmith | the implicit "admin if admin context but no token" thing kinda scares me | |
| 13:59:01 | rpodolyaka | ++ | |
| 13:59:26 | dansmith | actually, I guess that already happens in the code | |
| 13:59:51 | rpodolyaka | in the long term we should probably make sure we always pass admin=True to get_client() explicitly | |
| 13:59:53 | dansmith | I guess the second hunk there is just trying to clean up the logic? | |
| 13:59:58 | dansmith | nothing is actually changed I think | |
| 14:00:03 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Check if domain is persistent before detaching volume https://review.openstack.org/441204 | |
| 14:02:23 | johnthetubaguy | dansmith: thats refresh network_info I think, admin but no user context | |
| 14:02:38 | johnthetubaguy | (and a few other periodic tasks) | |
| 14:02:41 | dansmith | johnthetubaguy: metadata fetch is the example he gave | |
| 14:03:25 | mriedem | ok i was hazy on that, | |
| 14:03:31 | dansmith | johnthetubaguy: so does that patch seem reasonable? | |
| 14:03:33 | johnthetubaguy | dansmith: true, although I assumed we just used the version cached in the instance for that | |
| 14:03:36 | mriedem | so basically context.get_admin_context() and pass that down | |
| 14:03:41 | mriedem | so it doesn't have a token, | |
| 14:03:42 | dansmith | mriedem: right | |
| 14:03:45 | mriedem | but is_admin=True | |
| 14:03:46 | mriedem | ok | |
| 14:04:54 | johnthetubaguy | I would tempted to have a re-raise_401=True? | |
| 14:04:55 | mriedem | so in the commit message where it says we get the 401 b/c the token is expired, | |
| 14:05:02 | mriedem | if we didn't have a token to begin with, | |
| 14:05:08 | mriedem | is that referring to the token that neutronclient creates? | |
| 14:05:29 | johnthetubaguy | I think its the user token passed into the API, that happens to be admin, but expires by the time we talk to neutron | |
| 14:05:58 | johnthetubaguy | ... now if you enable service token auth (the second token) that whole crazy goes away | |
| 14:06:21 | dansmith | johnthetubaguy: so that callers of this have to pass it to get proper behavior you mean? | |
| 14:06:44 | johnthetubaguy | so, its a configuration option right now, to enable the service token to get sent with any user token | |
| 14:07:12 | johnthetubaguy | (line 159 I guess) | |
| 14:07:39 | mriedem | yeah so no one is using that yet, | |
| 14:07:41 | mriedem | so we can ignore that part, | |
| 14:07:54 | mriedem | i'm also a bit lost where you said "I think its the user token passed into the API, that happens to be admin, but expires by the time we talk to neutron" | |
| 14:08:06 | mriedem | i thought if you did context.get_admin_context() there is no token | |
| 14:08:12 | dansmith | mriedem: correct | |
| 14:08:16 | mriedem | hence the "and not context.auth_token" check | |
| 14:08:24 | johnthetubaguy | oh wait, I kinda hate what this is doing | |
| 14:09:11 | johnthetubaguy | I don't like converting what the user passed in, into our own service token | |
| 14:09:14 | johnthetubaguy | I missed that bit | |
| 14:09:22 | johnthetubaguy | well, miss-read it | |
| 14:09:43 | dansmith | converting what? | |
| 14:09:45 | dansmith | the admin flag? | |
| 14:10:52 | johnthetubaguy | so if the user passes in a token, I think we should pass that through to the next service | |
| 14:11:12 | johnthetubaguy | what I think is happening here is we ignore the user token, and just use a service token instead, which sounds bad | |
| 14:11:32 | dansmith | no, I don't think so | |
| 14:11:52 | mriedem | if the user passes in a token, the "and not context.auth_token" is False | |
| 14:11:54 | johnthetubaguy | actually, my brain is fried, its not doing that, got my admin's mixed up here | |
| 14:12:35 | johnthetubaguy | admin=True means use service admin token, rather than context.auth_token, which we do anyways (for port binding reasons) | |
| 14:12:50 | dansmith | this unifies the logic for determining if we use admin_auth with the logic for the admin= flag on ClientWrapper.. so they match | |
| 14:14:30 | dansmith | it doesn't do anything other than make sure the ClientWrapper gets the right admin flag so that it does the right raise behavior I think | |
| 14:14:44 | dansmith | L108 only I think | |
| 14:15:03 | johnthetubaguy | yeah, sorry, I keep going around in silly loops | |
| 14:15:22 | johnthetubaguy | I wonder is that admin flag should be supress_401_error=True? | |
| 14:15:43 | dansmith | oh is that what you meant? | |
| 14:16:09 | dansmith | if ClientWrapper isn't used anywhere else, then that's probably an okay change | |
| 14:16:19 | mriedem | that's pretty leaky tight coupling | |
| 14:16:51 | mriedem | i'd rather change the admin kwarg in ClientWrapper to "use_service_credentials" or "using_service_credentials" or something | |
| 14:16:54 | dansmith | mriedem: but ClientWrapper is really only used from right there and for this purpose | |
| 14:17:09 | dansmith | mriedem: but that isn't right | |
| 14:17:10 | dansmith | mriedem: it doesn't change that behavior | |
| 14:19:10 | johnthetubaguy | looking at the exception name I am tempted by is_token_from_config=True/False | |
| 14:19:15 | johnthetubaguy | in the client wrapper | |
| 14:19:39 | dansmith | yeah that's more correct | |
| 14:19:41 | johnthetubaguy | is_admin=False when context.is_admin = True, just doesn't sit right | |
| 14:20:07 | dansmith | is_from_an_api_request=True makes most sense maybe | |
| 14:20:20 | johnthetubaguy | yeah, that would work | |
| 14:21:16 | johnthetubaguy | although that implies you would just check "if not context.auth_token" | |
| 14:21:16 | openstackgerrit | Daniel Pawlik proposed openstack/nova master: Added kvm_hidden feature https://review.openstack.org/459753 | |
| 14:21:25 | dansmith | or is_internal_request | |
| 14:21:30 | dansmith | or is_nova_admin | |
| 14:21:54 | mriedem | so in the long ago we used to just get the 401 from neutronclient and we didn't handle it, and we got a 500 in the compute api, | |
| 14:22:05 | mriedem | then we handled the 401 from neutron and converted that to a nova 401 to fix the 500 | |
| 14:22:28 | johnthetubaguy | then we made the config error back into a 500? | |
| 14:22:37 | dansmith | in the long ago, we had nova-network. period. | |
| 14:22:40 | mriedem | and we get the 401 now if your token expired (came in through the user context or you used nova.conf?) | |
| 14:23:01 | johnthetubaguy | so its not 401 if you got the token from nova.conf right? | |
| 14:23:11 | johnthetubaguy | should be 500 I think | |
| 14:23:21 | mriedem | NeutronAdminCredentialConfigurationInvalid turns it into a 400 | |
| 14:23:39 | johnthetubaguy | oh... | |
| 14:23:50 | mriedem | i think this came up in the original review on sahid's change, because sdague thought the 500 was correct | |
| 14:24:00 | johnthetubaguy | that feels like it should be 500 | |
| 14:24:02 | mriedem | i.e. if you don't have nova.conf [neutron] configured correctly | |
| 14:24:37 | mriedem | would have to dig back through the git history of time to find out how they changed sean's mind | |
| 14:24:42 | dansmith | so the place I'm seeing this is in instance delete, | |
| 14:24:48 | dansmith | which is failing and breaking the instance state | |
| 14:25:10 | johnthetubaguy | which instance delete path? | |
| 14:25:37 | dansmith | from compute manager while deallocating network | |
| 14:26:00 | johnthetubaguy | oh, it takes a while, so gives the token plenty time to have timed out? | |
| 14:26:05 | mriedem | johnthetubaguy: looks like this is our fault https://review.openstack.org/#/c/312014/ | |
| 14:26:30 | johnthetubaguy | mriedem: I probably should have expected that | |
| 14:27:04 | mriedem | https://review.openstack.org/#/c/312014/14/nova/network/neutronv2/api.py | |
| 14:27:41 | dansmith | johnthetubaguy: I dunno why it fails, it's from heat so heat might've gotten its token a long time ago or something | |
| 14:28:24 | johnthetubaguy | dansmith: might be the trust based token has a shorter lifetime | |
| 14:29:08 | johnthetubaguy | dansmith: the more general fix for this has merged already of course, you configure a service token to stop that expiry happening | |
| 14:29:44 | dansmith | yeah | |
| 14:30:02 | dansmith | can't backport that though | |
| 14:31:10 | johnthetubaguy | so us triggering a 401 just changes the error code from 400, rather than make the delete work? | |
| 14:31:16 | johnthetubaguy | or is this something deeper? | |