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

Too many redirects #139

Open
sacdallago opened this issue Oct 23, 2016 · 14 comments
Open

Too many redirects #139

sacdallago opened this issue Oct 23, 2016 · 14 comments
Labels

Comments

@sacdallago
Copy link
Member

sacdallago commented Oct 23, 2016

Hey guys,

still working on the final deployment. Apart from it being hard to judge if stuff is not working because it's not working or it's not working because something is wrong on the deployment, I have one issue which I'm pretty sure is a mixture of how the app needs to be deployed and how you have implemented things.

If you try to go to the pokedex on htpps://predictemall.online and you open up your debug console, you'll notice the:
core.edb342e….bundle.js:2234 GET https://predictemall.online/api/pokemon/name/bul net::ERR_TOO_MANY_REDIRECTS error. This is bad. If you actually try to go to https://predictemall.online/api/pokemon/name/bul, you will have three redirects (in my opinion, didn't CURL fact-check it):

  1. to httpS://predictemall.online/api/pokemon/name/bul
  2. To http://api.predictemall.online/api/pokemon/name/bul
  3. to httpS://api.predictemall.online/api/pokemon/name/bul

Apart from the fact that there is no data coming back from the API (but I have complained about that as well, don't worry PokemonGoers/PokeData#191 ), this is something that needs to get fixed ASAP. Maybe assuming the protocol to be https instead of http will lower the number of redirects and chrome won't complain anymore. This should be an ENV var, if you are wondering: cos otherwise it's gonna fuck up development (no one develops in https)

@sacdallago sacdallago added the bug label Oct 23, 2016
@johartl
Copy link
Collaborator

johartl commented Oct 23, 2016

@sacdallago We already have environment variables for all API endpoints. See the defaults in the Dockerfile

// Dockerfile
ENV API_ENDPOINT=http://pokedata.c4e3f8c7.svc.dockerapp.io:65014
ENV WEBSOCKET_ENDPOINT=http://pokedata.c4e3f8c7.svc.dockerapp.io:65024

My first guess would be to set them as follows

ENV API_ENDPOINT=https://api.predictemall.online
ENV WEBSOCKET_ENDPOINT=https://api.predictemall.online:[websocket port?]

Edit: Btw, I don't see the errors in my console. Have you solved the issue already?

@sacdallago
Copy link
Member Author

What you are saying is very different from what I am complaining about :D I'm changing the env vars already through my docker-compose file ( read PokemonGoers/PokeData#191 ). My suggestion was to have another env var, which would avoid one redirect: the redirect from http to https as far as the protocol for the API goes. Your calls do the following: I call http://api.predictemall.online/api/somethingSomething, but nginx doesn't like when it get's the http request and answers with a 301 permanently moved, saying that the resource is now at https://api.predictemall.online/api/somethingSomething. Having that one redirect less, is already a great improvement!

Anyway, as I mentioned in a very eloquent and high english here, all problems seem to have been solved?! Magically!? Like.. No idea!!?

@johartl
Copy link
Collaborator

johartl commented Oct 23, 2016

@sacdallago In your docker-compose file from PokemonGoers/PokeData#191 (comment) you have

  environment:
    - API_ENDPOINT=api.predictemall.online
    - WEBSOCKET_ENDPOINT=api.predictemall.online:65024

My suggestion was to to add https:// to both API_ENDPOINT and WEBSOCKET_ENDPOINT.

But I guess since everything is working now (?) it's fine :D

@sacdallago
Copy link
Member Author

sacdallago commented Oct 24, 2016

wouldn't adding the protocol overwrite something somewhere? I imagine you use those variables either to:

  1. http.get(API_ENDPOINT)
  2. get("http://" + API_ENDPOINT)

So I fear that adding the protocol will either break the thing or be completely pointless(in case 1). It works, yes. But this is an issue anyway :)

@johartl
Copy link
Collaborator

johartl commented Oct 24, 2016

We are using the variable consistently in the first way and so far nothing has been broken yet ;)
And can you explain how the first way of using it is completely pointless?

@johartl johartl reopened this Oct 24, 2016
@MajorBreakfast
Copy link
Contributor

Just a side note:
There are protocol relative URLs, e.g. //example.com (double slash at the beginning)
https://www.paulirish.com/2010/the-protocol-relative-url/

@sacdallago
Copy link
Member Author

ok, I'll try out the suggestions on a test instance when I'm back from SFO in a week :)

But since this is working, I'll close for now, and sorry for talking to the dead :D

@sacdallago
Copy link
Member Author

Yay, it was too good to be true!

It now happens consistently :) And I have no idea of how to fix this, tried:

API_ENDPOINT=https://api.predictemall.online
WEBSOCKET_ENDPOINT=https://stream.predictemall.online
API_ENDPOINT=https://api.predictemall.online:443
WEBSOCKET_ENDPOINT=https://stream.predictemall.online:443
API_ENDPOINT=http://api.predictemall.online
WEBSOCKET_ENDPOINT=http://stream.predictemall.online
API_ENDPOINT=http://api.predictemall.online:80
WEBSOCKET_ENDPOINT=http://stream.predictemall.online:80
API_ENDPOINT=api.predictemall.online
WEBSOCKET_ENDPOINT=stream.predictemall.online

The last one didn't work at all :) the other ones give all sorts of problems, but mainly the TOO_MANY_REDIRECTS.

Don't worry about WEBSOCKET_ENDPOINT, for me what is important is that API_ENDPOINT works correctly.

@sacdallago sacdallago reopened this Oct 28, 2016
@johartl
Copy link
Collaborator

johartl commented Oct 29, 2016

@sacdallago Where do you see the TOO_MANY_REDIRECTS error? In the browser console or in the node server log of the CatchEmAll container?
Is there some way for me to reproduce it? For example using curl?

In theory, it should work like this:
(This is just my understanding of the proxy chain, please correct me where I'm wrong)

  1. The browser sends a request to window.location.origin + '/api/pokemon/name/bul'which should evaluate to https://predictemall.online/api/pokemon/name/bul.
  2. The nginx proxy server should proxy this request to http://catchemall.svc.dockerapp.io/api/pokemon/name/bul (domain made up) where the catchemall docker container runs at.
  3. The catchemall node server proxies the request to API_ENDPOINT=https://api.predictemall.online
  4. The nginx proxy server proxies the request to http://pokedata.svc.dockerapp.io/api/pokemon/name/bul where the pokedata container runs at (again, domain made up).
  5. The pokedata container receives the request, sends out the response and it goes all the way back.

To me this is a lot of proxying but I don't see any redirects here (as in 301 Moved Permanently). To the browser this should all be transparent, right?
Also, it would help my understanding to know where the pokedata and catchemall containers are running. Is this the same host as the nginx server? Maybe you could also provide the nginx configuration. It's a bit difficult to debug from my side without being able to reproduce the error.

@johartl
Copy link
Collaborator

johartl commented Oct 29, 2016

@sacdallago Can you please redeploy as soon as https://hub.docker.com/r/pokemongoers/catch-em-all/builds/bmhxzjysukghvxqaomvgxvn/ is done building to get the changes from #158.

@sacdallago
Copy link
Member Author

@johartl sorry for the delay: everything redeploys automatically (so as soon as stuff builds on hub, give it like 5 min?).

The errors happen in the chrome console @ predictemall.online . I will be a bit more active starting tomorrow (am currently at a Google conference)

@johartl
Copy link
Collaborator

johartl commented Oct 30, 2016

This afternoon when I checked there was still an old container active (from Oct 27th). So either something is wrong with the automated deployment or with my cache :/
Anyway, I don't see any redirect errors in the console right now for predictemall.online (and I think I've never seen any in the past).

@sacdallago
Copy link
Member Author

Yup, not happening on my machine anymore either. I'm not happy that this thing is not happening consistently. Need to stress test it once I'm back from SFO, btw check your email

@johartl
Copy link
Collaborator

johartl commented Nov 1, 2016

this thing is not happening consistently

I suspect this had something to do with the service worker cache not being flushed. I tackled this issue in #162.

Also, I found that some request were send to http://pokedata.c4e3f8c7.svc.dockerapp.io:65014 directly. I fixed the issue in #164, however, I don't think that this caused any redirects.

Thanks for that mail! I'm going to investigate further once #162 #163 and #164 are merged and deployed. Especially the broken service worker makes it really frustrating to debug :/

@johartl johartl changed the title Too many ridirects Too many redirects Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants