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

Add daemon command #52

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Add daemon command #52

merged 1 commit into from
Feb 2, 2023

Conversation

kylehuntsman
Copy link
Contributor

@kylehuntsman kylehuntsman commented Jan 30, 2023

Adds a lassie daemon command for running an http server. Some things are not very pretty, but the HTTP interface should be okay to use while we clean up everything behind.

Notable Changes:

  • daemon command that starts a running http server
  • version command for logging the version. lassie needs to be built with -ldflags="main.version=v0.0.0" to set the version.
  • -v and -vv verbose flags for additional logging
  • /ipfs endpoint that implements issue HTTP Daemon #34

Notes:

  • The abstracted retriever setup from fetch couldn't be easily reused by fetch because of the progress output. This code is slightly duplicated for the time being. That abstracted code has been put in a rough outline of a empty Lassie object that you can call Fetch on. Makes a nice lassie.Fetch() call.
  • If the Accept header includes application/vnd.ipld.car (including wildcard types), the request is accepted.
  • If the requested format query parameter is provided but is not the value car, the server responds with a 400.
  • If the filename query parameter value extension is not .car, the server responds with a 400.

Known TODOs:

  • Set the X-Ipfs-Roots header
  • Implement UnixFS pathing requests

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2023

Codecov Report

Merging #52 (7695126) into main (4ac7fe6) will decrease coverage by 11.51%.
The diff coverage is 44.17%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #52       +/-   ##
===========================================
- Coverage   58.28%   46.78%   -11.51%     
===========================================
  Files          20       27        +7     
  Lines        2069     2302      +233     
===========================================
- Hits         1206     1077      -129     
- Misses        829     1189      +360     
- Partials       34       36        +2     
Impacted Files Coverage Δ
cmd/lassie/daemon.go 0.00% <0.00%> (ø)
cmd/lassie/fetch.go 0.00% <0.00%> (ø)
cmd/lassie/main.go 0.00% <0.00%> (ø)
cmd/lassie/version.go 0.00% <0.00%> (ø)
internal/libp2p.go 0.00% <ø> (ø)
internal/putcbblockstore.go 0.00% <0.00%> (ø)
internal/setupretriever.go 0.00% <0.00%> (ø)
pkg/client/client.go 0.00% <0.00%> (ø)
pkg/lassie/lassie.go 0.00% <0.00%> (ø)
pkg/server/http/ipfs.go 0.00% <0.00%> (ø)
... and 16 more

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

The shutdown behavior can be simplified a lot

  • no need for interrupt code, urfave cli will pick up those signals and cancel its context
  • you can avoid handling context and server errors seperately but just setting the BaseContext propery on http.Server

cmd/lassie/main.go Show resolved Hide resolved
server/http/server.go Show resolved Hide resolved
func versionCommand(cctx *cli.Context) error {
if version == "" {
log.Warn("executable built without a version")
log.Warn("set version with `go build -ldflags=\"-X main.version=v0.0.0\"")
Copy link
Member

Choose a reason for hiding this comment

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

do we need to adapt our release build github workflow to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was meaning to look into that and haven't yet. Ideally we have that build flag use the version.json to set that value.

@kylehuntsman kylehuntsman force-pushed the feat/http-server branch 2 times, most recently from af88596 to 7695126 Compare February 1, 2023 05:46
@kylehuntsman kylehuntsman marked this pull request as ready for review February 1, 2023 05:50
internal/putcbblockstore.go Outdated Show resolved Hide resolved
pkg/lassie/lassie.go Outdated Show resolved Hide resolved
pkg/lassie/lassie.go Outdated Show resolved Hide resolved
pkg/lassie/lassie.go Outdated Show resolved Hide resolved
pkg/server/http/ipfs.go Outdated Show resolved Hide resolved
pkg/server/http/ipfs.go Outdated Show resolved Hide resolved
pkg/server/http/ipfs.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

various file organization feedback

but ultimately, two key pieces:

  • only one retriever instantiated for the daemon
  • remove putCBBlockstore where it's not needed

pkg/server/http/ipfs.go Outdated Show resolved Hide resolved
pkg/server/http/ipfs.go Outdated Show resolved Hide resolved
pkg/server/http/ipfs.go Outdated Show resolved Hide resolved
pkg/server/http/ipfs.go Outdated Show resolved Hide resolved
pkg/server/http/ipfs.go Outdated Show resolved Hide resolved
pkg/cli/flags.go Outdated Show resolved Hide resolved
pkg/lassie/lassie.go Outdated Show resolved Hide resolved
pkg/lassie/lassie.go Outdated Show resolved Hide resolved
@kylehuntsman kylehuntsman force-pushed the feat/http-server branch 3 times, most recently from fb9a511 to 6687eca Compare February 1, 2023 23:32
@kylehuntsman
Copy link
Contributor Author

kylehuntsman commented Feb 1, 2023

The server now instantiates a Lassie that is passed to the ipfsHandler. The ipfsHandler then uses that instance, passing Lassie.Fetch() it's own LinkSystem.

Lassie now has a LassieConfig and can be instantiated with LassieOptions, like an optional CandidateFinder. Examples of this usage can be found in the http server instantiation as well as the CLI fetch command.

@hannahhoward hannahhoward merged commit c40e57d into main Feb 2, 2023
@kylehuntsman kylehuntsman deleted the feat/http-server branch February 2, 2023 02:43
res.Header().Set("Cache-Control", "public, max-age=29030400, immutable")
res.Header().Set("Content-Type", "application/vnd.ipld.car; version=1")
res.Header().Set("Etag", fmt.Sprintf("%s.car", rootCid.String()))
res.Header().Set("C-Content-Type-Options", "nosniff")
Copy link

Choose a reason for hiding this comment

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

@hannahhoward typo :)

Comment on lines +156 to +157
res.Header().Set("X-Ipfs-Path", req.URL.Path)
// TODO: set X-Ipfs-Roots header
Copy link

@lidel lidel Feb 2, 2023

Choose a reason for hiding this comment

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

💭 fwiw these two make sense mostly in deserialized contexts like website hosting: allow for partial HTTP Cache invalidation based on DAG/path subset that changed (ipfs/kubo#8720, based on request from Cloudflare)

For requests for /ipfs/cid without any sub path they don't bring much value ( Etag is more than enough).

So fine to skip X-Ipfs-Roots header until you support sub-paths (if its even planned)

rvagg added a commit that referenced this pull request Feb 2, 2023
@rvagg
Copy link
Member

rvagg commented Feb 2, 2023

#61 follow-up from @lidel's review

kylehuntsman pushed a commit that referenced this pull request Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants