Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a 404 instead of a 500 http code when dnslink not found for domain #6746

Closed
olizilla opened this issue Nov 4, 2019 · 9 comments
Closed
Labels
kind/enhancement A net-new feature or improvement to an existing feature topic/gateway Topic gateway topic/rpc-api Issues related to Kubo RPC API at /api/v0

Comments

@olizilla
Copy link
Member

olizilla commented Nov 4, 2019

The gateways get a lot of dns api http requests for things like

https://ipfs.io/api/v0/dns/www.google.com?r=true

The api returns valid json and a 500 http code. It makes it hard to spot actual errors or other 500 repsonses from go-ipfs in the logs. A 404 would more closely match the situation that you asked for a dnslink resolution for a domain that does not have one.

@olizilla olizilla added the kind/enhancement A net-new feature or improvement to an existing feature label Nov 4, 2019
@olizilla
Copy link
Member Author

olizilla commented Nov 4, 2019

@lidel what say you?

@lidel
Copy link
Member

lidel commented Nov 4, 2019

This is part of a bigger problem: 500 is returned instead of 4xx all over the API and Gateway.

In this specific case it should be fairly safe to change response code: dnslink lookup result status code is usually compared against 200, so it does not matter if negative result is 500, 300 or even >200

Personal nit: I don't like 404 Not Found because it is vague: could mean "no dnslink" OR "someone misconfigured nginx and API endpoint does not exist". 204 No Content would be a better response here imo 👌

@lanzafame
Copy link
Contributor

Agree with @lidel regarding the 204 status code.

@olizilla olizilla added topic/gateway Topic gateway topic/rpc-api Issues related to Kubo RPC API at /api/v0 labels Nov 5, 2019
@Stebalien
Copy link
Member

The HTTP API is an RPC API over HTTP, not a REST API. Trying to make it conform to REST semantics was much more trouble than it was worth.

HTTP Codes are used at the RPC layer:

  • 500 - RPC endpoint returned an error.
  • 404 - RPC endpoint doesn't exist.
  • 400 - Malformed RPC, argument type error, etc.
  • 403 - RPC call forbidden.

In this case, "404" would mean that the /api/v0/dns RPC endpoint doesn't exist. 500 means that the function does exist, it just failed internally for some reason. To know that reason, you have to look at the "application layer" error (commands lib error).

@Stebalien
Copy link
Member

@lidel this really should be documented somewhere. Could you try to find a place to put it?

@olizilla
Copy link
Member Author

olizilla commented Nov 5, 2019

Can we discuss improving this?

@Stebalien
Copy link
Member

If someone implements a fix that actually works reliably, I'd be happy to accept it. However, I'm pretty sure there is no nice solution.

IIRC, there were two key issues:

  1. We'd like some way to distinguish between "command doesn't exist" and "the thing you asked for doesn't exist".
  2. The API supports streaming responses and "late" errors.

For example, a user might call ipfs cat FILE_A FILE_B FILE_C. In this case, we'll fetch the first file, then the second, then the third. If FILE_B doesn't exist, we'll return:

  1. Status 200.
  2. FILE_A.
  3. A trailing header with a JSON-encoded error.

Basically, the only error conditions we can reliably report in the status code are ones related to the RPC layer (endpoint not found, request malformed, etc.).

@Stebalien
Copy link
Member

For some context, I've sunk hundreds of hours into the commands lib and don't want to spend any more time on it unless absolutely necessary. Any work we would put into the commands lib should be put into replacing it.

@lidel
Copy link
Member

lidel commented Nov 5, 2019

@olizilla I am also having problems due to 500 (ipfs/ipfs-companion#755) but unless you are planning to change status code at nginx level, I don't think we can ship a fix for /api/v0. Like Steven said, better to invest time in ipfs/specs#224 and ipfs/specs#225 and solve this is /api/v1.

@lidel this really should be documented somewhere. Could you try to find a place to put it?

Added to https://docs.ipfs.io/reference/api/http/ in ipfs-inactive/http-api-docs#23 / ipfs-inactive/docs#368

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature topic/gateway Topic gateway topic/rpc-api Issues related to Kubo RPC API at /api/v0
Projects
None yet
Development

No branches or pull requests

4 participants