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

feat(gateway): _redirects file support #8890

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

justincjohnson
Copy link
Contributor

@justincjohnson justincjohnson commented Apr 15, 2022

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

This PR implements ipfs/specs#290.

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.

@justincjohnson justincjohnson changed the title Draft: feat(gateway): _redirects file support feat(gateway): _redirects file support Apr 15, 2022
@justincjohnson justincjohnson force-pushed the justincjohnson/redirects branch 5 times, most recently from c8a2631 to 3853b36 Compare Apr 21, 2022
@justincjohnson
Copy link
Contributor Author

justincjohnson commented Apr 25, 2022

@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

@justincjohnson justincjohnson force-pushed the justincjohnson/redirects branch 2 times, most recently from 24d60bd to 19d070b Compare Apr 28, 2022
@justincjohnson
Copy link
Contributor Author

justincjohnson commented Apr 29, 2022

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.

@justincjohnson justincjohnson marked this pull request as ready for review Apr 29, 2022
@justincjohnson justincjohnson requested a review from lidel as a code owner Apr 29, 2022
@justincjohnson
Copy link
Contributor Author

justincjohnson commented May 25, 2022

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.

@justincjohnson
Copy link
Contributor Author

justincjohnson commented May 27, 2022

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.

Copy link
Member

@lidel lidel left a comment

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 Outdated Show resolved Hide resolved
Copy link
Member

@lidel lidel left a comment

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.

test/sharness/t0109-gateway-web-_redirects.sh Outdated Show resolved Hide resolved
test/sharness/t0109-gateway-web-_redirects.sh Show resolved Hide resolved
// Check for root path.
_, err = i.api.Block().Get(r.Context(), rootPath)
if err != nil {
return nil
}
Copy link
Member

@lidel lidel Aug 10, 2022

Choose a reason for hiding this comment

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

What is the purpose of this check?

// 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
}
Copy link
Member

@lidel lidel Aug 10, 2022

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
core/corehttp/gateway_handler.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 In Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants