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

Local Gateway option #5649

Merged
merged 9 commits into from
Jan 7, 2019
Merged

Local Gateway option #5649

merged 9 commits into from
Jan 7, 2019

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Oct 26, 2018

This is an alternative / more contained version of #4150

((todo) some gx stuff still needs fixing)

@magik6k magik6k requested a review from Kubuxu as a code owner October 26, 2018 00:37
@magik6k magik6k changed the title Feat/gateway nofetch Local Gateway option Oct 26, 2018
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.

(I meant "shepherd" but this also works)

@Voker57, I'd appreciate your review if you have time.

core/coreapi/coreapi.go Outdated Show resolved Hide resolved
core/corehttp/gateway.go Outdated Show resolved Hide resolved
@magik6k magik6k mentioned this pull request Oct 26, 2018
@ghost ghost assigned magik6k Oct 26, 2018
@ghost ghost added the status/in-progress In progress label Oct 26, 2018
@Voker57
Copy link
Contributor

Voker57 commented Oct 27, 2018

This does not include a lot of code from my PR that tries to make the API actually offline beyond stuff that readonly gateway uses, but it could work I guess if nobody tries to use it for something else, mine does not cover all cases either.

Also, I don't like "negating" config option, it's a bit confusing.

@magik6k
Copy link
Member Author

magik6k commented Oct 27, 2018

Also, I don't like "negating" config option, it's a bit confusing.

When adding options to the config, old nodes will use zero value for added fields, which in this case would break existing setups.

@Stebalien
Copy link
Member

When adding options to the config, old nodes will use zero value for added fields, which in this case would break existing setups.

We really need a comprehensive config system with layers. One has to exist...

@magik6k
Copy link
Member Author

magik6k commented Oct 27, 2018

Also, convincing the read only HTTP API to not fetch blocks is going to be harder, we should probably just implement the context hints.

@Stebalien Stebalien added status/blocked Unable to be worked further until needs are met and removed status/in-progress In progress labels Nov 20, 2018
@Stebalien
Copy link
Member

@magik6k what's your opinion on this? Should we block this on context hints due to the read-only API (given that it's exposed on the gateway)?

@magik6k
Copy link
Member Author

magik6k commented Nov 21, 2018

I don't see a nice way to work around this without context hints, so we can probably call this blocked for now. I'll try to put context hints near the top on my todo list though.

@Stebalien
Copy link
Member

SGTM.

@Voker57
Copy link
Contributor

Voker57 commented Nov 30, 2018

@magik6k

When adding options to the config, old nodes will use zero value for added fields, which in this case would break existing setups.

Isn't that handled by go-ipfs-config? Voker57/go-ipfs-config@9c41d63

@magik6k magik6k removed the status/blocked Unable to be worked further until needs are met label Jan 3, 2019
@ghost ghost added the status/in-progress In progress label Jan 3, 2019
@magik6k magik6k force-pushed the feat/gateway-nofetch branch 3 times, most recently from 183a42f to 972e4c9 Compare January 3, 2019 23:38
magik6k and others added 3 commits January 4, 2019 02:37
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k magik6k added the topic/gateway Topic gateway label Jan 4, 2019
@magik6k
Copy link
Member Author

magik6k commented Jan 4, 2019

Rebased

This covers the gateway + commands that support CoreAPI, few things are missed:

  • /get - might be hard to port, but doable
  • /dns - needs to get into coreapi (probably under .Name())
  • /ls - Can be rewritten to use coreapi, but that's going to be quite hard (it can be worked around manually for now)
  • /dag/get - needs to be ported to coreapi
  • /dag/resolve - same as above

@magik6k
Copy link
Member Author

magik6k commented Jan 4, 2019

Isn't that handled by go-ipfs-config? Voker57/go-ipfs-config@9c41d63

Nope, code in config/init.go only gets called on init. If values are missing from existing node config, they are set to zero value (which is false in this case). Using FetchBlocks would basically disable fetching blocks on gateway for all existing nodes (and other programs that create config.Config manually, expecting that (almost) zero-value config will just work).

License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@Stebalien Stebalien requested review from michaelavila and removed request for michaelavila January 5, 2019 20:06
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.

Small nits.

Now, on to naming... thinking about this a bit, FetchBlocks/NoFetchBlocks is kind of cryptic. Most users just care about content, not blocks. Maybe something like RetrieveContent (download?)? I'm not sure but we should probably avoid using the word "blocks" if possible.

core/corehttp/gateway_handler.go Outdated Show resolved Hide resolved
core/corehttp/gateway_handler.go Show resolved Hide resolved
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k
Copy link
Member Author

magik6k commented Jan 5, 2019

Now, on to naming...

It's currently Gateway.NoFetch in config, which shouldn't be too confusing imo. (the coreapi option name is different, but I guess it can be because it's 'lower level')

@Stebalien
Copy link
Member

It's currently Gateway.NoFetch in config, which shouldn't be too confusing imo. (the coreapi option name is different, but I guess it can be because it's 'lower level')

Ah. Fair enough.

@magik6k
Copy link
Member Author

magik6k commented Jan 7, 2019

Should be ready

(Depends on ipfs/iptb-plugins#11, ipfs/go-ipfs-config#19)

@Stebalien Stebalien merged commit a142aec into master Jan 7, 2019
@Stebalien Stebalien deleted the feat/gateway-nofetch branch January 7, 2019 18:06
@ghost ghost removed the status/in-progress In progress label Jan 7, 2019
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants