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

enable IPv6 NAT candidate reporting #192

Merged
merged 4 commits into from
Feb 1, 2022
Merged

enable IPv6 NAT candidate reporting #192

merged 4 commits into from
Feb 1, 2022

Conversation

mcginty
Copy link
Collaborator

@mcginty mcginty commented Jan 31, 2022

This comes from the conversation in #191

previous behavior: filter out all IPv6 addresses from being reported as NAT candidates.

new behavior: Begin reporting IPv6 addresses that are global unicast scope only.

An argument could be made for also reporting unique local addresses as well, but I think it's less important and more likely to increase noise with little benefit to getting through any different types of NATs.

I'd love to hear from people more experienced with managing IPv6 addresses if that's not an appropriate strategy.

@mcginty mcginty marked this pull request as ready for review January 31, 2022 19:56
@mcginty mcginty merged commit 110bace into main Feb 1, 2022
@mcginty mcginty deleted the ipv6-nat-candidates branch February 1, 2022 03:21
Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

Quick post-mortem review: both intended functionality and code look good to me.

Comment on lines +114 to +115
|| matches!(ip,
IpAddr::V6(v6) if is_unicast_global(v6))
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice trick to concisely check something on an enum variant, I need to remember to use it, too.

(if only rustfmt didn't spoil it by being somewhat clueless with this expression)

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