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

IPIP-280: App Conventions for HTTP Gateway Detection #280

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

markg85
Copy link

@markg85 markg85 commented May 13, 2022

This should help other application developers to have an idea of what's needed to add IPFS support to their application.

cc @Stebalien and @autonome Conversations with mainly you two have led to this spec being implemented as is written here in ffmpeg.

This should help other application developers to have an idea of what's needed to add IPFS support to their application.
@lidel lidel mentioned this pull request Jun 7, 2022
18 tasks
@lidel lidel changed the title Add INTEGRATION document IPIP: IPFS_GATEWAY integration strategy for apps Jul 4, 2022
@lidel lidel self-requested a review July 19, 2022 16:28
@markg85
Copy link
Author

markg85 commented Jul 23, 2022

Ping. What's still required for this document to get merged?
Do note that the document is called INTEGRATION.md because it's intended to be used by those "integrating" IPFS support in their applications.

Don't confuse this with the document the add the gateway file in the IPFS data folder. While this document "sort of refers" to it in terms of functionality, it doesn't spec - nor should it - how that file is created. That detail should be of no concern to those integrating IPFS support.

@pimterry
Copy link

Hi @markg85, two quick comments on this from some related discussion in ipfs/in-web-browsers#197:

  • Could we extend this just slightly, to also specify gateway detection for browser JS environments? This is a common use case, which I think could easily closely match what you're suggesting here, by using a global window.IPFS_GATEWAY env var to allow browser and browser extensions to inject gateway configuration into JS apps. Similarly, those apps should use that configuration if available, or use the fallback gateway if not.

  • This spec covers gateways only, but that's not the only way people may want to integrate IPFS into their applications. There's many interesting use cases where 3rd-party applications might want to interact with the network more generally, by embedding a full IPFS node. When doing so, it would be useful to provide very similar mechanisms to detect a user's preferred network configuration (particularly for web applications - see As a user, there is no mechanism to control the IPFS networking of a webapp I use in-web-browsers#197 for lots more context). Of course I don't want to derail this PR, which can happily proceed ignoring all that, but I think this would fit well as an extension to this 'Integration' guide later on. Do you think that makes sense? How about if I open up a PR against your branch here to start discussion on that too, but independently?

@Jorropo Jorropo self-assigned this Aug 30, 2022
@Jorropo Jorropo requested review from Jorropo and removed request for lidel August 30, 2022 16:46
@lidel lidel marked this pull request as draft September 15, 2022 19:00
lidel added a commit to whizzzkid/ipfs-geoip that referenced this pull request Oct 18, 2022
ipfs/specs#280

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to whizzzkid/ipfs-geoip that referenced this pull request Oct 18, 2022
ipfs/specs#280

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to ipfs-shipyard/ipfs-geoip that referenced this pull request Oct 18, 2022
* Using async-await
* feat: support IPFS_GATEWAY env

ipfs/specs#280

Signed-off-by: Marcin Rataj <lidel@lidel.org>

Signed-off-by: Marcin Rataj <lidel@lidel.org>
Co-authored-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel changed the title IPIP: IPFS_GATEWAY integration strategy for apps IPIP-280: IPFS_GATEWAY integration strategy for apps Oct 26, 2022
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@markg85 time to revisit this, now that we have IPIP process. some pointers below :)

INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
@markg85
Copy link
Author

markg85 commented Nov 19, 2022

Hi @markg85, two quick comments on this from some related discussion in ipfs/in-web-browsers#197:

  • Could we extend this just slightly, to also specify gateway detection for browser JS environments? This is a common use case, which I think could easily closely match what you're suggesting here, by using a global window.IPFS_GATEWAY env var to allow browser and browser extensions to inject gateway configuration into JS apps. Similarly, those apps should use that configuration if available, or use the fallback gateway if not.

I get the mindset here and it would help a lot! Not just with apps using IPFS but also with js libraries (think for example video.js) where we'd like to have IPFS integration in at some point in time too. Having a window.IPFS_GATEWAY would surely help!

But.. how?

When IPFS Companion is used, injecting a variable is doable.
But for IPFS integration in brave, that would be a code change on the brave side.

The downside is another point of metric collection to profile people. And for that reason alone i don't think this variable would be accepted. Even if we would all like it and add it to the spec, the browser security minded folks would probably have an issue with it.

@lidel do you have an opinion on this?

  • This spec covers gateways only, but that's not the only way people may want to integrate IPFS into their applications. There's many interesting use cases where 3rd-party applications might want to interact with the network more generally, by embedding a full IPFS node. When doing so, it would be useful to provide very similar mechanisms to detect a user's preferred network configuration (particularly for web applications - see As a user, there is no mechanism to control the IPFS networking of a webapp I use in-web-browsers#197 for lots more context). Of course I don't want to derail this PR, which can happily proceed ignoring all that, but I think this would fit well as an extension to this 'Integration' guide later on. Do you think that makes sense? How about if I open up a PR against your branch here to start discussion on that too, but independently?

