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

gateway: make API commands configurable #5565

Closed
wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 5, 2018

This is for Delegated Routing in js-libp2p (ipfs/js-ipfs#1459 & libp2p/js-libp2p#120) and supersedes #4595.

It lets us configure the commands available through :8080/api/v0/ -- for delegated routing, we would then e.g. expose swarm/connect, refs, dht/findprovs, and dht/findpeer.

By default, Gateway.APICommands is empty, which means go-ipfs will use the default :8080 commands.

> ipfs init
> ipfs &
> curl localhost:5001/api/v0/config?arg=Gateway.APICommands | jq -r .Value
[]
> curl localhost:8080/api/v0/commands | jq -r .Subcommands[].Name
dns
version
commands
cat
get
dag
refs
block
ls
resolve
name
object
> killall ipfs

> ipfs config Gateway.APICommands --json '["config"]'
> ipfs daemon &
> curl localhost:8080/api/v0/commands | jq -r .Subcommands[].Name
config
commands
> killall ipfs

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost ghost added topic/gateway Topic gateway topic/api Topic api labels Oct 5, 2018
@ghost ghost requested review from keks, Stebalien and hannahhoward October 5, 2018 02:49
@ghost ghost requested a review from Kubuxu as a code owner October 5, 2018 02:49
@ghost ghost self-assigned this Oct 5, 2018
@ghost ghost added the status/in-progress In progress label Oct 5, 2018
@ghost
Copy link
Author

ghost commented Oct 5, 2018

Whoops, totally forgot updating the go tests. otherwise still happy about review :)

@ghost ghost force-pushed the feat/refactor-gateway-api branch 2 times, most recently from 24aa7ab to 0545005 Compare October 5, 2018 03:24
@ghost
Copy link
Author

ghost commented Oct 5, 2018

I am pretty sure that sharness t0236-cli-api-dns-resolve.sh is failing on Jenkins because of a nc or ipfs process leaking on the worker.

License: MIT
Signed-off-by: Lars Gierth <larsg@systemli.org>
@ghost ghost force-pushed the feat/refactor-gateway-api branch from 0545005 to 47c78e2 Compare October 5, 2018 11:38
@ghost
Copy link
Author

ghost commented Oct 15, 2018

This one is ready for review

License: MIT
Signed-off-by: keks <keks@cryptoscope.co>
@ghost ghost assigned keks Oct 16, 2018
}
}

if commands {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you do this inside the loop (at line 64, where you currently set commands to true)?


nextOut := new(cmds.Command)
*nextOut = *nextIn
nextOut.Subcommands = map[string]*cmds.Command{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, so you unconditionally create a new subcommand mal. That confuses me. Because if you have e.g. both dag/get and dag/resolve in allowed, you would overwrite dag and delete dag/get when adding dag/resolve.
Checking the tests now to see why this passes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the tests and now they fail. Could you fix this?

@ghost
Copy link
Author

ghost commented Jan 20, 2019

It will be much simpler to back delegated routing by a modified branch of go-ipfs, so I'm closing this PR.

  • Guarding security-relevant commands with configuration is more risky than simply avoiding code paths leading to them. Safest to just keep this a codified map as is.
  • The configuration code I wrote here turned out pretty complicated, best to just throw it away.
  • There's additional potential for problems whenever we change the set of commands exposed by default. Every new config option also potentially complicates future repo migrations, and this one feels like it does so significantly more than others.

@ghost ghost closed this Jan 20, 2019
@ghost ghost removed the status/in-progress In progress label Jan 20, 2019
@ghost ghost deleted the feat/refactor-gateway-api branch January 20, 2019 20:56
@lidel
Copy link
Member

lidel commented Jun 24, 2019

@lgierth did this ever land in some other PR? was there anything more to the story?

We were trying to use Gateway.APICommands in context of ipfs/js-ipfs#2155 (comment) but it did not work, and @mburns had to redirect to API port at Nginx level.

@Stebalien
Copy link
Member

We never did.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/api Topic api topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants