Should translate Unauthorized NeutronClientException in allocate_for_instance

[Matt Riedemann]

If your auth token expires while allocating the network for an instance with the neutronv2 API in nova, you'll get a 500 from the server because the Unauthorized neutron client exception isn't handled and translated to a more specific NovaException (expecting a 401 in this case). Paste here:

http://paste.openstack.org/show/74400/

We could decorate allocate_for_instance to catch Unauthorized and translate it to a more specific NovaException so the nova client gets a 401 instead of a 500.

[Matt Riedemann (mriedem)]

Discussed a bit with Aaron Rosen:

(4:29:00 PM) mriedem: arosen: quick question since i have to leave quick, but thinking about decorating nova.network.neturonv2.api.allocate_for_instance with something that would translate an Unauthorized(NeutronClientException) to something more specific as a NovaException, rather than getting a 500,
(4:29:04 PM) mriedem: do you see any issues with that?
(4:29:29 PM) mriedem: arosen: in case the token passed to create the neutron client expires while doing the network allocation
(4:32:04 PM) arosen: mriedem: hrm to me it seems like if that happens the client should be able to handle that and get a new one ? no?
(4:32:26 PM) mriedem: arosen: we don't pass the password to the client to get a new token
(4:32:47 PM) arosen: just the existing token if we have one?
(4:32:51 PM) mriedem: yup
(4:33:01 PM) mriedem: arosen: goes back to this: https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:stable/havana+topic:backport-neutron-auth-fixes,n,z
(4:33:11 PM) arosen: yup i remember
(4:33:40 PM) arosen: mriedem: hrm so one thing i'm thinking in my neutron-nova-event patch i made neutron use the service_tenant_id instead of username/password
(4:33:49 PM) arosen: that way i don't have to deal with tokens and keystone at all
(4:34:01 PM) arosen: I wonder if we should be doing that here as well?
(4:34:33 PM) arosen: or perhaps i shouldn't be doing that in neutron but it gets the token stuff out of the picture i believe
(4:34:53 PM) mriedem: arosen: but the neutron/nova event stuff still has config options for nova user/password right?
(4:35:26 PM) mriedem: arosen: but it'd be nice to get tokens out of the picture here
(4:35:31 PM) arosen: mriedem: right it does. Perhaps it does get tokens as well. :/
(4:35:33 PM) arosen: https://github.com/openstack/neutron/blob/master/neutron/notifiers/nova.py#L46
(4:35:34 PM) mriedem: since there is always going to be a window of failure
(4:35:49 PM) arosen: i agree.
(4:35:59 PM) arosen: and having to deal with that failure out of the client kind of sucks :/
(4:36:12 PM) ***arosen or makes it harder
(4:37:59 PM) arosen: mriedem: I think another option might be to pass the user/pass and the token to the client. Then, the client should use the token and if it gets a expired error then it can internally get a new token
(4:38:21 PM) arosen: though i'm not sure that's the right way because in that case we'd need to pass the token back to nova again so we don't keep getting a new one each time.
(4:38:29 PM) mriedem: arosen: so i tried that back in grizzly and it was rejected as being too brute force
(4:38:34 PM) mriedem: right
(4:38:46 PM) arosen: could be passed by via the param dict passed in though :)
(4:38:53 PM) arosen: but yea that's kinda messy i agree.

[Matt Riedemann (mriedem)]

(4:40:22 PM) arosen: mriedem: does the neutronclient actually raise something like Unauthorized when this happens?
(4:40:42 PM) mriedem: arosen: yeah http://paste.openstack.org/show/74400/
(4:41:16 PM) mriedem: arosen: yeah because of https://github.com/openstack/nova/blob/master/nova/network/neutronv2/__init__.py#L37
(4:41:36 PM) arosen: ah
(4:41:39 PM) mriedem: if the token goes bad, neutron client doesn't have any recourse
(4:42:32 PM) openstackgerrit: Robert Kukura proposed a change to openstack/nova: Use binding:vif_details to control firewall https://review.openstack.org/83190
(4:42:49 PM) arosen: mriedem: i wonder if we should have a wrapper that wraps all the neutron calls in try: neutron_call(): except exceptions.Unathorized: (get new token/recall the wrapper)
(4:42:59 PM) mriedem: arosen: yeah....
(4:43:02 PM) mriedem: was thinking retry
(4:43:04 PM) arosen: s/wrapper some method that does all this for us
(4:43:08 PM) mriedem: arosen: or could we use keystone v3 trusts?
(4:44:09 PM) arosen: It seems like going this route would help improve our error handing right now because we still have stuff like this: https://github.com/openstack/nova/blob/master/nova/network/neutronv2/api.py#L440
(4:44:40 PM) arosen: mriedem: unfortunately, I don't know much about the keystone v3 stuff (or anything at all to be honest)
(4:44:55 PM) mriedem: arosen: neither do i, but bknudson does and he sits 2 doors down :)

[Brant Knudson (blk-u)]

