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.

Sign up for GitHub

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

Jump to bottom

Fix EZP-25479: better support for nginx in front of varnish #93

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix EZP-25479: better support for nginx in front of varnish #93

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54ff1ee
Fix EZP-25479: better support for nginx in front of varnish
gggeek Feb 17, 2016
e996c96
Simplify management of X-Forwarded-for header; take into account the …
gggeek Mar 28, 2016
29019f2
Port latest changes to vcl v3 and improve docs
gggeek Mar 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions doc/varnish/vcl/varnish3.vcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ acl debuggers {
"192.168.0.0"/16;
}

// ACL for the proxies in front of Varnish (e.g. Nginx terminating https)
acl proxies {
"127.0.0.1";
"192.168.0.0"/16;
}

// Called at the beginning of a request, after the complete request has been received
sub vcl_recv {

Expand All @@ -32,10 +38,18 @@ sub vcl_recv {
// Add a unique header containing the client address (only for master request)
// Please note that /_fragment URI can change in Symfony configuration
if (!req.url ~ "^/_fragment") {
if (req.http.x-forwarded-for) {
set req.http.X-Forwarded-For = req.http.X-Forwarded-For + ", " + client.ip;
// only accept the x-forwarded-for header if the remote-proxy is trusted
// also we add our ip to the list of forwarders, as it is logged by apache by default
if (req.http.x-forwarded-for && client.ip ~ proxies) {
// funnily enough, it seems that this bit is executed twice, so we do a bit of dirty coding
if (req.http.X-Added-Forwarded-For) {
unset req.http.X-Added-Forwarded-For;
} else {
set req.http.X-Forwarded-For = req.http.X-Forwarded-For + ", " + server.ip;
set req.http.X-Added-Forwarded-For = "1";
}
} else {
set req.http.X-Forwarded-For = client.ip;
set req.http.X-Forwarded-For = "" + client.ip + ", " + server.ip;
Copy link
Contributor

Choose a reason for hiding this comment

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

why this part?

Copy link
Contributor

Choose a reason for hiding this comment

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

When you have varnish in front of eZ, if you don't do that you will receive only Varnish IP. That could be a shame if you track IP address (apache access log for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code put only the client ip in the x-forwarder-for header.

I have noticed that, unlike this, Nginx by default (in its stock reverse-proxy config) adds itself as well to the ips in x-forwarded-for.
And, in the Apache logs, 'common' format, the 1st field will contain both ips.
Which is a bit weird at 1st, but makes it actually easy to see which requests has come through the proxy and which ones not.
With this change, the apache log will contain the whole list of ips: client, nginx, varnish.

Eg:
192.168.127.1, 172.18.0.7, 172.18.0.3 - - [17/Feb/2016:10:55:08 +0000] "GET /setup/info/php HTTP/1.1" 200 ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so we should make sure it is in sync with what Symfony expects on this header then. But at least clear for me, and with a comment mentioning this it will be clear to others ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

}
}

Expand Down
20 changes: 17 additions & 3 deletions doc/varnish/vcl/varnish4.vcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ acl debuggers {
"192.168.0.0"/16;
}

// ACL for the proxies in front of Varnish (e.g. Nginx terminating https)
acl proxies {
"127.0.0.1";
"192.168.0.0"/16;
}

// Called at the beginning of a request, after the complete request has been received
sub vcl_recv {

Expand All @@ -34,10 +40,18 @@ sub vcl_recv {
// Add a unique header containing the client address (only for master request)
// Please note that /_fragment URI can change in Symfony configuration
if (!req.url ~ "^/_fragment") {
if (req.http.x-forwarded-for) {
set req.http.X-Forwarded-For = req.http.X-Forwarded-For + ", " + client.ip;
// only accept the x-forwarded-for header if the remote-proxy is trusted
// also we add our ip to the list of forwarders, as it is logged by apache by default
if (req.http.x-forwarded-for && client.ip ~ proxies) {
// funnily enough, it seems that this bit is executed twice, so we do a bit of dirty coding
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why this happens ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the 'adding happens twice' ? nopes :-(
but I tested it, and the behaviour was consistent - without the workaround either no ip added or added twice

Copy link
Contributor

Choose a reason for hiding this comment

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

ESI at play? @dspe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure - we do have ESI, but this was in the request for the main page.
Userhash at play?

Copy link
Contributor

Choose a reason for hiding this comment

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

Userhash at play?

maybe, some way to log the url for debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reading the vcl a bit more, I think that this might happen because of the way the user-hash request is sent. If this is the case, we might improve the code by testing for "req.restarts" instead...
Further digging ongoing

if (req.http.X-Added-Forwarded-For) {
unset req.http.X-Added-Forwarded-For;
} else {
set req.http.X-Forwarded-For = req.http.X-Forwarded-For + ", " + server.ip;
set req.http.X-Added-Forwarded-For = "1";
}
} else {
set req.http.X-Forwarded-For = client.ip;
set req.http.X-Forwarded-For = "" + client.ip + ", " + server.ip;
}
}

Expand Down