Re: Peers Protocol "Table Type"
On Tue, Jun 02, 2020 at 04:25:11PM +0200, Tim Düsterhus wrote: > This looks good to me now. I trust that you actually tested the changes. > > Reviewed-by: Tim Duesterhus Argh one minute too late, just applied :-/ Thanks anyway for your review Tim! Willy
Re: Peers Protocol "Table Type"
On Tue, Jun 02, 2020 at 03:10:08PM +0200, Emeric Brun wrote: > Thank you Tim! > > Here the updated patch. Thanks guys, now applied. Willy
Re: Peers Protocol "Table Type"
Emeric, Willy, Am 02.06.20 um 15:10 schrieb Emeric Brun: > Thank you Tim! > > Here the updated patch. This looks good to me now. I trust that you actually tested the changes. Reviewed-by: Tim Duesterhus Best regards Tim Düsterhus
Re: Peers Protocol "Table Type"
Hi All,
On 6/2/20 1:10 PM, Tim Düsterhus wrote:
> Emeric,
>
> Am 02.06.20 um 11:29 schrieb Emeric Brun:
>> In attachement a proposed patch for this issue.
>>
>
> Thanks. The changes to the doc look good to me.
>
> Regarding peers.c:
>
>> +/* network key types;
>> + * network types were directly and mistakenly
>> + * mapped on sample types, to keep backward
>> + * compatiblitiy we keep those values but
>> + * we now use a internal/network mapping
>> + * to avoid further mistakes adding or
>> + * modifying internals types
>> + */
>> +enum {
>> +PEER_KT_ANY = 0, /* any type */
>> +PEER_KT_RESV1,/* UNUSED */
>> +PEER_KT_SINT, /* signed 64bits integer type */
>> +PEER_KT_RESV2,/* UNUSED */
>
> Maybe call this RESV3 to make it clear that the numeric value is '3'.
>
>> +PEER_KT_IPV4, /* ipv4 type */
>> +PEER_KT_IPV6, /* ipv6 type */
>> +PEER_KT_STR, /* char string type */
>> +PEER_KT_BIN, /* buffer type */
>> +PEER_KT_TYPES /* number of types, must always be last */
>> +};
>
> -
>
>> + * Note: Undeclared mapping maps entry to SMP_ST_ANY == 0
>
> This should read SMP_T_ANY.
>
> Also backporting instructions are missing from the commit message.
>
> Other than that the patch looks good to me, but I didn't actually test a
> binary compiled from it.
>
> Best regards
> Tim Düsterhus
>
Thank you Tim!
Here the updated patch.
R,
Emeric
>From c58eed60557fd20e8b972cc3ab4b45e4e598e558 Mon Sep 17 00:00:00 2001
From: Emeric Brun
Date: Tue, 2 Jun 2020 11:17:42 +0200
Subject: [PATCH] BUG/MINOR: peers: fix internal/network key type mapping.
Network types were directly and mistakenly mapped on sample types:
This patch fix the doc with values effectively used to keep backward
compatiblitiy on existing implementations.
In addition it adds an internal/network mapping for key types to avoid
further mistakes adding or modifying internals types.
This patch should be backported on all maintained branches,
particularly until v1.8 included for documentation part.
---
doc/peers-v2.0.txt | 10 +-
src/peers.c| 46 --
2 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/doc/peers-v2.0.txt b/doc/peers-v2.0.txt
index 477e7bb84..344cb5609 100644
--- a/doc/peers-v2.0.txt
+++ b/doc/peers-v2.0.txt
@@ -191,11 +191,11 @@ between the "Sender Table ID" to identify it directly in case of "Table Switch M
Table Type present the numeric type of key used to store stick table entries:
integer
- 0: signed integer
- 1: IPv4 address
- 2: IPv6 address
- 3: string
- 4: binary
+ 2: signed integer
+ 4: IPv4 address
+ 5: IPv6 address
+ 6: string
+ 7: binary
Table Keylen present the key length or max length in case of strings or binary (padded with 0).
diff --git a/src/peers.c b/src/peers.c
index 2e54ab94d..9782ff3c1 100644
--- a/src/peers.c
+++ b/src/peers.c
@@ -125,6 +125,48 @@ enum {
PEER_MSG_ERR_SIZELIMIT,
};
+/* network key types;
+ * network types were directly and mistakenly
+ * mapped on sample types, to keep backward
+ * compatiblitiy we keep those values but
+ * we now use a internal/network mapping
+ * to avoid further mistakes adding or
+ * modifying internals types
+ */
+enum {
+PEER_KT_ANY = 0, /* any type */
+PEER_KT_RESV1,/* UNUSED */
+PEER_KT_SINT, /* signed 64bits integer type */
+PEER_KT_RESV3,/* UNUSED */
+PEER_KT_IPV4, /* ipv4 type */
+PEER_KT_IPV6, /* ipv6 type */
+PEER_KT_STR, /* char string type */
+PEER_KT_BIN, /* buffer type */
+PEER_KT_TYPES /* number of types, must always be last */
+};
+
+/* Map used to retrieve network type from internal type
+ * Note: Undeclared mapping maps entry to PEER_KT_ANY == 0
+ */
+static int peer_net_key_type[SMP_TYPES] = {
+ [SMP_T_SINT] = PEER_KT_SINT,
+ [SMP_T_IPV4] = PEER_KT_IPV4,
+ [SMP_T_IPV6] = PEER_KT_IPV6,
+ [SMP_T_STR] = PEER_KT_STR,
+ [SMP_T_BIN] = PEER_KT_BIN,
+};
+
+/* Map used to retrieve internal type from external type
+ * Note: Undeclared mapping maps entry to SMP_T_ANY == 0
+ */
+static int peer_int_key_type[PEER_KT_TYPES] = {
+ [PEER_KT_SINT] = SMP_T_SINT,
+ [PEER_KT_IPV4] = SMP_T_IPV4,
+ [PEER_KT_IPV6] = SMP_T_IPV6,
+ [PEER_KT_STR] = SMP_T_STR,
+ [PEER_KT_BIN] = SMP_T_BIN,
+};
+
/*
* Parameters used by functions to build peer protocol messages. */
struct peer_prep_params {
@@ -620,7 +662,7 @@ static int peer_prepare_switchmsg(char *msg, size_t size, struct peer_prep_param
/* encode table type */
- intencode(st->table->type, &cursor);
+ intencode(peer_net_key_type[st->table->type], &cursor);
/* encode table key size */
intencode(st->table->key_size, &cursor);
@@ -1655,7 +1697,7 @@ static inline int peer_treat_definemsg(struct appctx *appctx, struct peer *p,
if (!*msg_cur)
goto malformed_exit;
- if (p->remote_table->table->type != table_type
+ if (p->r
Re: Peers Protocol "Table Type"
Emeric,
Am 02.06.20 um 11:29 schrieb Emeric Brun:
> In attachement a proposed patch for this issue.
>
Thanks. The changes to the doc look good to me.
Regarding peers.c:
> +/* network key types;
> + * network types were directly and mistakenly
> + * mapped on sample types, to keep backward
> + * compatiblitiy we keep those values but
> + * we now use a internal/network mapping
> + * to avoid further mistakes adding or
> + * modifying internals types
> + */
> +enum {
> +PEER_KT_ANY = 0, /* any type */
> +PEER_KT_RESV1,/* UNUSED */
> +PEER_KT_SINT, /* signed 64bits integer type */
> +PEER_KT_RESV2,/* UNUSED */
Maybe call this RESV3 to make it clear that the numeric value is '3'.
> +PEER_KT_IPV4, /* ipv4 type */
> +PEER_KT_IPV6, /* ipv6 type */
> +PEER_KT_STR, /* char string type */
> +PEER_KT_BIN, /* buffer type */
> +PEER_KT_TYPES /* number of types, must always be last */
> +};
-
> + * Note: Undeclared mapping maps entry to SMP_ST_ANY == 0
This should read SMP_T_ANY.
Also backporting instructions are missing from the commit message.
Other than that the patch looks good to me, but I didn't actually test a
binary compiled from it.
Best regards
Tim Düsterhus
Re: Peers Protocol "Table Type"
Hi Tim, Willy,
On 3/20/20 3:01 PM, Tim Düsterhus wrote:
> Emeric,
>
> Am 20.03.20 um 14:29 schrieb Emeric Brun:
>> So I understand that since 1.6 the SMP_T are directly announced on the wire
>> for key types, and it brokes the documented values and this is hazardous to
>> rely on internal enum values.
>>
>> So we must re-introduce a mapping between internal and on-wire types.
>>
>> Some questions about choices:
>>
>> - Re-map types to documented values or Update the doc to match currently
>> used values?
>
> There's really only one sane choice after several years of not following
> the documentation:
>
> Update the documentation to match the currently used values. The peers
> protocol is HAProxy specific, so in practice the correct values are
> "what HAProxy does" (i.e. the protocol is defined by the reference
> implementation). The custom implementation during which I stumbled upon
> this issue is brand new and I needed to look into the code anyway,
> because the docs are incomplete (as I outlined before in this thread).
>
> Changing the code will cause larger breakage during a HAProxy bugfix
> upgrade if not all machines in a cluster are upgraded simultaneously.
>
> Best regards
> Tim Düsterhus
>
In attachement a proposed patch for this issue.
R,
Emeric
>From 3792e5fb69a10daf03f65d142fa308c8f2704588 Mon Sep 17 00:00:00 2001
From: Emeric Brun
Date: Tue, 2 Jun 2020 11:17:42 +0200
Subject: [PATCH] BUG/MINOR: peers: fix internal/network key type mapping.
Network types were directly and mistakenly mapped on sample types:
This patch fix the doc with values effectively used to keep backward
compatiblitiy on existing implementations.
In addition it adds an internal/network mapping for key types to avoid
further mistakes adding or modifying internals types.
---
doc/peers-v2.0.txt | 10 +-
src/peers.c| 46 --
2 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/doc/peers-v2.0.txt b/doc/peers-v2.0.txt
index 477e7bb84..344cb5609 100644
--- a/doc/peers-v2.0.txt
+++ b/doc/peers-v2.0.txt
@@ -191,11 +191,11 @@ between the "Sender Table ID" to identify it directly in case of "Table Switch M
Table Type present the numeric type of key used to store stick table entries:
integer
- 0: signed integer
- 1: IPv4 address
- 2: IPv6 address
- 3: string
- 4: binary
+ 2: signed integer
+ 4: IPv4 address
+ 5: IPv6 address
+ 6: string
+ 7: binary
Table Keylen present the key length or max length in case of strings or binary (padded with 0).
diff --git a/src/peers.c b/src/peers.c
index 2e54ab94d..91f2b3ad8 100644
--- a/src/peers.c
+++ b/src/peers.c
@@ -125,6 +125,48 @@ enum {
PEER_MSG_ERR_SIZELIMIT,
};
+/* network key types;
+ * network types were directly and mistakenly
+ * mapped on sample types, to keep backward
+ * compatiblitiy we keep those values but
+ * we now use a internal/network mapping
+ * to avoid further mistakes adding or
+ * modifying internals types
+ */
+enum {
+PEER_KT_ANY = 0, /* any type */
+PEER_KT_RESV1,/* UNUSED */
+PEER_KT_SINT, /* signed 64bits integer type */
+PEER_KT_RESV2,/* UNUSED */
+PEER_KT_IPV4, /* ipv4 type */
+PEER_KT_IPV6, /* ipv6 type */
+PEER_KT_STR, /* char string type */
+PEER_KT_BIN, /* buffer type */
+PEER_KT_TYPES /* number of types, must always be last */
+};
+
+/* Map used to retrieve network type from internal type
+ * Note: Undeclared mapping maps entry to PEER_KT_ANY == 0
+ */
+static int peer_net_key_type[SMP_TYPES] = {
+ [SMP_T_SINT] = PEER_KT_SINT,
+ [SMP_T_IPV4] = PEER_KT_IPV4,
+ [SMP_T_IPV6] = PEER_KT_IPV6,
+ [SMP_T_STR] = PEER_KT_STR,
+ [SMP_T_BIN] = PEER_KT_BIN,
+};
+
+/* Map used to retrieve internal type from external type
+ * Note: Undeclared mapping maps entry to SMP_ST_ANY == 0
+ */
+static int peer_int_key_type[PEER_KT_TYPES] = {
+ [PEER_KT_SINT] = SMP_T_SINT,
+ [PEER_KT_IPV4] = SMP_T_IPV4,
+ [PEER_KT_IPV6] = SMP_T_IPV6,
+ [PEER_KT_STR] = SMP_T_STR,
+ [PEER_KT_BIN] = SMP_T_BIN,
+};
+
/*
* Parameters used by functions to build peer protocol messages. */
struct peer_prep_params {
@@ -620,7 +662,7 @@ static int peer_prepare_switchmsg(char *msg, size_t size, struct peer_prep_param
/* encode table type */
- intencode(st->table->type, &cursor);
+ intencode(peer_net_key_type[st->table->type], &cursor);
/* encode table key size */
intencode(st->table->key_size, &cursor);
@@ -1655,7 +1697,7 @@ static inline int peer_treat_definemsg(struct appctx *appctx, struct peer *p,
if (!*msg_cur)
goto malformed_exit;
- if (p->remote_table->table->type != table_type
+ if (p->remote_table->table->type != peer_int_key_type[table_type]
|| p->remote_table->table->key_size != table_keylen) {
p->remote_table = NULL;
goto ignore_msg;
--
2.17.1
Re: Peers Protocol "Table Type"
Salut Willy, > >> The documented values are not used on any still supported haproxy's version. >> So I think it would be better to update the doc with the new ones >> and add a mapping to avoid further changes. > > Yep definitely. J'essaye de finir les scripts pour la gestion du SSD, et je suis pas dutout sur du haproxy en ce moment, alors j'espère qu'il n'y a pas urgence sur ce point ca m'arrangerait de m'en occuper qu'après. Emeric
Re: Peers Protocol "Table Type"
Hi Tim, On 3/20/20 3:01 PM, Tim Düsterhus wrote: > Emeric, > > Am 20.03.20 um 14:29 schrieb Emeric Brun: >> So I understand that since 1.6 the SMP_T are directly announced on the wire >> for key types, and it brokes the documented values and this is hazardous to >> rely on internal enum values. >> >> So we must re-introduce a mapping between internal and on-wire types. >> >> Some questions about choices: >> >> - Re-map types to documented values or Update the doc to match currently >> used values? > > There's really only one sane choice after several years of not following > the documentation: > > Update the documentation to match the currently used values. The peers > protocol is HAProxy specific, so in practice the correct values are > "what HAProxy does" (i.e. the protocol is defined by the reference > implementation). The custom implementation during which I stumbled upon > this issue is brand new and I needed to look into the code anyway, > because the docs are incomplete (as I outlined before in this thread). > > Changing the code will cause larger breakage during a HAProxy bugfix > upgrade if not all machines in a cluster are upgraded simultaneously. > > Best regards > Tim Düsterhus > So we all agree :) Emeric
Re: Peers Protocol "Table Type"
On Fri, Mar 20, 2020 at 03:12:46PM +0100, Emeric Brun wrote: > I understood that documented values are: > 0: signed integer > 1: IPv4 address > 2: IPv6 address > 3: string > 4: binary > > and currenty (since 1.6): >2 = signed int >4 = IPv4 >5 = IPv6 >6 = string >7 = binary OK that's indeed enough to fix the doc given that 1.6 is the oldest supported version. > I know an other soft talking peers protocol which is compatible with peers > proto for all haproxy's versions and which uses the new set of values. > It will break this compatibility re-mapping to documented IDs. That's why I > asked. Yes, I didn't have the differences in mind. But that's what I meant by "slightly adjust the doc" :-) > The documented values are not used on any still supported haproxy's version. > So I think it would be better to update the doc with the new ones > and add a mapping to avoid further changes. Yep definitely. Thanks, Willy
Re: Peers Protocol "Table Type"
Hi Willy, On 3/20/20 2:53 PM, Willy Tarreau wrote: > Hi Emeric, > > On Fri, Mar 20, 2020 at 02:29:48PM +0100, Emeric Brun wrote: >> So I understand that since 1.6 the SMP_T are directly announced on the wire >> for key types, and it brokes the documented values and this is hazardous to >> rely on internal enum values. > > Yes that's the issue Tim spotted. > >> So we must re-introduce a mapping between internal and on-wire types. > > Indeed. > >> Some questions about choices: >> >> - Re-map types to documented values or Update the doc to match currently >> used values? > > I'd say that what's currently on the wire seems to more or less match the > doc. Given that there are very few peers implementations, what's working > right now seems the most important to me. So I'd suggest that we change > the code to perform the mapping again and that we possibly update the doc > in case it's wrong on some of them. I understood that documented values are: 0: signed integer 1: IPv4 address 2: IPv6 address 3: string 4: binary and currenty (since 1.6): 2 = signed int 4 = IPv4 5 = IPv6 6 = string 7 = binary I know an other soft talking peers protocol which is compatible with peers proto for all haproxy's versions and which uses the new set of values. It will break this compatibility re-mapping to documented IDs. That's why I asked. The documented values are not used on any still supported haproxy's version. So I think it would be better to update the doc with the new ones and add a mapping to avoid further changes. > >> - About re-introduce the mapping, I'm not sure we should re-introduce the >> mapping between fetches and tables types. Table types could stay internal, >> using SMP_T... and we could map to protocol's values only when emitting the >> message. > > I didn't think about this possibility but I actually think it might be > the best one. The reason the types were changed was to simplify the > resolution of casts in "stick" rules, because if you remember we used > to have two types of casts in the past, sample-to-sample and > sample-to-table. But with your suggestion we'd keep the internals clean > and simple to maintain and only the protocol would be adjusted on the > wire, which is in fact what any agent should do. I really like this > approach! You'll the probably just need to define some PEERS_TABLE_TYPE_XXX > for example and map the table->type from SMP_T_* to those types and > conversely, that sounds really clean. Great! > Thanks, > Willy > Emeric
Re: Peers Protocol "Table Type"
Emeric, Am 20.03.20 um 14:29 schrieb Emeric Brun: > So I understand that since 1.6 the SMP_T are directly announced on the wire > for key types, and it brokes the documented values and this is hazardous to > rely on internal enum values. > > So we must re-introduce a mapping between internal and on-wire types. > > Some questions about choices: > > - Re-map types to documented values or Update the doc to match currently used > values? There's really only one sane choice after several years of not following the documentation: Update the documentation to match the currently used values. The peers protocol is HAProxy specific, so in practice the correct values are "what HAProxy does" (i.e. the protocol is defined by the reference implementation). The custom implementation during which I stumbled upon this issue is brand new and I needed to look into the code anyway, because the docs are incomplete (as I outlined before in this thread). Changing the code will cause larger breakage during a HAProxy bugfix upgrade if not all machines in a cluster are upgraded simultaneously. Best regards Tim Düsterhus
Re: Peers Protocol "Table Type"
Hi Emeric, On Fri, Mar 20, 2020 at 02:29:48PM +0100, Emeric Brun wrote: > So I understand that since 1.6 the SMP_T are directly announced on the wire > for key types, and it brokes the documented values and this is hazardous to > rely on internal enum values. Yes that's the issue Tim spotted. > So we must re-introduce a mapping between internal and on-wire types. Indeed. > Some questions about choices: > > - Re-map types to documented values or Update the doc to match currently used > values? I'd say that what's currently on the wire seems to more or less match the doc. Given that there are very few peers implementations, what's working right now seems the most important to me. So I'd suggest that we change the code to perform the mapping again and that we possibly update the doc in case it's wrong on some of them. > - About re-introduce the mapping, I'm not sure we should re-introduce the > mapping between fetches and tables types. Table types could stay internal, > using SMP_T... and we could map to protocol's values only when emitting the > message. I didn't think about this possibility but I actually think it might be the best one. The reason the types were changed was to simplify the resolution of casts in "stick" rules, because if you remember we used to have two types of casts in the past, sample-to-sample and sample-to-table. But with your suggestion we'd keep the internals clean and simple to maintain and only the protocol would be adjusted on the wire, which is in fact what any agent should do. I really like this approach! You'll the probably just need to define some PEERS_TABLE_TYPE_XXX for example and map the table->type from SMP_T_* to those types and conversely, that sounds really clean. Thanks, Willy
Re: Peers Protocol "Table Type"
On 3/14/20 12:47 PM, Willy Tarreau wrote: > On Sat, Mar 14, 2020 at 12:20:00PM +0100, Tim Düsterhus wrote: >> Willy, >> >> Am 14.03.20 um 12:13 schrieb Willy Tarreau: >>> Yes, feel free to do so, this will definitely help get it eventually done. >> >> Here it is: https://github.com/haproxy/haproxy/issues/548 >> >> I labeled it "bug", because it can be considered a bug that the >> on-the-wire format relies on some internal enum. > > Agreed, thanks! > Willy > So I understand that since 1.6 the SMP_T are directly announced on the wire for key types, and it brokes the documented values and this is hazardous to rely on internal enum values. So we must re-introduce a mapping between internal and on-wire types. Some questions about choices: - Re-map types to documented values or Update the doc to match currently used values? - About re-introduce the mapping, I'm not sure we should re-introduce the mapping between fetches and tables types. Table types could stay internal, using SMP_T... and we could map to protocol's values only when emitting the message. Your opinion? R, Emeric
Re: Peers Protocol "Table Type"
On Sat, Mar 14, 2020 at 12:20:00PM +0100, Tim Düsterhus wrote: > Willy, > > Am 14.03.20 um 12:13 schrieb Willy Tarreau: > > Yes, feel free to do so, this will definitely help get it eventually done. > > Here it is: https://github.com/haproxy/haproxy/issues/548 > > I labeled it "bug", because it can be considered a bug that the > on-the-wire format relies on some internal enum. Agreed, thanks! Willy
Re: Peers Protocol "Table Type"
Willy, Am 14.03.20 um 12:13 schrieb Willy Tarreau: > Yes, feel free to do so, this will definitely help get it eventually done. Here it is: https://github.com/haproxy/haproxy/issues/548 I labeled it "bug", because it can be considered a bug that the on-the-wire format relies on some internal enum. Best regards Tim Düsterhus
Re: Peers Protocol "Table Type"
On Sat, Mar 14, 2020 at 12:07:39PM +0100, Tim Düsterhus wrote: > Willy, > > Am 14.03.20 um 10:13 schrieb Willy Tarreau: > >> This is about the "Table Definition Message", more specifically the > >> "Encoded Table Type". > >> > >> docs/peers-v2.0.txt says: > > > > There's also doc/peers.txt which is more recent and more complete. I don't > > know if there are still some parts of peers-v2.0 which were not covered by > > peers.txt. > > Neither of them are sufficient to understand the protocol. Not even both > of them taken together. Some paragraphs are also pretty unclear. All in > all I had to read the code to understand what's happening. > > The peers.txt is more correct on what it says, but it's missing > essential information (e.g. what incremental updates are). > peers-v2.0.txt contains more information, but also more mistakes > (especially regarding the "magic numbers" such as the table types). I think peers-v2.0 was older but committed later to complement the missing info from the newer one. > > From now, the best solution likely is to check where the table type is > > used and instead go back to a table-specific type with hard-coded values > > matching what we have now. > > > > Should I file an issue for tracking that? Yes, feel free to do so, this will definitely help get it eventually done. Thanks, Willy
Re: Peers Protocol "Table Type"
Willy, Am 14.03.20 um 10:13 schrieb Willy Tarreau: >> This is about the "Table Definition Message", more specifically the >> "Encoded Table Type". >> >> docs/peers-v2.0.txt says: > > There's also doc/peers.txt which is more recent and more complete. I don't > know if there are still some parts of peers-v2.0 which were not covered by > peers.txt. Neither of them are sufficient to understand the protocol. Not even both of them taken together. Some paragraphs are also pretty unclear. All in all I had to read the code to understand what's happening. The peers.txt is more correct on what it says, but it's missing essential information (e.g. what incremental updates are). peers-v2.0.txt contains more information, but also more mistakes (especially regarding the "magic numbers" such as the table types). In short: There is much work to be done regarding those documents. I'm not sure whether I would recommend rewriting them from scratch, but that's probably easier than trying to fix them up. >> 1) Is my understanding correct that the type refers to SMP_T_*? > > I didn't think it was the case but it indeed looks like this is what > is being sent over the wire! I'm seeing in peers.c: > > /* encode table type */ > intencode(st->table->type, &cursor); > > Where table->type definitely is one of these SMP_T_* values. Okay, then I understood that line correctly. >> 2) Can I rely the order of the SMP_T_* enum *NOT* changing (i.e. new >> types are only added at the end)? Or rather: Is the peer protocol stable >> enough for third party implementations or can it change at will during >> HAProxy upgrades? > > No we shouldn't rely on this and we should instead change the peers code > to remap SMP_T_* to the on-wire value. I think this got broken a long > time ago when extending the sample types to support the ADDR type which > carries both IPv4 and IPv6, and nobody noticed that these types were > transported directly over the wire :-( Bingo, that was this patch > between 1.5 and 1.6 that merged the two: > > commit 5d24ebc3d70e7a0316d0954777a5f75a64fe7ac5 > Author: Thierry FOURNIER > Date: Fri Jul 24 08:46:42 2015 +0200 > > MEDIUM: stick-tables: use the sample type names > > This patch removes the special stick tables types names and > use the standard sample type names. This avoid the maintainance > of two types and remove the switch/case for matching a sample > type for each stick table type. > > So now from what I'm seeing, we should document that on the wire we > have this: > >2 = signed int >4 = IPv4 >5 = IPv6 >6 = string >7 = binary > Perfect, I'll use those values within my implementation. > From now, the best solution likely is to check where the table type is > used and instead go back to a table-specific type with hard-coded values > matching what we have now. > Should I file an issue for tracking that? Best regards Tim Düsterhus
Re: Peers Protocol "Table Type"
Hi Tim, On Wed, Mar 11, 2020 at 07:44:00PM +0100, Tim Düsterhus wrote: > Dear List > > I'm currently working a Peer Protocol implementation to passively > monitor what's happening within HAProxy. > > The protocol specification is horribly underdocumented (docs/peers.txt > is incomplete) and I believe I found a mistake in the existing > documentation: > > This is about the "Table Definition Message", more specifically the > "Encoded Table Type". > > docs/peers-v2.0.txt says: There's also doc/peers.txt which is more recent and more complete. I don't know if there are still some parts of peers-v2.0 which were not covered by peers.txt. > > Table Type present the numeric type of key used to store stick table > > entries: > > integer > > 0: signed integer > > 1: IPv4 address > > 2: IPv6 address > > 3: string > > 4: binary > > But I'm seeing the value 6 for string. After reading the source code the > type actually appears a member of SMP_T_* which would match up. I didn't remember that we remerged the types. Originally there were types for stick tables and types for samples, with an implicit cast between the two. The translation from a sample to a stick table key is done by smp_to_stkey(). > Two questions: > > 1) Is my understanding correct that the type refers to SMP_T_*? I didn't think it was the case but it indeed looks like this is what is being sent over the wire! I'm seeing in peers.c: /* encode table type */ intencode(st->table->type, &cursor); Where table->type definitely is one of these SMP_T_* values. > 2) Can I rely the order of the SMP_T_* enum *NOT* changing (i.e. new > types are only added at the end)? Or rather: Is the peer protocol stable > enough for third party implementations or can it change at will during > HAProxy upgrades? No we shouldn't rely on this and we should instead change the peers code to remap SMP_T_* to the on-wire value. I think this got broken a long time ago when extending the sample types to support the ADDR type which carries both IPv4 and IPv6, and nobody noticed that these types were transported directly over the wire :-( Bingo, that was this patch between 1.5 and 1.6 that merged the two: commit 5d24ebc3d70e7a0316d0954777a5f75a64fe7ac5 Author: Thierry FOURNIER Date: Fri Jul 24 08:46:42 2015 +0200 MEDIUM: stick-tables: use the sample type names This patch removes the special stick tables types names and use the standard sample type names. This avoid the maintainance of two types and remove the switch/case for matching a sample type for each stick table type. So now from what I'm seeing, we should document that on the wire we have this: 2 = signed int 4 = IPv4 5 = IPv6 6 = string 7 = binary >From now, the best solution likely is to check where the table type is used and instead go back to a table-specific type with hard-coded values matching what we have now. Thanks for spotting this! Willy
