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

Missing of headers support on UMF messages affecting hydra-router response on legacy environments #87

Closed
jkyberneees opened this issue Apr 19, 2017 · 5 comments

Comments

@jkyberneees
Copy link

jkyberneees commented Apr 19, 2017

Hydra stands on the UMF message spec for req/res as well as for real-time messaging. UMF messages does not consider possible HTTP headers except 'Content-Type' and 'Authorization'. On legacy environments, to integrate the hydra can suppose a problem because HTTP headers are heavily used.

As an example, let's analyze the response headers of an app built with hapi.js:

http://localhost:3000/echo/hello

Connection →keep-alive
Date →Wed, 19 Apr 2017 08:26:40 GMT
Transfer-Encoding →chunked
cache-control →no-cache
content-encoding →gzip
content-type →application/json; charset=utf-8
vary →accept-encoding

Now the response through the hydra-router:

http://localhost:5353/hapi-service-test/v1/welcome

Connection →keep-alive
Date →Wed, 19 Apr 2017 08:27:09 GMT
Transfer-Encoding →chunked
X-Hydra-Tracer →1lkglmzk4pn

As a result, the response from the router is unreadable because there are missing headers like content*

In my opinion, limiting the HTTP headers support on the hydra-router->hydra->UMF will limit the capabilities of the hydra ecosystem as well, at least on existing environments who want to get boosted by using hydra.

Here, I would suggest to introduce a "headers" field on the UMF spec, that could be used for communication headers like HTTP, in consequence it would make hydra-* more compatible on legacy environments.

Looking forward to your feedback.
Regards, Rolando.

@cjus
Copy link
Contributor

cjus commented Apr 19, 2017

@jkyberneees I'm doing some cleanup on Hydra-Router that will make this an easier conversation to have. Specifically, the HydraRouter routeRequest function is too large and in need of refactoring. Feedback on your points above to follow...

@jkyberneees
Copy link
Author

Great, I keep an eye on this.

@cjus
Copy link
Contributor

cjus commented May 4, 2017

@jkyberneees please see if hydra-router 1.3.1 addresses this issue. Also requires that the service themselves using the latest hydra / hydra-express as updates are in makeAPIRequest.

@jkyberneees
Copy link
Author

@cjus Hi Carlos, thanks for taking care of the issue/feature/suggestion.
I reviewed the code and I see that you inject the request headers and consider the response too, however I don't understand this exclusion: https://github.com/flywheelsports/hydra/blob/master/index.js#L1103

Unfortunately, the feature is not yet working as expected, since hapi services are still not properly routed ;(
Let me know if you need more details to reproduce.

Best Regards, Rolando

@cjus
Copy link
Contributor

cjus commented May 14, 2017

@jkyberneees the line you referenced checks whether instanceList.length is zero - if that's the case then there are no more service instances to try so a service unavailable response is necessary. The 1.3.1 release was found to have problems - please see if the issue is resolved in hydra-router:1.3.8

@cjus cjus closed this as completed Feb 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants