FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix support for services connected via IPv6 by edomora97 · Pull Request #1141 · cms-dev/cms · GitHub

/ cms Public

Fix support for services connected via IPv6 - #1141

Merged
andreyv merged 6 commits into
cms-dev:masterfrom
edomora97:fix/ipv6
Dec 17, 2021
ConversationCommits (6)ChecksFiles changed
Merged

Fix support for services connected via IPv6#1141
andreyv merged 6 commits into
cms-dev:masterfrom
edomora97:fix/ipv6

Conversation

edomora97 commented Oct 26, 2019 •
edited by stefano-maggiolo
Loading

Copy link
Copy Markdown
Contributor

Use the correct address family when connecting to the various RPC services.

The solution is basically the same suggested here:
#157 (comment)

This change is 

codecov Bot commented Oct 30, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1141 (03d6344) into master (de5a0ca) will increase coverage by 0.06%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #1141      +/-   ##
==========================================
+ Coverage   63.56%   63.63%   +0.06%     
==========================================
  Files         233      233              
  Lines       17117    17120       +3     
==========================================
+ Hits        10881    10894      +13     
+ Misses       6236     6226      -10
Flag Coverage Δ
functionaltests 47.44% <86.66%> (+0.35%) ⬆️
unittests 44.04% <80.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cms/io/rpc.py 94.00% <85.71%> (-0.16%) ⬇️
cms/io/service.py 72.78% <100.00%> (+1.44%) ⬆️
cms/service/Worker.py 82.65% <0.00%> (-4.09%) ⬇️
cms/service/workerpool.py 66.11% <0.00%> (-2.23%) ⬇️
cms/io/triggeredservice.py 84.53% <0.00%> (-2.07%) ⬇️
cms/db/base.py 87.12% <0.00%> (-1.00%) ⬇️
cms/db/util.py 52.59% <0.00%> (-0.75%) ⬇️
cms/grading/Sandbox.py 69.05% <0.00%> (+0.17%) ⬆️
cms/server/admin/handlers/base.py 68.97% <0.00%> (+0.33%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de5a0ca...03d6344. Read the comment docs.

andreyv requested changes Feb 4, 2021
View reviewed changes

andreyv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! Overall this looks good. Please see comments below.

Comment thread cms/io/rpc.py
Comment thread cms/io/rpc.py Outdated
Comment thread cms/io/service.py Outdated

edomora97 commented Feb 4, 2021

Copy link
Copy Markdown
Contributor Author

Sure! I'll take a look again at this in few days, I'm pretty busy right now!

edomora97 commented Dec 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Sorry for the small delay, I've rebased on the current master and fixed the comments you left. The first two commits are the exact same, the third fixes the code review (hopefully).

Not sure why the CI doesn't work, but it seems quite unrelated.

edomora97 mentioned this pull request Dec 11, 2021
Merged
andreyv requested changes Dec 12, 2021
View reviewed changes

andreyv left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi,

Thanks for the update. Now, re-reading the changes, I started wondering: do we need getaddrinfo() at all? Address already contains an IP address, so do we need to resolve it again?

It looks like it should be enough to just get ipaddress.ip_address(address[0]).version and select address family based on that. What do you think?

EDIT: We do need getaddrinfo(). socket.connect() accepts also hostnames, so removing address resolution would break that, while even CMS sample config lists localhost 🙂 So it's just one minor comment below.

Comment thread cms/io/rpc.py

edomora97 commented Dec 12, 2021

Copy link
Copy Markdown
Contributor Author

I don't think ipaddress.ip_address(address[0]).version is enough. That Address can also contain domain names (as you write them in cms.conf), and ipaddress.ip_address only parses plain IP addresses.

andreyv reviewed Dec 12, 2021
View reviewed changes
Comment thread cms/io/rpc.py Outdated

andreyv commented Dec 12, 2021

Copy link
Copy Markdown
Member

Thanks, the code looks good now and works fine with both IPv4 and IPv6.

Fix support for services connected via IPv6
a01d46e 
Use the correct address family when connecting to the various RPC services.

The solution is basically the same suggested here:
cms-dev#157 (comment)

This work is the result of the work of @edomora97, @Mark03 and @dariost.
Mock getaddrinfo in test_background_connect
c3863b3 
Calling getaddrinfo somehow breaks the mocking interface of socket,
replacing the MagicMock with the original function. This would break the
assert and the test in general.
To prevent so getaddrinfo is mocked as well, making it return just an
address without side effects.
Try all the resolved addresses
96ddba1 
- Try to resolve only with the SOCK_STREAM type
- Try to connect to all the resolved IP addresses, sometimes the first
  in the list is not the correct one (e.g. IPv4/v6)
- Remove redundant getaddrinfo in _connection_handler
Stop after the first connection
972c258
Pass family, type and proto to socket.socket()
a7129dd 
As suggested in https://docs.python.org/3/library/socket.html#socket.getaddrinfo

andreyv commented Dec 14, 2021

Copy link
Copy Markdown
Member

I rebased this branch onto de5a0ca.

One unit test fails, mock description needs to be adjusted:

AssertionError: expected call not found.
Expected: getaddrinfo('127.0.0.1', 36887)
Actual: getaddrinfo('127.0.0.1', 36887, type=<SocketKind.SOCK_STREAM: 1>)

andreyv commented Dec 17, 2021

Copy link
Copy Markdown
Member

Looks good, thanks.

andreyv merged commit a9fa374 into cms-dev:master Dec 17, 2021
stefano-maggiolo pushed a commit to stefano-maggiolo/cms that referenced this pull request Dec 21, 2021
Fix support for services connected via IPv6 (cms-dev#1141)
0c102eb 
* Fix support for services connected via IPv6

Use the correct address family when connecting to the various RPC services.

The solution is basically the same suggested here:
cms-dev#157 (comment)

This work is the result of the work of @edomora97, @Mark03 and @dariost.

* Mock getaddrinfo in test_background_connect

Calling getaddrinfo somehow breaks the mocking interface of socket,
replacing the MagicMock with the original function. This would break the
assert and the test in general.
To prevent so getaddrinfo is mocked as well, making it return just an
address without side effects.

* Try all the resolved addresses

- Try to resolve only with the SOCK_STREAM type
- Try to connect to all the resolved IP addresses, sometimes the first
  in the list is not the correct one (e.g. IPv4/v6)
- Remove redundant getaddrinfo in _connection_handler

* Stop after the first connection

* Pass family, type and proto to socket.socket()

As suggested in https://docs.python.org/3/library/socket.html#socket.getaddrinfo

* Fix tests
edomora97 deleted the fix/ipv6 branch December 27, 2021 17:43
RezwanArefin01 pushed a commit to RezwanArefin01/cms that referenced this pull request Dec 27, 2021
Fix support for services connected via IPv6 (cms-dev#1141)
f8b82ef 
* Fix support for services connected via IPv6

Use the correct address family when connecting to the various RPC services.

The solution is basically the same suggested here:
cms-dev#157 (comment)

This work is the result of the work of @edomora97, @Mark03 and @dariost.

* Mock getaddrinfo in test_background_connect

Calling getaddrinfo somehow breaks the mocking interface of socket,
replacing the MagicMock with the original function. This would break the
assert and the test in general.
To prevent so getaddrinfo is mocked as well, making it return just an
address without side effects.

* Try all the resolved addresses

- Try to resolve only with the SOCK_STREAM type
- Try to connect to all the resolved IP addresses, sometimes the first
  in the list is not the correct one (e.g. IPv4/v6)
- Remove redundant getaddrinfo in _connection_handler

* Stop after the first connection

* Pass family, type and proto to socket.socket()

As suggested in https://docs.python.org/3/library/socket.html#socket.getaddrinfo

* Fix tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

andreyv andreyv approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL