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.

Sign up for GitHub

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

Jump to bottom

[c#] support IPv6 ports as well #14913

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

[c#] support IPv6 ports as well #14913

wants to merge 3 commits into from

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Dec 18, 2024 •
edited by qodo-merge-pro bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Fixes #14910

Motivation and Context

Add support for IPv6 if IPv4 cannot be used. This is achieved by enabling dual-stack socket which accepts both IPv4 and IPv6.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix

Description

  • Fixed compatibility issue with pure IPv6 environments by implementing dual-stack socket support
  • Modified FindFreePort method to use AddressFamily.InterNetworkV6 instead of AddressFamily.InterNetwork
  • Added socket option IPv6Only = false to ensure backward compatibility with IPv4
  • Maintains existing functionality while adding IPv6 support, ensuring no breaking changes

Changes walkthrough 📝

Relevant files
Bug fix
PortUtilities.cs
Enable dual-stack socket support for IPv4 and IPv6             

dotnet/src/webdriver/Internal/PortUtilities.cs

  • Changed socket creation from IPv4-only to dual-stack IPv6 socket
  • Added socket option to support both IPv4 and IPv6 protocols
  • Modified port binding to work with both IPv4 and IPv6 addresses
    		•  +3/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @Delta456
    Revert "add alias attr to all edge structs"
    4001475 
    This reverts commit f17dd08dd32998a310510f0abc44e5b484202a4d.
    @Delta456
    [c#] support IPv6 ports as well
    cab48c6
    @Delta456
    how
    d0ad0ba
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14910 - Fully compliant

    Compliant requirements:

    • Fixed FindFreePort method to work in pure IPv6 environments by using dual-stack socket
    • Maintained backward compatibility with IPv4 by setting IPv6Only = false
    • Addressed the protocol incompatibility error by using InterNetworkV6
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Validation Required
    Verify that the socket binding works correctly when IPv4 is completely disabled in the system

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add system compatibility check for IPv6 support with IPv4 fallback to ensure broader system compatibility

    Add a check to verify if IPv6 is supported on the system, falling back to IPv4 if
    it's not available.

    dotnet/src/webdriver/Internal/PortUtilities.cs [41-44]

    -Socket portSocket = new Socket(AddressFamily.InterNetworkV6, SocketType.Stream, ProtocolType.Tcp);
+Socket portSocket;
 try
 {
-    portSocket.SetSocketOption(SocketOptionLevel.IPv6, SocketOptionName.IPv6Only, false);
+    if (Socket.OSSupportsIPv6)
+    {
+        portSocket = new Socket(AddressFamily.InterNetworkV6, SocketType.Stream, ProtocolType.Tcp);
+        portSocket.SetSocketOption(SocketOptionLevel.IPv6, SocketOptionName.IPv6Only, false);
+    }
+    else
+    {
+        portSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
+    }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical enhancement that adds robustness by checking system IPv6 support and providing IPv4 fallback, preventing potential failures on systems without IPv6 support. This significantly improves cross-platform compatibility.

    		9
    General
    Improve resource management by moving socket initialization into the try block to prevent resource leaks

    Move the socket declaration inside the try block to ensure proper resource cleanup
    in case of socket creation failure.

    dotnet/src/webdriver/Internal/PortUtilities.cs [41-44]

    -Socket portSocket = new Socket(AddressFamily.InterNetworkV6, SocketType.Stream, ProtocolType.Tcp);
+Socket portSocket = null;
 try
 {
+    portSocket = new Socket(AddressFamily.InterNetworkV6, SocketType.Stream, ProtocolType.Tcp);
     portSocket.SetSocketOption(SocketOptionLevel.IPv6, SocketOptionName.IPv6Only, false);
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Moving socket initialization into the try block is a good practice for resource management, ensuring proper cleanup in case of initialization failures. This improves code reliability and prevents potential resource leaks.

    		6

    @nvborisenko
    Copy link
    Member

    We have to carefully test it. What I see as potential issues:

    • make sure we use localhost instead of 127.0.0.1 on client side
    • when we start chromedriver --port={our_port} then what address it is listening to? v4 or v6? And when client resolves localhost then it will be resolved to v4 or v6? My concern here is that server and client should be aligned in terms of preferred ip version.

    It will be really nice if we can test it in different environment: v4 only, v4 + v6, v6 only.

    @Delta456
    Copy link
    Contributor Author

    I believe the Chrome driver will resolve to IPv4 so the localhost will also resolve to IPv4 by default.

    I am not familiar with C# so I am not sure how can I test in different environment.

    @nvborisenko
    Copy link
    Member

    I believe but don't trust.

    PS C:\Users\Nick> ping localhost

Pinging DESKTOP-SM1DOE9 [::1] with 32 bytes of data:
Reply from ::1: time<1ms
Reply from ::1: time<1ms
Reply from ::1: time<1ms
Reply from ::1: time<1ms

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 1
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: FindFreePort method hardcoded for IPv4 causing failure for pure IPv6 environments
    2 participants
    @Delta456 @nvborisenko