A 401 error indicates that the client didn't send an auth header, so I don't think that's an appropriate return code. See http://tools.ietf.org/html/rfc2616#section-10.4.2 . A 500 error seems appropriate here. The server encountered an unexpected error and wasn't able to complete the request.

Note that a user's token wind up becoming invalid for any number of reasons, not just because it expires. For example, if the administrator disables the user. So token expiration is only one way that this could happen.

From what I know of trust tokens, I think that getting a trust token from the user's token or using a nova service token would be the right way for nova to communicate with neutron.

[Matt Riedemann (mriedem)]

Brant, 401 also says this:

"If the request already included Authorization credentials, then the 401 response indicates that authorization has been refused for those credentials." The request had a token which was expired and failed, so the user could retry since a new request will create a new token.

But the points about other reasons that the token could become invalid make sense.

I think using a nova service token makes sense, but I don't know how to do that. Will have to investigate it.

[Matt Riedemann (mriedem)]

For what it's worth, I still think it's better to translate the Unauthorized exception from the neutron client in nova so the nova response is a 401 rather than 500, because we know it's a bad token, regardless of if the token expired or the user was disabled.

Handling this better with trusts or service tokens is a separate issue so we don't get the 401 in the first place.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/84035

[Matt Riedemann (mriedem)]

Bug 1297309 is related to this.

[Matt Riedemann (mriedem)]

http://logstash.openstack.org/#eyJzZWFyY2giOiJtZXNzYWdlOlwiSW52YWxpZFVzZXJUb2tlbjogVG9rZW4gYXV0aG9yaXphdGlvbiBmYWlsZWRcIiBBTkQgdGFnczpzY3JlZW4tcS1zdmMudHh0IiwiZmllbGRzIjpbXSwib2Zmc2V0IjowLCJ0aW1lZnJhbWUiOiI2MDQ4MDAiLCJncmFwaG1vZGUiOiJjb3VudCIsInRpbWUiOnsidXNlcl9pbnRlcnZhbCI6MH0sInN0YW1wIjoxMzk4NzAxNjI4NjI2fQ==

[Openstack Gerrit (openstack-gerrit)]

Reviewed: https://review.openstack.org/84035
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=c75a15a48981628e77d4178476c121693a656814
Submitter: Jenkins
Branch: master

commit c75a15a48981628e77d4178476c121693a656814
Author: Matt Riedemann <email address hidden>
Date: Sun Mar 30 14:40:38 2014 -0700

    Rename NotAuthorized exception to Forbidden

    The NotAuthorized NovaException has an internal code of 403 which is
    actually Forbidden, so rename it appropriately.

    This patch doesn't change the external behavior, the status code in
    responses will still be 403 but the exception is just named properly.

    This is also necessary to create an actual Unauthorized NovaException
    with code 401 for use in some Neutron API bug fixes for more granular
    error handling from python-neutronclient.

    Related-Bug: #1298075

    Change-Id: I691fac2e2c797f47c04da7965d7b1c8685c74edb

[haruka tanizawa (h-tanizawa)]

Already Fix Committed ?

[Matt Riedemann (mriedem)]

This is not really fixed, I've had an old WIP for awhile now that I haven't had time to get back to:

https://review.openstack.org/#/c/83282/

Basically I'd like to add some translation of neutronclient exceptions to nova exceptions in the nova/network/neutronv2 API code, part of the problem is that not all neutronclient exceptions map well to nova exceptions, so we might need some new nova exceptions for more neutron-specific errors. Another problem with my approach was the decorator is nice but it might not have the necessary args for a useful nova exception error message. That might need to be handled on a case-by-case basis.

This is probably something that would fit better into a neutronv2 API cleanup blueprint, which I think Brent Eagles was going to work on for Kilo.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Matt Riedemann (<email address hidden>) on branch: master
Review: https://review.openstack.org/83282
Reason: I think this can be done as some neutronv2 API cleanup blueprint work in early Kilo, so will revisit then.

[Shoham Peller (shoham-peller)]

Just saw this bug in Liberty.
This report hasn't been active for more than a year, are we planning to fix this?

Traceback here:
http://paste.openstack.org/show/506344/

[Shoham Peller (shoham-peller)]

1. I've checked how hard it is to apply the patch to current nova code.
   I can patch the neutron driver to translate the 401 messages, like in @mriedem patch, but problem is in nova today each api method tells which http response codes it may return. We need either to add 401 to all api methods' decorators, or to change the decorator logic to always allow 401 from any API call.

2. BTW, the problem gets more complex when nova-api sends the token to nova-compute, which uses it with neutron api. There's 2 problems with that:
   a. There's no way to return 401 to the user, which already got 202.
   b. nova-compute might put the server in "ERROR" status.
This actually happened to me in some case, but I'm not sure what nova-compute did with the VM - http://paste.openstack.org/show/506350/

[Matt Riedemann (mriedem)]

This is now a duplicate and has a fix:

https://review.openstack.org/#/c/312014/