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

Bugfix/rework: IPv6 scope errors #49705

Merged
merged 41 commits into from
Sep 26, 2018
Merged

Conversation

isbm
Copy link
Contributor

@isbm isbm commented Sep 18, 2018

What does this PR do?

  • Fixes IPv6 scope issues that aren't supported in Python ipaddress.
  • Support IPv6 scope at IPv6Address object level.
  • Reworks salt._compat for code duplication and other issues
  • Fixes import of ipaddress across everywhere
  • Uses proper ipaddress instance for Python 3
  • Adds debug logging instead of just pass.

What issues does this PR fix or reference?

Previous Behavior

If your /etc/resolv.conf has a scope in IPv6 address, something like this:

nameserver fe80::20d:b9ff:fe01:ea8%eth0

...then you are getting errors like this:

  2018-09-15 13:14:45,014 [salt.utils.dns   :1039][ERROR   ][845] 
  /etc/resolv.conf: u'fe80::20d:b9ff:fe01:ea8%eth0' does not
  appear to be an IPv4 or IPv6 address

This is hitting users since 2016: #37949

New Behavior

🤘 😎 🤘

Tests written?

Existing should just pass.

@isbm isbm requested a review from a team as a code owner September 18, 2018 22:36
@ghost ghost requested review from a team September 18, 2018 22:36
Copy link
Contributor

@meaksh meaksh left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@rallytime
Copy link
Contributor

Hi @isbm! 😄

There are some test failures that are related to this change:

  • integration.ssh.test_jinja_filters.SSHJinjaFiltersTest.test_network_ipaddr
  • unit.utils.test_jinja.TestCustomExtensions.test_ipaddr
  • unit.utils.test_jinja.TestCustomExtensions.test_ipv4
  • unit.utils.test_jinja.TestCustomExtensions.test_ipv6
  • unit.utils.test_jinja.TestCustomExtensions.test_is_ip
  • unit.utils.test_jinja.TestCustomExtensions.test_is_ipv4
  • unit.utils.test_jinja.TestCustomExtensions.test_is_ipv6

Can you take another look? 😎

https://jenkinsci.saltstack.com/job/pr-kitchen-centos7-py3/job/PR-49705/9/

@aphor
Copy link
Contributor

aphor commented Sep 24, 2018

FWIW, upstream in Python we have precedent to suggest upstream fixes in ipaddress module.

so I submitted https://bugs.python.org/issue34788

@isbm
Copy link
Contributor Author

isbm commented Sep 24, 2018

@aphor this doesn't really help, I've been there already. First, the issue is like three years old. Second, even they will fix in some upcoming versions, we still need this even on 2.7 and SUSE on 2.6 that is just simply EOL. Alas, if they finally get it done one day, we can incorporate that where it is needed. Will they fix it across everywhere? I doubt it.

But having Salt crashing on scoped IPv6 today is simply a no go.

@aphor
Copy link
Contributor

aphor commented Sep 24, 2018

@isbm I agree completely salt should not wait for upstream fix, but this is technical debt which would ideally be factored out of salt, and FWIW, salt isn't the only Python utility that I need to support IPv6.

I put that stuff here, mostly because it will be useful in the future to have it here, for reference, not to imply we should not merge this PR into salt.

@isbm
Copy link
Contributor Author

isbm commented Sep 25, 2018

@aphor after @rallytime puts that in (I am also going to backport that to 2018.3) let's look again at your PR. I am keeping an eye on yours and if you need any help, please ping me.

@rallytime rallytime merged commit f98839c into saltstack:develop Sep 26, 2018
@isbm isbm deleted the isbm-ipv6-scope-errors branch October 21, 2018 13:53
@avolmensky
Copy link

I have been playing around with 2019.02 and the netbox pillar module is broken with Python 3. The changes in this PR fix the problem I have been having. Will this be merged into the 2019.02 release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-jam has master-port port to master has been created
Projects
None yet
Development

Successfully merging this pull request may close these issues.