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 SetAllowedOrigins #2021

Merged
merged 2 commits into from
Jan 22, 2016
Merged

Fix SetAllowedOrigins #2021

merged 2 commits into from
Jan 22, 2016

Conversation

rht
Copy link
Contributor

@rht rht commented Dec 1, 2015

Fix #1966

@jbenet jbenet added the status/in-progress In progress label Dec 1, 2015
@rht rht added the need_tests label Dec 1, 2015
@rht rht force-pushed the fix/corsconfig branch 2 times, most recently from 2433100 to b6a5531 Compare December 1, 2015 12:33
@rht
Copy link
Contributor Author

rht commented Dec 1, 2015

I tested this by simulating the race condition when the ro gateway overtakes the api:

  1. On current master, deliberately launch the API 10s after the gateway. Get 403 resp code when uploading files on localhost:5001/webui
  2. On this branch, deliberately launch the API 10s after the gateway. Uploading files on localhost:5001/webui works.

@rht rht added RFCR and removed need_tests labels Dec 1, 2015
@rht rht mentioned this pull request Dec 1, 2015
14 tasks
@jbenet
Copy link
Member

jbenet commented Dec 1, 2015

@rht can we do that in less than 10s? make a sharness test?

@jbenet
Copy link
Member

jbenet commented Dec 1, 2015

thanks for picking this up @rht

@rht
Copy link
Contributor Author

rht commented Dec 1, 2015

The launch order change & time delay to ensure the order are hardcoded. It is not sensible to expose this to cli if the reason is only for sharness testing.

To reproduce the race test, move https://github.com/ipfs/go-ipfs/blob/c8e48456308cc68556f0804e8ecff9ef4ea21e32/cmd/ipfs/daemon.go#L253-L257 to after https://github.com/ipfs/go-ipfs/blob/c8e48456308cc68556f0804e8ecff9ef4ea21e32/cmd/ipfs/daemon.go#L268, and put <-time.After(time.Second*10) in between.

@jbenet
Copy link
Member

jbenet commented Dec 2, 2015

@rht could a test case run it a few times to attempt to trigger a race condition? or could do a golang test with a mock-- i want to make sure this does not become a problem in the future. we have many less tests than i would like as it is.

@rht rht force-pushed the fix/corsconfig branch 2 times, most recently from 7d88d5a to f98f7b1 Compare December 18, 2015 16:11
@whyrusleeping
Copy link
Member

couldnt we test for this by just building the binary we use for sharness tests with -race?

License: MIT
Signed-off-by: rht <[email protected]>
@rht rht force-pushed the fix/corsconfig branch 2 times, most recently from dba2816 to b67e220 Compare January 21, 2016 12:48
@rht
Copy link
Contributor Author

rht commented Jan 21, 2016

Just fixed this test.
ci errs are orthogonal to the part this pr fixes.

To explain how the test checks for race condition, currently, if the api (5001) starts before the gateway (8080), the curl command that works has to be curl -X GET -H "Origin:http://localhost:5001" -I http://localhost:5001/api/v0/version (api) and curl -X GET -H "Origin:http://localhost:5001" -I http://localhost:8080/api/v0/version (gateway).

@rht
Copy link
Contributor Author

rht commented Jan 22, 2016

RFCR, for this has issue count of 3: #1883, #1839, #1839.

@jbenet
Copy link
Member

jbenet commented Jan 22, 2016

this makes sense, thanks

jbenet added a commit that referenced this pull request Jan 22, 2016
@jbenet jbenet merged commit ffe6c85 into master Jan 22, 2016
@jbenet jbenet deleted the fix/corsconfig branch January 22, 2016 07:03
@MichaelMure MichaelMure mentioned this pull request Mar 6, 2016
@chrber
Copy link

chrber commented Aug 17, 2017

Hi,
I had this problem with my installation. I have been able to fix this in the meantime, as the description here helped. However, I have installed ipfs on a Debian wheezy.

Agentenversion go-ipfs/0.4.10/
Protokollversion ipfs/0.1.0

Am I wrong in thinking that this should be actually already default in 0.4.10? Is this maybe a regression?

I see it should be there, but why do I have this issue still

git tag --contains b970681
...
v0.4.10
v0.4.10-rc1
v0.4.11-pre
....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants