Skip to content

Sip proxy: Support for outbound transactions#23435

Closed
arejasco wants to merge 64 commits intoenvoyproxy:mainfrom
arejasco:sip_proxy_outbound_tx_support
Closed

Sip proxy: Support for outbound transactions#23435
arejasco wants to merge 64 commits intoenvoyproxy:mainfrom
arejasco:sip_proxy_outbound_tx_support

Conversation

@arejasco
Copy link
Copy Markdown
Contributor

@arejasco arejasco commented Oct 11, 2022 •
edited
Loading

Commit Message: Add support for outbound transactions to SIP proxy extension
Co-authored-by: Jonah Murphy jonamurp@cisco.com
Additional Description: See below
Risk Level: Medium
Testing: Unit testing & Manual testing
Docs Changes: API for SIP proxy plugin settings updated
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

This PR provides to SIP proxy extension for Envoy with support for handling outbound transactions, that is, transactions being initiated in the SIP server towards the client.

Background

SIP proxy contrib extension was implemented in September 2021 to enable Service Mesh for SIP protocol, taking into account SIP specific router mechanisms and features.

Current behavior of SIP proxy extension supports the forwarding of inbound messages (request coming from the client to be redirected to an endpoint), but only responses to existing transactions initiated by an inbound request are supported to be forwarded from the endpoint to the client. The forwarding of outbound request coming from an endpoint is not currently supported by SIP proxy extension, as Envoy does not know to which downstream connection has to forward the message when received.

This PR provides through the usage of a new header the routing mechanisms for forwarding an outbound request coming from an endpoint to the target client, providing in that way support for outbound initiated transactions.

Design

The basic idea is to provide support for outbound transactions by adding a new header, X-Envoy-Origin-Ingress, which will provide information about the thread and downstream connection where to forward any outbound transaction initiated. This X-Envoy-Origin-Ingress header will be composed of two parameters:

  • Thread ID: Identify the thread instance to which the client is connected, with a format thread-number@local-envoy-address.
  • Downstream connection ID: Identify the downstream connection instance being used by the thread instance for connecting with the client, with a format remote-client-address:remote-client-port@random-uuid.

These parameters will be initiated in the beginning, when a client connects to Envoy and a ConnectionManager is initiated. For each thread, a local map will relate the downstream connection ID with its ConnectionManager instance through a wrapper object called DownstreamConnection (new class).

Each message coming from the client (either request or response) will be modified by adding the X-Envoy-Origin-Ingress header before being forwarded to an endpoint.

The endpoint, when receiving a request coming from Envoy, will process the request as usual, but storing the value from X-Envoy-Origin-Ingress header for being used in future outbound transactions.

Each outbound request coming from a server will need to have the X-Envoy-Origin-Ingress header. It will be parsed and the DownstreamConnection associated will be recovered, providing the ConnectionManager instance to be used for forwarding the message to the client. An UpstreamTransaction instance (new class) will be created and mapped in a thread local map, saving the address of the endpoint sending the request.

Each response coming from the client to an outbound request will be processed by getting the UpstreamTransaction instance from the transaction ID and routing the message but setting the address of the endpoint which started the transaction as preferred destination. In that way the response to an outbound request will be routed by setting an affinity with the origin endpoint of the request, having the response forwarded to the same endpoint.

System architecture

This is the relationship between classes in the forwarding of an outbound initiated request to a client:

image

And this is the relationship between classes in the forwarding of the response to an outbound initiated request to the original endpoint:

image

Call flows

Here is the call flow for the connection and register of the client to the SIP server through Envoy:

image

Here is the call flow for a successful outbound transaction (an INVITE in this case):

image

Configuration

For configuring the SIP proxy plugin to allow the management of upstream transactions as an optional feature, API files for SIP proxy extension has been modified to include a new node for SIP proxy clusters, called upstream_transactions. In that way, it will be possible to enable/disable upstream transactions at a cluster level.

static_resources:
  clusters:
  - name: cluster_0
    type:
      ...
    typed_extension_protocol_options:
      envoy.filters.network.sip_proxy:
        "@type": type.googleapis.com/envoy.extensions.filters.network.sip_proxy.v3alpha.SipProtocolOptions
         ...
         upstream_transactions:
            enabled: true
         ...

This node will define for now just one parameter, enabled, which enables or disables the support for outbound transactions in that cluster. Disabling outbound transactions will provoke the following actions in the cluster:

  • X-Envoy-Origin-Ingress header won't be added in messages being forwarded upstream to the cluster.
  • Request messages received upstream will be discarded.
  • Indirectly, response messages received downstream will be always discarded, as the thread-local map for upstream transactions won't contain and upstream transaction instance with the transaction ID.

Main changes done

Here is the summary of the main changes done in this PR:

  • Parsing of X-Envoy-Origin-Ingress header during decoding, addition in all messages forwarded upstream and removal in all requests forwarded downstream.
  • Creation of DownstreamConnection class for mapping ConnectionManager instances to their downstream connection IDs. Creation of a TLS-base thread-local map for this mapping.
  • Modify ActiveTrans class to make it abstract, and create two implementations of this class:
    • DownstreamActiveTrans, representing all downstream initiated transactions. Replace the old use of ActiveTrans for DownstreamActiveTrans.
    • UpstreamActiveTrans, representing all upstream initiated transactions. Creation of a TLS-base thread-local map for mapping these upstream transactions with their transaction ID.
  • Rename of some existing classes for better sense:
    • Router::UpstreamRequest -> Router::UpstreamConnection.
    • Router::ResponseDecoder -> Router::UpstreamMessageDecoder.
  • Behavior of SIP proxy extension modified for working according to what's documented above.
  • Set of statistics created for upstream transactions, mirroring downstream ones (still pending to review stats behavior).
    • Current stats remains unchanged but could be worth to add downstream_ prefix to them.
  • Possibility to send local replies for outbound initiated transactions, although not used for now (for now all errors detected in outbound transactions will result in the discarding of the message).
  • Metadata function for validating message headers in both inbound and outbound paths, and logic for discarding in both paths the message if any of the mandatory SIP headers which may provoke a crash is not present.

jonahmurphy and others added 30 commits September 26, 2022 10:54
@jonahmurphy @arejasco
First pass at adding support for setting X-Envoy-Origin-Ingress header
10a84ab 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Pass IngressID through to the router
fc07d5e 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Add test to verify the router adds new header
68a9abc 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
tidyup
d536b59 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
First attempt at adding downstream conn cache, not working yet
45a07f7 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Broken using DownstreamConnectionInfos interface
4575a5e 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Refactor to fix deps and visibility
b29acab 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
DownstreamConnectionInfos should publicly interit DecoderFilterCallbacks
b1fb5e2 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Add error handling when routing an upstream request
6f0d407 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Move DowstreamConnectionInfoItem to a private inner class of
c82fbe9 
ConnectionManger

DownstreamConnectionInfos now needs to be a friend of the
ConnectionManager so that it may access the item class.

Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Basic support for sending a request from upstream to downstream
91b298a 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Ensure new upstream tx's don't rely on an old dowsntream tx
aa16bca 
Copy the required state from ActiveTrans decoderFilterCallbacks into the Upstream request,
as ActiveTrans will be deleted after tx time at 32 seconds.

Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Add basic transaction support for upstream transactions
1ebe008 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
Fix crash on deletion of UpstreamActiveTrans
a628d60 
Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@jonahmurphy @arejasco
UpstreamRequest must keep its own reference to SipFilterStats
1378f08 
DecoderFilterCallbacks may have been deleted before a downstream tx is recvd,
so UpstreamRequest must keep its own ref.

+Clean up logs a little.

Signed-off-by: Jonah Murphy <jonamurp@cisco.com>
@arejasco
Change X-Envoy-Origin-Ingress header format to the proposed one
2a74543 
Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Fix error getting local IP address string
052c7e6 
Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Solved problem processing X-Envoy-Origin-Ingress header after format …
2c93a4e 
…change

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
-Strip X-Envoy-Origin-Ingress header from upstream messages.
58aaefc 
-X-Envoy-Origin-Ingress header added to all messages (not only requests)

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
- Solve problem regarding connection event coming later than downstre…
a987d15 
…am connection creation (might require hardening).

- Additional debug logs for debugging downstream connection map management (remove them when productizing)

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Solve error of empty line after stripping X-Envoy-Origin-Ingress head…
5e053d3 
…er heading downstream

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Solved problem of using hostname instead of IPs at configuration
947e57d 
Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
- Upstream local replies working from UpstreamRequest and ResponseDec…
7b170d3 
…oder

- AppException class being able to work with other error codes besides 503

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Use IP instead of hostname in upstream request log
180e4b7 
Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Upstream local responses working in case of upstream active transacti…
5bc3a92 
…ons failure

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
upstream transaction cache modifications for implementing a timeout s…
91c3ef8 
…ystem for clearing cache

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
X-Envoy-Origin-Ingress header replaced if incoming message arrives wi…
f956ca3 
…th it

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Final logs and mods for hardened X-Envoy-Origin-Ingress header
24ac9b2 
Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Changes after applying code style format rules
618e896 
Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
Code styling modifications. Parsing X-Envoy-Origin-Ingress header wit…
d812228 
…hout std:regex, against code styling

Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco
solve problem with final line
e80a4fb 
Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@arejasco arejasco marked this pull request as ready for review November 1, 2022 17:45
@arejasco
Outdated todo removal
f7b9498 
Signed-off-by: Adrian Rejas Conde <arejasco@cisco.com>
@phlax
Copy link
Copy Markdown
Member

