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 ?
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.
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 [].
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.
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?
(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.
(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.
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'] )
Addint a dedicated exception makes sense. I just opened bug 1467969 to request that.
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.
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.
ok, closing. Please reopen if still relevant/you want to work on it.