Re: [PATCH] Minor improvements to doc "http-request set-src"
On Tue, Apr 21, 2020 at 04:36:55PM +0200, Tim Düsterhus wrote: > Olivier, > > Am 21.04.20 um 16:34 schrieb Olivier D: > > ;) > > Patch updated attached. > > > > Now LGTM. > > Reviewed-by: Tim Duesterhus Thanks guys, now applied. Olivier, I noticed something strange, your patch was produced without the usual a/ b/ prefixes and the file started at doc/. It looks as if you had produced the patches using "--no-prefix", which then fails to apply, so it's worth having a look at your setup. I didn't have problems applying it with "patch -p0<" though, so the rest was OK. Thanks, Willy
Re: [PATCH] Minor improvements to doc "http-request set-src"
On Tue, Apr 21, 2020 at 12:56:51PM +0200, Tim Düsterhus wrote: > PS: Personal opinion, but I prefer quotes in replies to be shortened as > much as possible, while still providing context. I don't want to scroll > through kilobytes of stuff I've already seen :-) Rest assured it's a shared opinion, as I also hate having to scroll far away, and sometimes even miss isolated responses! :-) Willy
Re: [PATCH] Minor improvements to doc "http-request set-src"
Hi,
Le mar. 21 avr. 2020 à 12:56, Tim Düsterhus a écrit :
> Olivier,
>
> PS: Personal opinion, but I prefer quotes in replies to be shortened as
> much as possible, while still providing context. I don't want to scroll
> through kilobytes of stuff I've already seen :-)
>
;)
Patch updated attached.
From e6b11f3a795ec40c8b802d9d1190f3f6bbd15f5d Mon Sep 17 00:00:00 2001
From: Olivier Doucet
Date: Tue, 21 Apr 2020 09:32:56 +0200
Subject: [PATCH] DOC: Improve documentation on http-request set-src
This patch adds more explanation on how to use "http-request set-src"
and a link to "option forwardfor".
This patch can be applied to all previous version starting at 1.6
---
doc/configuration.txt | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git doc/configuration.txt doc/configuration.txt
index 5d01835d7..e695ab7f5 100644
--- doc/configuration.txt
+++ doc/configuration.txt
@@ -5114,16 +5114,23 @@ http-request set-src [ { if | unless }
]
This is used to set the source IP address to the value of specified
expression. Useful when a proxy in front of HAProxy rewrites source IP, but
provides the correct IP in a HTTP header; or you want to mask source IP for
- privacy.
+ privacy. All subsequent calls to "src" fetch will return this value
+ (see example).
Arguments :
Is a standard HAProxy expression formed by a sample-fetch followed
by some converters.
+ See also "option forwardfor".
+
Example:
http-request set-src hdr(x-forwarded-for)
http-request set-src src,ipmask(24)
+# After the masking this will track connections
+# based on the IP address with the last byte zeroed out.
+http-request track-sc0 src
+
When possible, set-src preserves the original source port as long as the
address family allows it, otherwise the source port is set to 0.
--
2.18.0.windows.1
Re: [PATCH] Minor improvements to doc "http-request set-src"
Olivier, Am 21.04.20 um 09:37 schrieb Olivier D: > Thank you for your valuable feedback. Find attached a new patch will all > your comments taken into account. > I've missed two more little things during my initial review: 1. The Subject of the patch should start with "DOC:" instead of "[DOC]". 2. All subsequent calls to src field will return this value (see example). -> It's not "field", but "fetch". Not sure whether "src" should also be quoted in there. Other than that it looks good to me now. Best regards Tim Düsterhus PS: Personal opinion, but I prefer quotes in replies to be shortened as much as possible, while still providing context. I don't want to scroll through kilobytes of stuff I've already seen :-)
Re: [PATCH] Minor improvements to doc "http-request set-src"
Hello,
Le lun. 20 avr. 2020 à 20:37, Tim Düsterhus a écrit :
> Olivier,
>
> Am 20.04.20 um 20:03 schrieb Olivier D:
> > I'm using gmail so I add to attach patches and was not able to send them
> > directly. If format is wrong, tell me :)
> >
>
> Format looks good to me. Your commit message however does not (fully)
> follow the instructions within the CONTRIBUTING file
> (
> https://github.com/haproxy/haproxy/blob/dfad6a41ad9f012671b703788dd679cf24eb8c5a/CONTRIBUTING#L562-L567
> ):
>
> >As a rule of thumb, your patch MUST NEVER be made only of a subject
> line,
> >it *must* contain a description. Even one or two lines, or indicating
> >whether a backport is desired or not. It turns out that single-line
> commits
> >are so rare in the Git world that they require special manual (hence
> >painful) handling when they are backported, and at least for this
> reason
> >it's important to keep this in mind.
>
> Regarding the patch itself:
>
> > diff --git doc/configuration.txt doc/configuration.txt
> > index 5d01835d7..ddfabcd92 100644
> > --- doc/configuration.txt
> > +++ doc/configuration.txt
> > @@ -6735,7 +6735,8 @@ option forwardfor [ except ] [ header
> ] [ if-none ]
> >header for a known source address or network by adding the "except"
> keyword
> >followed by the network address. In this case, any source IP matching
> the
> >network will not cause an addition of this header. Most common uses
> are with
> > - private networks or 127.0.0.1.
> > + private networks or 127.0.0.1. Another way to do it is to tell
> HAProxy to
> > + trust a custom header with "http-request set-src".
>
> This change looks incorrect to me. "option forwardfor" is for sending,
> not "receiving" IP addresses.
>
> >Alternatively, the keyword "if-none" states that the header will only
> be
> >added if it is not present. This should only be used in perfectly
> trusted
> > @@ -6760,6 +6761,14 @@ option forwardfor [ except ] [ header
> ] [ if-none ]
> > mode http
> > option forwardfor header X-Client
> >
> > + Example :
> > +# Trust a specific header and use it as origin IP.
> > +# If not found, source IP will be used.
> > +frontend www
> > +mode http
> > +http-request set-src CF-Connecting-IP
>
> I believe this should read `http-request set-src
> %[req.hdr(CF-Connecting-IP)]`. However:
>
> 1. I don't like having company specific headers in there. Especially
> since Cloudflare supports the standard XFF.
> 2. I don't consider that a useful addition.
>
> > +option forwardfor
> > +
> >See also : "option httpclose", "option http-server-close",
> > "option http-keep-alive"
> >
>
> Patch 2:
>
> > diff --git doc/configuration.txt doc/configuration.txt
> > index ddfabcd92..49324fa53 100644
> > --- doc/configuration.txt
> > +++ doc/configuration.txt
> > @@ -5114,7 +5114,8 @@ http-request set-src [ { if | unless }
> ]
> >This is used to set the source IP address to the value of specified
> >expression. Useful when a proxy in front of HAProxy rewrites source
> IP, but
> >provides the correct IP in a HTTP header; or you want to mask source
> IP for
> > - privacy.
> > + privacy. All subsequent calls to src field will return this value
> > + (see example).
>
> This change looks good to me.
>
> >Arguments :
> >Is a standard HAProxy expression formed by a sample-fetch
> followed
> > @@ -5124,6 +5125,11 @@ http-request set-src [ { if | unless }
> ]
> > http-request set-src hdr(x-forwarded-for)
> > http-request set-src src,ipmask(24)
> >
> > + Example:
>
> Only a single "Example:" heading is used throughout the documentation.
> As the first line can be shared with the previous example you could
> write something like: # After the masking this will track connections
> based on the IP address with the last octet zeroed out.
>
> > +# This will track connection based on header IP
> > +http-request set-src hdr(x-forwarded-for)
> > +http-request track-sc0 src
> > +
> >When possible, set-src preserves the original source port as long as
> the
> >address family allows it, otherwise the source port is set to 0.
>
Thank you for your valuable feedback. Find attached a new patch will all
your comments taken into account.
Olivier
>
> Best regards
> Tim Düsterhus
>
From e6b11f3a795ec40c8b802d9d1190f3f6bbd15f5d Mon Sep 17 00:00:00 2001
From: Olivier Doucet
Date: Tue, 21 Apr 2020 09:32:56 +0200
Subject: [PATCH] [DOC] Improve documentation on http-request set-src
This patch adds more explanation on how to use "http-request set-src"
and a link to "option forwardfor".
This patch can be applied to all previous version starting at 1.6
---
doc/configuration.txt | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git doc/configuration.txt doc/configuration.txt
index 5d01835d7..e695ab7f5 100644
--- doc/configuration.txt
+++ doc/configuration.txt
@@ -5114,16 +5114,23 @@
Re: [PATCH] Minor improvements to doc "http-request set-src"
Olivier,
Am 20.04.20 um 20:03 schrieb Olivier D:
> I'm using gmail so I add to attach patches and was not able to send them
> directly. If format is wrong, tell me :)
>
Format looks good to me. Your commit message however does not (fully)
follow the instructions within the CONTRIBUTING file
(https://github.com/haproxy/haproxy/blob/dfad6a41ad9f012671b703788dd679cf24eb8c5a/CONTRIBUTING#L562-L567):
>As a rule of thumb, your patch MUST NEVER be made only of a subject line,
>it *must* contain a description. Even one or two lines, or indicating
>whether a backport is desired or not. It turns out that single-line commits
>are so rare in the Git world that they require special manual (hence
>painful) handling when they are backported, and at least for this reason
>it's important to keep this in mind.
Regarding the patch itself:
> diff --git doc/configuration.txt doc/configuration.txt
> index 5d01835d7..ddfabcd92 100644
> --- doc/configuration.txt
> +++ doc/configuration.txt
> @@ -6735,7 +6735,8 @@ option forwardfor [ except ] [ header
> ] [ if-none ]
>header for a known source address or network by adding the "except" keyword
>followed by the network address. In this case, any source IP matching the
>network will not cause an addition of this header. Most common uses are
> with
> - private networks or 127.0.0.1.
> + private networks or 127.0.0.1. Another way to do it is to tell HAProxy to
> + trust a custom header with "http-request set-src".
This change looks incorrect to me. "option forwardfor" is for sending,
not "receiving" IP addresses.
>Alternatively, the keyword "if-none" states that the header will only be
>added if it is not present. This should only be used in perfectly trusted
> @@ -6760,6 +6761,14 @@ option forwardfor [ except ] [ header
> ] [ if-none ]
> mode http
> option forwardfor header X-Client
>
> + Example :
> +# Trust a specific header and use it as origin IP.
> +# If not found, source IP will be used.
> +frontend www
> +mode http
> +http-request set-src CF-Connecting-IP
I believe this should read `http-request set-src
%[req.hdr(CF-Connecting-IP)]`. However:
1. I don't like having company specific headers in there. Especially
since Cloudflare supports the standard XFF.
2. I don't consider that a useful addition.
> +option forwardfor
> +
>See also : "option httpclose", "option http-server-close",
> "option http-keep-alive"
>
Patch 2:
> diff --git doc/configuration.txt doc/configuration.txt
> index ddfabcd92..49324fa53 100644
> --- doc/configuration.txt
> +++ doc/configuration.txt
> @@ -5114,7 +5114,8 @@ http-request set-src [ { if | unless }
> ]
>This is used to set the source IP address to the value of specified
>expression. Useful when a proxy in front of HAProxy rewrites source IP, but
>provides the correct IP in a HTTP header; or you want to mask source IP for
> - privacy.
> + privacy. All subsequent calls to src field will return this value
> + (see example).
This change looks good to me.
>Arguments :
>Is a standard HAProxy expression formed by a sample-fetch
> followed
> @@ -5124,6 +5125,11 @@ http-request set-src [ { if | unless }
> ]
> http-request set-src hdr(x-forwarded-for)
> http-request set-src src,ipmask(24)
>
> + Example:
Only a single "Example:" heading is used throughout the documentation.
As the first line can be shared with the previous example you could
write something like: # After the masking this will track connections
based on the IP address with the last octet zeroed out.
> +# This will track connection based on header IP
> +http-request set-src hdr(x-forwarded-for)
> +http-request track-sc0 src
> +
>When possible, set-src preserves the original source port as long as the
>address family allows it, otherwise the source port is set to 0.
Best regards
Tim Düsterhus
