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 readonly api to gateway #1389

Closed

Conversation

travisperson
Copy link
Member

Issue #1322

Added the ability to expose individual commands to a read-only style
handler that can be used on any http interface.

Added the read-only handler to the gateway.

Tests to come

@GitCop
Copy link

GitCop commented Jun 18, 2015

There were the following issues with your Pull Request

  • Commit: fa319e7
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>

Guidelines are available to help. Your feedback on GitCop is welcome on this issue


This message was auto-generated by https://gitcop.com

@jbenet jbenet added the backlog label Jun 18, 2015
Issue ipfs#1322

Added the ability to expose individual commands to a read-only style
handler that can be used on any http interface.

Added the read-only handler to the gateway.

License: MIT
Signed-off-by: Travis Person <[email protected]>
@travisperson travisperson force-pushed the feat/read-only-api-gateway branch from fa319e7 to 988c310 Compare June 18, 2015 01:48
License: MIT
Signed-off-by: Travis Person <[email protected]>
@travisperson
Copy link
Member Author

Reworking to instead add a read-only flag (maybe called gatewayAccessible) to the commands instead of a white list in handlers.go

@travisperson
Copy link
Member Author

I actually want some input on the idea above.

One rework of this is to add a GatewayAccess flag to each command, this will make it a bit easier to understand which commands can be used on the gateway.

I also think having a way to define which commands can be used on a given http interface would be nice, because it will allow us to later define restricted interfaces.

So right now I'm almost thinking of a mesh of these two.

This would leave to a way to pass in a whitelist to the NewReadOnlyHandler, which would be renamed to NewWhiteListedHandler.

Issue with this is I'm not sure how to get all the commands with the GatewayAccess flag set into the whitelist.

So for now, I'd like to get some ideas about the direction this should go.

@@ -71,6 +77,30 @@ func NewHandler(ctx cmds.Context, root *cmds.Command, allowedOrigin string) *Han
return &Handler{internal, c.Handler(internal)}
}

func NewReadOnlyHandler(ctx cmds.Context, root *cmds.Command, allowedOrigin string) *Handler {
Copy link
Member

Choose a reason for hiding this comment

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

problem with making two handlers is drift in the implementations.

maybe instead of creating two different handlers, make the original handler always check a list of available commands, and then we can just instantiate the read-only and read-write handlers by passing two different command lists.

that way the

if i.readOnly == true {
  if _, ok := readOnlyCmds[req.Command()]; !ok { ... }
}

can turn into

if _, ok := availableCmds[req.Command()]; !ok { ... }

This PR adds the ability to expose commands to the gateway.

Each commands handler uses a whitelist to expose the commands
on the http interface.

All commands listed under the root command are exposed by default
to the commands port.

Only commands with the `GatewayAccess` flag will be exposed on the
gateway.

License: MIT
Signed-off-by: Travis Person <[email protected]>
Update the test to include the command lists

Added test to to make sure an emptyu command list rejects requests

License: MIT
Signed-off-by: Travis Person <[email protected]>
@travisperson
Copy link
Member Author

@harlantwood @jbenet

Sorry for this taking so long.

If I can get a quick CR on this as well as a list of command/subcommands that need gateway access I think we could merge this sometime this weekend (maybe tomorrow).

@travisperson
Copy link
Member Author

(I think I may of accidentally overwrote one of my commits, I seem to be missing some code that lets it compile by adding the command lists to NewHandler)

License: MIT
Signed-off-by: Travis Person <[email protected]>
@@ -25,7 +29,13 @@ func NewGateway(conf GatewayConfig) *Gateway {
}
}

func (g *Gateway) ServeOption() ServeOption {
func (g *Gateway) ServeOption(cctx * commands.Context) ServeOption {
Copy link
Member

Choose a reason for hiding this comment

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

i dont think this is properly go fmt-ed ?

@jbenet
Copy link
Member

jbenet commented Jun 26, 2015

comments above, but this LGTM. i dont like the cmds.Context seeping into everything... but dont know an easier workaround atm.

for now, list of (readonly!!) cmds to have:

ipfs refs
ipfs object get
ipfs object stat
ipfs object data
ipfs object links
ipfs block get
ipfs block stat
ipfs name resolve
ipfs cat
ipfs ls

@travisperson
Copy link
Member Author

Ya agree on the context being moved around so much. I'll put that as a note and this weekend I may investigate the context a bit. I noticed there was a comment to replace the structure or get rid of it, and this PR definitely doesn't help with that idea.

I'll get those read-only commands added this afternoon.

@harlantwood
Copy link
Contributor

Awesome, thanks @travisperson.

License: MIT
Signed-off-by: Travis Person <[email protected]>
@wking
Copy link
Contributor

wking commented Jul 3, 2015

On Fri, Jun 26, 2015 at 02:17:47AM -0700, Juan Batiz-Benet wrote:

ipfs ls

Maybe ‘ipfs file ls’ too?

@harlantwood
Copy link
Contributor

I'll test this out for the dataviz case when it's ready.

@jbenet
Copy link
Member

jbenet commented Jul 7, 2015

Hey @travisperson were you able to take a look at not makign the cmds context expand in scope?

@travisperson
Copy link
Member Author

I took a quick look, but didn't really do too much digging yet.

@jbenet
Copy link
Member

jbenet commented Aug 2, 2015

ok, can someone with some gateway experience take care of this? it's been outstanding for months now... :(

@jbenet
Copy link
Member

jbenet commented Aug 7, 2015

cc @diasdavid


commandList := map[*commands.Command]bool{}

handler := NewHandler(commands.Context{}, corecommands.Root, "*", commandList)
Copy link
Contributor

Choose a reason for hiding this comment

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

@travisperson another option is to define corecommands.RootRO inside corecommands and use it here. This way, no need for extra commandList arguments elsewhere.

@jbenet
Copy link
Member

jbenet commented Aug 15, 2015

@rht could you take this one over?

rht added a commit to rht/go-ipfs that referenced this pull request Aug 15, 2015
Based on ipfs#1389

License: MIT
Signed-off-by: rht <[email protected]>
@rht rht mentioned this pull request Aug 15, 2015
rht added a commit to rht/go-ipfs that referenced this pull request Aug 15, 2015
Based on ipfs#1389

License: MIT
Signed-off-by: rht <[email protected]>
rht added a commit to rht/go-ipfs that referenced this pull request Aug 15, 2015
Based on ipfs#1389

License: MIT
Signed-off-by: rht <[email protected]>
@whyrusleeping
Copy link
Member

I beleive this can be closed now. @rht ?

@rht
Copy link
Contributor

rht commented Aug 16, 2015

@whyrusleeping I think so.

@jbenet jbenet closed this Aug 16, 2015
@jbenet jbenet removed the backlog label Aug 16, 2015
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
Based on ipfs/kubo#1389

License: MIT
Signed-off-by: rht <[email protected]>


This commit was moved from ipfs/kubo@dd99a70
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.

7 participants