Skip to content
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

Add support for domain names (and IPv6) to RTP forwarders #1778

Merged
merged 6 commits into from
Oct 28, 2019

Conversation

lminiero
Copy link
Member

As the title says, this is an alternative implementation to #1774. It adds support for domain names as the host part of RTP forwarders, which so far they had to be IP addresses. Incidentally, it also includes support for IPv6, as that's that the domain name may resolve to: this meant updating all the socket stuff related to RTP forwarders in both AudioBridge and VideoRoom, as they were hardcoded to IPv4.

What still needs to be updated is the Streaming plugin, which still only supports binding to IPv4 addresses: as such, you won't be able to test an IPv6 RTP forwarder with the Streaming plugin for now. I'll probably add that as part of this pull request, so that everything is updated consistently.

Please test and provide feedback.

@lminiero
Copy link
Member Author

Just added an optional host_family field that should address that: if missing, we use the first address we receive from the resolver, otherwise we use the first address of the specified family. Seems to be working as expected.

What doesn't seem to be working yet is forwarding on IPv6, at least looking at Wireshark dumps... I'll work on that next, together with IPv6 support in the Streaming plugin.

@mirkobrankovic
Copy link
Contributor

I will give it a test as soon as I get free time :D

@lminiero
Copy link
Member Author

This should complete the refactoring of the RTP forwarder, to support name resolving and IPv6. I'll work on IPv6 support in the Streaming plugin in a separate effort, as that might take longer and I don't want to block this PR.

Feedback welcome!

@lminiero
Copy link
Member Author

lminiero commented Oct 2, 2019

@mirkobrankovic did you have a chance to test if this works for you?

@lminiero lminiero requested a review from atoppi October 2, 2019 12:25
Copy link
Member

@atoppi atoppi left a comment

Choose a reason for hiding this comment

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

lgtm, submitted just some hints

if(getaddrinfo(host, NULL, NULL, &res) == 0) {
start = res;
while(res != NULL) {
if(family != 0 && family != res->ai_family) {
Copy link
Member

Choose a reason for hiding this comment

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

the output from getaddrinfo can be filtered by the requested family just using the third param

/* get only IPv4 */
struct addrinfo hints;
memset(&hints, 0, sizeof(hints));
hints.ai_family = AF_INET;
getaddrinfo(host, NULL, &hints, &res);

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 👍

inet_pton(AF_INET, host, &(forward->serv_addr.sin_addr));
forward->serv_addr.sin_port = htons(port);
/* Check if the host address is IPv4 or IPv6 */
if(strstr(host, ":") != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

A RegEx would be better imho, but given that host has been sanitized in the caller method it should be ok.

@lminiero
Copy link
Member Author

Merging.

@lminiero lminiero merged commit 211a6fb into master Oct 28, 2019
@lminiero lminiero deleted the forward-resolve branch October 28, 2019 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants