extented haproxy protocol support #3731

sergey-safarov · 2024-01-28T19:26:10Z

Pre-Submission Checklist

Commit message has the format required by CONTRIBUTING guide
Commits are split per component (core, individual modules, libs, utils, ...)
Each component has a single commit (if not, squash them into one commit)
No commits to README files for modules (changes must be done to docbook files
in doc/ subfolder, the README file is autogenerated)

Type Of Change

Small bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds new functionality)
Breaking change (fix or feature that would change existing functionality)

Checklist:

PR should be backported to stable branches
Tested changes locally
Related to issue Redefine Advertised socket URI when used haproxy protocol #3667

Description

Allow creating Record-Route and Via header using destination address and port in the haproxy protocol header.

PR created behalf @ivanuschak

sergey-safarov · 2024-01-28T19:33:55Z

We work to clean warning

CC (gcc) [kamailio]    core/daemonize.o
CC (gcc) [kamailio]    core/msg_translator.o
core/msg_translator.c: In function ‘lumps_len’:
core/msg_translator.c:558:6: warning: variable ‘recv_port_no’ set but not used [-Wunused-but-set-variable]
  int recv_port_no = 0;
      ^~~~~~~~~~~~
core/msg_translator.c: In function ‘process_lumps’:
core/msg_translator.c:974:6: warning: variable ‘recv_port_no’ set but not used [-Wunused-but-set-variable]
  int recv_port_no = 0;
      ^~~~~~~~~~~~
CC (gcc) [kamailio]    core/fmsg.o

miconda · 2024-01-29T21:26:12Z

src/core/forward.h

+		if(!dst->id) {
+			dst->id = con->id;
+		}
+


Any more details about why this is needed? I cannot easy relate it to the commit message.

This change is needed for siptrace. Below in this file at the line 334 dst is passed as an argument to the SREV_NET_DATA_SENT related function call.

If the tcp connection ID is assigned to dst param then the tcp_connection search is correct and we have a proper siptrace with HAPROXY IP addresses (IP address of client and IP address of HAPROXY) , otherwise tcp connection search is based on real IP addresses (IP addresses of haproxy and IP address of kamailio) and in this case the siptrace contains not the correct IP-addresses.

src/core/msg_translator.c

miconda · 2024-01-29T21:30:07Z

src/core/msg_translator.c

@@ -2845,6 +2876,47 @@ int branch_builder(unsigned int hash_index,
 	return size;
 }

+static int is_haproxy(struct dest_info *send_info, struct receive_info *haproxy_rcv) {


Can you add some comment to describe the purpose of the function? The name is rather generic, from C code point of view it looks like it tries to figure out if the target via via haproxy...

The function goal is as follows:

it retrieves the tcp_connetion_id from the dest_info parameter

if this TCP connection is a haproxy connection, then the haproxy_rcv param is filled with the corresponding haproxy data and the function returns non-zero value

otherwise if the TCP connection is not a connection to haproxy then the function returns zero value.

We will add the comments for this function in the source code.

miconda · 2024-01-29T21:36:23Z

src/core/parser/msg_parser.h

@@ -396,6 +396,7 @@ typedef struct sip_msg
 	char *unparsed; /*!< here we stopped parsing*/

 	struct receive_info rcv; /*!< source & dest ip, ports, proto a.s.o*/
+	struct receive_info haproxy_rcv; /*!< source & dest ip, ports, proto a.s.o for the message from haproxy*/


Traffic via haproxy is not very common, does it make sense to add a full value-structure for every message instead of a pointer that is allocated only when it is the case. Practically the size of message increases for common traffic over direct UDP/TCP/TLS/SCTP/WebSocket. On the other hand, cloning in shm has to be done if allocated. This is just to ponder on what would be better...

Thanks for highlighting that.
We will check what would be less expensive in terms of resource usage - to have a full value-structure, or to have a pointer that needs to be cloned in shm.

miconda · 2024-01-29T21:38:10Z

src/core/tcp_read.c

-			local_timer_add(&tcp_reader_ltimer, &con->timer,
-					S_TO_TICKS(TCP_CHILD_TIMEOUT), t);
+			if(con->rcv.proto_reserved2 && con->type == PROTO_WSS) {
+				//not setting up timers for haproxy WSS connections


The patch is quite consistent to important parts of the core. It has to be reviewed properly, so far I did a quick walk through, other developers are welcome to jump in.

Please pay attention at this particular change - from line 1846 to line 1856.

The purpose of this change is to keep the WSS connection connected. When testing this haproxy change without this particular piece of change (from line 1846 to line 1856) we observed that kamailio was dropping WSS connection by timer, and this drop looked strange, because it happened in about 5-7 seconds after the connection was established. One of the requirement for haproxy was to keep TLS/WSS connection established during all the dialog and not to drop it without any important reason. I tried to find how to make it not to be dropped by timer, but found the only way is to not to set the timer up for WSS. Also the drop cause is still not cleat for me.
I don't think this is a good solution, but I was not able to find a better one. I would appreciate if you check this change attentively. If needed I can provide with the logs where kamailio drops WSS connection.

linuxmaniac · 2024-01-30T11:21:18Z

I was thinking. Could we have something generic that abstracts the rcv parameter of the message?

miconda · 2024-02-23T11:40:50Z

Indeed, it would be good to try to find a more generic way to approach this kind of needs to store intermediary hop address. The patch is rather consistent for the core part and couldn't assert the impact so far.

…cessing - updated TCP connection selection when relaying through haproxy - Via header content is changed onto haproxy address if haproxy is used

github-actions · 2024-05-01T02:41:46Z

This PR is stale because it has been open 6 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.

sergey-safarov requested review from linuxmaniac and miconda January 28, 2024 19:26

miconda reviewed Jan 29, 2024

View reviewed changes

sergey-safarov force-pushed the gh3683 branch 2 times, most recently from 361326d to b0efcbe Compare March 18, 2024 19:36

ivanuschak and others added 2 commits March 18, 2024 21:38

core, rr, path: updated relaying logic when haproxy is used

bf71af1

fixed transport via haproxy

383015a

sergey-safarov force-pushed the gh3683 branch from b0efcbe to 4e70f50 Compare March 18, 2024 19:38

core: updated TCP connection selection for haproxy and Via header pro…

929854e

…cessing - updated TCP connection selection when relaying through haproxy - Via header content is changed onto haproxy address if haproxy is used

sergey-safarov force-pushed the gh3683 branch from 4e70f50 to 444e422 Compare March 18, 2024 19:41

ivanuschak added 2 commits March 18, 2024 21:47

siptrace: fixed siptrace when tcp haproxy is used

fc83552

siprec for haproxy fix

37f1b63

sergey-safarov force-pushed the gh3683 branch from 444e422 to 37f1b63 Compare March 18, 2024 20:24

github-actions bot added the Stale label May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extented haproxy protocol support #3731

extented haproxy protocol support #3731

sergey-safarov commented Jan 28, 2024

sergey-safarov commented Jan 28, 2024

miconda Jan 29, 2024

ivanuschak Jan 30, 2024

miconda Jan 29, 2024

ivanuschak Jan 30, 2024

miconda Jan 29, 2024

ivanuschak Jan 30, 2024

miconda Jan 29, 2024

ivanuschak Jan 30, 2024

linuxmaniac commented Jan 30, 2024

miconda commented Feb 23, 2024

github-actions bot commented May 1, 2024

extented haproxy protocol support #3731

Are you sure you want to change the base?

extented haproxy protocol support #3731

Conversation

sergey-safarov commented Jan 28, 2024

Pre-Submission Checklist

Type Of Change

Checklist:

Description

sergey-safarov commented Jan 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linuxmaniac commented Jan 30, 2024

miconda commented Feb 23, 2024

github-actions bot commented May 1, 2024