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.

Sign up for GitHub

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

fix: local running dx improvements #12

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: local running dx improvements #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bbbd12
fix: fall back to http for fetching version
SgtPooki Mar 27, 2024
b1b7c54
feat: allow custom env vars for ipfs service
SgtPooki Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 51 additions & 1 deletion cmd_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ package main

import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"math/rand"
"net/http"
"time"

shell "github.com/ipfs/go-ipfs-api"
Expand Down Expand Up @@ -90,6 +93,12 @@ var RunCommand = &cli.Command{
EnvVars: []string{"TIROS_RUN_IPFS_HOST", "TIROS_RUN_KUBO_HOST" /* <- legacy */},
Value: "localhost",
},
&cli.StringFlag{
Name: "ipfs-api-host",
Usage: "port to reach a Kubo-compatible RPC API",
EnvVars: []string{"TIROS_RUN_IPFS_API_HOST"},
Value: "127.0.0.1",
},
Comment on lines +96 to +101
Copy link
Author

Choose a reason for hiding this comment

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

we should also update the multiaddr used in ipfsClient = shell.NewShell(fmt.Sprintf("/ip4/127.0.0.1/tcp/%d", c.Int("ipfs-api-port"))) to use this, but I wanted to submit PR with smaller changes and get your thoughts on this @dennis-tra

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ipfs-api-host different from the ipfs-host? This would mean that the IPFS gateway has a different host than the API, is that the case for helia?

Just curious, I'm totally fine adding this additional flag but if we can get away with just using the ipfs-host flag, I would prefer just keeping that. If we indeed need both and ipfs-api-host is unset, I would fall back to whatever is set with ipfs-host.

Copy link
Author

Choose a reason for hiding this comment

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

The difference is kubo has two endpoints, an api endpoint and http gateway endpoint. Helia-http-gateway ( and others might) has only one.

Youre right that we should probably use the single http host for fallback to version via http. I will update this

&cli.IntFlag{
Name: "ipfs-api-port",
Usage: "port to reach a Kubo-compatible RPC API",
Expand Down Expand Up @@ -276,8 +285,49 @@ func RunAction(c *cli.Context) error {
return nil
}

// GetIpfsVersion tries to retrieve the IPFS version using the IPFS API client.
// If it fails, it falls back to an HTTP request to the specified endpoint, using host and port from the context.
func (t *tiros) GetIpfsVersion(c *cli.Context) (version string, sha string, err error) {
version, sha, err = t.ipfs.Version()
if err == nil {
return version, sha, nil
}

// Use host and port from the context for the fallback HTTP request
apiHost := c.String("ipfs-api-host")
apiPort := c.Int("ipfs-api-port")
apiUrl := fmt.Sprintf("http://%s:%d/api/v0/version", apiHost, apiPort)
// log that we're falling back to HTTP url for version
log.WithField("url", apiUrl).Debugln("Falling back to HTTP request for version")

resp, httpErr := http.Get(apiUrl)
if httpErr != nil {
return "", "", fmt.Errorf("ipfs api and http fallback both failed: %v, fallback error: %w", err, httpErr)
}
defer resp.Body.Close()

body, readErr := io.ReadAll(resp.Body)
if readErr != nil {
return "", "", fmt.Errorf("error reading version endpoint response: %w", readErr)
}
// log the entire body of the response
log.WithField("body", string(body)).Debugln("HTTP response body")

var data struct {
Version string `json:"Version"`
Commit string `json:"Commit"`
}

jsonErr := json.Unmarshal(body, &data)
if jsonErr != nil {
return "", "", fmt.Errorf("error parsing version endpoint JSON: %w", jsonErr)
}

return data.Version, data.Commit, nil
Comment on lines +297 to +326
Copy link
Contributor

@dennis-tra dennis-tra Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Under the hood t.ipfs.Version() is doing almost the same HTTP request to fmt.Sprintf("http://%s:%d/api/v0/version", apiHost, apiPort). The client is not using gRPC or anything. The only difference here seems to be that the Kubo HTTP endpoints expect a POST request [docs]. Therefore, t.ipfs.Version() is also doing a POST instead of a GET as in line 303.

Would it work if we changed helia to also expect a POST instead of a GET request?

If that is possible AND we really need the ipfs-api-host flag from above, I think we could get away with just changing:

ipfsClient = shell.NewShell(fmt.Sprintf("/ip4/127.0.0.1/tcp/%d", c.Int("ipfs-api-port")))

to

ipfsClient = shell.NewShell(fmt.Sprintf("/ip4/%s/tcp/%d", c.String("ipfs-api-host"), c.Int("ipfs-api-port")))

Copy link
Author

@SgtPooki SgtPooki Mar 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Post requests for version are also supported in helia-http-gateway and this fails against helia-http-gateway when ran locally. Maybe due to docker networking on mac?

I tried using the change you suggest and that didnt work, while explicitly doing an http request did

}

func (t *tiros) InitRun(c *cli.Context) (*models.Run, error) {
version, sha, err := t.ipfs.Version()
version, sha, err := t.GetIpfsVersion(c)
if err != nil {
return nil, fmt.Errorf("ipfs api offline: %w", err)
}
Expand Down
4 changes: 3 additions & 1 deletion docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ version: "3.9"
name: tiros
services:
ipfs:
image: ipfs/kubo:v0.19.0
image: ${IPFS_IMAGE:-ipfs/kubo:v0.19.0}
Copy link
Author

Choose a reason for hiding this comment

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

allows overriding the image easily, so I can rebuild helia-http-gateway locally and have it update here.

restart: unless-stopped
volumes:
- ipfs_path:/data/ipfs
Expand All @@ -13,6 +13,8 @@ services:
- "4001:4001/udp"
- "0.0.0.0:5001:5001"
- "0.0.0.0:8080:8080"
env_file:
- ${IPFS_ENV_FILE:-/dev/null}
Comment on lines +16 to +17
Copy link
Author

@SgtPooki SgtPooki Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

dockerfile syntax of optional env files doesn't work well, so this is a fix for ignoring env file if IPFS_ENV_FILE is not set. see docker/compose#3560 (comment) for more info

This allows me to set .env files for testing helia-dr(.env.helia-dr), helia(.env.helia), and helia-tg(.env.helia-tg) and easily change them with the following config:

# .envrc (for direnv)
dotenv .env.local
dotenv .env

# .env.local (already in this repo
...

# .env
TIROS_RUN_IPFS_API_HOST="localhost"
TIROS_RUN_IPFS_API_PORT=8080
IPFS_ENV_FILE=.env.helia-dr
IPFS_IMAGE=helia-http-gateway:local-arm64

and all i have to do is update .env and run direnv reload then docker compose up -d to get updated services.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

neat!

chrome:
image: browserless/chrome:latest
ports:
Expand Down