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

Allow specifying multiple IP addresses for 1-1 NAT. #2279

Merged
merged 6 commits into from
Jul 23, 2020

Conversation

fancycode
Copy link
Contributor

This can be used if Janus is deployed in a DMZ between two 1-1 NAT firewalls for external and internal users.

I am using something like this for the scenario described in https://groups.google.com/d/msg/meetecho-janus/x-LhB_cTs7k/fHT4i5pzBQAJ

@lminiero
Copy link
Member

Thanks! This does seem useful, but I'll have to do a deeper review, since there's a lot of new code. I'm a bit busy these days, so that may take a while. Please ping me if I haven't come back to you in a few days.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

Did a first review, and added a few notes. I think the way the IPs are added/kept should be changed, you can find more details in the comments.

conf/janus.jcfg.sample.in Show resolved Hide resolved
janus.c Outdated Show resolved Hide resolved
janus.c Show resolved Hide resolved
janus.c Outdated Show resolved Hide resolved
janus.c Outdated Show resolved Hide resolved
janus.c Outdated Show resolved Hide resolved
janus.c Outdated Show resolved Hide resolved
@fancycode
Copy link
Contributor Author

Thanks for your feedback, all comments addressed. Public IPs are now stored in a GHashTable to handle duplicates and are extracted with g_hash_table_get_keys (which is cached).

janus.c Outdated Show resolved Hide resolved
@fancycode
Copy link
Contributor Author

Should I rebase now or do you want to finish your review first?

@lminiero
Copy link
Member

I won't be able to do another review today, sorry... I'll check again tomorrow.

@fancycode
Copy link
Contributor Author

No worries, take your time. I can rebase whenever you are ready with the review.

janus.c Show resolved Hide resolved
@lminiero
Copy link
Member

I added a small note. The code looks fine to me (I'll test after it's rebased, before I merge), I'm just a bit concerned by the fact we now have a bunch of do..while that would in the vast majority of cases (no nat-1-1) not be needed. I guess there won't be much of a performance impact though.

@fancycode fancycode requested a review from lminiero July 23, 2020 11:53
janus.c Outdated Show resolved Hide resolved
@lminiero
Copy link
Member

Thanks! I'll make a couple of tests after lunch and merge if I don't spot anything strange 👍

@lminiero
Copy link
Member

Just made a few tests and it seems to be working as expected, merging!

@lminiero lminiero merged commit ca4e3a3 into meetecho:master Jul 23, 2020
@fancycode fancycode deleted the multiple-1-1-nat branch July 24, 2020 09:38
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.

2 participants