-
Notifications
You must be signed in to change notification settings - Fork 215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RESTCOM-884 : Implement Multiprovider Voice 2b #2191
Conversation
…ences to CallRequest.Type and CreateCall.Type to CreateCallType commons enum
…quest, IExtensionCreateSmsSessionRequest interfaces
…sionRequest. Decorate ExtensionRequest with IExtensionRequest interface
…ename preexisting x-headers to rcmlHeaders. Decorate CreateCall with IExtensionCreateCallRequest interface. Modify CM.outbound to use IExtensionCreateCallRequest to extensions. Modify outboundToPstn to use outboundProxy
a230349
to
b41ce41
Compare
…ing. Reorder methods
Hi @abdulazizali77 I have started the review and so far I like the changes you did at the extensions api, great work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We should also add an execution point for calls from WebRTC clients that are proxied out to PSTN -
Line 536 in b41ce41
proxyThroughMediaServer(request, client, toUser); - Please remove FIXME and TODO comments
Last, please elaborate on what is the plan for CallRequest and CreatCall, why we need to keep them both and if you plan to remove on of them.
@@ -547,10 +547,10 @@ private void invite(final Object message) throws IOException, NumberParseExcepti | |||
// proxy DID or number if the outbound proxy fields are not empty in the restcomm.xml | |||
if (proxyURI != null && !proxyURI.isEmpty()) { | |||
// String destination = ((SipURI)request.getTo().getURI()).getUser(); | |||
CallRequest callRequest = new CallRequest(fromUser,toUser, CallRequest.Type.PSTN, client.getAccountSid(), false, false); | |||
CallRequest callRequest = new CallRequest(fromUser,toUser, CreateCallType.PSTN, client.getAccountSid(), false, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 the CallRequest created here is never used. The ExtensionController will check an empty ExtensionRequest. Am I missing something or this is wrong here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was my mistake. Changed
@@ -547,10 +547,10 @@ private void invite(final Object message) throws IOException, NumberParseExcepti | |||
// proxy DID or number if the outbound proxy fields are not empty in the restcomm.xml | |||
if (proxyURI != null && !proxyURI.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 at this point we have the following:
- INVITE from registered client
client
- INVITE is not for a registered IncomingPhoneNumber
- INVITE is not from a WebRT client
- INVITE should be proxied out to PSTN
So in that case, shouldn't support also here the multi-providers and custom headers per account, similar to what we do for the outbound() method (that is executed for Dial verb) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for now, but need to rethink about better way to add headers. Currently only duplicated code from Call.addHeadersToMessage
@@ -547,10 +547,10 @@ private void invite(final Object message) throws IOException, NumberParseExcepti | |||
// proxy DID or number if the outbound proxy fields are not empty in the restcomm.xml | |||
if (proxyURI != null && !proxyURI.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we don't have a system wide outbound proxy, so proxyUri==null
, but the multi provider extension will provide the outbound proxy?
We should take care of this case also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -189,10 +192,11 @@ | |||
private SipURI from; | |||
private javax.servlet.sip.URI to; | |||
// custom headers for SIP Out https://bitbucket.org/telestax/telscale-restcomm/issue/132/implement-twilio-sip-out | |||
private Map<String, String> headers; | |||
private Map<String, String> rcmlHeaders; | |||
private Map<String, ArrayList<String>> headers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 please add comments here to make clear where each of the headers map object is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have added the comments and renamed headers
to extensionHeaders
When i think about it extensionHeaders
is used for too many things, potentially shouldve separated the header property modification and header addition into separate maps.
@@ -291,6 +302,12 @@ public Call(final SipFactory factory, final ActorRef mediaSessionController, fin | |||
if (statusCallback != null) { | |||
downloader = downloader(); | |||
} | |||
this.rcmlHeaders = new HashMap<String, String>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 this is minor but better to instantiate the rcmlHeaders only when is needed
Restcomm-Connect/restcomm/restcomm.telephony/src/main/java/org/restcomm/connect/telephony/Call.java
Line 838 in b41ce41
if (toHeaderString.indexOf('?') != -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
for (Map.Entry<String, String> entry : rcmlHeaders.entrySet()) { | ||
String headerName = keyPrepend + entry.getKey(); | ||
String headerVal = message.getHeader(headerName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 why you check the header value from the SIP message here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake. Removed.
String headerVal = message.getHeader(headerName); | ||
|
||
//FIXME: do getValue check first? | ||
StringBuilder sb = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 better put this after the line 1049 if(headerVal!=null && !headerVal.isEmpty()) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
StringBuilder sb = new StringBuilder(); | ||
//if(entry.getValue() instanceof ArrayList){ | ||
for(String pair : entry.getValue()){ | ||
logger.debug("pair="+pair); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 either remove this log statement or surround it with if (logger.isDebugEnabled)
and change the message to be meaningful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Removed
…ew comments: Remove CreateCall, unused *Response, change method interfaces. Remove CallManager.handleExtensionResponse. Add extension check for proxyDID scenario.
@gvagenas Also currently have not tested functionality in the |
@abdulazizali77 for the webrtc calls as I wrote you in my previous comment:
If we don't block the WebRTC calls at CallManager stage, we will spend ALL the resources related to an outgoing call: Call actor, VoiceInterpreter actor, MmsCallControler actor, etc and RMS resources. |
@abdulazizali77 what kind of testing is missing? You mean live test calls ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will repeat my previous comment here:
@abdulazizali77 for the webrtc calls as I wrote you in my previous comment:
Even though this kind of calls will be checked later at the outbound() method (because they will use Dial verb) If we do the extensions check at this point we will save RMS and RC resources.
If we don't block the WebRTC calls at CallManager stage, we will spend ALL the resources related to an outgoing call: Call actor, VoiceInterpreter actor, MmsCallControler actor, etc and RMS resources.
So it is very important to check and block any call as soon as possible.
The only issue here is that the call will be checked again at the outbound method and we need to be checked there, but the cost of executing the extensions for second time is less than create all the Call resources and release them later.
Also what tests you are missing? You mean live test calls or test cases in the testsuite?
George
* @param SipServletRequest message | ||
* @param Map<String, ArrayList<String> > headers | ||
*/ | ||
private void addHeadersToMessage(SipServletRequest message, Map<String, ArrayList<String> > headers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 maybe we should apply more strict rules here about what headers should be possible to be added. We should check if header is RURI or a custom headers (starting with X-).
@deruelle @scottbarstow do you think we should allow more than that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deruelle @scottbarstow should we do any further restrictions on modification of System headers, non system headers and custom headers?
We are only checking for two cases here, if a header is a Request-URI or not.
System headers and non System headers will all equally be modified given any headers that we have defined in the db config.
An interesting scenario is if we have contending rcml headers and extension headers, have not tried this scenario. We can definitely do a string to check to completely exclude any custom 'X-' header modification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @abdulazizali77, we definitely need to surround this in a try catch clause in order to catch exceptions when user will try to pass/modify system headers. In that case, Sip Servlet will not allow the modification and will throw an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvagenas @abdulazizali77 if we want to restrict headers we should do that at API level when they are first provided in the extension so we do it only once and not deep in the code everytime.
String toHost = ""; | ||
String scheme = ""; | ||
boolean isSecure = false; | ||
if(request.to().contains("@") && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 why we need this part of the code?
Since we reach to this point we have the following:
- Previouly we checked and we know this is a SIP URI (contains @ in the request.to()).
- After we create the
SipURI to = (SipURI) sipFactory.createURI(request.to());
we can check if its secure or not using the API provided methodto.isSecure();
.
If the request.to()
contains sips
then the resulted SipURI will be secure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two reasons i see
-
rcml couldve been defined with either
<Sip>sip:+1234@host.com:5070</Sip>
or
<Sip>sips:+1234@host.com:5070</Sip>
-
and we need to replace (or not) the host part with whatever host we want to replace it with
But I guess i can see what you prefer to do, is it with two calls to SipFactory
?
final String uri = (request.getOutboundProxy()!= null && (!request.getOutboundProxy().isEmpty())) ? request.getOutboundProxy() : "";
SipURI to = (SipURI) sipFactory.createURI(request.to());
if(!uri.isEmpty() {
String [] tokens = request.to().split("[@:]");
String toUser = tokens[1];
String toHost = tokens[2];
boolean isSecure = to.isSecure();
to = sipFactory.createSipURI(toUser, toHost);
to.setSecure(isSecure);
}
is this preferable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 You can do all using the SipURI, check javadoc here https://docs.oracle.com/cd/E28280_01/apirefs.1111/e17883/javax/servlet/sip/SipURI.html
The to = (SipURI) sipFactory.createURI(request.to());
will check that we have sips
and will make the new SipURI secure.
Also, you can change the host part after you create the SipURI using to.setHost()
.
//headers defined in rcml | ||
private Map<String, String> rcmlHeaders; | ||
//headers populated by extension to modify existing headers and add new headers | ||
private Map<String, ArrayList<String>> extensionHeaders; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 your comment:
shouldve separated the header property modification and header addition into separate maps.
makes sense but not high priority for now. If you think its not big task please implement it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its quite an effort to design at this stage, we have to expose a new method signature to the IExtensionCreateCallRequest
interface and we also have to potentially change the Call
constructor signature etc amongst other things. I would prefer leaving this refactoring for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok makes sense
… host replacement
@gvagenas Right now for tests, i do not have Telscale int test scenarios to run the P2P, proxyDID, WebRTC paths, can you advise? |
I will review the last modification @abdulazizali77 and later when finish the Telscale review we can check about the missing tests |
@gvagenas |
@abdulazizali77 please check the last comment on https://github.com/RestComm/Restcomm-Connect/pull/2191/files/1eecc8d22acdc67578010e6ac1e71ca3cc37e721#diff-23ea966e4b31dbd40ea4acf8439e22ed for the SipURI and SipFactory use. |
@gvagenas |
@@ -533,35 +531,64 @@ private void invite(final Object message) throws IOException, NumberParseExcepti | |||
|
|||
if (isWebRTC(request)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 if this is a WebRTC request it will be proxied to PSTN (with RMS in the path) so the IExtensionCreateCallRequest
should be for CreateCallType.PSTN
Also, the extension execution here is duplicate and we can execute only once for both WebRTC or regular sip calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if we execute extensions before isWebRTC()
and proxy out, we can fail fast in case the extension doesn't allow the call to proceed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
try { | ||
String headerVal = message.getHeader(headerName); | ||
if(headerVal!=null && !headerVal.isEmpty()) { | ||
message.setHeader(headerName , headerVal+sb.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 I think it's dangerous to modify header value without knowing what is the header. Is it a requirement to append values to existing header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes George it is, it is a functional requirement to modify all headers including System headers and the Request-URI
However i have to admit that i did not read this functional requirement anywhere. (Nor did i detail the requirement in the design document)
@deruelle @scottbarstow Jean, Scott. thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again those headers should be validated at API level when we configure the extension and not on a per request basis. If we want validation of headers (which we do only if we ever expose the API for this publicly), then this should be done at API level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deruelle you mean during the load of the configuration, right? because here is the extension API level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvagenas I meant validation at the REST API that does the configuration for the account. When the PUT is done we can validate if the headers are valid/allowed or not
int equalsPos = keyValPair.indexOf("="); | ||
parName = keyValPair.substring(0, equalsPos); | ||
parVal = keyValPair.substring(equalsPos+1); | ||
reqURI.setParameter(parName, parVal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 I believe we should completely replace the RURI instead of just modify the parameters
Can you point me to the design document for this as it seems I am not sync on the custom headers requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
George my interpretation of the requirement is to add parameters to all headers including System headers to provide for extensibility and custom processing by any outbound provider.
I believe there are no specific requirements from any outbound provider to how they need a SIP Message needs to be 'extended', hence how we allow it to be extended has had to be very generic and open ended. (hence header parameters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct @abdulazizali77. @abdulazizali77 please sync with @gvagenas offline to give him the link to the requirements and have a call together to go through it
@@ -1727,11 +1816,15 @@ private void outboundToPstn(final CreateCall request, final ActorRef sender) thr | |||
|
|||
|
|||
private void outboundToSip(final CreateCall request, final ActorRef sender) throws ServletParseException { | |||
final String uri = (request.getOutboundProxy()!= null && (!request.getOutboundProxy().isEmpty())) ? request.getOutboundProxy() : ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 I believe this is wrong here. We shouldn't route SIP calls using outbound proxy.
If we want to dial a specific SIP URI then the SIP URI contains the user and host to reach the destination and we must not change the host to outbound proxy.
Again, I might be missing something from the design and the requirements of the feature so please point me to the doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
George, this is a specific requirement for URI-Rewrite. I think my understanding of SIP and usage of host versus proxies is still limited and incomplete (even after reading 3261) but the behaviour should be that the message going to sip:number@host.com
will be routed to outboundProxy
if it exists when we call the extension.
In a specific case the outboundProxy
here would be NetworkIntelligence. They will then take the message we have forwarded which was rewritten to sip:number@ni.com
and potentially modified with custom parameters and forward it to the original sip:number@host.com
after they have processed the message.
https://docs.google.com/document/d/1RL9j1PHqHorhflroKBDyNs7ghH29t72IFMnh0JZGYTY/edit#heading=h.8s6nwkogs9wk
What is alternative to achieve this functionality aside from setting the host in the SipURI
?
I do see a message.getProxy
but there doesnt seem to be a message.setProxy
@deruelle @scottbarstow Jean, Scott, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvagenas @abdulazizali77 is correct. There is 2 different requirements in the doc :
-
Allow to configure the outbound proxy per Account
-
Allow to rewrite the host part of the URL so from a user standpoint they don't have to know the details of through which hops the route will be going through or if we are interconnected directly or not with that route.
headers.put("RestComm-CallSid", instanceId+"-"+id.toString()); | ||
} | ||
|
||
private void addHeadersToMessage(SipServletRequest message, Map<String, String> headers, String keyPrepend) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulazizali77 same comments for the addHeadersToMessage at CallManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…ert mistaken rcmlHeaders functions changes, instantiate rcmlHeaders outside of if block
@gvagenas George, please have a look. Quite a bit of changes. In Voice 2b proposal were moving away from ExtensionResponse and only passing in mutable Request interfaces. One advantage of this is that we dont have to have
handleExtensionsResponse
mapping. We already define by the interfaces what any extensions developer can modify.-CreateCallType had to be defined in commons to solve the circular dependency problem. This unfortunately touched many files.
-To pass in the new headers
Call
constructor takes in Map of ArrayList headers that is passed through theIExtensionCreateCallRequest
-All the proxy properties are also exposed through
IExtensionCreateCallRequest
-We define
isAllowed
onIExtensionRequest
itself so we can removeExtensionReponse
altogether when doing message processing.-Potentially in the future we should remove the Configurations set/getter
IExtensionCreateSmsSessionRequest
and only expose specific properties forSmsService
andSmppMessageHandler
.-A bit of a workaround/hack is the
invite
method for handlingSipServletRequests
. We had to constructCreateCall
directly rather than pass a decorated sipServlet message to the extensions.TODO:
ApiRequest
inIncomingPhoneNumbers
EP is still usingExtensionResponse
hence this is retained.