| Message ID | 20171222195337.28983-1-tiagolam@gmail.com |
|---|---|
| Headers | show |
| Series | Initial support for new SIP Alg. | expand |
Tiago Lam <tiagolam@gmail.com> writes: > This patch-set is an initial approach at implementing the new SIP Alg, > mentioned by Aaron at [1]. Thanks for this work, Tiago! > I'm mostly interested in getting to know your thoughts of how this is > headed. There are a couple of points that are worth bringing up: > - As mentioned in patches 1/3 and 2/3, this is still a preliminary > implementation, and some work will be needed to move away from some > assuptions, like assuming the SIP traffic is always going over IPv4 > and TCP; > - At the moment, the sip state is being stored in the conn struct. I > followed the example of seq_skew_dir here, which is also stored there, > but realise this is not ideal. It seems storing it somewhere agnostic > will be ideal in the future, to avoid polluting that struct with > different Alg's details; > - The SIP helpers functions and structures are in conntrack-sip.h and > conntrack-sip.c. This can create confusion when comparing to > conntrack-tcp.c and other protocols since SIP is an Alg and is at a > different level. > > With regards to testing, for now, this has been tested manually, by > setting up the flows mentioned in patch 2/3 and having two VMs connected > to OvS, both using SIPp to simulate real traffic both ways. I'm going to > have a look at how this can be automated and added to > tests/system-traffic.at, together with the rest of the already existing > tests. Please do. I would consider the test-suite an important part for acceptance. > [1] [CONNTRACK] Discussions at OvS 2017: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341089.html > > Tiago Lam (3): > Conntrack: Add new API for future SIP Alg. > Conntrack: Add initial support for new SIP Alg. > Conntrack: Support asymmetric RTP port for SIP. > > include/openvswitch/ofp-actions.h | 4 + > lib/automake.mk | 2 + > lib/conntrack-private.h | 2 + > lib/conntrack-sip.c | 491 ++++++++++++++++++++++++++++++++++++++ > lib/conntrack-sip.h | 123 ++++++++++ > lib/conntrack.c | 254 +++++++++++++++++++- > lib/ofp-parse.c | 5 + > ofproto/ofproto-dpif-xlate.c | 3 + > 8 files changed, 883 insertions(+), 1 deletion(-) > create mode 100644 lib/conntrack-sip.c > create mode 100644 lib/conntrack-sip.h
Thanks for the series/work; I’ll be reviewing this series, but focusing on the high level aspects initially.
Some high level comments:
I noticed that your own comments in the series often pointed out various issues with this series, such as
assuming TCP transport, which is ‘unusual’, no NAT support, single media session only, lack of testing support etc.
Although SIP does not store as much dynamic state as other algs, it still needs to have a separate hidden portion of memory
for that. Embedding that in struct conn at top level is ‘not ideal’.
As I mentioned I would in the below referred thread, I sent a patch to support more algs, including SIP.
https://patchwork.ozlabs.org/patch/853010/
My patch is generic plumbing for additional algs, including NAT aspects, for internal future alg requirements and SIP.
It will help this series and remove a bunch of duplicated code.
Patch 2: sip_delete_conn() should not exist; when a conn is deleted, just check for SIP state to clean and call a SIP function
Same idea for sip_set_conn_state()
handle_sip() should be in conntrack-sip.c
sip_expectation_create() – remove function and use my patch, calling with correct arguments since it is more
general and handles NAT.
Patch 3 should not be a separate patch; it is basic SIP and clearer to fold into patch 2.
I did not review Patch 1 yet in enough depth to provide useful comments.
BTW, I already have some testing infra for SIP, but I am not sure I will submit soon since I am doing something else at the
moment. I’ll hold off on this for now.
Maybe an RFC label would be better initially.
Darrell
On 12/22/17, 11:54 AM, "ovs-dev-bounces@openvswitch.org on behalf of Tiago Lam" <ovs-dev-bounces@openvswitch.org on behalf of tiagolam@gmail.com> wrote:
This patch-set is an initial approach at implementing the new SIP Alg,
mentioned by Aaron at [1].
I'm mostly interested in getting to know your thoughts of how this is
headed. There are a couple of points that are worth bringing up:
- As mentioned in patches 1/3 and 2/3, this is still a preliminary
implementation, and some work will be needed to move away from some
assuptions, like assuming the SIP traffic is always going over IPv4
and TCP;
- At the moment, the sip state is being stored in the conn struct. I
followed the example of seq_skew_dir here, which is also stored there,
but realise this is not ideal. It seems storing it somewhere agnostic
will be ideal in the future, to avoid polluting that struct with
different Alg's details;
Embed it deeper into struct conn as pointer to SIP stuff
- The SIP helpers functions and structures are in conntrack-sip.h and
conntrack-sip.c. This can create confusion when comparing to
conntrack-tcp.c and other protocols since SIP is an Alg and is at a
different level.
SIP is complex and deserves separate files.
I don’t think it is confusing – it is fine.
With regards to testing, for now, this has been tested manually, by
setting up the flows mentioned in patch 2/3 and having two VMs connected
to OvS, both using SIPp to simulate real traffic both ways. I'm going to
have a look at how this can be automated and added to
tests/system-traffic.at, together with the rest of the already existing
tests.
[1] [CONNTRACK] Discussions at OvS 2017:
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017-2DNovember_341089.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=DDHX2MTCsXS7GD8ie27aEdUDgGRK2EIntHQAxtrkWmI&s=md5csJDVqD97O6SvpYWNjbuQZYN2sfYKe4cF1-dzt1A&e=
Tiago Lam (3):
Conntrack: Add new API for future SIP Alg.
Conntrack: Add initial support for new SIP Alg.
Conntrack: Support asymmetric RTP port for SIP.
include/openvswitch/ofp-actions.h | 4 +
lib/automake.mk | 2 +
lib/conntrack-private.h | 2 +
lib/conntrack-sip.c | 491 ++++++++++++++++++++++++++++++++++++++
lib/conntrack-sip.h | 123 ++++++++++
lib/conntrack.c | 254 +++++++++++++++++++-
lib/ofp-parse.c | 5 +
ofproto/ofproto-dpif-xlate.c | 3 +
8 files changed, 883 insertions(+), 1 deletion(-)
create mode 100644 lib/conntrack-sip.c
create mode 100644 lib/conntrack-sip.h
--
2.14.3
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=DDHX2MTCsXS7GD8ie27aEdUDgGRK2EIntHQAxtrkWmI&s=ouDOBTdmowKrm1jxoMzaRvMz2-dkf_rTN2DyWa6_P_c&e=
Hi Darrell, Thanks for your initial review. I'll work on incorporating your comments into v2 (which I'm planning to submit already with some tests). A couple of small comments are in-line. On 01/05/2018 06:40 AM, Darrell Ball wrote: > Thanks for the series/work; I’ll be reviewing this series, but focusing on the high level aspects initially. > > Some high level comments: > > I noticed that your own comments in the series often pointed out various issues with this series, such as > assuming TCP transport, which is ‘unusual’, no NAT support, single media session only, lack of testing support etc. > > Although SIP does not store as much dynamic state as other algs, it still needs to have a separate hidden portion of memory > for that. Embedding that in struct conn at top level is ‘not ideal’. > > As I mentioned I would in the below referred thread, I sent a patch to support more algs, including SIP. > https://patchwork.ozlabs.org/patch/853010/ > My patch is generic plumbing for additional algs, including NAT aspects, for internal future alg requirements and SIP. > It will help this series and remove a bunch of duplicated code. That was going to be part of my next iteration as well - I've seen you just sent v3 of the series, so I'll work based on that. I mainly wanted to get what I had "out there" in order to start syncing, so worked mainly based on master (and the previous "conntrack: Alg improvements." patch that was merged back in December, https://patchwork.ozlabs.org/cover/844311/). > Patch 2: sip_delete_conn() should not exist; when a conn is deleted, just check for SIP state to clean and call a SIP function > Same idea for sip_set_conn_state() > handle_sip() should be in conntrack-sip.c > sip_expectation_create() – remove function and use my patch, calling with correct arguments since it is more > general and handles NAT. > > Patch 3 should not be a separate patch; it is basic SIP and clearer to fold into patch 2. > Sure, I'll merge that (3/3) into 2/3 in v2 as well. > I did not review Patch 1 yet in enough depth to provide useful comments. > > BTW, I already have some testing infra for SIP, but I am not sure I will submit soon since I am doing something else at the > moment. I’ll hold off on this for now. > > Maybe an RFC label would be better initially. Good point. Thought of doing it but then missed it when actually sending. Will do in v2, thanks. > > Darrell > > > On 12/22/17, 11:54 AM, "ovs-dev-bounces@openvswitch.org on behalf of Tiago Lam" <ovs-dev-bounces@openvswitch.org on behalf of tiagolam@gmail.com> wrote: > > This patch-set is an initial approach at implementing the new SIP Alg, > mentioned by Aaron at [1]. > > I'm mostly interested in getting to know your thoughts of how this is > headed. There are a couple of points that are worth bringing up: > - As mentioned in patches 1/3 and 2/3, this is still a preliminary > implementation, and some work will be needed to move away from some > assuptions, like assuming the SIP traffic is always going over IPv4 > and TCP; > - At the moment, the sip state is being stored in the conn struct. I > followed the example of seq_skew_dir here, which is also stored there, > but realise this is not ideal. It seems storing it somewhere agnostic > will be ideal in the future, to avoid polluting that struct with > different Alg's details; > > Embed it deeper into struct conn as pointer to SIP stuff That's what I've at the moment, having set a sip_state struct for that. Tiago. > > - The SIP helpers functions and structures are in conntrack-sip.h and > conntrack-sip.c. This can create confusion when comparing to > conntrack-tcp.c and other protocols since SIP is an Alg and is at a > different level. > > SIP is complex and deserves separate files. > I don’t think it is confusing – it is fine. > > > With regards to testing, for now, this has been tested manually, by > setting up the flows mentioned in patch 2/3 and having two VMs connected > to OvS, both using SIPp to simulate real traffic both ways. I'm going to > have a look at how this can be automated and added to > tests/system-traffic.at, together with the rest of the already existing > tests. > > [1] [CONNTRACK] Discussions at OvS 2017: > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017-2DNovember_341089.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=DDHX2MTCsXS7GD8ie27aEdUDgGRK2EIntHQAxtrkWmI&s=md5csJDVqD97O6SvpYWNjbuQZYN2sfYKe4cF1-dzt1A&e=
On 12/22/2017 01:53 PM, Tiago Lam wrote: > This patch-set is an initial approach at implementing the new SIP Alg, > mentioned by Aaron at [1]. > > I'm mostly interested in getting to know your thoughts of how this is > headed. There are a couple of points that are worth bringing up: > - As mentioned in patches 1/3 and 2/3, this is still a preliminary > implementation, and some work will be needed to move away from some > assuptions, like assuming the SIP traffic is always going over IPv4 > and TCP; > - At the moment, the sip state is being stored in the conn struct. I > followed the example of seq_skew_dir here, which is also stored there, > but realise this is not ideal. It seems storing it somewhere agnostic > will be ideal in the future, to avoid polluting that struct with > different Alg's details; > - The SIP helpers functions and structures are in conntrack-sip.h and > conntrack-sip.c. This can create confusion when comparing to > conntrack-tcp.c and other protocols since SIP is an Alg and is at a > different level. > > With regards to testing, for now, this has been tested manually, by > setting up the flows mentioned in patch 2/3 and having two VMs connected > to OvS, both using SIPp to simulate real traffic both ways. I'm going to > have a look at how this can be automated and added to > tests/system-traffic.at, together with the rest of the already existing > tests. > > [1] [CONNTRACK] Discussions at OvS 2017: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341089.html > > Tiago Lam (3): > Conntrack: Add new API for future SIP Alg. > Conntrack: Add initial support for new SIP Alg. > Conntrack: Support asymmetric RTP port for SIP. > > include/openvswitch/ofp-actions.h | 4 + > lib/automake.mk | 2 + > lib/conntrack-private.h | 2 + > lib/conntrack-sip.c | 491 ++++++++++++++++++++++++++++++++++++++ > lib/conntrack-sip.h | 123 ++++++++++ > lib/conntrack.c | 254 +++++++++++++++++++- > lib/ofp-parse.c | 5 + > ofproto/ofproto-dpif-xlate.c | 3 + > 8 files changed, 883 insertions(+), 1 deletion(-) > create mode 100644 lib/conntrack-sip.c > create mode 100644 lib/conntrack-sip.h > Hi Tiago, Before starting in my current role, I worked for ten years doing VoIP development with a strong focus on SIP. SIP is a beast of a protocol, and seeing this patchset, I made some notes about how things might possibly go wrong, and ... let's just say there are quite a few :). I know that you are writing this as a proof of concept and that you have many TODO-style comments owning up to the fact that you know about some things that are missing, but I think it may go deeper than you or anyone on the OVS development team realize. I'm willing to help out in this effort by donating SIP knowledge, but before that, I'm curious what the end goal of the SIP ALG actually is. I have a feeling that the effort and expense of writing, and more importantly maintaining, a proper SIP ALG may not be worth it. As a side point, when I was working on VoIP software, one of the first things we'd tell people who came to us with connectivity issues was "turn off the SIP ALG in your router". This on its own resolved issues with an alarming frequency. Mark
On 1/9/18, 11:33 AM, "ovs-dev-bounces@openvswitch.org on behalf of Mark Michelson" <ovs-dev-bounces@openvswitch.org on behalf of mmichels@redhat.com> wrote:
On 12/22/2017 01:53 PM, Tiago Lam wrote:
> This patch-set is an initial approach at implementing the new SIP Alg,
> mentioned by Aaron at [1].
>
> I'm mostly interested in getting to know your thoughts of how this is
> headed. There are a couple of points that are worth bringing up:
> - As mentioned in patches 1/3 and 2/3, this is still a preliminary
> implementation, and some work will be needed to move away from some
> assuptions, like assuming the SIP traffic is always going over IPv4
> and TCP;
> - At the moment, the sip state is being stored in the conn struct. I
> followed the example of seq_skew_dir here, which is also stored there,
> but realise this is not ideal. It seems storing it somewhere agnostic
> will be ideal in the future, to avoid polluting that struct with
> different Alg's details;
> - The SIP helpers functions and structures are in conntrack-sip.h and
> conntrack-sip.c. This can create confusion when comparing to
> conntrack-tcp.c and other protocols since SIP is an Alg and is at a
> different level.
>
> With regards to testing, for now, this has been tested manually, by
> setting up the flows mentioned in patch 2/3 and having two VMs connected
> to OvS, both using SIPp to simulate real traffic both ways. I'm going to
> have a look at how this can be automated and added to
> tests/system-traffic.at, together with the rest of the already existing
> tests.
>
> [1] [CONNTRACK] Discussions at OvS 2017:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017-2DNovember_341089.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=XHeuwSeMKQzqBJ1r6eAv0vsWJV6qkTRe0-B_iqUOl7Y&s=IDGICUZlL3v-yH_pfcZvMh00v6bCssbJ3bn-JvDuqWo&e=
>
> Tiago Lam (3):
> Conntrack: Add new API for future SIP Alg.
> Conntrack: Add initial support for new SIP Alg.
> Conntrack: Support asymmetric RTP port for SIP.
>
> include/openvswitch/ofp-actions.h | 4 +
> lib/automake.mk | 2 +
> lib/conntrack-private.h | 2 +
> lib/conntrack-sip.c | 491 ++++++++++++++++++++++++++++++++++++++
> lib/conntrack-sip.h | 123 ++++++++++
> lib/conntrack.c | 254 +++++++++++++++++++-
> lib/ofp-parse.c | 5 +
> ofproto/ofproto-dpif-xlate.c | 3 +
> 8 files changed, 883 insertions(+), 1 deletion(-)
> create mode 100644 lib/conntrack-sip.c
> create mode 100644 lib/conntrack-sip.h
>
Hi Tiago,
Before starting in my current role, I worked for ten years doing VoIP
development with a strong focus on SIP.
SIP is a beast of a protocol, and seeing this patchset, I made some
notes about how things might possibly go wrong, and ... let's just say
there are quite a few :). I know that you are writing this as a proof of
concept and that you have many TODO-style comments owning up to the fact
that you know about some things that are missing, but I think it may go
deeper than you or anyone on the OVS development team realize.
That is definitely an assumption…
I'm willing to help out in this effort by donating SIP knowledge, but
before that, I'm curious what the end goal of the SIP ALG actually is. I
have a feeling that the effort and expense of writing, and more
importantly maintaining, a proper SIP ALG may not be worth it.
As a side point, when I was working on VoIP software, one of the first
things we'd tell people who came to us with connectivity issues was
"turn off the SIP ALG in your router". This on its own resolved issues
with an alarming frequency.
Not a surprise; this was brought up internally when SIP support was first mentioned by Redhat.
At a minimum, SIP ALGs would have a specific knob to enable them globally.
Mark
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=XHeuwSeMKQzqBJ1r6eAv0vsWJV6qkTRe0-B_iqUOl7Y&s=nXBmkfSg5YmenXwzNh-0RDz6C2MEB--PZaZpvHc0E2U&e=
Hi Mark, Your email strikes me as if you haven't read the previous email exchange at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341089.html (TL;DR I've asked similar questions there) Having said that, and knowing this is happening, my intention is exactly this; Contribute with knowledge, where and how I can, in the form of code or otherwise, _to help_ bring that into life. I'm sure no-one is underestimating how complex SIP is, I certainly am not. As a side note, it is also true that we are not looking into writing a complete SIP stack, but something that allows us to verify the exchanged traffic; It is my understanding that many shortcuts can be taken with this in mind. Hopefully this, together with the email I pointed to, clarifies things a bit better. Tiago On 01/09/2018 07:33 PM, Mark Michelson wrote: > On 12/22/2017 01:53 PM, Tiago Lam wrote: >> This patch-set is an initial approach at implementing the new SIP Alg, >> mentioned by Aaron at [1]. >> >> I'm mostly interested in getting to know your thoughts of how this is >> headed. There are a couple of points that are worth bringing up: >> - As mentioned in patches 1/3 and 2/3, this is still a preliminary >> implementation, and some work will be needed to move away from some >> assuptions, like assuming the SIP traffic is always going over IPv4 >> and TCP; >> - At the moment, the sip state is being stored in the conn struct. I >> followed the example of seq_skew_dir here, which is also stored there, >> but realise this is not ideal. It seems storing it somewhere agnostic >> will be ideal in the future, to avoid polluting that struct with >> different Alg's details; >> - The SIP helpers functions and structures are in conntrack-sip.h and >> conntrack-sip.c. This can create confusion when comparing to >> conntrack-tcp.c and other protocols since SIP is an Alg and is at a >> different level. >> >> With regards to testing, for now, this has been tested manually, by >> setting up the flows mentioned in patch 2/3 and having two VMs connected >> to OvS, both using SIPp to simulate real traffic both ways. I'm going to >> have a look at how this can be automated and added to >> tests/system-traffic.at, together with the rest of the already existing >> tests. >> >> [1] [CONNTRACK] Discussions at OvS 2017: >> https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341089.html >> >> Tiago Lam (3): >> Conntrack: Add new API for future SIP Alg. >> Conntrack: Add initial support for new SIP Alg. >> Conntrack: Support asymmetric RTP port for SIP. >> >> include/openvswitch/ofp-actions.h | 4 + >> lib/automake.mk | 2 + >> lib/conntrack-private.h | 2 + >> lib/conntrack-sip.c | 491 ++++++++++++++++++++++++++++++++++++++ >> lib/conntrack-sip.h | 123 ++++++++++ >> lib/conntrack.c | 254 +++++++++++++++++++- >> lib/ofp-parse.c | 5 + >> ofproto/ofproto-dpif-xlate.c | 3 + >> 8 files changed, 883 insertions(+), 1 deletion(-) >> create mode 100644 lib/conntrack-sip.c >> create mode 100644 lib/conntrack-sip.h >> > > Hi Tiago, > > Before starting in my current role, I worked for ten years doing VoIP > development with a strong focus on SIP. > > SIP is a beast of a protocol, and seeing this patchset, I made some > notes about how things might possibly go wrong, and ... let's just say > there are quite a few :). I know that you are writing this as a proof of > concept and that you have many TODO-style comments owning up to the fact > that you know about some things that are missing, but I think it may go > deeper than you or anyone on the OVS development team realize. > > I'm willing to help out in this effort by donating SIP knowledge, but > before that, I'm curious what the end goal of the SIP ALG actually is. I > have a feeling that the effort and expense of writing, and more > importantly maintaining, a proper SIP ALG may not be worth it. > > As a side point, when I was working on VoIP software, one of the first > things we'd tell people who came to us with connectivity issues was > "turn off the SIP ALG in your router". This on its own resolved issues > with an alarming frequency. > > Mark >
Hi Tiago, Apologies if my message came across as dismissive or offensive (and to you as well, Darrell). I realize after re-reading it that I could have stated my intentions better. I never intended to discourage you in what you are contributing. Thanks for the mailing list link. I had missed that conversation. I read through the thread and it sounds like the main goal is to be able to associate potential media addresses/ports with the corresponding signaling. Several of the scenarios I thought of yesterday (forking, early media, SIP UPDATE, RTCP-MUX, and BUNDLE) are not as problematic as I had previously thought. My concern had been that any of the above could change the addresses used for media beyond a simple reading of an SDP. However, for conntrack purposes, this isn't probably that big a deal since you're not trying to zero in on *the* exact negotiated addresses that will be used for media. You're more concerned with being prepared for what addresses/ports potentially will be used for media and associating traffic on those addresses/ports appropriately. If you're prepared to handle traffic from some address/port but don't actually receive any media from there, it's not a big issue. The biggest item in my list that still could be of concern is ICE. With ICE, endpoints list candidate addresses and ports to use for media and then negotiate with each other to find the best candidate pair to use. So for conntrack purposes, this would require some more in-depth SDP parsing. The rest of my notes are on more general findings in the code submission itself (and in some cases, you've acknowledged the potential problems yourself). I'll perform a code review sometime soon with this information. What's important is clearly defining the scope and use cases that will be supported. For a traditional telephone scenario, what you have written is on the right track. If we have no intentions of supporting more "advanced" scenarios (e.g. multiple media streams, WebRTC, ICE) then that's fine, as long as we're clear about it. On 01/10/2018 05:47 AM, Tiago Lam wrote: > Hi Mark, > > Your email strikes me as if you haven't read the previous email exchange > at: > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341089.html > > (TL;DR I've asked similar questions there) > > Having said that, and knowing this is happening, my intention is exactly > this; Contribute with knowledge, where and how I can, in the form of > code or otherwise, _to help_ bring that into life. > > I'm sure no-one is underestimating how complex SIP is, I certainly am > not. As a side note, it is also true that we are not looking into > writing a complete SIP stack, but something that allows us to verify the > exchanged traffic; It is my understanding that many shortcuts can be > taken with this in mind. > > Hopefully this, together with the email I pointed to, clarifies things a > bit better. > > Tiago > > On 01/09/2018 07:33 PM, Mark Michelson wrote: >> On 12/22/2017 01:53 PM, Tiago Lam wrote: >>> This patch-set is an initial approach at implementing the new SIP Alg, >>> mentioned by Aaron at [1]. >>> >>> I'm mostly interested in getting to know your thoughts of how this is >>> headed. There are a couple of points that are worth bringing up: >>> - As mentioned in patches 1/3 and 2/3, this is still a preliminary >>> implementation, and some work will be needed to move away from some >>> assuptions, like assuming the SIP traffic is always going over IPv4 >>> and TCP; >>> - At the moment, the sip state is being stored in the conn struct. I >>> followed the example of seq_skew_dir here, which is also stored >>> there, >>> but realise this is not ideal. It seems storing it somewhere >>> agnostic >>> will be ideal in the future, to avoid polluting that struct with >>> different Alg's details; >>> - The SIP helpers functions and structures are in conntrack-sip.h and >>> conntrack-sip.c. This can create confusion when comparing to >>> conntrack-tcp.c and other protocols since SIP is an Alg and is at a >>> different level. >>> >>> With regards to testing, for now, this has been tested manually, by >>> setting up the flows mentioned in patch 2/3 and having two VMs connected >>> to OvS, both using SIPp to simulate real traffic both ways. I'm going to >>> have a look at how this can be automated and added to >>> tests/system-traffic.at, together with the rest of the already existing >>> tests. >>> >>> [1] [CONNTRACK] Discussions at OvS 2017: >>> >>> https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341089.html >>> >>> Tiago Lam (3): >>> Conntrack: Add new API for future SIP Alg. >>> Conntrack: Add initial support for new SIP Alg. >>> Conntrack: Support asymmetric RTP port for SIP. >>> >>> include/openvswitch/ofp-actions.h | 4 + >>> lib/automake.mk | 2 + >>> lib/conntrack-private.h | 2 + >>> lib/conntrack-sip.c | 491 >>> ++++++++++++++++++++++++++++++++++++++ >>> lib/conntrack-sip.h | 123 ++++++++++ >>> lib/conntrack.c | 254 +++++++++++++++++++- >>> lib/ofp-parse.c | 5 + >>> ofproto/ofproto-dpif-xlate.c | 3 + >>> 8 files changed, 883 insertions(+), 1 deletion(-) >>> create mode 100644 lib/conntrack-sip.c >>> create mode 100644 lib/conntrack-sip.h >>> >> >> Hi Tiago, >> >> Before starting in my current role, I worked for ten years doing VoIP >> development with a strong focus on SIP. >> >> SIP is a beast of a protocol, and seeing this patchset, I made some >> notes about how things might possibly go wrong, and ... let's just say >> there are quite a few :). I know that you are writing this as a proof of >> concept and that you have many TODO-style comments owning up to the fact >> that you know about some things that are missing, but I think it may go >> deeper than you or anyone on the OVS development team realize. >> >> I'm willing to help out in this effort by donating SIP knowledge, but >> before that, I'm curious what the end goal of the SIP ALG actually is. I >> have a feeling that the effort and expense of writing, and more >> importantly maintaining, a proper SIP ALG may not be worth it. >> >> As a side point, when I was working on VoIP software, one of the first >> things we'd tell people who came to us with connectivity issues was >> "turn off the SIP ALG in your router". This on its own resolved issues >> with an alarming frequency. >> >> Mark >>
On 1/9/18, 12:44 PM, "ovs-dev-bounces@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf of dball@vmware.com> wrote:
On 1/9/18, 11:33 AM, "ovs-dev-bounces@openvswitch.org on behalf of Mark Michelson" <ovs-dev-bounces@openvswitch.org on behalf of mmichels@redhat.com> wrote:
On 12/22/2017 01:53 PM, Tiago Lam wrote:
> This patch-set is an initial approach at implementing the new SIP Alg,
> mentioned by Aaron at [1].
>
> I'm mostly interested in getting to know your thoughts of how this is
> headed. There are a couple of points that are worth bringing up:
> - As mentioned in patches 1/3 and 2/3, this is still a preliminary
> implementation, and some work will be needed to move away from some
> assuptions, like assuming the SIP traffic is always going over IPv4
> and TCP;
> - At the moment, the sip state is being stored in the conn struct. I
> followed the example of seq_skew_dir here, which is also stored there,
> but realise this is not ideal. It seems storing it somewhere agnostic
> will be ideal in the future, to avoid polluting that struct with
> different Alg's details;
> - The SIP helpers functions and structures are in conntrack-sip.h and
> conntrack-sip.c. This can create confusion when comparing to
> conntrack-tcp.c and other protocols since SIP is an Alg and is at a
> different level.
>
> With regards to testing, for now, this has been tested manually, by
> setting up the flows mentioned in patch 2/3 and having two VMs connected
> to OvS, both using SIPp to simulate real traffic both ways. I'm going to
> have a look at how this can be automated and added to
> tests/system-traffic.at, together with the rest of the already existing
> tests.
>
> [1] [CONNTRACK] Discussions at OvS 2017:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017-2DNovember_341089.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=XHeuwSeMKQzqBJ1r6eAv0vsWJV6qkTRe0-B_iqUOl7Y&s=IDGICUZlL3v-yH_pfcZvMh00v6bCssbJ3bn-JvDuqWo&e=
>
> Tiago Lam (3):
> Conntrack: Add new API for future SIP Alg.
> Conntrack: Add initial support for new SIP Alg.
> Conntrack: Support asymmetric RTP port for SIP.
>
> include/openvswitch/ofp-actions.h | 4 +
> lib/automake.mk | 2 +
> lib/conntrack-private.h | 2 +
> lib/conntrack-sip.c | 491 ++++++++++++++++++++++++++++++++++++++
> lib/conntrack-sip.h | 123 ++++++++++
> lib/conntrack.c | 254 +++++++++++++++++++-
> lib/ofp-parse.c | 5 +
> ofproto/ofproto-dpif-xlate.c | 3 +
> 8 files changed, 883 insertions(+), 1 deletion(-)
> create mode 100644 lib/conntrack-sip.c
> create mode 100644 lib/conntrack-sip.h
>
Hi Tiago,
Before starting in my current role, I worked for ten years doing VoIP
development with a strong focus on SIP.
SIP is a beast of a protocol, and seeing this patchset, I made some
notes about how things might possibly go wrong, and ... let's just say
there are quite a few :). I know that you are writing this as a proof of
concept and that you have many TODO-style comments owning up to the fact
that you know about some things that are missing, but I think it may go
deeper than you or anyone on the OVS development team realize.
That is definitely an assumption…
I'm willing to help out in this effort by donating SIP knowledge, but
before that, I'm curious what the end goal of the SIP ALG actually is. I
have a feeling that the effort and expense of writing, and more
importantly maintaining, a proper SIP ALG may not be worth it.
As a side point, when I was working on VoIP software, one of the first
things we'd tell people who came to us with connectivity issues was
"turn off the SIP ALG in your router". This on its own resolved issues
with an alarming frequency.
Not a surprise; this was brought up internally when SIP support was first mentioned by Redhat.
At a minimum, SIP ALGs would have a specific knob to enable them globally.
Tiago/Mark
s/SIP ALGs would have a specific knob/SIP ALG’s behaviors would have a specific knob/
meaning certain behaviors (or capabilities) should be easily independently enabled/disabled in the code
and later supported by user command to enable.
I have one general behavior identified so far.
Thanks Darrell
Mark
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=XHeuwSeMKQzqBJ1r6eAv0vsWJV6qkTRe0-B_iqUOl7Y&s=nXBmkfSg5YmenXwzNh-0RDz6C2MEB--PZaZpvHc0E2U&e=
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=MluiN-u13FZ3Q-MHU-jOtNsKLSW2KgdIaHtHXNxBMYA&s=M24vIhozi06cN8-vKg0kIhmsgPGMzljkDC7J963K9lw&e=
Hi Tiago, I've given patches 1 and 2 a review. I did not have any comments to add for patch 3. I did not add any comments for items that you already had XXX lines for, since you already understand what is missing. Looking forward to the next patchset! Mark On 12/22/2017 01:53 PM, Tiago Lam wrote: > This patch-set is an initial approach at implementing the new SIP Alg, > mentioned by Aaron at [1]. > > I'm mostly interested in getting to know your thoughts of how this is > headed. There are a couple of points that are worth bringing up: > - As mentioned in patches 1/3 and 2/3, this is still a preliminary > implementation, and some work will be needed to move away from some > assuptions, like assuming the SIP traffic is always going over IPv4 > and TCP; > - At the moment, the sip state is being stored in the conn struct. I > followed the example of seq_skew_dir here, which is also stored there, > but realise this is not ideal. It seems storing it somewhere agnostic > will be ideal in the future, to avoid polluting that struct with > different Alg's details; > - The SIP helpers functions and structures are in conntrack-sip.h and > conntrack-sip.c. This can create confusion when comparing to > conntrack-tcp.c and other protocols since SIP is an Alg and is at a > different level. > > With regards to testing, for now, this has been tested manually, by > setting up the flows mentioned in patch 2/3 and having two VMs connected > to OvS, both using SIPp to simulate real traffic both ways. I'm going to > have a look at how this can be automated and added to > tests/system-traffic.at, together with the rest of the already existing > tests. > > [1] [CONNTRACK] Discussions at OvS 2017: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341089.html > > Tiago Lam (3): > Conntrack: Add new API for future SIP Alg. > Conntrack: Add initial support for new SIP Alg. > Conntrack: Support asymmetric RTP port for SIP. > > include/openvswitch/ofp-actions.h | 4 + > lib/automake.mk | 2 + > lib/conntrack-private.h | 2 + > lib/conntrack-sip.c | 491 ++++++++++++++++++++++++++++++++++++++ > lib/conntrack-sip.h | 123 ++++++++++ > lib/conntrack.c | 254 +++++++++++++++++++- > lib/ofp-parse.c | 5 + > ofproto/ofproto-dpif-xlate.c | 3 + > 8 files changed, 883 insertions(+), 1 deletion(-) > create mode 100644 lib/conntrack-sip.c > create mode 100644 lib/conntrack-sip.h >
Hi Mark, Thanks for the review (and pointers), I very much appreciate it. I've only skimmed through it so far, but I'll take it into account when sending v2. As I mentioned before I want to put some testing in place first and haven't had time to get that into place yet, hopefully this coming week. Regards, Tiago On 01/15/2018 04:03 PM, Mark Michelson wrote: > Hi Tiago, > > I've given patches 1 and 2 a review. I did not have any comments to add > for patch 3. I did not add any comments for items that you already had > XXX lines for, since you already understand what is missing. > > Looking forward to the next patchset! > Mark
This patch-set is an initial approach at implementing the new SIP Alg, mentioned by Aaron at [1]. I'm mostly interested in getting to know your thoughts of how this is headed. There are a couple of points that are worth bringing up: - As mentioned in patches 1/3 and 2/3, this is still a preliminary implementation, and some work will be needed to move away from some assuptions, like assuming the SIP traffic is always going over IPv4 and TCP; - At the moment, the sip state is being stored in the conn struct. I followed the example of seq_skew_dir here, which is also stored there, but realise this is not ideal. It seems storing it somewhere agnostic will be ideal in the future, to avoid polluting that struct with different Alg's details; - The SIP helpers functions and structures are in conntrack-sip.h and conntrack-sip.c. This can create confusion when comparing to conntrack-tcp.c and other protocols since SIP is an Alg and is at a different level. With regards to testing, for now, this has been tested manually, by setting up the flows mentioned in patch 2/3 and having two VMs connected to OvS, both using SIPp to simulate real traffic both ways. I'm going to have a look at how this can be automated and added to tests/system-traffic.at, together with the rest of the already existing tests. [1] [CONNTRACK] Discussions at OvS 2017: https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341089.html Tiago Lam (3): Conntrack: Add new API for future SIP Alg. Conntrack: Add initial support for new SIP Alg. Conntrack: Support asymmetric RTP port for SIP. include/openvswitch/ofp-actions.h | 4 + lib/automake.mk | 2 + lib/conntrack-private.h | 2 + lib/conntrack-sip.c | 491 ++++++++++++++++++++++++++++++++++++++ lib/conntrack-sip.h | 123 ++++++++++ lib/conntrack.c | 254 +++++++++++++++++++- lib/ofp-parse.c | 5 + ofproto/ofproto-dpif-xlate.c | 3 + 8 files changed, 883 insertions(+), 1 deletion(-) create mode 100644 lib/conntrack-sip.c create mode 100644 lib/conntrack-sip.h