phlax commented Nov 7, 2022

@durd07 would you mind giving this another pass

@durd07
Copy link
Copy Markdown
Member

durd07 commented Nov 7, 2022

As this is so big a PR, I need some time to review, I will began to review this soon.

@phlax
Copy link
Copy Markdown
Member

phlax commented Nov 7, 2022

thanks

/wait-any

@arejasco
Copy link
Copy Markdown
Contributor Author

arejasco commented Nov 7, 2022

Thanks @durd07 @phlax

durd07
durd07 suggested changes Nov 28, 2022
View reviewed changes
Comment thread contrib/sip_proxy/filters/network/source/app_exception_impl.cc

#include <sstream>

#include "sip.h"
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use full path

Comment thread contrib/sip_proxy/filters/network/source/config.cc
#include "contrib/sip_proxy/filters/network/source/filters/well_known_names.h"
#include "contrib/sip_proxy/filters/network/source/router/router_impl.h"
#include "contrib/sip_proxy/filters/network/source/stats.h"
#include "filters/filter.h"
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full path

Comment thread contrib/sip_proxy/filters/network/source/conn_manager.cc
setLocalResponseSent(decoder_->metadata()->transactionId().value());

if (decoder_->metadata()->msgType() == MsgType::Request) {
sendLocalReply(*(decoder_->metadata()), ex, false);
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap to method

Comment thread contrib/sip_proxy/filters/network/source/conn_manager.cc
// }
std::string&& k = std::string(metadata->transactionId().value());
if (metadata->msgType() == MsgType::Request) {
stats_.counterFromElements(methodStr[metadata->methodType()], "request_received").inc();
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low performance, I have changed, will merge in my next PR.

Comment thread contrib/sip_proxy/filters/network/source/conn_manager.cc
FilterStatus ConnectionManager::UpstreamActiveTrans::transportEnd() {
parent_.stats_.upstream_response_.inc();
parent_.stats_
.counterFromElements(methodStr[metadata_->methodType()], "upstream_response_proxied")
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance issue

Comment thread contrib/sip_proxy/filters/network/source/decoder.h
}

void complete();
void complete() { complete(true); };
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use default value.

Comment thread contrib/sip_proxy/filters/network/source/metadata.cc
ASSERT(new_headers_pos_ != absl::string_view::npos);

absl::string_view str_type = HeaderTypes::get().header2Str(type);
std::string header = std::string(str_type) + ": " + value + "\r\n";
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use sstream instead

Comment thread contrib/sip_proxy/filters/network/source/metadata.cc
}

bool MessageMetadata::hasMsgHeader(HeaderType type) {
auto init_pos = raw_msg_.find(HeaderTypes::get().header2Str(type).data());
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check whether header name exist in headers_

Comment thread contrib/sip_proxy/filters/network/source/metadata.h
*/
void setTransactionId(absl::string_view data);

OriginIngress* originIngress() { return origin_ingress_.get(); };
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need pointer here？

Comment thread contrib/sip_proxy/filters/network/source/sip.h
{"P-Nokia-Cookie-IP-Mapping", HeaderType::PCookieIPMap},
{"X-Envoy-Origin-Ingress", HeaderType::XEnvoyOriginIngress}};

const std::map<HeaderType, absl::string_view> sip_header_type_reverse_map_{
Copy link
Copy Markdown
Member

@durd07 durd07 Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please help to replace std::map into absl_flat_map

@arejasco
Copy link
Copy Markdown
Contributor Author

arejasco commented Nov 28, 2022

Thanks for your review @durd07. I'll try to address all your comments in the following days

@durd07
Copy link
Copy Markdown
Member

durd07 commented Dec 4, 2022

@arejasco I merged one PR #24165 (comment) with the latest stats change, please merge this and adapt the new stats changes.

Sorry for any inconvenience.

@alyssawilk alyssawilk self-assigned this Dec 5, 2022
@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented Dec 5, 2022

assigning myself to merge after this has @durd07's LGTM
/wait

@alyssawilk alyssawilk removed their assignment Dec 15, 2022
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 4, 2023

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 4, 2023
@arejasco
Copy link
Copy Markdown
Contributor Author

arejasco commented Jan 10, 2023

Sorry for the lack of activity on this PR, I've been on a long abscence. I'll try to address your comments between this week and the next one

@github-actions github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 10, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 9, 2023

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 9, 2023
@arejasco
Copy link
Copy Markdown
Contributor Author

arejasco commented Feb 15, 2023

Still working on it

@github-actions github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 16, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 18, 2023

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 18, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 25, 2023

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot closed this Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@durd07 durd07 durd07 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@durd07 durd07

Labels

stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arejasco @durd07 @phlax @alyssawilk @jonahmurphy