Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
On Fri, Mar 13, 2020 at 03:33:38PM +0100, Tim Düsterhus wrote: > The v2-series is good from my side. I don't plan any more changes. If > you are happy as well then please take it. Perfect, series now applied, thanks! Willy
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
Willy, Am 13.03.20 um 14:18 schrieb Willy Tarreau: > I was willing to take it as is but since I had a comment for the last > one and I know that many times when reworking a patch we spot things > we wish has been done differently in the previous ones, I let you > decide what suits you best :-) If you want me to take it now I can. In fact I added a blank line in that patch. The v2-series is good from my side. I don't plan any more changes. If you are happy as well then please take it. Best regards Tim Düsterhus
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
On Fri, Mar 13, 2020 at 11:57:14AM +0100, Tim Düsterhus wrote: > Willy, > > Am 13.03.20 um 08:44 schrieb Willy Tarreau: > >> I've added the new `struct connection` member at the end. Please check > >> whether you think that is the appropriate place for it or if it should > >> be moved somewhere else because of holes or caches. > > > > I'm seeing that the struct connection has become huge (160 bytes now) > > I suspected something like that, that's why I specifically mentioned > that piece. > > > But for now I'm OK with taking your patch above. > > > > I'm not seeing anything in git, yet. > > Is the patch okay as-is and you are just looking into the `struct > connection` refactoring before actually taking it or would you like me > to make any changes to it? I was willing to take it as is but since I had a comment for the last one and I know that many times when reworking a patch we spot things we wish has been done differently in the previous ones, I let you decide what suits you best :-) If you want me to take it now I can. Willy
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
Willy, Am 13.03.20 um 08:44 schrieb Willy Tarreau: >> I've added the new `struct connection` member at the end. Please check >> whether you think that is the appropriate place for it or if it should >> be moved somewhere else because of holes or caches. > > I'm seeing that the struct connection has become huge (160 bytes now) I suspected something like that, that's why I specifically mentioned that piece. > But for now I'm OK with taking your patch above. > I'm not seeing anything in git, yet. Is the patch okay as-is and you are just looking into the `struct connection` refactoring before actually taking it or would you like me to make any changes to it? Best regards Tim Düsterhus
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
On Fri, Mar 13, 2020 at 08:44:59AM +0100, Willy Tarreau wrote: > I'm seeing that the struct connection has become huge (160 bytes now) By the way, regarding this, I'm seeing this order of criticity in terms of structures sizes: 1) struct fdtab : 64 bytes, cache-line-aligned, shared between threads, no room for negociation, the only way to add something is to replace something else with an added benefit. 2) struct tasklet : 48 bytes, shares the first 32 or so with struct task (must alias), we don't want it to go beyond a cache line 3) struct task: 160 bytes already :-( Must be very carefully arranged to group fields used together as close as possible to benefit from the appear in the cache line when accessed 4) struct connection: 160 bytes, many of which are never used. I'd love to shrink it down to 128 by moving PPv2 away from it 5) struct session: 184 bytes, nor optimized, 3 cache lines already, could get a bit more love. 6) struct conn_stream: 40 bytes, OK to grow up to a cache line 7) struct stream (1 kB) and all others at the same level. Willy
Re: [PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
Hi Tim, On Fri, Mar 06, 2020 at 01:15:39PM +0100, Tim Duesterhus wrote: > For the whole unique ID processing I've used the new `ist` helpers, it > might make sense to update the authority processing to make use of ist > in a future CLEANUP patch. > > I've added the new `struct connection` member at the end. Please check > whether you think that is the appropriate place for it or if it should > be moved somewhere else because of holes or caches. I'm seeing that the struct connection has become huge (160 bytes now) and is going to be inflated by 10% with this change. The authority used to save a few bytes by keeping the length on a uint8_t but that's still a detail. I think that the real problem is the proxy protocol processing here. It's very rarely used by those where the connection size matters, and when we look at it, we can see : proxy_netns (8) proxy_authority (8) proxy_authority_len (1) proxy_unique_id (16) That's a total of 33 bytes (well, 40-48 in practice due to holes around) that are there only for the proxy proto. What could possibly be done to improve the situation would be to create a pool of proxy-proto entries which would contain room for netns, auth and unique_id and which would be allocated on the fly when receiving such TLV fields. We'd only inflate it when adding new fields and that would only eat memory in such a case. Thinking that the change above alone is enough to go back to 128 bytes hence two cache lines makes me really tempted to have a look at it. But for now I'm OK with taking your patch above. Willy
[PATCH 2/3] MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
Willy,
while my primary motivation of this series is to *send* the unique ID,
instead of *receiving* it the patch to receive it comes first, because
it's super straight forward. I could basically copy the implementation
for the authority TLV.
For the whole unique ID processing I've used the new `ist` helpers, it
might make sense to update the authority processing to make use of ist
in a future CLEANUP patch.
I've added the new `struct connection` member at the end. Please check
whether you think that is the appropriate place for it or if it should
be moved somewhere else because of holes or caches.
I've also added a reg-test that verifies that pulling unique IDs works
properly. My first attempt was to use a LOCAL connection, because then
I would not need to send IP addresses in HEX encoding, however HAProxy
does not appear to read TLVs in LOCAL mode. I've attempted to find out
whether TLVs are supported for LOCAL mode or not in the proxy-protocol
specification, but it was not terribly clear. Supporting unique IDs in
LOCAL mode would definitely make sense to me. And supporting the CRC32
checksum would also make sense I guess.
So maybe the proxy protocol ingesting should be updated to process TLV
values for both PROXY and LOCAL mode? Do you have an opinion regarding
that?
Best regards
Tim Düsterhus
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
This patch reads a proxy protocol v2 provided unique ID and makes it
available using the `fc_pp_unique_id` fetch.
---
doc/configuration.txt | 4 +++
include/proto/connection.h| 5 +++
include/types/connection.h| 1 +
reg-tests/stream/unique-id-from-proxy.vtc | 38 +
src/connection.c | 41 +++
5 files changed, 89 insertions(+)
create mode 100644 reg-tests/stream/unique-id-from-proxy.vtc
diff --git a/doc/configuration.txt b/doc/configuration.txt
index b508db217..a078942bb 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -15122,6 +15122,10 @@ fc_pp_authority : string
Returns the authority TLV sent by the client in the PROXY protocol header,
if any.
+fc_pp_unique_id : string
+ Returns the unique ID TLV sent by the client in the PROXY protocol header,
+ if any.
+
fc_rcvd_proxy : boolean
Returns true if the client initiated the connection with a PROXY protocol
header.
diff --git a/include/proto/connection.h b/include/proto/connection.h
index fb264d2b5..9b8eb8ad3 100644
--- a/include/proto/connection.h
+++ b/include/proto/connection.h
@@ -325,6 +325,7 @@ static inline void conn_init(struct connection *conn)
conn->src = NULL;
conn->dst = NULL;
conn->proxy_authority = NULL;
+ conn->proxy_unique_id = IST_NULL;
}
/* sets as the connection's owner */
@@ -458,6 +459,10 @@ static inline void conn_free(struct connection *conn)
pool_free(pool_head_authority, conn->proxy_authority);
conn->proxy_authority = NULL;
}
+ if (isttest(conn->proxy_unique_id)) {
+ pool_free(pool_head_uniqueid, conn->proxy_unique_id.ptr);
+ conn->proxy_unique_id = IST_NULL;
+ }
/* By convention we always place a NULL where the ctx points to if the
* mux is null. It may have been used to store the connection as a
diff --git a/include/types/connection.h b/include/types/connection.h
index 0c2d960b9..30cb895ff 100644
--- a/include/types/connection.h
+++ b/include/types/connection.h
@@ -469,6 +469,7 @@ struct connection {
char *proxy_authority;/* Value of authority TLV received via
PROXYv2 */
unsigned int idle_time; /* Time the connection was
added to the idle list, or 0 if not in the idle list */
uint8_t proxy_authority_len; /* Length of authority TLV received via
PROXYv2 */
+ struct ist proxy_unique_id; /* Value of the unique ID TLV received via
PROXYv2 */
};
/* PROTO token registration */
diff --git a/reg-tests/stream/unique-id-from-proxy.vtc
b/reg-tests/stream/unique-id-from-proxy.vtc
new file mode 100644
index 0..81ee3dea9
--- /dev/null
+++ b/reg-tests/stream/unique-id-from-proxy.vtc
@@ -0,0 +1,38 @@
+varnishtest "Check that we are able to read a unique-id from PROXYv2"
+
+#REQUIRE_VERSION=2.2
+
+feature ignore_unknown_macro
+
+haproxy h1 -conf {
+defaults
+mode http
+timeout connect 1s
+timeout client 1s
+timeout server 1s
+
+frontend echo
+bind "fd@${fe1}" accept-proxy
+http-after-response set-header echo %[fc_pp_unique_id,hex]
+http-request return status 200
+} -start
+
+client c1 -connect ${h1_fe1_sock} {
+# PROXY v2 signature
+sendhex "0d 0a 0d 0a 00 0d 0a 51 55 49 54 0a"
+# version + PROXY
+sendhex "21"
+# TCP4
+sendhex "11"
+# length of the address (12) + length of the TLV (8)
+sendhex "00 14"
+# 127.0.0.1
