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

Fix #2335 - If IPFS_HOST is ipfs pass localhost to IPFSJS #2336

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

mbeacom
Copy link
Contributor

@mbeacom mbeacom commented Oct 3, 2018

Description

The goal of this PR is to resolve a bug encountered while attempting to communicate with the local IPFS docker instance from IPFS JS by defaulting to localhost for IPFS JS if IPFS_HOST == 'ipfs'.

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)

ipfs, context, settings

Testing

Locally

Refers/Fixes

Fix #2335

@mbeacom mbeacom added bug This is something that isn't working as intended. backend This needs backend expertise. labels Oct 3, 2018
@mbeacom mbeacom self-assigned this Oct 3, 2018
@codecov
Copy link

codecov bot commented Oct 3, 2018

Codecov Report

Merging #2336 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2336      +/-   ##
==========================================
+ Coverage   27.75%   27.75%   +<.01%     
==========================================
  Files         145      145              
  Lines       11642    11643       +1     
  Branches     1568     1568              
==========================================
+ Hits         3231     3232       +1     
  Misses       8299     8299              
  Partials      112      112
Impacted Files Coverage Δ
app/app/context.py 0% <ø> (ø) ⬆️
app/app/settings.py 79.31% <100%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe62dc0...f0f8224. Read the comment docs.

@jasonrhaas
Copy link
Contributor

Also we need to provide a ipfs config file to handle the CORS issues. What I did in my branch is this:

  ipfs:
    image: ipfs/go-ipfs:release
    restart: unless-stopped
    volumes:
      - './ipfsconfig.json:/data/ipfs/config'
      - 'ipfsexport:/export'
      - 'ipfsdata:/data/ipfs'
    ports:
      - "4001:4001"
      - "4002:4002"
      - "5001:5001"
      - "8080:8080"

The ipfsconfig.json contains the config file for allowing us to use IPFS from the localhost browser.

@mbeacom
Copy link
Contributor Author

mbeacom commented Oct 3, 2018

@jasonrhaas I think manually defining the config would cause some problems, honestly. We want the local instance to provision itself and assign newly initialized Identity.PeerID|PrivKey entries in the json config. If the config is present: https://github.com/ipfs/go-ipfs/blob/master/bin/container_daemon#L17, the ipfs init step is skipped.

@jasonrhaas
Copy link
Contributor

jasonrhaas commented Oct 3, 2018

@mbeacom I'm with you, I did not want to have to pass a config file for IPFS. I would prefer to be able to pass some ENV vars, or command line options. I looked into this, and I don't see any way of doing it, outside of defining our own Dockerfile for IPFS, or config file. We need IPFS to be set up like this for local testing:

https://github.com/INFURA/tutorials/wiki/IPFS-and-CORS#configuration-of-cors

Changing the ALLOW HOSTS to the docker host if you can find it, or *.

What I did was run those commands, which altered the config file inside of the container. Then I took that and saved the output to ipfsconfig.json.

If you can figure out a better way to set up IPFS with those settings I'm definitely open to it.

@mbeacom
Copy link
Contributor Author

mbeacom commented Oct 4, 2018

@jasonrhaas I came to the same conclusion. It's fairly frustrating there's no entry options to pass custom config properties to the CLI or the docker entry script 🤔 - Maybe they'll be open to an upstream change to allow the container_start.sh script to accept input config options. It's either that or we fork it for now, make the change, build a new image, adjust our docker-compose.yml to use that image, and ultimately wait for ipfs to either merge the change or deny it xD

@jasonrhaas
Copy link
Contributor

Yes I'm surprised no one has raised an issue about this in the repo! The closest one I could find is this one, from 2014.

ipfs/kubo#251

The way they have their configuration set up, its just a giant JSON file with a lot of ugly field names. I guess that's why they don't have ENV var support.

However, it would be fairly easy to add support for some ENV vars, by just taking them in through docker, and executing some commands here:

https://github.com/ipfs/go-ipfs/blob/master/bin/container_daemon#L21

The commands would only run on the first run though, and then it would pull the file from the data volume, which is probably not what we want.

Going full circle, is there some version of saving a config file that would not be prone to breaking? Referencing your comment above about Identity.PeerID|PrivKey. Providing a custom config file isn't the cleanest solution, but right now it is the easiest/fastest option.

@mbeacom
Copy link
Contributor Author

mbeacom commented Oct 4, 2018

The main reason I referenced those keys above is because during ipfs init, these values are determined based on the initial provisioning. If we provided a cookiecutter config for the local ipfs container, the init step would never be reached in container_start and might ultimately bomb out due to duplicate and/or invalid keys. My thoughts on approaching this:

Ideal, but probably slow:

PR changes to container_start that would check for either envvars or additional starting values passed via entry CMD and if it starts with a specific prefix, loop through those values against: ipfs config ... following ipfs init

Fast:

Extend (or recreate) go-ipfs docker image in our repo and either manually set cors by overwriting the startup script OR the approach above and define the necessary items in our base .env or directly on in the compose yaml via env

@jasonrhaas
Copy link
Contributor

jasonrhaas commented Oct 4, 2018

Lets go with the "Fast" approach, using the 2nd method (env vars). After that we can submit an issue or PR to the main repo and try to get them to merge it in if it makes sense for the broader community.

So then we should:

  • Fork the repo
  • Create a new branch
  • Edit the container_daemon script to handle our new ENV vars
  • If the ENV vars are found, execute the appropriate ipfs config commands
  • Set the ENV vars in the docker-compose.yml file

@jasonrhaas
Copy link
Contributor

@mbeacom Want me to try to take care of this one? I need it for testing on the other project anyway.

@jasonrhaas
Copy link
Contributor

jasonrhaas commented Oct 9, 2018

I temporarily got around this in my test script by running this as part of my fresh_start.sh script:

# Enable CORS for local IPFS testing
docker-compose exec ipfs sh -c 'ipfs config --json API.HTTPHeaders.Access-Control-Allow-Origin '\''["*"]'\'''
docker-compose exec ipfs sh -c 'ipfs config --json API.HTTPHeaders.Access-Control-Allow-Methods '\''["PUT", "GET", "POST"]'\'''
docker-compose restart ipfs

Above that I also blow away the IPFS volumes, so that it starts from scratch. Otherwise it will just read in the existing settings.

@mbeacom mbeacom merged commit 64f594d into master Nov 19, 2018
@mbeacom mbeacom deleted the fix-2335-ipfs-localhost branch November 19, 2018 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend This needs backend expertise. bug This is something that isn't working as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local testing with IPFS does not work from browser
4 participants