Issue #844 Add host information to log messages #45
Conversation
|
By the way, I guess the real host info won't be available when the API node is running behind a reverse proxy. Unfortunately most of API nodes in production are running this way. |
|
Hm, could use the x-forwarded-for header in this case. But need a flag if that header should be trusted. |
|
|
About trust: basically, if you're running nodes behind reverse proxies, the proxies are trustworthy for the nodes. So the proxy should add "x-forwarded-for" header according whatever standard and the nodes extract real addresses of clients. But there would be multiple or nested/chained "x-forwarded-for" header if the original request has that header already, in this case the node need to know which one is added by the reverse proxy, and likely don't want to trust the address in the original header. From this post, it's good practice to use a self-defined header, so I think
|
|
Need to take care of the new logging added by #62. |
|
@jmjatlanta please address the remaining comments so we can merge |
src/network/http/websocket.cpp
Outdated
| @@ -173,6 +154,14 @@ namespace fc { namespace http { | |||
| return _ws_connection->get_request_header(key); | |||
| } | |||
|
|
|||
| virtual std::string get_host()override | |||
There was a problem hiding this comment.
Please take a look at what I am doing here. I am attempting to get a "custom" header to help with the log message. This is untested code, but I would like some input to (in)validate my approach.
|
@abitmore please see https://github.com/bitshares/bitshares-fc/pull/45/files#r244624285 and let me know what you think. Thanks. |
jmjatlanta
force-pushed
the
jmj_issue_844
branch
2 times, most recently
from
74e9f6a
to
c3941d8
Compare
|
@jmjatlanta sorry I don't have enough time to review this. Wish others could help. |
src/network/http/websocket.cpp
Outdated
| virtual std::string get_host()override | ||
| { | ||
| auto header = get_request_header("BS-Forwarded"); | ||
| if (header.length() > 2) |
There was a problem hiding this comment.
Because I love magic numbers. Perhaps you don't like 2, so I replaced it with 3. (actually replaced with !string::empty() )
| auto header = get_request_header("BS-Forwarded"); | ||
| if (header.length() > 2) | ||
| return header; | ||
| return _ws_connection->get_host(); |
There was a problem hiding this comment.
I think get_host is wrong, according to the docs it returns the host component of the requested URI, not the remote host.
There was a problem hiding this comment.
I will investigate. Please do not bother reviewing until I edit this comment.
src/network/http/websocket.cpp
Outdated
| @@ -173,6 +154,14 @@ namespace fc { namespace http { | |||
| return _ws_connection->get_request_header(key); | |||
| } | |||
|
|
|||
| virtual std::string get_host()override | |||
| { | |||
| auto header = get_request_header("BS-Forwarded"); | |||
There was a problem hiding this comment.
Header name should be configurable like @abitmore said, and absence of configured value means ignore all headers and use remote host of connection.
There was a problem hiding this comment.
Implemented a configurable header. Will need to adjust in bitshares-core to make use of it.
|
Replaced by #134 |
Issue bitshares/bitshares-core#844. Adding host information to the rpc log.
Related: bitshares/bitshares-core#802
This adds the host to the log message. I don't particularly care for the output style, but I didn't want to declare a variable just for a log message. Perhaps I should use a lower level function than wdump(). What do you think?