Skip to content

Conversation

@MichelHollands
Copy link
Contributor

@MichelHollands MichelHollands commented Aug 11, 2020

This adds a field with the SourceIPs derived from X-Forwarded-For and the request address to the logging and tracing middleware. The server.log-source-ips setting is used to turn this one. By default it is not turned on.

The Forwarded, X-Real-IP and X-Forwarded headers are examined in this order. If you want to use your own header provide a header name in server.log-source-ips-header and a regex to extract it in server.log-source-ips-regex.

Logs for successful requests are logged at debug level. This is how the log looks like after this change and after the logging level has been set to debug:

level=debug ts=2020-08-11T09:46:43.207546Z caller=logging.go:63 traceID=1f208b9870d8cfd1 sourceIPs="1.2.3.4, ::1, ::1" msg="POST /api/v1/push (400) 2.357651ms"

A sourceIPs tag is added while tracing:

Screenshot 2020-08-14 at 09 39 15

Signed-off-by: Michel Hollands [email protected]

Copy link
Collaborator

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR; I made some comments below, mostly stylistic.

I think I can see three different changes here: removing lines from go.mod, adding source logging, and making standard middleware optional. Those should at least be three commits, but I wouldn't expect go.mod deletions in a PR like this. Maybe go.mod is wrong, but that would be a separate issue.

@MichelHollands
Copy link
Contributor Author

Thanks for the PR; I made some comments below, mostly stylistic.

I think I can see three different changes here: removing lines from go.mod, adding source logging, and making standard middleware optional. Those should at least be three commits, but I wouldn't expect go.mod deletions in a PR like this. Maybe go.mod is wrong, but that would be a separate issue.

I've reverted the go.mod and go.sum changes. My mistake about the separate commits. I keep forgetting to do that.

@MichelHollands MichelHollands marked this pull request as ready for review August 14, 2020 08:51
@MichelHollands MichelHollands changed the title Log X-Forwarded-For for every request Log X-Forwarded-For (or similar) for every request Aug 14, 2020
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job @MichelHollands! I left few comments, but overall LGTM 👍

Signed-off-by: Michel Hollands <[email protected]>
Signed-off-by: Michel Hollands <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (modulo a couple of nits)

Signed-off-by: Michel Hollands <[email protected]>
@bboreham
Copy link
Collaborator

Can you explain a bit more where you envisage the optional header and regex flags being useful?

@MichelHollands
Copy link
Contributor Author

Can you explain a bit more where you envisage the optional header and regex flags being useful?

Some enterprise customers use non-standard headers (ie different from Forwarded, X-Real-IP and X-Forwarded-For) in their custom reverse proxies. Using a regex as a way to configure this was suggested by Dee Kitchen. He used to work at Cloudflare where they used this successfully.

An added benefit is that customers can configure the headers themselves. Some customers are not keen on giving details about their internal proxies or any internal networking setup for that matter.

Copy link
Collaborator

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear this feature is too complicated for a library like this, but I cannot spot any more flaws and it's behind a flag so I'll merge it.
Thanks!

@bboreham bboreham merged commit 403bec7 into weaveworks:master Aug 20, 2020
@bboreham bboreham mentioned this pull request Aug 20, 2020
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

Successfully merging this pull request may close these issues.

3 participants