Bug 1467912 - [RFE] improve consistency in API faults
[RFE] improve consistency in API faults
Status: NEW
Product: ovirt-engine
Classification: oVirt
Component: RestAPI (Show other bugs)
4.1.3.5
Unspecified Unspecified
unspecified Severity low (vote)
: ---
: ---
Assigned To: Juan Hernández
Pavel Stehlik
: FutureFeature
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-05 09:38 EDT by Fabrice Bacchella
Modified: 2017-07-06 06:38 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: Infra
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oourfali: ovirt‑future?
rule-engine: planning_ack?
rule-engine: devel_ack?
rule-engine: testing_ack?


Attachments (Terms of Use)

  None (edit)
Description Fabrice Bacchella 2017-07-05 09:38:28 EDT
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 10:34:59 EDT
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 10:41:17 EDT
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 10:54:34 EDT
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 11:07:11 EDT
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 11:24:45 EDT
(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 11:49:47 EDT
(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 12:07:15 EDT
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 13:01:53 EDT
Addint a dedicated exception makes sense. I just opened bug 1467969 to request that.
Comment 9 Fabrice Bacchella 2017-07-06 04:50:52 EDT
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.

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