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

Revert removal of IPv6 #3184

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Revert removal of IPv6 #3184

wants to merge 2 commits into from

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Mar 4, 2025

Revert commits:

  • 62fbcb4 wifi: networkmanager: disable ipv6
  • 715af4e core:tools:blueos_startup_update: No IPV6

It is incorrect to disable IPv6 and unclear why this was done in the first place.
I believe the underlying motivation of this change may have been DHCP4 issues which these PRs did not properly address. #1991

Summary by Sourcery

Reverts the disabling of IPv6 in the blueos_startup_update tool and the wifi service.

Copy link

sourcery-ai bot commented Mar 4, 2025

Reviewer's Guide by Sourcery

This pull request reverts the changes introduced by commit 715af4e, effectively re-enabling IPv6. The changes involve removing the IPv6 disabling logic from the blueos_startup_update.py script and the wifi connection setup in networkmanager.py.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Reverts the disabling of IPv6 in the system configuration.
  • Removes the function ensure_ipv6_disabled from blueos_startup_update.py. This function was responsible for ensuring IPv6 was disabled by modifying /etc/sysctl.conf.
  • Removes the call to ensure_ipv6_disabled from the main function in blueos_startup_update.py.
  • Removes the IPv6 configuration from the wifi connection setup in networkmanager.py.
core/tools/blueos_startup_update/blueos_startup_update.py
core/services/wifi/wifi_handlers/networkmanager/networkmanager.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@rotu rotu changed the title Revert "core:tools:blueos_startup_update: No IPV6" Revert removal of IPv6 Mar 4, 2025
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @rotu - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding a comment explaining why this revert is necessary.
  • Check if there are any dependencies or configurations that relied on the reverted commit.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@Williangalvani
Copy link
Member

I don''t disagree that this is "wrong" in principle. The issue is that historically, ipv6 has given us more issues that it has helped, and we don't current have enough workforce to address the issue fully.

This patch will likely case more issues as it will result in a mix of ipv4+ipv6 and ipv4-only systems. the correct fix would be to actually have a patch reverting the systems which are already ipv4-only, too.

Meanwhile, I recommend relying on #3049 to disable the "noIPV6" patch

@rotu
Copy link
Contributor Author

rotu commented Mar 6, 2025

The issue is that historically, ipv6 has given us more issues that it has helped, and we don't current have enough workforce to address the issue fully.

Even so, I'm not sure that this code correctly addresses issues caused by IPv6.

Loopback is still using IPv6 so I think that line net.ipv6.conf.lo.disable_ipv6=1 is not working at all by default. localhost will still have any problems that you associate with IPv6.

Disabling IPv6 on Ethernet causes Windows to prefer literally any other interface that has IPv6, even if it has a higher metric (like wifi)! I'm now realizing that trying to disable IPv4 is responsible for blueos-avahi.local preferring the wifi interface and zerotier over the ethernet adapter. AFAICT, the reason you need to edit the wifi connection is to fix the problems caused by disabling IPv6 on ethernet.

I think the right fix, if you want to not accidentally use IPv6 is to address the host using a static IPv4 address. If you want to use automatic routing with multiple network interfaces, then the solution is to to configure the MDNS daemon appropriately.

the correct fix would be to actually have a patch reverting the systems which are already ipv4-only, too.

Which systems are ipv4-only?

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