feat(gateway): _redirects file support by justindotpub · Pull Request #8890 · ipfs/kubo

justindotpub · 2022-04-15T15:09:59Z

Context: ipfs/specs#290, part of ipfs/specs#257

Details

Now that #8885 has landed, I've moved my refactored _redirects changes here from #8816. This is on my personal account now, so the CircleCI permission issues should be resolved. 😅

@lidel, as mentioned at #8816 (comment), there are some challenges with trying to fully move _redirects logic out of gateway_handler.go as you requested. The changes in this PR aren't ready (full review not needed yet), but would you be able to take a look and let me know if you approve of how I've split code into handleUnixfsPathResolution and handleNonUnixfsPathResolution?

Also, note that I've intentionally not moved 404 related functions from gateway_handler.go to gateway_handler_unixfs__redirects.go yet, to avoid extra PR noise.

justindotpub · 2022-04-25T17:13:12Z

@lidel When you have time, gentle bump on getting your feedback to the comments above. I really appreciate the time and feedback you've given me already. 🙏

At this point I have what feels like the right MVP functionality working (from to [status][!] including 200 rewrite for SPA support). I have more tests to write but would really like your thoughts on how close this code is to done so I can try to get this over the finish line.

Also, related to your go-redirects comments... it feels to me like we should not use a library dedicated to parsing Netlify's _redirect file, and instead have our own library whose specific goal is to support the functionality in the redirect IPFS spec (to be written). For now I'm thinking this is essentially a fork of go-redirects that only supports from to [status][!] and the first version of the spec will only include this functionality. All of this functionality is working now on this PR's branch and justincjohnson/go-ipfs-redirects. If you are okay with this approach, I can keep this project in my personal account or we could move it to the ipfs org before this ships if that is preferable.

If you disagree and want me to stick with a repo that specifically supports Netlify's syntax, I still had to fork the repo since it is no longer maintained, and it seems to me there wouldn't be any reason to add all the existing Netlify parsing logic right away, since the MVP won't support all of those features anyway.

For convenience for anyone wanting to evaluate the changes...

# Build and run
make build
GOLOG_LOG_LEVEL="core/server=debug" ./cmd/ipfs/ipfs daemon

# web site
mkdir -p ~/testredirect/
echo "index" > ~/testredirect/index.html
echo "one" > ~/testredirect/one.html
echo "two" > ~/testredirect/two.html
echo "404" > ~/testredirect/404.html
echo "existing" > ~/testredirect/existing.html
echo "forced redirect, never to be seen" > ~/testredirect/forced-redirect.html
mkdir -p ~/testredirect/articles/2022/04/25/hello-world
echo "hello world" > ~/testredirect/articles/2022/04/25/hello-world/index.html

# _redirects file
echo "/redirect-one /one.html" > ~/testredirect/_redirects
echo "/301-redirect-one /one.html 301" >> ~/testredirect/_redirects
echo "/302-redirect-two /two.html 302" >> ~/testredirect/_redirects
echo "/200-index /index.html 200" >> ~/testredirect/_redirects
# existing file with no force, so won't redirect
echo "/existing.html /two.html 200" >> ~/testredirect/_redirects
# existing file with force, so will redirect
echo '/forced-redirect.html /index.html 301!' >> ~/testredirect/_redirects
# Redirect with placeholder.  Try /posts/2022/04/25/hello-world in browser
echo "/posts/:year/:month/:day/:title /articles/:year/:month/:day/:title 301" >> ~/testredirect/_redirects
# Try /splat/index.html or /splat/one.html
echo "/splat/* /:splat 301" >> ~/testredirect/_redirects
# Pretty 404 logic
echo "/en/* /404.html 404" >> ~/testredirect/_redirects
# SPA, as last rule 
echo "/* /index.html 200" >> ~/testredirect/_redirects

CID=$(./cmd/ipfs/ipfs add -r ~/testredirect -Q)
open http://$(echo $CID | ./cmd/ipfs/ipfs cid base32).ipfs.localhost:8080

justindotpub · 2022-04-29T19:55:45Z

FYI, I'll be out for a few weeks but will be checking email in case there is any PR feedback. I'm sure there will be things to address but I'm going to mark this as ready for review now.

justindotpub · 2022-05-25T16:47:30Z

After some discussion with @b5 and @dignifiedquire, I'm leaning toward pulling out forced redirect support to avoid a performance hit for anything other than non-existent paths (i.e. read _redirects file and check for matching redirects if and only if the path didn't resolve). If someone wants to force a redirect, they can just remove or rename the existing path that's preventing redirect rules from firing.

justindotpub · 2022-05-27T18:51:21Z

Review status... @lidel is wrapping up specs for existing gateway functionality and then hopes to get to this, possibly next week or the week after.

lidel

First, thank you for your patience @justincjohnson 🙌

I will be OOO Thu-Fri due to Holiday, and will do proper review of go code in this PR when I am back, but to get things going, quick feedback:

Moving go-ipfs-redirects to ipfs org

Good call on making a dedicated lib.
If you add me as Admin to https://github.com/justincjohnson/go-ipfs-redirects i'll move it to ipfs org + ensure you still have your permissions.

Creating RFC with specs

We want this to be a part of web gateway specs, and not just go-ipfs feature.
Good news is that we now have all the pieces in place to do the proper spec work:

HTTP Gateway specs that describe state in go-ipfs 0.13 are in ipfs/specs#283 (still gathering feedback, but will be merged soon)
Light RFC process for IPFS specs is proposed in ipfs/specs#286 and ipfs/specs#289

Do you mind opening a PR against HTTP Gateway specs from ipfs/specs#283 that adds:

http-gateways/REDIRECTS_FILE.md describing this feature
Include a copy of RFC/0000-template.md from ipfs/specs#289 describing motivation for adding _redirects support.
- we want this to be a lightweight process, so a single paragraph is enough to get the discussion started :)

When we have ipfs/specs PR with RFC,
I'll ping folks from other implementations to take a look and provide feedback, to ensure they are onboard too 👍

Thanks!

test/sharness/t0109-gateway-web-_redirects.sh

core/coreapi/name.go

core/corehttp/gateway_handler_unixfs__redirects.go

lidel

Thank you for your angelic patience @justincjohnson ❤️

I did the first pass review of this along with IPIP (specifications) at ipfs/specs#290 – some questions / asks apply to both – details inline.

General ask for this PR is to rebase on top of latest master to include new tests added in past few weeks.

core/corehttp/gateway_handler_unixfs__redirects.go

test/sharness/t0109-gateway-web-_redirects.sh

core/corehttp/gateway_handler_unixfs__redirects.go

lidel · 2022-08-10T21:34:18Z

core/corehttp/gateway_handler_unixfs__redirects.go

