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

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Voker57
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Kubuxu

Kubuxu Aug 18, 2017

Member

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

Member

Kubuxu commented Aug 18, 2017

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

@Voker57

This comment has been minimized.

Show comment
Hide comment
@Voker57

Voker57 Aug 18, 2017

Contributor

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

Contributor

Voker57 commented Aug 18, 2017

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

@magik6k

This comment has been minimized.

Show comment
Hide comment
@magik6k

magik6k Aug 18, 2017

Member

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

Member

magik6k commented Aug 18, 2017

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

@whyrusleeping

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

This comment has been minimized.

Show comment
Hide comment
@magik6k

magik6k Aug 28, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@Stebalien

Stebalien Aug 29, 2017

Contributor

How about something like: #4180

Contributor

Stebalien commented Aug 29, 2017

How about something like: #4180

@lgierth

This comment has been minimized.

Show comment
Hide comment
@lgierth

lgierth Aug 29, 2017

Member

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.

Member

lgierth 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.

@lgierth

This comment has been minimized.

Show comment
Hide comment
@lgierth

lgierth Aug 29, 2017

Member

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

Member

lgierth commented Aug 29, 2017

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

@Voker57

This comment has been minimized.

Show comment
Hide comment
@Voker57

Voker57 Aug 29, 2017

Contributor

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

Contributor

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

@lgierth

This comment has been minimized.

Show comment
Hide comment
@lgierth

lgierth Aug 29, 2017

Member

Yeah you're right that's already useful

Member

lgierth commented Aug 29, 2017

Yeah you're right that's already useful

@whyrusleeping

This comment has been minimized.

Show comment
Hide comment
@whyrusleeping

whyrusleeping Sep 6, 2017

Member

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

Member

whyrusleeping commented Sep 6, 2017

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

@Voker57 Voker57 changed the title from 'Offline' gateway option to 'FetchBlocks' gateway option Sep 9, 2017

@Voker57

This comment has been minimized.

Show comment
Hide comment
@Voker57

Voker57 Sep 9, 2017

Contributor

Renamed option to FetchBlocks and moved to config.

Contributor

Voker57 commented Sep 9, 2017

Renamed option to FetchBlocks and moved to config.

@nicola

This comment has been minimized.

Show comment
Hide comment
@nicola

nicola Nov 14, 2017

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@Voker57

Voker57 Dec 31, 2017

Contributor

@nicola yes.

Contributor

Voker57 commented Dec 31, 2017

@nicola yes.

@whyrusleeping whyrusleeping requested review from Stebalien, kevina and frrist Dec 31, 2017

Voker57 added some commits Aug 30, 2017

Refactor GatewayOption to pass in options via GatewayConfig
License: MIT
Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
Add option to construct CoreAPI which uses only offline DAGService
License: MIT
Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
Add 'FetchBlocks' argument to Gateway constructor
If FetchBlocks=false, don't fetch blocks which are not
already present in storage.

Add Gateway.FetchBlocks key to config.

License: MIT
Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment