icinga2 icon indicating copy to clipboard operation
icinga2 copied to clipboard

All API endpoints should return proper HTTP status codes

Open julianbrost opened this issue 3 years ago • 1 comments

Some API endpoints already have some logic to return helpful HTTP status codes, for example /v1/actions/...: https://github.com/Icinga/icinga2/blob/9be02e3f0486bd4c3aa9ecd55dda94febf5a1f79/lib/remote/actionshandler.cpp#L102-L130

However, many others don't and always return 200 OK, like for example POST to /v1/objects/...: https://github.com/Icinga/icinga2/blob/9be02e3f0486bd4c3aa9ecd55dda94febf5a1f79/lib/remote/modifyobjecthandler.cpp#L116

This leads to responses like the following when for example trying to set a non-existing attribute on some object:

$ curl -iskSu root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/objects/hosts/master-1' -d '{"attrs":{"does_not_exist": 42}, "pretty": true}'
HTTP/1.1 200 OK
Server: Icinga/v2.13.0-448-g9be02e3f0
Content-Type: application/json
Content-Length: 228

{
    "results": [
        {
            "code": 500,
            "name": "master-1",
            "status": "Attribute 'does_not_exist' could not be set: Error: Invalid field ID.\n",
            "type": "Host"
        }
    ]
}

git grep -F 'response.result(http::status::ok);' shows many more instances of this and especially for error handling in API clients, it would be great that if there was an error, something else than 200 OK is returned.

julianbrost avatar Oct 11 '22 09:10 julianbrost

I think Icinga supports multiple object modifications in a single request. Icinga then returns HTTP 200 and a set of results, each with its own status code.

This is not unreasonable, since its not obvious how to aggregate the results into one HTTP status code.

However I suggest 2 improvements:

  1. Return HTTP error-code, if any of the object-modifications fail. In this case the result set can still be inspected, to find out what exactly caused the error.
  2. If the request does not modify multiple objects but just one, the HTTP status code should match that of the single entry in the result set.

nr954 avatar Jan 25 '24 10:01 nr954