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

Do not limit org.ice4j.ice.harvest.DISABLE_LINK_LOCAL_ADDRESSES to IPv6. #156

Merged
merged 2 commits into from Jan 11, 2019

Conversation

Projects
None yet
4 participants
@mstyura
Copy link
Contributor

mstyura commented Nov 10, 2018

When link local addresses disabled via setting org.ice4j.ice.harvest.DISABLE_LINK_LOCAL_ADDRESSES = true it only disabled IPv6 link-local addresses.
On some environments like Mac and Windows (with virtual Hyper-V networks adapters) there are link local network interfaces with IPv4 addresses, but they can not be filtered via org.ice4j.ice.harvest.DISABLE_LINK_LOCAL_ADDRESSES .
This PR fixes this and filter both IPv6 and IPv6 local addresses.
Relevant brief discussion on community.jitsi.org

@mstyura

This comment has been minimized.

Copy link
Contributor

mstyura commented Nov 10, 2018

@saghul could you please review?

@saghul

This comment has been minimized.

Copy link
Member

saghul commented Nov 11, 2018

LGTM, but I think @bbaldino or @bgrozev are more qualified to judge here.

@bbaldino

This comment has been minimized.

Copy link
Member

bbaldino commented Nov 12, 2018

my concern is that perhaps we (or someone) is relying on the previous behavior working the way it did (despite an overly-vague config property name). @paweldomas do you know?

@saghul

This comment has been minimized.

Copy link
Member

saghul commented Nov 13, 2018

FWIW, I’ve seen iOS use link local addresses when connecting in P2P mode with a desktop machine. Now, that said, if those addresses were not included it would have worked all the same with the actual IP of the default interface, as it does often.

So, my 2 cents are that we should merge and revert if there are major issues, but I can’t foresee any at the moment.

@mstyura

This comment has been minimized.

Copy link
Contributor

mstyura commented Nov 13, 2018

@saghul thank you very much for support. I do like your approach with try and revert (in this particular case I believe risk is low), such way iterations will go faster and we could make Jitsi better and better quickly.

@bbaldino

This comment has been minimized.

Copy link
Member

bbaldino commented Nov 13, 2018

I'm just a bit more cautious because ice4j is a very fundamental library used by a lot of code, so an issue here will affect a lot of components.

@mstyura

This comment has been minimized.

Copy link
Contributor

mstyura commented Nov 13, 2018

I'm just a bit more cautious because ice4j is a very fundamental library used by a lot of code, so an issue here will affect a lot of components.

this actually is far less important PR for me compared to all others, so we could postpone it until add separate switch for IPv4 addresses, if making sure nothing broken is so important.

@mstyura mstyura changed the title Do not limit org.ice4j.ice.harvest.DISABLE_LINK_LOCAL_ADDRESSES to IPv6. WIP: Do not limit org.ice4j.ice.harvest.DISABLE_LINK_LOCAL_ADDRESSES to IPv6. Nov 17, 2018

@bgrozev

This comment has been minimized.

Copy link
Member

bgrozev commented Jan 10, 2019

I traced this back to this commit. I don't expect anything to break if we fix it to apply to ipv4, and it makes sense to me to try and revert if we see any issues.

@mstyura mstyura changed the title WIP: Do not limit org.ice4j.ice.harvest.DISABLE_LINK_LOCAL_ADDRESSES to IPv6. Do not limit org.ice4j.ice.harvest.DISABLE_LINK_LOCAL_ADDRESSES to IPv6. Jan 10, 2019

@bgrozev

This comment has been minimized.

Copy link
Member

bgrozev commented Jan 10, 2019

I would go and update the documentation for the property in StackProperties.java, since it correctly describes the current (weird) behavior.

@mstyura

This comment has been minimized.

Copy link
Contributor

mstyura commented Jan 10, 2019

@bgrozev then I'm removing "WIP" and it can be merged if it is ok

@mstyura mstyura force-pushed the mstyura:disable_link_local_addresses_ipv4 branch from 7266414 to 90658cb Jan 10, 2019

@mstyura

This comment has been minimized.

Copy link
Contributor

mstyura commented Jan 10, 2019

Removed IPv6 from comment for DISABLE_LINK_LOCAL_ADDRESSES.

@mstyura

This comment has been minimized.

Copy link
Contributor

mstyura commented Jan 10, 2019

@bgrozev could you please merge this PR if comment fix and everything else is ok, or let me know if I should additionally fix anything?

@bgrozev bgrozev merged commit ce42c77 into jitsi:master Jan 11, 2019

1 check passed

default 488 tests run, 356 skipped, 0 failed.
Details

@mstyura mstyura deleted the mstyura:disable_link_local_addresses_ipv4 branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment