parse hostname from options instead of headers #430
Conversation
Codecov Report
@@ Coverage Diff @@
## master #430 +/- ##
=======================================
Coverage 82.71% 82.71%
=======================================
Files 36 36
Lines 1741 1741
=======================================
Hits 1440 1440
Misses 301 301 Continue to review full report at Codecov.
|
| } | ||
|
|
||
| RemoteRequestData.prototype.init = function init(req, res, downstreamXRayEnabled) { | ||
| RemoteRequestData.prototype.init = function init(req, res, downstreamXRayEnabled, options) { | ||
| const useOptionsHost = options && options.hostname; |
There was a problem hiding this comment.
Isn't the original issue wanting to read from the request url, not a fixed option?
There was a problem hiding this comment.
The original issue said it was problematic to read from the header, and instead preferred reading from the options object. But I guess reading from the request host would work just as well and not require a signature change.
There was a problem hiding this comment.
Yeah let's just change the logic to read the request URL. It seems like a bug we use the host header (I suspect other languages / instrumentations aren't doing so) so just a simple be bugfix
There was a problem hiding this comment.
Updated to prefer request.host and fall back to the header value. It looks like request.host was only added in Node 12, so I guess that's why it was this way before.
| @@ -50,5 +50,14 @@ describe('RemoteRequestData', function() { | |||
| '' | |||
| ); | |||
| }); | |||
| it('should use the host from the request object over headers', () => { | |||
| const requestWithHost = Object.assign(request, { host: 'different-site.com' }); | |||
There was a problem hiding this comment.
Hmm - could cleanup in another PR, getHeader() { return 'host.com' } seems like very suspicious code :) But I guess that's what makes sure we're overwriting the header.
By the way, if not in a unit test is it possible to verify the behavior with fetch which was the example in the filed issue? I assume it's node-fetch. Don't see a reason it wouldn't work though
https://github.com/node-fetch/node-fetch/blob/master/src/request.js#L211
There was a problem hiding this comment.
I updated the getHeader to be less sus :)
As for testing node-fetch, we could and probably should, but that would require a whole lot more stubbing than is probably in scope for this change since we're only testing the underlying Node HTTP client today.
Issue #, if available:
#401
Description of changes:
Adds optional parameter for
optionsobject, from which we can read thehostnamefield instead of the headers. In most cases, this will be exactly the same string, but as pointed out in #401 sometimes thehostheader has a different host from the actual target's hostname.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.