Skip to content

fix(gateway): return 500 for all /ip[nf]s/id failures #187

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.

Sign up for GitHub

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

Merged
lidel merged 1 commit into from
Feb 28, 2023
Merged

fix(gateway): return 500 for all /ip[nf]s/id failures #187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gateway/gateway_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,9 +352,11 @@ func TestGatewayGet(t *testing.T) {
}{
{"127.0.0.1:8080", "/", http.StatusNotFound, "404 page not found\n"},
{"127.0.0.1:8080", "/" + k.Cid().String(), http.StatusNotFound, "404 page not found\n"},
{"127.0.0.1:8080", "/ipfs/this-is-not-a-cid", http.StatusInternalServerError, "failed to resolve /ipfs/this-is-not-a-cid: invalid path \"/ipfs/this-is-not-a-cid\": invalid CID: illegal base32 data at input byte 3\n"},
Copy link
Copy Markdown
Member Author

@hacdias hacdias Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like this tests where we compare the body for errors. Error information may change slightly depending on the backing implementation. I think rewriting these tests can be part of #156 or even #183.

Yes, we should compare the body for files we are expecting I think, and stuff like custom 404s, but not necessarily for errors. As long as the status is correct, the test should pass. Similarly to TestGatewayInternalServerErrorInvalidPath.

Copy link
Copy Markdown
Member

@lidel lidel Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hacdias these don't do much for UX, but also we dont expect these messages to change often.

Once set in stone, make good canaries to hint someone in transitive dependencies messed up refactor :)
I would keep this, at least until go-libipfs/gateway and rhea work is in progress – better to have more tests than less.

I remember in the past an external library (like go-cid or go-multihash/base, don't remember which one) changed something, and we detected it in Kubo, 2 layers later, because a specific error message changed to a generic one.

ps. At some point we will add more friendly HTML error responses if a request was made with Accept that includes text/html (web browser), like we did for dag-cbor.

{"127.0.0.1:8080", k.String(), http.StatusOK, "fnord"},
{"127.0.0.1:8080", "/ipns/nxdomain.example.com", http.StatusInternalServerError, "failed to resolve /ipns/nxdomain.example.com: " + namesys.ErrResolveFailed.Error() + "\n"},
{"127.0.0.1:8080", "/ipns/%0D%0A%0D%0Ahello", http.StatusInternalServerError, "failed to resolve /ipns/\\r\\n\\r\\nhello: " + namesys.ErrResolveFailed.Error() + "\n"},
{"127.0.0.1:8080", "/ipns/k51qzi5uqu5djucgtwlxrbfiyfez1nb0ct58q5s4owg6se02evza05dfgi6tw5", http.StatusInternalServerError, "failed to resolve /ipns/k51qzi5uqu5djucgtwlxrbfiyfez1nb0ct58q5s4owg6se02evza05dfgi6tw5: " + namesys.ErrResolveFailed.Error() + "\n"},
{"127.0.0.1:8080", "/ipns/example.com", http.StatusOK, "fnord"},
{"example.com", "/", http.StatusOK, "fnord"},

Expand Down
26 changes: 6 additions & 20 deletions gateway/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,9 @@ import (
cid "github.com/ipfs/go-cid"
ipld "github.com/ipfs/go-ipld-format"
logging "github.com/ipfs/go-log"
"github.com/ipfs/go-namesys"
"github.com/ipfs/go-path"
"github.com/ipfs/go-path/resolver"
coreiface "github.com/ipfs/interface-go-ipfs-core"
ipath "github.com/ipfs/interface-go-ipfs-core/path"
routing "github.com/libp2p/go-libp2p/core/routing"
mc "github.com/multiformats/go-multicodec"
prometheus "github.com/prometheus/client_golang/prometheus"
"go.opentelemetry.io/otel"
Expand Down Expand Up @@ -564,8 +561,6 @@ func webRequestError(w http.ResponseWriter, err *requestError) {

func webError(w http.ResponseWriter, err error, defaultCode int) {
switch {
case errors.Is(err, path.ErrInvalidPath{}):
webErrorWithCode(w, err, http.StatusBadRequest)
case isErrNotFound(err):
webErrorWithCode(w, err, http.StatusNotFound)
case errors.Is(err, ErrGatewayTimeout):
Expand All @@ -580,16 +575,13 @@ func webError(w http.ResponseWriter, err error, defaultCode int) {
}

func isErrNotFound(err error) bool {
return isErrNoLink(err) ||
errors.Is(err, routing.ErrNotFound) ||
err == routing.ErrNotFound ||
Comment on lines -584 to -585
Copy link
Copy Markdown
Member

@lidel lidel Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 👍 for removing these, it looked like a dead code that could cause hard-to-debug bugs in the futture.

ipld.IsNotFound(err)
}
if ipld.IsNotFound(err) {
return true
}

// isErrNoLink checks if err is a resolver.ErrNoLink. resolver.ErrNoLink
// does not implement a .Is interface and cannot be directly compared to.
// Therefore, errors.Is always returns false with it.
func isErrNoLink(err error) bool {
// Checks if err is a resolver.ErrNoLink. resolver.ErrNoLink does not implement
// the .Is interface and cannot be directly compared to. Therefore, errors.Is
// always returns false with it.
for {
_, ok := err.(resolver.ErrNoLink)
if ok {
Expand Down Expand Up @@ -772,11 +764,6 @@ func (i *handler) handlePathResolution(w http.ResponseWriter, r *http.Request, r
err = fmt.Errorf("failed to resolve %s: %w", debugStr(contentPath.String()), err)
webError(w, err, http.StatusServiceUnavailable)
return nil, nil, false
case namesys.ErrResolveFailed:
// Note: webError will replace http.StatusBadRequest with StatusNotFound or StatusRequestTimeout if necessary
err = fmt.Errorf("failed to resolve %s: %w", debugStr(contentPath.String()), err)
webError(w, err, http.StatusInternalServerError)
return nil, nil, false
default:
// The path can't be resolved.
if isUnixfsResponseFormat(responseFormat) {
Expand All @@ -801,7 +788,6 @@ func (i *handler) handlePathResolution(w http.ResponseWriter, r *http.Request, r
}
}

// Note: webError will replace http.StatusInternalServerError with StatusNotFound or StatusRequestTimeout if necessary
err = fmt.Errorf("failed to resolve %s: %w", debugStr(contentPath.String()), err)
webError(w, err, http.StatusInternalServerError)
return nil, nil, false
Expand Down
4 changes: 2 additions & 2 deletions gateway/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func (api *errorMockAPI) ResolvePath(ctx context.Context, ip ipath.Path) (ipath.
return nil, api.err
}

func TestGatewayBadRequestInvalidPath(t *testing.T) {
func TestGatewayInternalServerErrorInvalidPath(t *testing.T) {
api, _ := newMockAPI(t)
ts := newTestServer(t, api)
t.Logf("test server url: %s", ts.URL)
Expand All @@ -82,7 +82,7 @@ func TestGatewayBadRequestInvalidPath(t *testing.T) {
res, err := ts.Client().Do(req)
assert.Nil(t, err)

assert.Equal(t, http.StatusBadRequest, res.StatusCode)
assert.Equal(t, http.StatusInternalServerError, res.StatusCode)
}

func TestGatewayTimeoutBubblingFromAPI(t *testing.T) {
Expand Down