Bug 1467912 - [RFE] improve consistency in API faults
Summary: [RFE] improve consistency in API faults
Keywords:
Status: CLOSED DEFERRED
Alias: None
Product: ovirt-engine
Classification: oVirt
Component: RestAPI
Version: 4.1.3.5
Hardware: Unspecified
OS: Unspecified
unspecified
low
Target Milestone: ---
: ---
Assignee: Juan Hernández
QA Contact: Lukas Svaty
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-07-05 13:38 UTC by Fabrice Bacchella
Modified: 2020-04-01 14:49 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-04-01 14:44:29 UTC
oVirt Team: Infra
Embargoed:
oourfali: ovirt-future?
rule-engine: planning_ack?
rule-engine: devel_ack?
rule-engine: testing_ack?


Attachments (Terms of Use)

Description Fabrice Bacchella 2017-07-05 13:38:28 UTC
If I run a action with insufficient privileges, I get this in fault:
{'_detail': 'query execution failed due to insufficient permissions.', '_reason': 'Operation Failed', '_href': None}

If I try to start a running VM, I get this:
{'_href': None, '_reason': 'Operation Failed', '_detail': '[Cannot run VM because the VM is in Up status.]'}

If I try to operate with wrong credentials, I get this exception:
{'code': None, 'fault': None}

Why the [] around the message for a failed operation and not for insufficient permissions ? Why fault is None if credentials are missing ?

Comment 1 Juan Hernández 2017-07-05 14:34:59 UTC
There is no fault when credentials are missing because the server doesn't send a request body that the SDK can handle, see bug 1445681.

The use of [] are added around messages in situations where there may be multiple error messages. This is an implementation detail, and not consistent, should be fixed.

However, I suggest you avoid using the the detail messages except for debugging and logging. See bug 1435704 for a proposal to enhance how we report error messages.

As there is nothing to change in the SDK I am moving the bug to the RestAPI component. We will fix it making the use of [] more consistent. That may be using them where we don't currently, or not using them at all. I vote for not using them at all.

Comment 2 Juan Hernández 2017-07-05 14:41:17 UTC
The [] are added here:

  https://github.com/oVirt/ovirt-engine/blob/ovirt-engine-4.1.4/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BaseBackendResource.java#L320-L326

  protected String localize(List<String> errors) {
      BackendLocal backend = getBackend();
      Locale locale = getEffectiveLocale();
      return locale != null
             ? backend.getErrorsTranslator().translateErrorText(errors, locale).toString()
             : backend.getErrorsTranslator().translateErrorText(errors).toString();
  }

The calls to toString() convert a list of one element to a string with the surrounding [].

Comment 3 Fabrice Bacchella 2017-07-05 14:54:34 UTC
You mean that ?

  <fault id="E1034234" href="/ovirt-engine/api/errorcodes/E1034234">
    <reason>Whatever</reason>
    <detail>....</detail>
  </fault>

I hope then we will get an id for authentication error, or missing object, that just return a 404 for now.

Comment 4 Juan Hernández 2017-07-05 15:07:11 UTC
For authentication errors we return 401 already. What we are missing is a <fault ...> in the response body with the details. That is what bug 1445681 is about. When that is fixed the SDK will handle the response body and provide the details wrapped in a ovirtsdk4.types.Fault object.

Usually when an object is missing you already know what is the id if the missing object, because you requested it. Are you suggesting that the id of that missing object should also be included in the details of the message?

Comment 5 Fabrice Bacchella 2017-07-05 15:24:45 UTC
(In reply to Juan Hernández from comment #4)
> For authentication errors we return 401 already.

Not always:
< POST /ovirt-engine/sso/oauth/token HTTP/1.1
< password=&scope=ovirt-app-api&grant_type=password&username=rexecutor%40internal
...
> HTTP/1.1 200 OK
> {"error_code":"access_denied","error":"Cannot authenticate user 'rexecutor@internal': The user name or password is incorrect.."}

Look at the empty password.

> Are you suggesting that the id of that missing object should also be included in the details of the message?

Just that once you start to take the fault id path, one should always be returned but the SDK. Even if it's a connection reset from the ovirt engine.

Comment 6 Juan Hernández 2017-07-05 15:49:47 UTC
(In reply to Fabrice Bacchella from comment #5)
> (In reply to Juan Hernández from comment #4)
> > For authentication errors we return 401 already.
> 
> Not always:
> < POST /ovirt-engine/sso/oauth/token HTTP/1.1
> <
> password=&scope=ovirt-app-
> api&grant_type=password&username=rexecutor%40internal
> ...
> > HTTP/1.1 200 OK
> > {"error_code":"access_denied","error":"Cannot authenticate user 'rexecutor@internal': The user name or password is incorrect.."}
> 
> Look at the empty password.
>

That is not the API, but the SSO service. The SSO service doesn't return "faults", so the SDK won't return faults in this case either. We should copy the error message provided by the SSO service to the raised exception. Please check that and open another bug if we don't.

> > Are you suggesting that the id of that missing object should also be included in the details of the message?
> 
> Just that once you start to take the fault id path, one should always be
> returned but the SDK. Even if it's a connection reset from the ovirt engine.

There are situations where the server can't return an HTTP response body, like when you are actually talking to the SSO service, or when the server can't be reached. In those cases the SDK will raise an exception with an informative message. In all other cases, and assuming that the proposal described in e bug 1435704 is accepted, then we will always return an error code, which will be handled by the application.

Comment 7 Fabrice Bacchella 2017-07-05 16:07:15 UTC
The SSO errors are totally indistinguishable from other exceptions:
class is ovirtsdk4.Error, but fault and code are both None. So to detect it's that kind of error, one must check inside args[0] for the string 'Error during SSO authentication access_denied'.
Perhaps a dedicated exception then could be helpful. How can I extract error_code and error in this way:

            raise Error(
                'Error during SSO revoke %s : %s' % (
                    sso_response['error_code'],
                    sso_response['error']
                )

Comment 8 Juan Hernández 2017-07-05 17:01:53 UTC
Addint a dedicated exception makes sense. I just opened bug 1467969 to request that.

Comment 9 Fabrice Bacchella 2017-07-06 08:50:52 UTC
Another error case management that could be improved:

I'm doing a wrong query (because of a bug in my code):
< GET /ovirt-engine/api/users/<built-in function id> HTTP/1.1

And getting of course an error:
> HTTP/1.1 400 Bad Request
> Content-Type: text/html; charset=iso-8859-1

But because of the wrong content type, the real cause (400 Bad Request) is eaten and not displayed, I'm getting:
The action "user export" failed with: The response content type 'text/html; charset=iso-8859-1' isn't the expected XML

So either the check for status code (400) should be done before the check for Content-Type or a custom handler for error should be added in jboss settings.

Comment 10 Michal Skrivanek 2020-03-19 15:41:06 UTC
We didn't get to this bug for more than 2 years, and it's not being considered for the upcoming 4.4. It's unlikely that it will ever be addressed so I'm suggesting to close it.
If you feel this needs to be addressed and want to work on it please remove cond nack and target accordingly.

Comment 11 Michal Skrivanek 2020-04-01 14:44:29 UTC
ok, closing. Please reopen if still relevant/you want to work on it.

Comment 12 Michal Skrivanek 2020-04-01 14:49:28 UTC
ok, closing. Please reopen if still relevant/you want to work on it.


Note You need to log in before you can comment on or make changes to this bug.