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

Replace ipv4 v6 sc 37307 #1729

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Replace ipv4 v6 sc 37307 #1729

merged 1 commit into from
Dec 5, 2024

Conversation

LeiGlobus
Copy link
Contributor

@LeiGlobus LeiGlobus commented Nov 20, 2024

One of the steps in making sure ipv6 is supported is too remove the usage of the ipv4 loopback address 127.0.0.1, replacing it with ::1 to confirm that things work with ipv6. (127.0.0.1 has already been working since beginning of history, so doesn't need to be kept in most circumstances.

We also decided that we should not go the extra mile to ensure that ipv6 is compatible with HTEX, as that's being deprecated. All the 127.0.0.1 should be only in the htex module.

[sc-37307]

@LeiGlobus LeiGlobus added the no-news-is-good-news This change does not require a news file label Nov 20, 2024
docs/autobuild.sh Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
Comment on lines 303 to 306
ipaddress.ip_address(address=address)
if address != "localhost":
# 'localhost' works for both v4 and v6 in some circumstances
# where an actual numeric IP isn't required
ipaddress.ip_address(address=address)
Copy link
Contributor

Choose a reason for hiding this comment

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

If localhost is allowed, then any name should be allowed. This has historically required a valid IP address, which is coming from either the default (signature argument) or the caller of this __init__ method, so I don't think this needs changing here.

Testing will confirm or rebuke the need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not informed enough regarding how this address is used, other than the general 'local' communication need. However, I did test (on Ubuntu) a few different parameters, after removing this check.

  1. localhost works
  2. an arbitrary address obviously doesn't work: zmq.error.ZMQError: Cannot assign requested address (addr='tcp://cnn.com:54419')

so, to be safe and modify the current behavior as little as possible, I only make an exception for 'localhost' but do ip address validation otherwise. If I remove the check altogether, that would need more testing of non-localhost DNS names, which I don't know how many lower layers can handle.

So, still leaning toward only making an exception for localhost to get rid of the dependence on 127.0.0.1. Note that '::1' does not work when directly passed to zmq, even if it's a valid ipv6 address.

Copy link
Contributor

Choose a reason for hiding this comment

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

cnn.com won't work because that's, well, not this host (which I think your 2nd point is saying?). But a friendly reminder that the technical specification of ZMQ's socket .bind() method says:

When assigning a local address to a socket using zmq_bind() with the tcp transport, the endpoint shall be interpreted as an interface followed by a colon and the TCP port number to use.

An interface may be specified by either of the following:

  • The wild-card *, meaning all available interfaces.

  • The primary IPv4 or IPv6 address assigned to the interface, in its numeric representation.

  • The non-portable interface name as defined by the operating system.

Meaning, it would, absent our ministrations, be completely valid to specify an interface -- indeed, that's the more fundamental detail from ZMQ's perspective! We could handle localhost singularly, but I think we can be more general about this solve.

Copy link
Contributor

Choose a reason for hiding this comment

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

But ... I think I've changed from my initial comment. Meh. This is a smaller detail than the need to make sure ipv6 is well handled. Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the validation to allow both hostname and ipv4/ipv6 addresses. Added the utility method in class and in test, and if it's used elsewhere, we can move it to a more general location.

@LeiGlobus LeiGlobus force-pushed the replace_ipv4_v6-sc-37307 branch 4 times, most recently from 912434e to 63cbaff Compare November 22, 2024 21:29
@LeiGlobus LeiGlobus force-pushed the replace_ipv4_v6-sc-37307 branch from 3874ea6 to 565333e Compare November 22, 2024 22:01
@LeiGlobus LeiGlobus requested a review from khk-globus November 25, 2024 16:27
@LeiGlobus LeiGlobus force-pushed the replace_ipv4_v6-sc-37307 branch 5 times, most recently from 8b13bff to c04a101 Compare December 2, 2024 20:45
@LeiGlobus LeiGlobus force-pushed the replace_ipv4_v6-sc-37307 branch 2 times, most recently from 3d8bdd1 to f17188e Compare December 5, 2024 20:39
@LeiGlobus LeiGlobus force-pushed the replace_ipv4_v6-sc-37307 branch 2 times, most recently from 4195b4c to bd63ff5 Compare December 5, 2024 20:53
@LeiGlobus LeiGlobus force-pushed the replace_ipv4_v6-sc-37307 branch from bd63ff5 to acd1d4f Compare December 5, 2024 20:54
@LeiGlobus LeiGlobus merged commit 0870047 into main Dec 5, 2024
21 checks passed
@LeiGlobus LeiGlobus deleted the replace_ipv4_v6-sc-37307 branch December 5, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-news-is-good-news This change does not require a news file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants