Pass ip, ips, and hostname to Request object by shwei · Pull Request #1476 · fastify/fastify

shwei · 2019-02-25T06:41:35Z

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

Add ip, ips, and hostname in Request object. Related to @delvedor's feedback at #1086 (comment). All credits go to @nwoltman . Without his guidance, the work won't be possible.

benchmarks

mcollina

LGTM

delvedor

Is this a breaking change?
We are no longer writing the values in the Node Core request object, but in ours.

shwei · 2019-02-26T08:33:21Z

Is this a breaking change?
We are no longer writing the values in the Node Core request object, but in ours.

Hi @delvedor: Do you mean this particular commit a breaking change or this pull request?

The call to handleTrustProxy(req) assigns req.ip to some value depending on the proxy option. Isn't the req object in this case a Node Core one? https://github.com/fastify/fastify/blob/master/fastify.js#L303

Thanks, Tomas. New to fastify.

@mcollina

Dismissing my "Accept" because I agree with @mcollina's suggestion to refactor the code so that it doesn't allocate any new objects.

nwoltman · 2019-02-27T06:03:02Z

@delvedor

Is this a breaking change?
We are no longer writing the values in the Node Core request object, but in ours

Yes, removing those values makes this a breaking change. Another option would be to add the values to Fastify's Request object now, and remove them from the Node core req object for the next major.

Ethan-Arrowood · 2019-02-28T02:53:23Z

Changes LGTM ✨

mcollina

Can you run a benchmark, like so:

node examples/example

autocannon -c 100 -d 40 -p 10 localhost:3000

And post the results, before and after this PR?

shwei · 2019-02-28T22:30:39Z

master branch:

Switched to branch 'master'
Your branch is ahead of 'origin/master' by 6 commits.
  (use "git push" to publish your local commits)
>$ node examples/example.js 
server listening on 3000
>$ autocannon -c 100 -d 40 -p 10 localhost:3000
Running 40s test @ http://localhost:3000
100 connections with 10 pipelining factor

┌─────────┬──────┬──────┬───────┬───────┬─────────┬──────────┬───────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%   │ Avg     │ Stdev    │ Max       │
├─────────┼──────┼──────┼───────┼───────┼─────────┼──────────┼───────────┤
│ Latency │ 0 ms │ 0 ms │ 47 ms │ 54 ms │ 4.31 ms │ 13.76 ms │ 400.59 ms │
└─────────┴──────┴──────┴───────┴───────┴─────────┴──────────┴───────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 11895   │ 11895   │ 22175   │ 26943   │ 22765.1 │ 3482.72 │ 11890   │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 1.95 MB │ 1.95 MB │ 3.64 MB │ 4.42 MB │ 3.73 MB │ 571 kB  │ 1.95 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.

911k requests in 40.07s, 149 MB read

pass-ip-to-request-constructor:

>$ git checkout pass-ip-to-request-constructor
Switched to branch 'pass-ip-to-request-constructor'
Your branch is ahead of 'origin/pass-ip-to-request-constructor' by 2 commits.
  (use "git push" to publish your local commits)
>$ node examples/example.js 
server listening on 3000
>$ autocannon -c 100 -d 40 -p 10 localhost:3000
Running 40s test @ http://localhost:3000
100 connections with 10 pipelining factor

┌─────────┬──────┬──────┬───────┬───────┬─────────┬──────────┬───────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%   │ Avg     │ Stdev    │ Max       │
├─────────┼──────┼──────┼───────┼───────┼─────────┼──────────┼───────────┤
│ Latency │ 0 ms │ 0 ms │ 47 ms │ 54 ms │ 4.27 ms │ 13.28 ms │ 409.13 ms │
└─────────┴──────┴──────┴───────┴───────┴─────────┴──────────┴───────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 12271   │ 12271   │ 23583   │ 26463   │ 23005.3 │ 2918.96 │ 12270   │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 2.01 MB │ 2.01 MB │ 3.87 MB │ 4.34 MB │ 3.77 MB │ 478 kB  │ 2.01 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.

920k requests in 40.07s, 151 MB read

Per @mcollina 's comment:

Can you run a benchmark, like so:
node examples/example
autocannon -c 100 -d 40 -p 10 localhost:3000
And post the results, before and after this PR?

mcollina

LGTM

mcollina

Can you please make this change non semver major? The properties must still be on the raw objects.

nwoltman

LGTM

…t-constructor

mcollina

Can you run a quick autocannon benchmark against the example before & after this change with the option on and off?

Also, the typescript def & test should be updated.

mcollina · 2019-03-07T07:06:49Z

+    var logger = log.child({ reqId: req.id })
+    if (optAddToNodeReq) {
+      req.log = res.log = logger
+    }


We might want to add ip here as well.
Would that be useful?

Something like this?

if (optAddToNodeReq) { req.ip = req.connection.remoteAddress req.log = res.log = logger }

I don't know in what scenario how it would be useful... so I would not add it.

mcollina

LGTM

github-actions · 2022-02-10T09:20:59Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

shwei added 3 commits February 22, 2019 14:39

add ip, ips and hostname in fastify's request obj

88f4c8a

refactor based on @nwoltman comments in 88f4c8a

35e3d40

add descriptions to ip, ips, and hostname in Request README

3dc46f3

mcollina approved these changes Feb 25, 2019

View reviewed changes

nwoltman suggested changes Feb 25, 2019

View reviewed changes

Comment thread fastify.js Outdated

store ip as a variable for later use

e446c9f

nwoltman previously approved these changes Feb 25, 2019

View reviewed changes

delvedor reviewed Feb 26, 2019

View reviewed changes

Ethan-Arrowood suggested changes Feb 26, 2019

View reviewed changes

Comment thread fastify.js Outdated

Comment thread lib/request.js

mcollina requested changes Feb 26, 2019

View reviewed changes

Comment thread fastify.js Outdated

Comment thread fastify.js Outdated

shwei added 2 commits February 27, 2019 17:40

add the new property, ips in Request

b542fb8

refactor trust proxy handling logic -> it doesn't allocate new object

43657f8

Ethan-Arrowood approved these changes Feb 28, 2019

View reviewed changes

mcollina reviewed Feb 28, 2019

View reviewed changes

Comment thread fastify.d.ts Outdated

Comment thread fastify.js Outdated

Comment thread fastify.js

shwei added 2 commits February 28, 2019 13:22

change ips' type to string[] to match proxyAddr.all output

efef8cb

refactor: make a const to the outer scope and update the references

fe3b111

shwei changed the title ~~Pass ip, ips, and hostname to request constructor~~ Pass ip, ips, and hostname to Request object Feb 28, 2019

mcollina approved these changes Mar 2, 2019

View reviewed changes

mcollina requested changes Mar 2, 2019

View reviewed changes

nwoltman suggested changes Mar 2, 2019

View reviewed changes

Comment thread fastify.js

shwei added 3 commits March 4, 2019 01:03

add a test of x-forwarded-host before setting it to hostname

3ce1c00

add ip, ips and hostname back to the Node req object.

156ce72

refactor: make the comment 1 line

933d4d3

mcollina reviewed Mar 4, 2019

View reviewed changes

Comment thread test/trust-proxy.test.js

keep previous assertions

c680bee

nwoltman approved these changes Mar 5, 2019

View reviewed changes

nwoltman mentioned this pull request Mar 6, 2019

How to deploy on Firebase Functions/Google Cloud Functions #946

Closed

shwei added 4 commits March 6, 2019 21:53

add addPropertiesToNodeReq to options for ip, ips, hostname, log

f112ed0

refactor: use logger from Fastify's log instead of from reply.res

c1043ab

add a test case for addPropertiesToNodeReq that mirrors trust proxy test

bdd709e

Merge remote-tracking branch 'upstream/master' into pass-ip-to-reques…

1d44c14

…t-constructor

mcollina reviewed Mar 7, 2019

View reviewed changes

shwei added 4 commits March 7, 2019 01:29

add modifyCoreObjects to ServerOptions type

42d3e0a

change the option name to modifyCoreObjects

cab709f

make modifyCoreObjects optional in the type and add it to types test

fc7a56e

change addPropertiesToNodeReq to modifyCoreObjects

1f4a424

mcollina approved these changes Mar 7, 2019

View reviewed changes

mcollina merged commit 6e57aa3 into fastify:master Mar 7, 2019

fralonra mentioned this pull request Mar 7, 2019

文档持续更新List fastify/docs-chinese#18

Closed

21 tasks

nwoltman reviewed Mar 7, 2019

View reviewed changes

Comment thread docs/Server.md

nwoltman mentioned this pull request Mar 7, 2019

Fix the default for the modifyCoreObjects option #1517

Merged

4 tasks

nwoltman added the semver-minor Issue or PR that should land as semver minor label Mar 7, 2019

fralonra mentioned this pull request Mar 23, 2019

Update: pr#1476 pr#1517 pr#1550 fastify/docs-chinese#91

Merged

delvedor mentioned this pull request Dec 27, 2019

Set modifyCoreObjects to false by default in v3 #2013

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Feb 10, 2022

Uh oh!

Conversation

shwei commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

shwei commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nwoltman commented Feb 27, 2019

Uh oh!

Ethan-Arrowood commented Feb 28, 2019

Uh oh!

mcollina left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shwei commented Feb 28, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nwoltman left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

shwei Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Feb 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shwei commented Feb 25, 2019 •

edited

Loading

shwei commented Feb 26, 2019 •

edited

Loading

mcollina left a comment •

edited

Loading

shwei Mar 7, 2019 •

edited

Loading