feat(gateway): _redirects file support #8816

justindotpub · 2022-03-22T17:54:12Z

Rebased Cliff's changes with some upstream gateway refactorings. Will retest from here and then continue the work.

justindotpub · 2022-03-22T18:52:50Z

Ah, I see now that draft PRs kick off welcome processes. 😅 When anyone from the IPFS project looks at this, please let me know if draft PRs are discouraged. I'm just getting started on this work and was assuming a draft PR would be a reasonable way to track my progress. Thanks. 🙏🏻

lidel

Thanks for picking this up! ❤️

Quick feedback:

do we need to look at every level? (is netlify / cloudflare looking for _redirects in subdirs, only check if it is present in the top one?)
this should be only applied to default unixfs response types, and ignored by other ones (see comment inline)
this should be enabled only when we have Origin isolation
- (when URL.pathname / starts at the IPFS content root /ipfs/{cid}/ and cant go beyond it)
- namely, when DNSLink website is loaded based on Host header or if Host is a subdomain gateway

core/corehttp/gateway_handler.go

justindotpub · 2022-04-01T21:59:30Z

core/corehttp/gateway_handler.go

+	// For Unixfs, when a path can't be resolved we need to check for redirects and pretty 404 page files.
+	if responseFormat == "" {
+		logger.Debugw("dispatching to getOrHeadHandlerUnixfs")
+		i.getOrHeadHandlerUnixfs(w, r, begin, logger)


Had to dispatch to getOrHeadHandlerUnixfs here, to avoid any early 404s, which would then short circuit the _redirects lookup.

This is a bit invasive change, duplicates code paths which are mostly the same.
This makes it harder to maintain, reason about metrics etc.

Could we keep the old way where we had a single getOrHeadHandler?
See my idea in https://github.com/ipfs/go-ipfs/pull/8816/files#r840944672

This link just takes me to my comment "Moved pretty 404 logic over to gateway_handler_unixfs.go". Is that what you intended to link me?

justindotpub · 2022-04-01T22:02:28Z

core/corehttp/gateway_handler.go

 	switch err {
 	case nil:
 	case coreiface.ErrOffline:
 		webError(w, "ipfs resolve -r "+debugStr(contentPath.String()), err, http.StatusServiceUnavailable)
 		return
 	default:
-		// if Accept is text/html, see if ipfs-404.html is present


Moved pretty 404 logic over to gateway_handler_unixfs.go

core/corehttp/gateway_handler.go

justindotpub · 2022-04-01T22:06:19Z

core/corehttp/gateway_handler_unixfs.go

+				// TODO(JJ): During tests we get multibase.ErrUnsupportedEncoding
+				// This comes from multibase and I assume is due to a fake or otherwise bad CID being in the test.
+				// So for now any errors getting the redirect file are silently ignored.
+				// internalWebError(w, err)


General question about how we should handle errors reading and parsing the redirects if present. Also note my comment about issues with test. I need to dive into this more.

I think it is better to return an error than to ignore it – website owner should get immediate feedback they made a typo or something.

core/corehttp/gateway_handler_unixfs.go

justindotpub · 2022-04-01T22:07:52Z

core/corehttp/gateway_handler_unixfs.go

+		return
+	default:
+		// if Accept is text/html, see if ipfs-404.html is present
+		if i.servePretty404IfPresent(w, r, contentPath) {


Pretty 404 logic moved here

💭 Not the best, this introduced a lot of code duplication, even with extracted functions we duplicate ResolvePath, switch etc.

Can we move it back and have a single getOrHeadHandler? 🙏
The refactor does not seem to be worth it.

See my comments at #8816 (comment).

core/corehttp/gateway_handler_unixfs.go

justindotpub · 2022-04-01T22:08:54Z

core/corehttp/gateway_handler_unixfs.go

+	}
+}
+
+// TODO(JJ): I was thinking about changing this to just look at the root path as well, but the docs say it searches up


I assume you agree?

To be honest, I was not involved in this feature and not sure what relies on it aside from docs.ipfs.io
To limit the blast radius of this PR I'd keep the existing logic.

Hopefully it will die eventually. We won't be documenting this, we will be documenting _redirects instead.

Okay, makes sense. So I assume I should keep existing pretty 404 logic, but only fall back to it if there isn't any netlify style custom 404 page handling entry in the _redirects file. And only the new _redirects file 404 logic will be documented. 👌

justindotpub · 2022-04-01T22:10:56Z

core/corehttp/gateway_handler_unixfs.go

 		return
 	}
 	logger.Debugw("serving unixfs directory", "path", contentPath)
 	i.serveDirectory(w, r, resolvedPath, contentPath, dir, begin, logger)
 }
+
+// redirect returns redirected, newPath (if rewrite), error
+func (i *gatewayHandler) redirect(w http.ResponseWriter, r *http.Request, path ipath.Resolved) (bool, string, error) {


I haven't touched any of this redirect file parsing logic yet. Will need to settle on a format, ensure we handle optional bits like status code, etc. The example file I've been using is...

# redirects file test / 301 ^/hir$ /hi.html 302 ^/hi /hi.html 200

See if we could reuse https://github.com/tj/go-redirects (or other library, if you find a better one) and avoid the need for maintaining the parser for _redirects?

You may find some prior art use in https://github.com/search?l=Go&q=tj%2Fgo-redirects&type=Code

Good idea. I'm guessing at the very least we'll have to contribute to another library to get feature parity with what Netlify currently supports (and I'm assuming that's our target to start with), but I'd rather not completely reinvent the wheel.

justindotpub · 2022-04-01T22:11:34Z

core/corehttp/gateway_handler_unixfs.go

+	return "", "", fmt.Errorf("there is no 404 file for the requested content types")
+}
+
+// TODO(JJ): Pretty sure this is incorrect.  Validate the correct approach.


Any guidance is appreciated. 🙏

Yeah, may not be enough to work on DNSLink websites.

Perhaps you could set dnslink-hostname in core/corehttp/hostname.go (around line 222)? That way you could check it here, similar to gw-hostname.

We need sharness test for DNSLink case

See DNSLINK_FQDN tests in t0114-gateway-subdomains.sh to see how you can inject fake DNSLink mapping for use in tests.

- _redirects: add support for 200 rewrite - add support for regex in redirects

…nixfs reponse format - Write functions for logic in getOrHeadHandler, to make it easier to read and to enable reuse in getOrHeadHandlerUnixfs - Move Unixfs specific functions to gateway_handler_unixfs.go - Check for _redirects file if we have origin isolation

- Add test for no redirect due to no origin isolation

- Use http.* for status codes instead of hardcoded numbers - Test default of no status code in redirects

lidel

Thank you for taking a stab at this! ❤️

Wrote comments inline, but a quick summary:

let's try to keep the changes minimal – cleanup refactors are good )and ok to keep them, but in the future, such things could be a separate PR :)
bit worried about impact of servePretty404IfPresent being bigger than the actual _redirects – I am ok with us keeping it the way it was before just to limit the surface of this PR, seems that I overestimated how easy it will be to limit its scope, apologies! 🙈
- namely, we should get back to a single getOrHeadHandler 🙏
needs DNSLink and catch-all tests
try reusing existing parser lib, if possible. If anything is missing in existing one(s), fine to fork it for now.

core/corehttp/gateway_handler.go

lidel · 2022-04-07T00:15:24Z

core/corehttp/gateway_handler_unixfs.go

+	return "", "", fmt.Errorf("there is no 404 file for the requested content types")
+}
+
+// TODO(JJ): Pretty sure this is incorrect.  Validate the correct approach.


Yeah, may not be enough to work on DNSLink websites.

Perhaps you could set dnslink-hostname in core/corehttp/hostname.go (around line 222)? That way you could check it here, similar to gw-hostname.

We need sharness test for DNSLink case

See DNSLINK_FQDN tests in t0114-gateway-subdomains.sh to see how you can inject fake DNSLink mapping for use in tests.

lidel · 2022-04-07T00:22:20Z

core/corehttp/gateway_handler_unixfs.go

 		return
 	}
 	logger.Debugw("serving unixfs directory", "path", contentPath)
 	i.serveDirectory(w, r, resolvedPath, contentPath, dir, begin, logger)
 }
+
+// redirect returns redirected, newPath (if rewrite), error
+func (i *gatewayHandler) redirect(w http.ResponseWriter, r *http.Request, path ipath.Resolved) (bool, string, error) {


Mind moving this to gateway_handler_unixfs__redirects.go?
And renaming to something like

Suggested change

func (i *gatewayHandler) redirect(w http.ResponseWriter, r *http.Request, path ipath.Resolved) (bool, string, error) {

func (i *gatewayHandler) handledRedirectsFile(w http.ResponseWriter, r *http.Request, path ipath.Resolved) (bool, string, error) {

lidel · 2022-04-07T00:30:18Z

core/corehttp/gateway_handler_unixfs.go

+		return false, "", fmt.Errorf("redirect, could not convert node to file")
+	}
+
+	redirs := newRedirs(f)


with https://github.com/tj/go-redirects

Suggested change

redirs := newRedirs(f)

redirs, err := redirects.ParseString("<_redirects>")

lidel · 2022-04-07T00:41:45Z

core/corehttp/gateway_handler_unixfs.go

 		return
 	}
 	logger.Debugw("serving unixfs directory", "path", contentPath)
 	i.serveDirectory(w, r, resolvedPath, contentPath, dir, begin, logger)
 }
+
+// redirect returns redirected, newPath (if rewrite), error
+func (i *gatewayHandler) redirect(w http.ResponseWriter, r *http.Request, path ipath.Resolved) (bool, string, error) {


See if we could reuse https://github.com/tj/go-redirects (or other library, if you find a better one) and avoid the need for maintaining the parser for _redirects?

You may find some prior art use in https://github.com/search?l=Go&q=tj%2Fgo-redirects&type=Code

core/corehttp/gateway_handler_unixfs.go

justindotpub · 2022-04-07T02:48:47Z

@lidel thank you for your excellent feedback. 🙏 I'm addressing the individual suggestions and comments separately but I want to make sure we're on the same page about something.

bit worried about impact of servePretty404IfPresent being bigger than the actual _redirects – I am ok with us keeping it the way it was before just to limit the surface of this PR, seems that I overestimated how easy it will be to limit its scope, apologies! 🙈

From this comment and some others, it sounds like you might think the larger reorg and duplicate code paths were only introduced due to your request that I move pretty 404 logic into gateway_handler_unixfs.go. Just in case you missed it, what I was trying to work around is the fact that any 404 here can't be handled without first looking for the _redirects file to ensure that the 404 isn't actually a redirect (or rewrite).

In other words, in this code at https://github.com/ipfs/go-ipfs/blob/master/core/corehttp/gateway_handler.go#L336-L350 (on master) we're not handling unixfs yet, but since when resolving the path for ETag purposes the resolution fails and we hit the switch's default case and need to return a 404, even if I keep the pretty 404 logic here, how can I be sure it is truly a 404 without first processing the _redirects file? But I can't process that file if not unixfs, which we're trying to move over to gateway_handler_unixfs.go. So it seems to force a fork in the execution, or some other reconsideration of the organization of this code.

Does that make sense? I'm totally open to your thoughts, but your comments made me think you might not see the issue I was trying to work around. Thanks again.

justindotpub · 2022-04-07T03:18:25Z

Also, to be clear, assuming a larger refactoring does indeed end up being needed, I will gladly split the refactoring out into a separate PR first to make it easier to review, and then rebase this PR on top of that.

justindotpub · 2022-04-07T14:10:39Z

See #8855 for an initial refactoring that doesn't do anything redirect related yet.

justindotpub · 2022-04-08T21:18:19Z

@lidel When you have time, can you take a look at https://github.com/fission-suite/go-ipfs/pull/43/files and let me know if something like this would be acceptable? This PR is in fission's fork of go-ipfs currently and is built on top of the cleanup PR above. If something along these lines is more acceptable, I'll keep working through the remaining items and get them back on the branch tied to this PR for review. Thank you!

justindotpub · 2022-04-15T15:11:13Z

Given the issues that PRs in the fission-suite org are causing, I've moved these changes over to #8890, which is tied to my personal account. 🙏

This comment was marked as resolved.

Sign in to view

This was referenced Mar 22, 2022

implement support for _redirects file fission-codes/kubo#4

Closed

Manifest files for unixfs loaded via HTTP Gateway ipfs/specs#257

Open

lidel reviewed Mar 24, 2022

View reviewed changes

core/corehttp/gateway_handler.go Outdated Show resolved Hide resolved

core/corehttp/gateway_handler.go Outdated Show resolved Hide resolved

BigLep assigned justindotpub Mar 24, 2022

BigLep added this to the Best Effort Track milestone Mar 24, 2022

lidel changed the title ~~(WIP) HTTP Gateway redirects support~~ feat(gateway): _redirects file support Mar 25, 2022

justindotpub force-pushed the justincjohnson/redirects branch from eee1dd5 to e2707f1 Compare April 1, 2022 21:44

justindotpub commented Apr 1, 2022

View reviewed changes

core/corehttp/gateway_handler.go Outdated Show resolved Hide resolved

justindotpub commented Apr 1, 2022

View reviewed changes

core/corehttp/gateway_handler.go Outdated Show resolved Hide resolved

justindotpub commented Apr 1, 2022

View reviewed changes

core/corehttp/gateway_handler_unixfs.go Outdated Show resolved Hide resolved

justindotpub commented Apr 1, 2022

View reviewed changes

core/corehttp/gateway_handler_unixfs.go Show resolved Hide resolved

justindotpub commented Apr 1, 2022

View reviewed changes

justindotpub force-pushed the justincjohnson/redirects branch from e2707f1 to 51ee5b0 Compare April 4, 2022 19:05

cbrake and others added 2 commits April 4, 2022 14:33

- implement basic redirect

6152fa0

- _redirects: add support for 200 rewrite - add support for regex in redirects

justindotpub force-pushed the justincjohnson/redirects branch from 51ee5b0 to 0395527 Compare April 4, 2022 20:03

Justin Johnson added 2 commits April 4, 2022 15:26

- More descriptive sharness test names

39e82fb

- Add test for no redirect due to no origin isolation

- More error handling

ce9e6cf

- Use http.* for status codes instead of hardcoded numbers - Test default of no status code in redirects

justindotpub mentioned this pull request Apr 5, 2022

_redirects spec fission-codes/kubo#41

Closed

lidel added the topic/gateway Topic gateway label Apr 5, 2022

lidel mentioned this pull request Apr 6, 2022

chore: add CODEOWNERS #8852

Merged

lidel requested changes Apr 7, 2022

View reviewed changes

Justin Johnson added 2 commits April 6, 2022 21:05

Cleanup function names

6a855f8

Wrap error with valuable context before returning

b7422d7

Justin Johnson added 2 commits April 7, 2022 06:38

More cleanup

c47eb7b

Separate redirects logic from legacy redirect code

9ef1863

justindotpub mentioned this pull request Apr 7, 2022

chore(gw): extract logical functions to improve readability #8855

Closed

justindotpub mentioned this pull request Apr 15, 2022

feat(gateway): _redirects file support #8890

Merged

justindotpub closed this Apr 17, 2022

lidel mentioned this pull request Sep 23, 2022

IPIP: Gateway _redirects File ipfs/specs#290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gateway): _redirects file support #8816

feat(gateway): _redirects file support #8816

justindotpub commented Mar 22, 2022

This comment was marked as resolved.

justindotpub commented Mar 22, 2022

lidel left a comment •

edited

Loading

justindotpub Apr 1, 2022

lidel Apr 6, 2022

justindotpub Apr 7, 2022

justindotpub Apr 1, 2022

justindotpub Apr 1, 2022

lidel Apr 6, 2022

justindotpub Apr 1, 2022

lidel Apr 6, 2022

justindotpub Apr 7, 2022

justindotpub Apr 1, 2022

lidel Apr 6, 2022

justindotpub Apr 7, 2022

justindotpub Apr 1, 2022

lidel Apr 7, 2022

justindotpub Apr 7, 2022

justindotpub Apr 1, 2022

lidel Apr 7, 2022

lidel left a comment •

edited

Loading

lidel Apr 7, 2022

lidel Apr 7, 2022

lidel Apr 7, 2022

lidel Apr 7, 2022

justindotpub commented Apr 7, 2022 •

edited

Loading

justindotpub commented Apr 7, 2022

justindotpub commented Apr 7, 2022

justindotpub commented Apr 8, 2022

justindotpub commented Apr 15, 2022

	func (i gatewayHandler) redirect(w http.ResponseWriter, r http.Request, path ipath.Resolved) (bool, string, error) {
	func (i gatewayHandler) handledRedirectsFile(w http.ResponseWriter, r http.Request, path ipath.Resolved) (bool, string, error) {

	redirs := newRedirs(f)
	redirs, err := redirects.ParseString("<_redirects>")

feat(gateway): _redirects file support #8816

feat(gateway): _redirects file support #8816

Conversation

justindotpub commented Mar 22, 2022

This comment was marked as resolved.

justindotpub commented Mar 22, 2022

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justindotpub commented Apr 7, 2022 • edited Loading

justindotpub commented Apr 7, 2022

justindotpub commented Apr 7, 2022

justindotpub commented Apr 8, 2022

justindotpub commented Apr 15, 2022

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading

justindotpub commented Apr 7, 2022 •

edited

Loading