serve command (http api)

[verokarhu]

This pull request adds a serve command which hosts a http server that listens for POST and GET requests. At the moment it's only possible to GET and POST individual blocks (blobs?).

I decided against implementing the API like it was described in issue #2 because

• having a /ctrl/add path is not restful since / is the location where the POSTed files end up
• distinguishing between uploaded blocks and files is unnecessary since a client can just chop the file up themselves if they want a chunked upload

[whyrusleeping]

I dont know how i feel about defaulting to 80, since that requires super user priveleges on most systems

[verokarhu]

A fair point, forgot that ports below 1024 require root. 8080 makes more
sense as a default.

On Thu, Aug 7, 2014 at 1:22 AM, Jeromy Johnson notifications@github.com
wrote:

In cmd/ipfs/serve.go:

• "github.com/gonuts/flag"
• "github.com/jbenet/commander"
• h "github.com/jbenet/go-ipfs/http"
• "strconv"
  +)

+var cmdIpfsServe = &commander.Command{

• UsageLine: "serve",
• Short: "Serve an HTTP API",
• Long: ipfs serve - Serve an http gateway into ipfs.,
• Run: serveCmd,
• Flag: *flag.NewFlagSet("ipfs-serve", flag.ExitOnError),
  +}

+func init() {

• cmdIpfsServe.Flag.Uint("port", 80, "Port number")

I dont know how i feel about defaulting to 80, since that requires super
user priveleges on most systems

—
Reply to this email directly or view it on GitHub
https://github.com/jbenet/go-ipfs/pull/24/files#r15907722.

[jbenet]

@verokarhu great!

having a /ctrl/add path is not restful since / is the location where the POSTed files end up

Makes sense.

distinguishing between uploaded blocks and files is unnecessary since a client can just chop the file up themselves if they want a chunked upload

Yep, this is fine. Eventually we will want more complicated things like, rabin fingerprint chunking done on the server and perhaps even support multipart upload. Not important anytime soon.

I'll CR now.

[jbenet]

let's make this command either serve-http or just do serve http (subcommand), as serve is not protocol ambiguous. (I can imagine ipfs serve git and so on in the future)

[jbenet]

Okay, made some comments. Everything else LGTM. I haven't tested this-- if you can create a test file, would be great. Don't worry about query params, we can make that a future issue.

[verokarhu]

I left the multiaddr change and a test file out for now. Not familiar with how http testing in go is done but I'll look into it for a future pull request if/when I get around to doing more features for the http api.

Hopefully my assurances that it has been thoroughly tested on Win7 is enough for now. :)

As a sidenote, I wonder if the windows version of the mount command could use this API. @whyrusleeping mentioned that he had worked on an FTP server in issue #9 but I haven't thought about it enough to know if this API would be more suitable.

[whyrusleeping]

@verokarhu http testing that ive done in the past has used the napping library to query a server you start up within the test to ensure that the data is consistent with the you expect. I would love to get this code polished and merged in soon. It will be very nice to have as an alternative to fuse (since fuse is being a little problematic for people to easily set up at the moment)

[whyrusleeping]

also, take a look at how the mount command works with the daemon code (on the daemon branch) and make serve do the same thing.

[verokarhu]

I'm no git master but wouldn't it be simplest to get the daemon branch merged before using those features in this pull request?

[whyrusleeping]

Yeah, hold off on the daemon stuff for a little bit until we get that finalized and merged in, but if you could write some sort of unit tests for the http interface, that would be extremely helpful!

[verokarhu]

All right, I'll do that. Thanks for the tip on napping btw, I'll look into it. Have been meaning to figure out http testing for another project (and ipfs of course =) ). There seems to have been some changes to the master that this pull request isn't compatible with so I'll have to fix that as well.

[whyrusleeping]

Just a ping to let you know that the daemon code is merged in!

[verokarhu]

There is now tests for posthandler and servehttp. I suppose one could refactor it a bit and enable testing the routes as well but that seems overkill ATM.

The biggest problem now is getting gorilla vendored. Try as I may, godep seems to remove the entire workspace when attempting to add gorilla/mux...

I'm curious, why is both godep and vendoring inside the repo used simultaneously? This hybrid approach between vendoring and revision locking seems odd, though I'm not exactly familiar with godep.

[whyrusleeping]

We use godep for vendoring, the make file should do it for you.

[btc]

godep is used to copy all dependencies into Godeps/_workspace. Throughout the codebase, import statements are rewritten to point to this local workspace.

The command make vendor in package root performs this task.

[verokarhu]

I don't have make (i'm not sure if that's available on windows) but I ran the command contained in the makefile. This resulted in godep deleting the workspace (?) and the following error message:

C:\ipfs\src\github.com\jbenet\go-ipfs>godep save -r ./...
godep: rename C:\IPFS\src\github.com\jbenet\go-ipfs\server\http\http.go.temp C:\IPFS\src\github.com\jbenet\go-ipfs\server\http\http.go: Cannot create a file when that file already exists.

Anyways, I was able to manually merge the half-done state godep left things in and it seems to work. Hopefully the build completes succesfully.

[verokarhu]

Looks like the build still fails. Am I correct in assuming that the builds were broken already?

The tests in dht_test.go look peculiar, is it making an actual node that it contacts? If so, that should really be mocked out.

[whyrusleeping]

Yeah, the tests that are failing in your code dont seem to be related to the work youve done. Ill take a little further look at them to make sure, see if i can repro

[whyrusleeping]

@verokarhu Yeap, confirmed that the tests fail on master. Your stuff looks good, once we get a go ahead from @jbenet and @perfmode we can get this merged in!

[whyrusleeping]

Im not sure i agree with the need for an interface here.

[verokarhu]

I did it with an interface because it makes it easy to mock out the ipfs methods by swapping in a test interface. Another way of doing it would be to have function variables but that felt less go-ey and more like javascript. If there is a third, nicer way of mocking them out I'm of course open to suggestions.

[whyrusleeping]

No, that makes good sense. Youll just need to add a function to get a reader for a node (via merkledag/dagreader)

[jbenet]

👍 to this interface. We should probably have something similar in /core/interface.go later on, or something.

[mildred]

I haven't looked at everything, but why is HTTP POST being used to upload files and not HTTP PUT. I believe the PUT method is better suited for that purpose (being idempotent is a good thing, and the client can know the hash in advance).

A description can be found on stackoverflow. Basically:

• Do you name your URL objects you create explicitly, or let the server decide? If you name them then use PUT. If you let the server decide then use POST.
• PUT is idempotent, so if you PUT an object twice, it has no effect. This is a nice property, so I would use PUT when possible.
• You can update or create a resource with PUT with the same object URL
• With POST you can have 2 requests coming in at the same time making modifications to a URL, and they may update different parts of the object.

[verokarhu]

I think it's good to go. Some of the things you mentioned are unfixed like returning a 404 error since it seemed like that might need work in other parts of ipfs as well (the error returned was context deadline exceeded). The setup part in serve.go is also still manual.

[jbenet]

@verokarhu travis build failed, and failure is in TestServeHTTP:
https://travis-ci.org/jbenet/go-ipfs/jobs/36706143

--- FAIL: TestServeHTTP (0.00 seconds)
panic: runtime error: slice bounds out of range [recovered]
    panic: runtime error: slice bounds out of range
goroutine 3 [running]:
runtime.panic(0x6b5dc0, 0xaa5b6a)
    /home/travis/.gvm/gos/go1.2.2/src/pkg/runtime/panic.c:266 +0xb6
testing.func·005()
    /home/travis/.gvm/gos/go1.2.2/src/pkg/testing/testing.go:385 +0xe8
runtime.panic(0x6b5dc0, 0xaa5b6a)
    /home/travis/.gvm/gos/go1.2.2/src/pkg/runtime/panic.c:248 +0x106
github.com/jbenet/go-ipfs/server/http.(*handler).ServeHTTP(0xc21001eaf0, 0x7fc23411f638, 0xc21000a560, 0xc210049820)
    /home/travis/gopath/src/github.com/jbenet/go-ipfs/server/http/http.go:36 +0x2f4
github.com/jbenet/go-ipfs/server/http.TestServeHTTP(0xc21004e120)
    /home/travis/gopath/src/github.com/jbenet/go-ipfs/server/http/http_test.go:35 +0x26d
testing.tRunner(0xc21004e120, 0xaa0480)
    /home/travis/.gvm/gos/go1.2.2/src/pkg/testing/testing.go:391 +0x8b
created by testing.RunTests
    /home/travis/.gvm/gos/go1.2.2/src/pkg/testing/testing.go:471 +0x8b2
goroutine 1 [chan receive]:
testing.RunTests(0x79c5d8, 0xaa0480, 0x2, 0x2, 0x1)
    /home/travis/.gvm/gos/go1.2.2/src/pkg/testing/testing.go:472 +0x8d5
testing.Main(0x79c5d8, 0xaa0480, 0x2, 0x2, 0xaa9580, ...)
    /home/travis/.gvm/gos/go1.2.2/src/pkg/testing/testing.go:403 +0x84
main.main()
    github.com/jbenet/go-ipfs/server/http/_test/_testmain.go:49 +0x9c
FAIL    github.com/jbenet/go-ipfs/server/http   0.012s

[verokarhu]

Oh man, that's what I get for forgetting to run the tests before the last changes. Should really start using an IDE so they are run automatically. Should be ok now.

[jbenet]

Ok, tests pass, LGTM! thanks @verokarhu 👍

[whyrusleeping]

Im so happy about this.