Usually i'd say "yes, go for it!" but in this case, waiting is probably the smarter move. If you'd go for that you'd have a spec wip branch on top of my wip branch. That's a little too much wip. Wait till this spec lands (it's mostly there) before you consider making a spec on top of this one.

@markg85
Copy link
Author

markg85 commented Nov 19, 2022

@markg85 time to revisit this, now that we have IPIP process. some pointers below :)

Sweet!
Will update it all in the next couple days. Thank you for all the pointers, much appreciated!

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Quick feedback:

  • make this Kubo-agnostic
  • document configuration location on MS Windows and macOS
  • ensure manually added entries are not removed by starting/stopping local node
  • allow for multiple local gateways to co-exist, and not "steal" traffic

INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
INTEGRATION.md Outdated Show resolved Hide resolved
This was referenced Dec 15, 2022
@dirkf
Copy link

dirkf commented Apr 19, 2023

For Windows, it's not clear that the Roaming config (%APPDATA%) is suitable. You may not want your gateway setting on machine A (eg, with no local proxy) to be synced to machine B (eg, with a local proxy that has set a global gateway file). Perhaps use %LOCALAPPDATA%, or let it take precedence?

@dirkf
Copy link

dirkf commented Apr 19, 2023

Shouldn't the spec should say what text format is used? Is the filename in the file limited to ASCII, or otherwise is the file UTF-8, or in the encoding for the system locale, the system file encoding, etc?

@dirkf
Copy link

dirkf commented Apr 19, 2023

What happens if a gateway adds its URL to the global gateways and crashes without removing it (say, power loss)? Should there a clean-up system and/or some way to separate admin-specified defaults at the end of the file from dynamic entries at the start of the file?

@lidel lidel requested a review from a team as a code owner October 3, 2023 23:51
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@markg85 I've rebased and split IPIP into spec and IPIP memo, integrated feedback from @dirkf and others, and turned this into a spec we would be sending to applications to simplify integrations like ones in ffmpeg, curl etc.

lmk if I've removed something that is still relevant, or if you feel we should include something new.

Comment on lines +58 to +68
5. Try OS-specific paths
- Windows
1. `%LOCALAPPDATA%/ipfs/gateway` (local user)
2. `%APPDATA%/ipfs/gateway` (roaming user)
3. `%PROGRAMDATA%/ipfs/gateway` (global)
- macOS
1. `$HOME/Library/Application Support/ipfs/gateway` (user)
2. `/Library/Application Support/ipfs/gateway` (global)
- Linux
1. `$HOME/.config/ipfs/gateway` (user)
2. `/etc/ipfs/gateway` (global)
Copy link
Member

@lidel lidel Oct 3, 2023

Choose a reason for hiding this comment

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

@markg85 I'm not feeling strongly about having OS-specific paths or ~/.config and /etc: keep or remove?
afaik nothing implements these atm, but does not hurt to have them is spec to future-proof things.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not feeling strong about those either.
They don't hurt or complicate things. The only reason i'd lean towards keeping it is because they clarify the windows part, mac/linux is effectively handled by points 1-4 as well.

So if it doesn't hurt, keep it. Else remove it.

Comment on lines +70 to +75
When `gateway` file is present, the file contents MUST be interpreted as line
separated (`\n` or `\r\n`) `text/plain` file.

The first line of `gateway` file MUST be a valid `http://` or `https://` URL.

Implementations that only need one URL SHOULD use the first one and ignore the rest of the file.
Copy link
Member

@lidel lidel Oct 4, 2023

Choose a reason for hiding this comment

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

@markg85 I wrote this bit naively, and now I realized that having more than one URL will cause issues in curl (code).

Any concerns if we simplify the spec and state that only one URL is allowed in the gateway file?

Copy link
Author

Choose a reason for hiding this comment

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

Ohh ouch!

For this spec. I'd keep it at a first line requirement but not limited to 1 line. Aka, it's fine as-is.

Curl needs some attention to catch the multiline scenario regardless of this spec. As just any other data after the first line is gonna break it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, my understanding is this problem was solved in curl/curl@859e88f#diff-1fa338b6a8d4b9a9ffd2243527ac4951c2576cb32dc28440f36f4f59e60643e2R785, the gateway file can now have more than one line, curl ignores everything after the first one.

Copy link
Author

Choose a reason for hiding this comment

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

Yes!

I'm leaving it up to your digression if you want to follow the curl 8.4.0 approach (meaning the spec needs to be reworded to 1 line and 1 only) or the curl 8.5.0 approach (first line must be the gateway, rest is ignored).

I would go for the 8.5.0 approach (aka, the spec as-written is ok). Given the other IPFS fixes in 8.5.0, that release is certainly the one to recommend for IPFS usage. Also, it's about to be released on Dec. 6th 2023.

@lidel lidel changed the title IPIP-280: IPFS_GATEWAY integration strategy for apps IPIP-280: Conventions for HTTP Gateway Detection Oct 4, 2023
@lidel lidel changed the title IPIP-280: Conventions for HTTP Gateway Detection IPIP-280: App Conventions for HTTP Gateway Detection Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏃 In Progress
Development

Successfully merging this pull request may close these issues.

7 participants