kubo icon indicating copy to clipboard operation
kubo copied to clipboard

feat(gateway): _redirects file support

Open justindotpub opened this issue 4 years ago • 5 comments

Context: https://github.com/ipfs/specs/pull/290, part of https://github.com/ipfs/specs/issues/257

This PR implements https://github.com/ipfs/specs/pull/290.

Details

Now that https://github.com/ipfs/go-ipfs/pull/8885 has landed, I've moved my refactored _redirects changes here from https://github.com/ipfs/go-ipfs/pull/8816. This is on my personal account now, so the CircleCI permission issues should be resolved. 😅

@lidel, as mentioned at https://github.com/ipfs/go-ipfs/pull/8816#issuecomment-1091022658, 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 avatar Apr 15 '22 15:04 justindotpub

@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 avatar Apr 25 '22 17:04 justindotpub

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 avatar Apr 29 '22 19:04 justindotpub

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 avatar May 25 '22 16:05 justindotpub

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.

justindotpub avatar May 27 '22 18:05 justindotpub

@lidel I believe I've addressed all feedback now. From my perspective the only question that is outstanding is https://github.com/ipfs/specs/pull/290#discussion_r944384586. 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.

justindotpub avatar Aug 12 '22 20:08 justindotpub

@lidel regarding broken tests, I'm waiting for you to merge changes in go-ipfs-redirects-file first. 🙏

I'll take a look at the diff you sent, and today I learned about git push --force-with-lease . 🥳

I would love this to be included in the release, but understand the lack of bandwidth.

justindotpub avatar Sep 21 '22 17:09 justindotpub

@lidel your diff's are now pushed on my branch. All tests pass locally for me when go-ipfs-redirects-file is replaced with my local clone.

justindotpub avatar Sep 21 '22 18:09 justindotpub

  • 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) :crossed_fingers:
    • I want to merge specs (https://github.com/ipfs/specs/pull/290) AFTER this PR lands

lidel avatar Sep 22 '22 16:09 lidel

Thanks for all the work here all! For being included in 0.16 (https://github.com/ipfs/kubo/issues/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.

BigLep avatar Sep 23 '22 15:09 BigLep

The changes are in and ready for review @BigLep 🙏

justindotpub avatar Sep 23 '22 15:09 justindotpub

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

justindotpub avatar Sep 23 '22 17:09 justindotpub

@justincjohnson fysa we are writing end-user docs for docs.ipfs.tech too, PR draft in https://github.com/ipfs/ipfs-docs/pull/1275

lidel avatar Sep 23 '22 18:09 lidel