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

socket_mode Connection: Add support for IPv6 #1036

Merged
merged 3 commits into from Jun 15, 2021
Merged

socket_mode Connection: Add support for IPv6 #1036

merged 3 commits into from Jun 15, 2021

Conversation

KostyaEsmukov
Copy link
Contributor

@KostyaEsmukov KostyaEsmukov commented Jun 14, 2021

Summary

The current implementation uses hardcoded AF_INET (IPv4) family and a first DNS record for connection. This doesn't work in IPv6-only environments (via NAT64):

ERROR:slack_sdk.rtm_v2:Failed to establish a connection (session id: <STRIPPED>, error: [Errno -2] Name or service not known)
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/slack_sdk/socket_mode/builtin/connection.py", line 116, in connect
    trace_enabled=self.trace_enabled,
  File "/usr/lib/python3/dist-packages/slack_sdk/socket_mode/builtin/internals.py", line 123, in _establish_new_socket_connection
    sock.connect((server_hostname, server_port))
  File "/usr/lib/python3.6/ssl.py", line 1109, in connect
    self._real_connect(addr, False)
  File "/usr/lib/python3.6/ssl.py", line 1096, in _real_connect
    socket.connect(self, addr)
socket.gaierror: [Errno -2] Name or service not known

This is fixed here by switching to the socket.create_connection function, which correctly handles IPv4+IPv6 families and additionally iterates through all DNS records until the connection is made.

Category (place an x in each of the [ ])

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.models (UI component builders)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.rtm (RTM client)
  • slack_sdk.signature (Request Signature Verifier)
  • /docs-src (Documents, have you run ./docs.sh?)
  • /docs-src-v2 (Documents, have you run ./docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python setup.py validate after making the changes. [Some e2e tests don't pass with a ConnectionResetError: [Errno 54] Connection reset by peer error]

@gitwave gitwave bot added the untriaged label Jun 14, 2021
@seratch seratch added enhancement M-T: A feature request for new functionality socket-mode Version: 3x and removed untriaged labels Jun 14, 2021
@seratch seratch added this to the 3.7.0 milestone Jun 14, 2021
@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #1036 (aa19a09) into main (32d8ae9) will increase coverage by 0.07%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1036      +/-   ##
==========================================
+ Coverage   84.34%   84.41%   +0.07%     
==========================================
  Files          95       95              
  Lines        8877     8869       -8     
==========================================
  Hits         7487     7487              
+ Misses       1390     1382       -8     
Impacted Files Coverage Δ
slack_sdk/socket_mode/builtin/internals.py 48.43% <20.00%> (+0.37%) ⬆️
slack_sdk/socket_mode/client.py 72.56% <0.00%> (+2.65%) ⬆️

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 32d8ae9...aa19a09. Read the comment docs.

@seratch seratch self-requested a review June 14, 2021 20:57
@seratch
Copy link
Member

seratch commented Jun 14, 2021

Hi @KostyaEsmukov, thanks for taking the time to make this pull request! It looks great to me but I would like to verify the behavior with proxy before merging this change (As codecov pointed out, the pattern with proxy is not covered by unit tests yet).

The integration tests have a few proxy examples. I will manually check the behavior later in my timezone (+09:00) https://github.com/slackapi/python-slack-sdk/blob/main/integration_tests/samples/socket_mode/builtin_proxy_example.py

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the perfect work. I've manually verified if there is no regression with this change with the proxy settings. Looks great to me 👍

@seratch seratch merged commit 3fcdbe1 into slackapi:main Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality socket-mode Version: 3x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants