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

Move api version check to header #2037

Merged
merged 2 commits into from
Dec 28, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
70 changes: 5 additions & 65 deletions cmd/ipfs/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -416,17 +416,13 @@ func commandShouldRunOnDaemon(details cmdDetails, req cmds.Request, root *cmds.C
return nil, err
}

if client != nil { // daemon is running
if client != nil { // api file exists
if details.cannotRunOnDaemon {
e := "cannot use API with this command."

// check if daemon locked. legacy error text, for now.
daemonLocked, _ := fsrepo.LockedByOtherProcess(req.InvocContext().ConfigRoot)
if daemonLocked {
e = "ipfs daemon is running. please stop it to run this command"
if daemonLocked, _ := fsrepo.LockedByOtherProcess(req.InvocContext().ConfigRoot); daemonLocked {
return nil, cmds.ClientError("ipfs daemon is running. please stop it to run this command")
}

return nil, cmds.ClientError(e)
return nil, nil
Copy link
Member

Choose a reason for hiding this comment

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

  • why is this no longer an error?
  • and why nil, instead of func () {} ? nevermind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ok sounds good

}

return client, nil
Expand Down Expand Up @@ -604,63 +600,7 @@ func getApiClient(repoPath, apiAddrStr string) (cmdsHttp.Client, error) {
return nil, err
}

client, err := apiClientForAddr(addr)
if err != nil {
return nil, err
}

// make sure the api is actually running.
// this is slow, as it might mean an RTT to a remote server.
// TODO: optimize some way
if err := apiVersionMatches(client); err != nil {
return nil, err
}

return client, nil
}

// apiVersionMatches checks whether the api server is running the
// same version of go-ipfs. for now, only the exact same version of
// client + server work. In the future, we should use semver for
// proper API versioning! \o/
func apiVersionMatches(client cmdsHttp.Client) (err error) {
ver, err := doVersionRequest(client)
if err != nil {
return err
}

currv := config.CurrentVersionNumber
if ver.Version != currv {
return fmt.Errorf("%s (%s != %s)", errApiVersionMismatch, ver.Version, currv)
}
return nil
}

func doVersionRequest(client cmdsHttp.Client) (*coreCmds.VersionOutput, error) {
cmd := coreCmds.VersionCmd
optDefs, err := cmd.GetOptions([]string{})
if err != nil {
return nil, err
}

req, err := cmds.NewRequest([]string{"version"}, nil, nil, nil, cmd, optDefs)
if err != nil {
return nil, err
}

res, err := client.Send(req)
if err != nil {
if isConnRefused(err) {
err = repo.ErrApiNotRunning
}
return nil, err
}

ver, ok := res.Output().(*coreCmds.VersionOutput)
if !ok {
return nil, errUnexpectedApiOutput
}
return ver, nil
return apiClientForAddr(addr)
}

func apiClientForAddr(addr ma.Multiaddr) (cmdsHttp.Client, error) {
Expand Down
3 changes: 1 addition & 2 deletions commands/http/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,7 @@ func (c *client) Send(req cmds.Request) (cmds.Response, error) {
} else {
httpReq.Header.Set(contentTypeHeader, applicationOctetStream)
}
version := config.CurrentVersionNumber
httpReq.Header.Set(uaHeader, fmt.Sprintf("/go-ipfs/%s/", version))
httpReq.Header.Set(uaHeader, config.ApiVersion)

ec := make(chan error, 1)
rc := make(chan cmds.Response, 1)
Expand Down
24 changes: 23 additions & 1 deletion commands/http/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

cors "github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/rs/cors"
context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
"github.com/ipfs/go-ipfs/repo/config"

cmds "github.com/ipfs/go-ipfs/commands"
logging "github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log"
Expand All @@ -35,7 +36,10 @@ type Handler struct {
corsHandler http.Handler
}

var ErrNotFound = errors.New("404 page not found")
var (
ErrNotFound = errors.New("404 page not found")
errApiVersionMismatch = errors.New("api version mismatch")
)

const (
StreamErrHeader = "X-Stream-Error"
Expand Down Expand Up @@ -423,3 +427,21 @@ func allowReferer(r *http.Request, cfg *ServerConfig) bool {

return false
}

// apiVersionMatches checks whether the api client is running the
// same version of go-ipfs. for now, only the exact same version of
// client + server work. In the future, we should use semver for
// proper API versioning! \o/
func apiVersionMatches(r *http.Request) error {
clientVersion := r.UserAgent()
// skips check if client is not go-ipfs
if clientVersion == "" || !strings.Contains(clientVersion, "/go-ipfs/") {
return nil
}

daemonVersion := config.ApiVersion
if daemonVersion != clientVersion {
return fmt.Errorf("%s (%s != %s)", errApiVersionMismatch, daemonVersion, clientVersion)
}
return nil
}
11 changes: 9 additions & 2 deletions commands/http/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,20 @@ func Parse(r *http.Request, root *cmds.Command) (cmds.Request, error) {

stringArgs := make([]string, 0)

if err := apiVersionMatches(r); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

the check should not be in the commands lib, as the commands lib is not-ipfs specific and will be ripped out into its own lib.

it should be in ipfs specific stuff. if no function called globally for all daemon commands then we should add one that could run before the command is invoked.

Copy link
Member

Choose a reason for hiding this comment

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

I would argue that an api version check belongs in the commands lib. Thats a nice feature for the commands library to provide. Being able to have your automatically generated http api do version sanity checks is pretty slick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Been trying to make sense of this.
The client version header is set/hardcoded in the commands lib https://github.com/ipfs/go-ipfs/blob/9066b12d33cd67e083c15c58767c5a629c5165e2/commands/http/client.go#L104.
While the string /go-ipfs/%s/ is ipfs specific, the api versioning scheme itself isn't.

Copy link
Member

Choose a reason for hiding this comment

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

I would argue that an api version check belongs in the commands lib. Thats a nice feature for the commands library to provide. Being able to have your automatically generated http api do version sanity checks is pretty slick.

agreed there, but then this should be properly done so it does not include ipfs code https://github.com/ipfs/go-ipfs/pull/2037/files#diff-4df02c629d914567df45314a464367b9R17

Copy link
Member

Choose a reason for hiding this comment

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

we could define an API version in the root or something... not sure how this does with command trees and so on. seems like a bigger change than we want this PR to be. (if we want it for this one, then do that PR first and rebase this on that one... but i would say just do the simpler thing now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Migrated to #2075

if path[0] != "version" { // compatibility with previous version check
return nil, err
}
}

cmd, err := root.Get(path[:len(path)-1])
if err != nil {
// 404 if there is no command at that path
return nil, ErrNotFound

} else if sub := cmd.Subcommand(path[len(path)-1]); sub == nil {
}

if sub := cmd.Subcommand(path[len(path)-1]); sub == nil {
if len(path) <= 1 {
return nil, ErrNotFound
}
Expand All @@ -34,7 +42,6 @@ func Parse(r *http.Request, root *cmds.Command) (cmds.Request, error) {
// e.g. /objects/Qabc12345 (we are passing "Qabc12345" to the "objects" command)
stringArgs = append(stringArgs, path[len(path)-1])
path = path[:len(path)-1]

} else {
cmd = sub
}
Expand Down
2 changes: 2 additions & 0 deletions repo/config/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
// CurrentVersionNumber is the current application's version literal
const CurrentVersionNumber = "0.3.11-dev"

const ApiVersion = "/go-ipfs/" + CurrentVersionNumber + "/"

// CurrentCommit is the current git commit, this is set as a ldflag in the Makefile
var CurrentCommit string

Expand Down