+// Returns the root CID Path for the given path
+func getRootPath(path string) (ipath.Path, error) {
+	if isIpfsPath(path) {
+		parts := strings.Split(path, "/")
+		return ipath.New(gopath.Join(ipfsPathPrefix, parts[2])), nil
+	}
+
+	if isIpnsPath(path) {
+		parts := strings.Split(path, "/")
+		return ipath.New(gopath.Join(ipnsPathPrefix, parts[2])), nil
+	}


nit: if you switch path from string to contentPath ipath.Path, then you will be able to read namespace via contentPath.Namespace() and simplify this

go.mod

core/corehttp/gateway_handler.go

justindotpub · 2022-08-12T20:19:19Z

@lidel I believe I've addressed all feedback now. From my perspective the only question that is outstanding is ipfs/specs#290 (comment). I did add code to handle these extra http status codes, but it still feels to me like it may not make sense.

My hope is that we are finally close to having this pushed across the finish line. 🤞

Thanks again for your assistance.

lidel

Overall looks good, I've added some tests and moved library to the main org at https://github.com/ipfs/go-ipfs-redirects-file.

@justincjohnson will you have time to rebase this PR on top of master again, so it builds with go1.19? When you do that, please address small asks below + ipfs/go-ipfs-redirects-file#11

Would like to ship this in Kubo 0.16 (or 0.17, depending on external factors, such as spec work and today's Implementers review), building with 1.19 is the main blocker.

lidel · 2022-09-14T21:44:21Z

core/corehttp/gateway_handler_unixfs__redirects.go

+			// The path can't be resolved.
+			// If we have origin isolation, attempt to handle any redirect rules.
+			if hasOriginIsolation(r) {
+				redirectsFile := i.getRedirectsFile(r, contentPath, logger)


nit: please move logic related to handling redirectsFile to serveRedirectsIfPresent similar to serveLegacy404IfPresent

Will make it easier to reason about, and easier to refactor in the future.

@lidel Do you feel strongly about this nit? I ask because refactoring this leads to a similar situation we had with past refactor suggestions where the resulting code has to return multiple bools to signal when the caller should return and with what, etc... in this case needing to return 2 paths and 2 bools. If you feel strongly, I'll see what I can do. Thank you!

I refactored as requested. Hopefully you're okay with the booleans.

core/corehttp/gateway_handler_unixfs__redirects.go

core/corehttp/gateway_handler.go

…s-file

…-ipfs-redirects-file

https://github.com/ipfs/go-ipfs-redirects-file/releases/tag/v0.1.0

lidel · 2022-09-22T16:16:50Z

Tagged and switched to release version of the lib: https://github.com/ipfs/go-ipfs-redirects-file/releases/tag/v0.1.0
Rebased on master, resolved conflicts
Things are green, I will do final pass on this before EOW (making sure we did not miss anything like we did with cache-control).
If nothing unexpected but there is high chance this will land in 0.16 (which got moved to Monday) 🤞
- I want to merge specs (IPIP: Gateway _redirects File specs#290) AFTER this PR lands

- this function is core to all response types, does not belong to _redirects specific file - small improvements that improve readability and remove surface for bugs

lidel

Reviewed again and pushed some tweaks.
No real blockers, should be good enough for inclusion in 0.16 RC, but see the ask below.

lidel · 2022-09-22T23:23:12Z

core/corehttp/gateway_handler_unixfs__redirects.go

+				return true, toPath, err
+			}
+
+			if rule.Status == 410 {


@justincjohnson I missed these before:

410 and 451 are missing sharness tests

410 and 451 should behave similarly to what we do for 404, allowing for custom error pages to be rendered informing user why content was taken down.

perhaps reuse i.serve404 – rename it to i.serve4xx and make it accept specific code as a parameter?

coreiface.ErrResolveFailed should not be used for them, create dedicated errors

lmk if you have time to address this before Monday, ideally we would include examples of these in the text fixture that is listed in specs (sadly the CID needs to be updated once again).

No problem. I'll get this done tomorrow.

Thanks, please avoid force-push this time, will make review easier.

I have made the changes and pushed (not force!).

CIDs have been updated in the spec and IPIP as well.

BigLep · 2022-09-23T15:10:33Z

Thanks for all the work here all! For being included in 0.16 (#9237), we'll need to have this ready for review today (2022-09-23) because the RC gets cut on 2022-09-26 Europe time.

justindotpub · 2022-09-23T15:19:26Z

The changes are in and ready for review @BigLep 🙏

https://github.com/ipfs/go-ipfs-redirects-file/releases/tag/v0.1.1

Reduce noise by moving legacy redirect logic to *redirects.go

lidel

Thank you @johnnymatthews 👍 ❤️

Switched to v0.1.1 and pushed some final cleanup.

I believe this is good enough to ship in 0.16 RC (#9237) next week and take it for a spin in the real world.

Merging! (I'll squash, because we moved things around many times).

justindotpub · 2022-09-23T17:01:21Z

@lidel thanks a ton for all the work you put into this!

Based on things provided by ipfs/kubo#8890

lidel · 2022-09-23T18:15:05Z

@justincjohnson fysa we are writing end-user docs for docs.ipfs.tech too, PR draft in ipfs/ipfs-docs#1275

ipfs/kubo#8890 ipfs/specs#290 This commit was moved from ipfs/kubo@bcaacdd

ipfs/kubo#8890 ipfs/specs#290

justindotpub mentioned this pull request Apr 15, 2022

feat(gateway): _redirects file support #8816

Closed

justindotpub changed the title ~~Draft: feat(gateway): _redirects file support~~ feat(gateway): _redirects file support Apr 15, 2022

justindotpub marked this pull request as ready for review April 29, 2022 19:56

justindotpub requested a review from lidel as a code owner April 29, 2022 19:56

justindotpub mentioned this pull request May 27, 2022

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

Open

2 tasks

BigLep added this to the Best Effort Track milestone Jun 3, 2022

BigLep assigned justindotpub Jun 3, 2022

This was referenced Jun 6, 2022

Add HTTP Gateway Specs ipfs/specs#283

Merged

Lightweight RFC Process ipfs/specs#286

Closed

ipfs-404.html is not documented ipfs/ipfs-docs#1155

Closed

lidel requested changes Jun 14, 2022

View reviewed changes

test/sharness/t0109-gateway-web-_redirects.sh Outdated Show resolved Hide resolved

justindotpub commented Jun 16, 2022

View reviewed changes

core/coreapi/name.go Show resolved Hide resolved

lidel reviewed Jul 7, 2022

View reviewed changes

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

lidel mentioned this pull request Jul 19, 2022

Improve performance of servePretty404IfPresent in absence of a pretty 404 #9067

Open

3 tasks

justindotpub mentioned this pull request Jul 20, 2022

Draft: _redirects support refactored fission-codes/kubo#43

Closed

lidel mentioned this pull request Aug 4, 2022

IPIP: Gateway _redirects File ipfs/specs#290

Merged

lidel requested changes Aug 10, 2022

View reviewed changes

lidel mentioned this pull request Aug 30, 2022

[EPIC] Kubo v0.15 Release ipfs/ipfs-docs#1261

Closed

3 tasks

justindotpub mentioned this pull request Sep 1, 2022

Support some deployment options out of the box oddsdk/odd-app-template#9

Closed

lidel assigned lidel and unassigned justindotpub Sep 14, 2022

lidel requested changes Sep 15, 2022

View reviewed changes

lidel reviewed Sep 15, 2022

View reviewed changes

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

justindotpub mentioned this pull request Sep 15, 2022

Validate status codes ipfs/go-ipfs-redirects-file#13

Closed

BigLep assigned justindotpub Sep 20, 2022

Justin Johnson and others added 9 commits September 22, 2022 17:06

Move status code validation logic to github.com/ipfs/go-ipfs-redirect…

9778197

…s-file

fix comment, in part to avoid inconsistent go fmt issue with 1.19

6334c5a

Update comment

f25ed84

Fix test to match new output with status code parsing happening in go…

5b6d7e2

…-ipfs-redirects-file

Add test for max file size

38ac69d

Refactor as requested in review

2a7b0b6

Missing changes from lidel

462cd1a

fix go mod tidy error in ci

8bfa333

chore: go-ipfs-redirects-file v0.1.0

eb7138d

https://github.com/ipfs/go-ipfs-redirects-file/releases/tag/v0.1.0

lidel mentioned this pull request Sep 22, 2022

[do not merge] testing PR 8890 #9300

Closed

refactor: move handlePathResolution

0ae8545

- this function is core to all response types, does not belong to _redirects specific file - small improvements that improve readability and remove surface for bugs

lidel requested changes Sep 22, 2022

View reviewed changes

Serve custom 410 and 451 pages as well

a9a0658

lidel added 2 commits September 23, 2022 18:09

core: go-ipfs-redirects-file v0.1.1

e18d6c7

https://github.com/ipfs/go-ipfs-redirects-file/releases/tag/v0.1.1

refactor: move serveLegacy404IfPresent

907dc6d

Reduce noise by moving legacy redirect logic to *redirects.go

lidel approved these changes Sep 23, 2022

View reviewed changes

lidel merged commit bcaacdd into ipfs:master Sep 23, 2022

justindotpub deleted the justincjohnson/redirects branch September 23, 2022 17:01

lidel added a commit to ipfs/ipfs-docs that referenced this pull request Sep 23, 2022

Update content based on merged code

e0843d5

Based on things provided by ipfs/kubo#8890

Winterhuman mentioned this pull request Sep 23, 2022

CID to IPNS _redirects ipfs/specs#325

Closed

2color mentioned this pull request Nov 14, 2022

Proposal: native redirects config #7392

Closed

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

feat(gateway): _redirects file support (#8890)

59b3312

ipfs/kubo#8890 ipfs/specs#290 This commit was moved from ipfs/kubo@bcaacdd

hacdias mentioned this pull request Apr 6, 2023

feat(gateway)!: deserialised responses turned off by default ipfs/boxo#252

Merged

hannahhoward pushed a commit to filecoin-project/kubo-api-client that referenced this pull request Jun 19, 2023

feat(gateway): _redirects file support (#8890)

f1e00a6

ipfs/kubo#8890 ipfs/specs#290

jphastings mentioned this pull request Jul 8, 2023

Support Query Parameters? ipfs/go-ipfs-redirects-file#20

Closed

Uh oh!

Conversation

justindotpub commented Apr 15, 2022 • edited by lidel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Uh oh!

justindotpub commented Apr 25, 2022

Uh oh!

justindotpub commented Apr 29, 2022

Uh oh!

justindotpub commented May 25, 2022

Uh oh!

justindotpub commented May 27, 2022

Uh oh!

lidel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Moving go-ipfs-redirects to ipfs org

Creating RFC with specs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justindotpub commented Aug 12, 2022

Uh oh!

lidel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidel commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BigLep commented Sep 23, 2022

Uh oh!

justindotpub commented Sep 23, 2022

Uh oh!

lidel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justindotpub commented Sep 23, 2022

justindotpub commented Apr 15, 2022 •

edited by lidel

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading

lidel commented Sep 22, 2022 •

edited

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading