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

gateway: unify root identifier resolution errors #185

Closed
4 tasks done
lidel opened this issue Feb 24, 2023 · 4 comments · Fixed by #187
Closed
4 tasks done

gateway: unify root identifier resolution errors #185

lidel opened this issue Feb 24, 2023 · 4 comments · Fixed by #187
Assignees
Labels
need/analysis Needs further analysis before proceeding need/triage Needs initial labeling and prioritization topic/gateway Issues related to HTTP Gateway

Comments

@lidel
Copy link
Member

lidel commented Feb 24, 2023

Extracted from ipfs/kubo#9660 (review) so we dont forget

I was thinking about this again, both from semantic perspective, but also given your feedback on how hairy this gets.
I am leaning towards returning 500s for all invalid root identifiers, just for the sake of consistency and simplicity

@hacdias would like to hear your thoughts if below makes sense (if you dont have any concerns, feel free to execute on this):

  • /ipfs/this-is-not-a-cid → we were thinking about 400 Bad Request, but it may be better/easier to return 500 Server Error, because maybe today we know this CID is not valid, but in 10 years the same software could be wrong? Maybe it is a valid CIDv8 that this old software can't recognize? 400 could be wrong, 500 will always kinda fit.
  • /ipns/this-domain-does-not-exist → 500 (we dont know if domain does not exist, or was resolution error)
  • /ipns/{cid-libp2p-key-that-cant-be-resolved} → 500 (if it is a valid key, resolution error, if not, we will fallbck to dnslink, and that also cant be proven negative)

TLDR

Return HTTP 500 Server Error for all /ip[nf]s/id failures.

TODO

@lidel lidel added need/analysis Needs further analysis before proceeding need/triage Needs initial labeling and prioritization topic/gateway Issues related to HTTP Gateway labels Feb 24, 2023
lidel added a commit to ipfs/kubo that referenced this issue Feb 24, 2023
@lidel lidel changed the title gateway: unify mutable name resolution errors gateway: unify root identifier resolution error Feb 24, 2023
@lidel lidel changed the title gateway: unify root identifier resolution error gateway: unify root identifier resolution errors Feb 25, 2023
@hacdias
Copy link
Member

hacdias commented Feb 27, 2023

@lidel I completely agree. It's very hard to detect where these errors are coming from without changing a lot of libraries and ensuring we clearly bubble up the errors. I will work on the code to make all requests return HTTP 500 here.

@iand
Copy link
Contributor

iand commented Mar 9, 2023

400 bad request is more appropriate for a CID that the server cannot parse since the server defines what request semantics it supports. The server hasn't failed and the user should not retry the request. A 500 response indicates to the user that they can retry it and expect it to work when the server has recovered.

@hacdias
Copy link
Member

hacdias commented Mar 9, 2023

As I mentioned in Slack, I think we could keep /ipns the way it is and only revert to 400 in case there is an invalid CID under the /ipfs namespace. That may require some further changes but it is likely possible. However, that's also assuming a CID today and a CID in 10 years will look the same and/or be compatible.

@lidel
Copy link
Member Author

lidel commented Mar 9, 2023

@hacdias do you mind filling an issue for this so we tackle it after #176 ? (unless you've found an easy fix – maybe detect CID parsing errors in webError?)

I hoped this will be cosmetic, but this will be coming back, just like did for the opposite "why 400 on /ipns/, my domain is ok".
(We need to resolve this in both the go-libiofs/gateway library and ipfs/specs).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/analysis Needs further analysis before proceeding need/triage Needs initial labeling and prioritization topic/gateway Issues related to HTTP Gateway
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants