Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Hi guys, On Mon, Jan 22, 2018 at 08:13:59AM +0200, Jarno Huuskonen wrote: > I did some brief testing on saturday and with this silly config > frontend test4 > bind ipv4@127.0.0.1:8080 > bind ipv6@::1:8080 > > http-request track-sc1 src,ipmask(32,128) table test_be > > http-request track-sc0 req.hdr_ip(X,1),ipmask(24,64) table test_be > http-request set-var(sess.myx) req.hdr_ip(X,1),ipmask(24,64) > http-response set-header X-MY %[var(sess.myx)] > default_backend test_be > > backend test_be > stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt > server wp1 some.ip.addr.ess:80 id 1 > > and curl -4|-6 -H "X: ipv4.or.ipv6.address" http://localhost:8080/ > > everything I tested seemed to work (I tested Tim's patch with for size_t > outside > of loop). OK thanks a lot for the tests. Tim, there were a few versions of some of these patches in the series, I would appreciate it if you can send me a complete series of what you want me to apply. It would be nice to add a Tested-By: tag mentionning Jarno (unless he doesn't want :-)). I noticed that two patches "only" fix comments and were tagged either BUG or DOC, I'll re-tag them as CLEANUP since they have no impact on the code nor on the user-facing doc and will not be eligible for backporting. It's trivial for me to do this here on the patches themselves, do not waste your time rebasing if you don't need to change anything else. Thanks! Willy
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Hi, On Sun, Jan 21, Tim Düsterhus wrote: > Jarno, > > Am 15.01.2018 um 14:28 schrieb Jarno Huuskonen: > > I'll try to test later (might be couple of days). > > Did you find time, yet? I did some brief testing on saturday and with this silly config frontend test4 bind ipv4@127.0.0.1:8080 bind ipv6@::1:8080 http-request track-sc1 src,ipmask(32,128) table test_be http-request track-sc0 req.hdr_ip(X,1),ipmask(24,64) table test_be http-request set-var(sess.myx) req.hdr_ip(X,1),ipmask(24,64) http-response set-header X-MY %[var(sess.myx)] default_backend test_be backend test_be stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt server wp1 some.ip.addr.ess:80 id 1 and curl -4|-6 -H "X: ipv4.or.ipv6.address" http://localhost:8080/ everything I tested seemed to work (I tested Tim's patch with for size_t outside of loop). -Jarno -- Jarno Huuskonen
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Jarno, Willy, Am 15.01.2018 um 14:28 schrieb Jarno Huuskonen: > On Sun, Jan 14, Tim Düsterhus wrote: >>> Have you tested that req.hdr_ip / stick tables work w/both masks ? I >>> used something like: >>> http-request track-sc0 req.hdr_ip(X,1),ipmask(24,64) table test_be >>> http-request set-var(sess.myx) req.hdr_ip(X,1),ipmask(24,64) >>> http-response set-header X-MY %[var(sess.myx)] >>> >>> backend test_be >>> stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt >> I just read up on stick tables and think I figured it out: haproxy.cfg > defaults > log global > modehttp > option httplog > option dontlognull > timeout connect 5000 > timeout client 5 > timeout server 5 > > frontend fe > bind :::8080 v4v6 > > http-request track-sc0 src,ipmask(24,64) table be > # Masked IPv4 for IPv4, empty for IPv6 (with and without this commit) > http-response set-header Test %[src,ipmask(24)] > # Correctly masked IP addresses for both IPv4 and IPv6 > http-response set-header Test2 %[src,ipmask(24,:::::)] > # Correctly masked IP addresses for both IPv4 and IPv6 > http-response set-header Test3 %[src,ipmask(24,64)] > http-response set-header Stick %[sc0_conn_cnt(be)] > > default_backend be > > backend be > stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt > server s example.com:80 Calls: > [timwolla@/s/haproxy (master)]http --headers 127.0.0.3:8080 |grep Stick > Stick: 3 > [timwolla@/s/haproxy (master)]http --headers 127.0.0.1:8080 |grep Stick > Stick: 1 > [timwolla@/s/haproxy (master)]http --headers 127.0.0.2:8080 |grep Stick > Stick: 2 > [timwolla@/s/haproxy (master)]http --headers 127.0.0.3:8080 |grep Stick > Stick: 3 > [timwolla@/s/haproxy (master)]http --headers 192.168.178.38:8080 |grep Stick > Stick: 1 > [timwolla@/s/haproxy (master)]http --headers [::1]:8080 |grep Stick > Stick: 1 > [timwolla@/s/haproxy (master)]http --headers [::1]:8080 |grep Stick > Stick: 2 I think this is looking good, no? Best regards Tim Düsterhus
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Willy, Am 21.01.2018 um 14:07 schrieb Willy Tarreau: > There's no problem here because these ones are word-aligned anyway. > There are other such occurrences elsewhere in the code, so don't > worry for this one. Okay, I sent an updated patch that uses the cast to uint32_t. > From what I've seen till now your patches look fine but like you I'm > interested in Jarno's feedback ;-) Could you possibly review pick the following list of patches (all of them bugfixes only; mostly for docs) already? - v2 1/8 -2/8 -3/8 -4/8 Possibly also: -5/8 -6/8 -7/8 which don't really touch live code (apart from 5). It would allow me to clean up a bit of my local repository. Best regards Tim Düsterhus
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Hi Tim, On Sun, Jan 21, 2018 at 01:24:06PM +0100, Tim Düsterhus wrote: > Jarno, > > Am 15.01.2018 um 14:28 schrieb Jarno Huuskonen: > > I'll try to test later (might be couple of days). > > Did you find time, yet? > > > I'll get a compilation error on centos7: > > src/sample.c: In function 'sample_conv_ipmask': > > src/sample.c:1623:3: error: 'for' loop initial declarations are only > > allowed in C99 mode > >for (size_t i = 0; i < 16; i++) > >^ > > src/sample.c:1623:3: note: use option -std=c99 or -std=gnu99 to compile > > your code > > > > So maybe move size_t i outside of loop or unroll loop, something like Yes, please avoid declaring variables inside the for() loop or in the middle of the code, over time they make code maintenance more painful because when you look for the type of a variable you have to scrutinize the code backwards till the beginning of the function instead of quickly glancing at the beginning of each block. > > + *(uint32_t*)>data.u.ipv6.s6_addr[0] &= > > *(uint32_t*)_p[1].data.ipv6.s6_addr[0]; > > + *(uint32_t*)>data.u.ipv6.s6_addr[4] &= > > *(uint32_t*)_p[1].data.ipv6.s6_addr[4]; > > + *(uint32_t*)>data.u.ipv6.s6_addr[8] &= > > *(uint32_t*)_p[1].data.ipv6.s6_addr[8]; > > + *(uint32_t*)>data.u.ipv6.s6_addr[12] &= > > *(uint32_t*)_p[1].data.ipv6.s6_addr[12]; > > > > Thinking about this for some time: I am not sure whether casting a char > pointer to a uint32_t pointer is safe from a language lawyer perspective > in all cases. I certainly can update the patch when Willy tells me his > preference (just move the declaration up or perform the casting). There's no problem here because these ones are word-aligned anyway. There are other such occurrences elsewhere in the code, so don't worry for this one. >From what I've seen till now your patches look fine but like you I'm interested in Jarno's feedback ;-) Cheers, Willy
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Jarno, Am 15.01.2018 um 14:28 schrieb Jarno Huuskonen: > I'll try to test later (might be couple of days). Did you find time, yet? > I'll get a compilation error on centos7: > src/sample.c: In function ‘sample_conv_ipmask’: > src/sample.c:1623:3: error: ‘for’ loop initial declarations are only allowed > in C99 mode >for (size_t i = 0; i < 16; i++) >^ > src/sample.c:1623:3: note: use option -std=c99 or -std=gnu99 to compile your > code > > So maybe move size_t i outside of loop or unroll loop, something like > + *(uint32_t*)>data.u.ipv6.s6_addr[0] &= > *(uint32_t*)_p[1].data.ipv6.s6_addr[0]; > + *(uint32_t*)>data.u.ipv6.s6_addr[4] &= > *(uint32_t*)_p[1].data.ipv6.s6_addr[4]; > + *(uint32_t*)>data.u.ipv6.s6_addr[8] &= > *(uint32_t*)_p[1].data.ipv6.s6_addr[8]; > + *(uint32_t*)>data.u.ipv6.s6_addr[12] &= > *(uint32_t*)_p[1].data.ipv6.s6_addr[12]; > Thinking about this for some time: I am not sure whether casting a char pointer to a uint32_t pointer is safe from a language lawyer perspective in all cases. I certainly can update the patch when Willy tells me his preference (just move the declaration up or perform the casting). Best regards Tim Düsterhus
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Hi Tim, On Sun, Jan 14, Tim Düsterhus wrote: > > Have you tested that req.hdr_ip / stick tables work w/both masks ? I > > used something like: > > http-request track-sc0 req.hdr_ip(X,1),ipmask(24,64) table test_be > > http-request set-var(sess.myx) req.hdr_ip(X,1),ipmask(24,64) > > http-response set-header X-MY %[var(sess.myx)] > > > > backend test_be > > stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt > > I must admit that I never used stick tables in haproxy before. The use > case that lead me to implement this patch is privacy reasons (mask off > the lower bits for the logs). I verified my patch using the haproxy.cfg > given in the commit message and don't see a reason why it should not > work in other places. > I'd be grateful if you could verify the correct workings of stick > tables, as you already know how they are supposed to work. I'll try to test later (might be couple of days). I'll get a compilation error on centos7: src/sample.c: In function ‘sample_conv_ipmask’: src/sample.c:1623:3: error: ‘for’ loop initial declarations are only allowed in C99 mode for (size_t i = 0; i < 16; i++) ^ src/sample.c:1623:3: note: use option -std=c99 or -std=gnu99 to compile your code So maybe move size_t i outside of loop or unroll loop, something like + *(uint32_t*)>data.u.ipv6.s6_addr[0] &= *(uint32_t*)_p[1].data.ipv6.s6_addr[0]; + *(uint32_t*)>data.u.ipv6.s6_addr[4] &= *(uint32_t*)_p[1].data.ipv6.s6_addr[4]; + *(uint32_t*)>data.u.ipv6.s6_addr[8] &= *(uint32_t*)_p[1].data.ipv6.s6_addr[8]; + *(uint32_t*)>data.u.ipv6.s6_addr[12] &= *(uint32_t*)_p[1].data.ipv6.s6_addr[12]; might work. -Jarno -- Jarno Huuskonen
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Jarno, Am 14.01.2018 um 10:01 schrieb Jarno Huuskonen: > Quick question: how does your patch handle ipv4mapped ipv6 > address(:::1.2.3.4) ? Does it use v4 or v6 mask for these addresses ? > (From code I think v4 (first tries to convert v6 to v4 with c_ipv62ip) ?) Yes, you are correct. I took take care that these are handled as IPv4. > Do you see any use for ip6mask sample that would always use ipv6 mask > and only work with ipv6 addresses (ipv4mapped and pure ipv6) ? That basically would boil down to completely masking away IPv4 addresses. You can do that using: `ipmask(0,64)` with my patch as well (resulting in 0.0.0.0 for IPv4 addresses). Personally I don't see a real use case for handling IPv6 only. > Patch8 could add comment about arg_p parameters to sample_conv_ipmask > maybe something like: > +/* takes the ipv4 netmask in arg_p[0] and optional ipv6 netmask in > arg_p[1] */ Yes, thank you. I also noticed that the parameter is called `args` if multiple parameters are accepted. I fixed the comment, renamed the parameter and will send the updated patch in a separate mail to the list. > Have you tested that req.hdr_ip / stick tables work w/both masks ? I > used something like: > http-request track-sc0 req.hdr_ip(X,1),ipmask(24,64) table test_be > http-request set-var(sess.myx) req.hdr_ip(X,1),ipmask(24,64) > http-response set-header X-MY %[var(sess.myx)] > > backend test_be > stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt I must admit that I never used stick tables in haproxy before. The use case that lead me to implement this patch is privacy reasons (mask off the lower bits for the logs). I verified my patch using the haproxy.cfg given in the commit message and don't see a reason why it should not work in other places. I'd be grateful if you could verify the correct workings of stick tables, as you already know how they are supposed to work. Thanks Tim Düsterhus
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Hi, On Sat, Jan 13, Tim Duesterhus wrote: > this is a patch series ultimately leading to IPv6 support for the > ipmask converter. Please note the following: +1 I did something similar (never 100% finished) (http://haproxy.formilux.narkive.com/PZHJ5xNH/rfc-ipv6mask-converter). Quick question: how does your patch handle ipv4mapped ipv6 address(:::1.2.3.4) ? Does it use v4 or v6 mask for these addresses ? (From code I think v4 (first tries to convert v6 to v4 with c_ipv62ip) ?) Do you see any use for ip6mask sample that would always use ipv6 mask and only work with ipv6 addresses (ipv4mapped and pure ipv6) ? Patch8 could add comment about arg_p parameters to sample_conv_ipmask maybe something like: +/* takes the ipv4 netmask in arg_p[0] and optional ipv6 netmask in arg_p[1] */ Have you tested that req.hdr_ip / stick tables work w/both masks ? I used something like: http-request track-sc0 req.hdr_ip(X,1),ipmask(24,64) table test_be http-request set-var(sess.myx) req.hdr_ip(X,1),ipmask(24,64) http-response set-header X-MY %[var(sess.myx)] backend test_be stick-table type ipv6 size 20 expire 180s store gpc0,conn_cnt -Jarno > - The first patch contains non-US-ASCII characters, as the sole purpose > of it is to remove the non-US-ASCII characters from the source file. > Please take extra care when applying the patch to make sure the result > is good. > - The next two patches fix two small bugs in the sample converters and > are of general utility. > - The last five patches implement the progression to IPv6 support for the > ipmask converter and only make changes to the mask handling. > > Think of this as 3 logical patch series when reviewing. > > Tim Duesterhus (8): > BUG/MINOR: sample: Fix encoding of sample.c > DOC: sample: Fix outdated comment about sample casts functions > BUG/MINOR: sample: Fix output type of c_ipv62ip > DOC: Fix typo in ARGT_MSK6 comment > CLEANUP: standard: Use len2mask4 in str2mask > MINOR: standard: Add str2mask6 function > MINOR: config: Add support for ARGT_MSK6 > MEDIUM: sample: Add IPv6 support to the ipmask converter > > doc/configuration.txt | 11 +++ > include/common/standard.h | 6 ++ > include/types/arg.h | 2 +- > src/arg.c | 11 +-- > src/hlua.c| 7 +++ > src/sample.c | 31 ++- > src/standard.c| 28 > 7 files changed, 76 insertions(+), 20 deletions(-) > > -- > 2.15.1 > > -- Jarno Huuskonen
Re: [PATCH 0/8] Add IPv6 support to the ipmask converter
Hi please forgive my messed up threading, I misconfigured the rate limiting on my mailserver and a few emails could not be sent on the first try and while resending I must have made an error while manually setting the message headers. I can resend if anything is missing or not looking good. Best regards Tim Düsterhus
[PATCH 0/8] Add IPv6 support to the ipmask converter
Hi this is a patch series ultimately leading to IPv6 support for the ipmask converter. Please note the following: - The first patch contains non-US-ASCII characters, as the sole purpose of it is to remove the non-US-ASCII characters from the source file. Please take extra care when applying the patch to make sure the result is good. - The next two patches fix two small bugs in the sample converters and are of general utility. - The last five patches implement the progression to IPv6 support for the ipmask converter and only make changes to the mask handling. Think of this as 3 logical patch series when reviewing. Tim Duesterhus (8): BUG/MINOR: sample: Fix encoding of sample.c DOC: sample: Fix outdated comment about sample casts functions BUG/MINOR: sample: Fix output type of c_ipv62ip DOC: Fix typo in ARGT_MSK6 comment CLEANUP: standard: Use len2mask4 in str2mask MINOR: standard: Add str2mask6 function MINOR: config: Add support for ARGT_MSK6 MEDIUM: sample: Add IPv6 support to the ipmask converter doc/configuration.txt | 11 +++ include/common/standard.h | 6 ++ include/types/arg.h | 2 +- src/arg.c | 11 +-- src/hlua.c| 7 +++ src/sample.c | 31 ++- src/standard.c| 28 7 files changed, 76 insertions(+), 20 deletions(-) -- 2.15.1
