Skip to content

Switch from express-http-proxy to node-http-proxy, proxy to [::1]:3000 #11

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

Merged
merged 4 commits into from
Feb 26, 2016
Merged

Switch from express-http-proxy to node-http-proxy, proxy to [::1]:3000 #11

merged 4 commits into from
Feb 26, 2016

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Feb 23, 2016

node-http-proxy development is more active, and it seems it can handle both Transfer-Encoding: chunked and proxying to http://[::1]:3000/api.

Closes #8

@AparnaKarve can you test it too please?

node-http-proxy development is more active, and it seems it can handle both `Transfer-Encoding: chunked` and proxying to `http://[::1]:3000/api`.

Closes #8
@himdel
Copy link
Contributor Author

himdel commented Feb 23, 2016

node-http-proxy also supports websockets...

@AparnaKarve
Copy link
Contributor

@himdel This does seem to work most of the times. However, I occasionally see an error when I click on Service Catalogs - it is simply unable to fetch the list and this is what I see in the log -

[----] I, [2016-02-22T19:24:38.548839 #66659:3fcb26abcabc]  INFO -- : Processing by ApiController#show as JSON

[----] I, [2016-02-22T19:24:38.548985 #66659:3fcb26abcabc]  INFO -- :   Parameters: {"expand"=>"resources", "attributes"=>"picture,picture.image_href,service_template_catalog.name", "filter"=>["service_template_catalog_id>0", "display=true"], "collection"=>"service_templates"}

[----] I, [2016-02-22T19:24:38.551148 #66659:3fcb26ab8fc0]  INFO -- : Processing by ApiController#show as JSON

[----] I, [2016-02-22T19:24:38.551245 #66659:3fcb26ab8fc0]  INFO -- :   Parameters: {"expand"=>"resources", "sort_by"=>"name", "sort_options"=>"ignore_case", "collection"=>"service_catalogs"}

[----] I, [2016-02-22T19:24:38.603918 #66659:3fcb26ab8fc0]  INFO -- : Completed 400 Bad Request in 53ms (Views: 0.6ms | ActiveRecord: 0.0ms)

One other thing that I noticed was, none of the images are loaded in the list views and detailed views.
It looks like it is not able to fetch the image hrefs.

@himdel
Copy link
Contributor Author

himdel commented Feb 23, 2016

One other thing that I noticed was, none of the images are loaded in the list views and detailed views.
It looks like it is not able to fetch the image hrefs.

Oh, thanks for noticing, that's caused by the SSUI split .. only affects SSUI in devel mode, otherwise /pictures is mapped fine .. but d6571a4 should fix it by also proxying the /pictures folder to the old UI.

@himdel
Copy link
Contributor Author

himdel commented Feb 23, 2016

The Service Catalog thing.. sometimes the API returns {"error":{"kind":"bad_request","message":"name is not a valid attribute for ServiceTemplateCatalog","klass":"ApiController::BadRequestError"}} .. but the problem is on the API side, not be caused by the proxy.. trying the same request the second time works.

@himdel
Copy link
Contributor Author

himdel commented Feb 23, 2016

@AparnaKarve I've created ManageIQ/manageiq#6878 for the Service Catalog problem.

@himdel
Copy link
Contributor Author

himdel commented Feb 24, 2016

I'd like to get this merged.. @h-kataria @imtayadeway would you mind giving this a whilr as well?

@h-kataria h-kataria added the bug label Feb 25, 2016
@h-kataria
Copy link
Contributor

ran SSUI with this PR, seems to be working fine. Labeled this PR as bug as this is related to some people seeing issues with Rails 5.

@dclarizio
Copy link

@matthewd please take a look

@matthewd
Copy link

Sounds reasonable.

My only concern is that as we're hard-coding the IPv6 IP, this won't work for someone who has IPv6 disabled, say... or has a more unusual value for localhost. Perhaps it'd be worth making it (plus the port, I guess) overridable with an env var?

@himdel
Copy link
Contributor Author

himdel commented Feb 25, 2016

@matthewd That's my concern too, unfortunately it seems impossible to force puma to listen on both ::1 and 127.0.0.1 when both are valid localhost resolutions, and node always seems to pick the other choice than ruby :).

(In fact, @h-kataria already had to

- ::1 localhost6
+ ::1 localhost

in /etc/hosts, so it may be an issue, for anyone with a system old enough. (Checked that our appliances have localhost there so probably no problem for new installs.))

So.. overridable with an env var sounds really great.. will do that, thanks! :)

@himdel
Copy link
Contributor Author

himdel commented Feb 25, 2016

Done, we're now reading a PROXY_HOST environment variable and building the urls from that, defaulting to [::1]:3000. (Aand added an example of just that in the README :). )

@miq-bot
Copy link
Member

miq-bot commented Feb 25, 2016

Checked commits https://github.com/himdel/manageiq-ui-self_service/compare/397e3e00da5b9a4f82d583a6b6b6597f0f8d95f7~...514fb4a32bdf25b7a41a354e74c790da29195e2c with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
0 files checked, 0 offenses detected
Everything looks good. 👍

@imtayadeway
Copy link
Contributor

@AparnaKarve I believe your issue should be addressed in ManageIQ/manageiq#6892

@imtayadeway
Copy link
Contributor

@himdel that does the trick for me 👍

@AparnaKarve
Copy link
Contributor

@imtayadeway Thanks! I will try how ManageIQ/manageiq#6892 works.

@himdel @matthewd Thanks for the environment variable trick. 👍 Backward compatibility with ipv4 was the first thing that I thought about when I saw this PR.

dclarizio pushed a commit that referenced this pull request Feb 26, 2016
Switch from express-http-proxy to node-http-proxy, proxy to [::1]:3000
@dclarizio dclarizio merged commit 99200ce into ManageIQ:master Feb 26, 2016
@dclarizio dclarizio added this to the Sprint 37 Ending Mar 7, 2016 milestone Feb 26, 2016
@himdel himdel deleted the node-http-proxy branch February 26, 2016 21:07
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.

7 participants