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

'FetchBlocks' gateway option #4150

Closed
wants to merge 2 commits into from
Closed

Conversation

Voker57
Copy link
Contributor

@Voker57 Voker57 commented Aug 17, 2017

If FetchBlocks=false, don't fetch blocks which are not already present in storage.

Also add Gateway.FetchBlocks option

This mode can be useful for creating gateways which are supposed to serve only chosen content.

@Kubuxu
Copy link
Member

Kubuxu commented Aug 18, 2017

There is already ipfs daemon --offline. Is it not good enough?

@Voker57
Copy link
Contributor Author

Voker57 commented Aug 18, 2017

@Kubuxu --offline will also disable fetching via API. I want only gateway to be restricted.

@magik6k
Copy link
Member

magik6k commented Aug 18, 2017

I think it might be better to implement this as a config option

@@ -148,6 +149,7 @@ Headers.
cmds.BoolOption(initOptionKwd, "Initialize ipfs with default settings if not already initialized").Default(false),
cmds.StringOption(routingOptionKwd, "Overrides the routing option").Default("dht"),
cmds.BoolOption(mountKwd, "Mounts IPFS to the filesystem").Default(false),
cmds.BoolOption(offlineGatewayKwd, "Do not fetch blocks because of gateway requests").Default(false),
Copy link
Member

Choose a reason for hiding this comment

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

s/because of/for/

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This looks pretty reasonable to me. My only concern is that we don't have a solid general way of setting up configuration for the gateway. I hesitate to add more flags to ipfs daemon without having a good plan for future gateway configuration options, every flag we add is really hard to remove (has to go through deprecation period, documentation, etc) and flags suddenly being gone makes future documentation people might read confusing.

@Voker57 @Kubuxu @magik6k @lgierth can we try and put together some ideas for this?

@magik6k
Copy link
Member

magik6k commented Aug 28, 2017

IMO daemon already has too many flags (--enable-gc as one example that could be in config). Gateway already has it's config scope, this could be done in config as Gateway.Offline. I'd say that all options that make sense in live deployments should be in config.

@Stebalien
Copy link
Member

How about something like: #4180

@ghost
Copy link

ghost commented Aug 29, 2017

Phew I recovered it -- deleting my two comments above.


Good idea 👍

Agreed that a config option is desirable over a command flag, and we'll also want to refactor the gateway a bit. It should get a GatewayOpts struct, similar to the HostOpts struct I implemented in libp2p/go-libp2p#197

I'll put up to debate whether "offline" is a good name though. It already means a couple of different things in go-ipfs. This option seems spiritiually related to the ipfs add --local and ipfs name resolve --local flags, in that it only operates locally (apart form IPNS lookups I guess). I'm not ready to say one is better of the other, just marking this as an important bit. The reason for it's importance is probably outside of this PR and more in the already existing meanings of "offline". It's important the proposed feature fits in nicely with the existing modes of connectedness and disconnectedness and doesn't unneccessarily complicate them.

@ghost
Copy link

ghost commented Aug 29, 2017

Btw, mentioning the --local flag, I noticed that its description in ipfs --help is completely inaccurate. See #4182.

@Voker57
Copy link
Contributor Author

Voker57 commented Aug 29, 2017

@lgierth Isn't GatewayConfig already doing same job as proposed GatewayOpts? https://github.com/ipfs/go-ipfs/blob/master/core/corehttp/gateway.go#L14

@ghost
Copy link

ghost commented Aug 29, 2017

Yeah you're right that's already useful

@whyrusleeping
Copy link
Member

Maybe the better term would be NoFetch as a field inside the gateway options?

@Voker57 Voker57 changed the title 'Offline' gateway option 'FetchBlocks' gateway option Sep 9, 2017
@Voker57
Copy link
Contributor Author

Voker57 commented Sep 9, 2017

Renamed option to FetchBlocks and moved to config.

@nicola
Copy link
Member

nicola commented Nov 14, 2017

I want this so badly :(

I want to host an IPFS node on my machine, but I don't want people to get content from hashes that I don't allow.. :/ Does this fix that?

@Voker57
Copy link
Contributor Author

Voker57 commented Dec 31, 2017

@nicola yes.

@Voker57 Voker57 force-pushed the feat/offline-gateway branch 2 times, most recently from 7d2dc12 to c83b869 Compare October 10, 2018 12:23
@Kubuxu
Copy link
Member

Kubuxu commented Oct 10, 2018

Where is the option handled in the gateway? I see it only added to GatewayOptions.

@Voker57
Copy link
Contributor Author

Voker57 commented Oct 10, 2018

Sorry, I screwed up a rebase. Will update.

@Kubuxu
Copy link
Member

Kubuxu commented Oct 10, 2018

Also:

+ ./bin/gx deps dupes

+ test -z package go-ipfs-config imported as both QmSoYrBMibm2T3LupaLuez7LPGnyrJwdRxvTfPUyCp691u and QmQwsBSJXHs3nHBLPJKd1iBNpW9uJ9acSpqbbjafvNJQc3

and as tests passed on circleci and travis, they seem to be too weak. I would recommend using IPTB to setup 2 node cluster.

@Voker57 Voker57 force-pushed the feat/offline-gateway branch 3 times, most recently from 4de08dd to a1920ac Compare October 11, 2018 10:46
@Voker57
Copy link
Contributor Author

Voker57 commented Oct 11, 2018

Fixed the PR and improved the tests.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

@magik6k or @schomatis, would one of you see this to completion (i.e., handle reviews)? This will need a bit more work to be a bit less invasive but it would be really nice to have it.

}

// NewCoreAPI creates new instance of IPFS CoreAPI backed by go-ipfs Node.
func NewCoreAPI(n *core.IpfsNode) coreiface.CoreAPI {
api := &CoreAPI{n}
func NewCoreAPI(n *core.IpfsNode, offlineMode bool) coreiface.CoreAPI {
Copy link
Member

Choose a reason for hiding this comment

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

This is really not the right place to configure 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.

why not? I don't see how @magik6k solution is better, except for modifying less code.

Copy link
Member

Choose a reason for hiding this comment

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

In retrospect, it's probably a reasonable place to do this (although I would have used functional options). Really, I was reacting to the fact that the CoreAPI needed to store a separate DAGService (I don't want it to turn into yet another "IpfsNode"). However, it turns out that we have to do that for sessions (no way around that as far as I can tell given the current sessions design) so this really isn't an issue.

Copy link
Member

Choose a reason for hiding this comment

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

CoreAPI is what will eventually be exposed as the interface to go-ipfs-*, so we want to make sure it's designed fairly well. The offline mode thing could be interpreted as a 'global option', and in CoreAPI we want to:

  • Hide all options and use sane defaults (mostly done with functional options)
  • Have a standard way to set them on the interface (for global options applying to all commands this is probably going to be in form of context hints)

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 also define a set of functional options for use with CoreAPI constructors, but I don't really like that as some implementations such as RPC will likely need to establish a new connection if user needs a global option for one command but not others

)

var log = logging.Logger("core/coreapi")

type CoreAPI struct {
node *core.IpfsNode
dag ipld.DAGService
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be a footgun. Now the CoreAPI needs to make sure to use it's dag instead of the global one.

Maybe #4009? This keeps on haunting us and I've yet to see a better proposal.

@magik6k
Copy link
Member

magik6k commented Oct 25, 2018

I'll handle this

@magik6k magik6k mentioned this pull request Oct 26, 2018
@Voker57 Voker57 force-pushed the feat/offline-gateway branch 2 times, most recently from bb2d521 to e711316 Compare December 1, 2018 12:17
@Voker57
Copy link
Contributor Author

Voker57 commented Jan 4, 2019

Rebased and updated to use new API. It is now similar to #5649 but more limited in scope: will still resolve IPNS fully, which is my desired behavior. Not sure how to cover this by tests, ones included in this PR would still pass but IPNS will not be queried fully with #5649.

License: MIT
Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
License: MIT
Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Right, this option shouldn't prevent IPNS from working.

The Api.FetchBlocks option also needs to be set for commands as some are accessible through the gateway port. There are also some other fixes in the other PR that would need to be ported here.

I'll import the FetchBlocks api option to #5649

@@ -217,6 +216,10 @@ func (api *CoreAPI) WithOptions(opts ...options.ApiOption) (coreiface.CoreAPI, e

}

if !settings.FetchBlocks {
subApi.dag = dag.NewDAGService(bserv.New(subApi.blockstore, offlinexch.Exchange(subApi.blockstore)))
Copy link
Member

Choose a reason for hiding this comment

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

settings.FetchBlocks needs to set exchange/blocks too, otherwise it still will be possible to use gateway to fetch stuff from the network with things like /api/v0/block/get


id "gx/ipfs/QmRBaUEQEeFWywfrZJ64QgsmvcqgLSK3VbvGMR2NM2Edpf/go-libp2p/p2p/protocol/identify"
)

type GatewayConfig struct {
Headers map[string][]string
Writable bool
FetchBlocks bool
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, leftover.

@magik6k
Copy link
Member

magik6k commented Jan 4, 2019

IPNS should now work in #5649, can you check if it works for you?

@Voker57
Copy link
Contributor Author

Voker57 commented Jan 4, 2019

IPNS should now work in #5649, can you check if it works for you?

yes, now this PR works like I need it. I'm closing this one.

@Voker57 Voker57 closed this Jan 4, 2019
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.

6 participants