fix(cncf-kubernetes): use Configuration.socket_options instead of monkey-patching urllib3#68557
Open
anmolxlight wants to merge 1 commit into
Open
fix(cncf-kubernetes): use Configuration.socket_options instead of monkey-patching urllib3#68557anmolxlight wants to merge 1 commit into
anmolxlight wants to merge 1 commit into
Conversation
anmolxlight
requested review from
hussein-awala,
jedcunningham,
jscheffl and
shahar1
as code owners
boring-cyborg
Bot
added
area:providers
provider:cncf-kubernetes
anmolxlight
force-pushed
the
fix-tcp-keepalive-urllib3-v2
branch
from
9b380b4 to
27a9bca
Compare
…key-patching urllib3 (apache#68396) The `enable_tcp_keepalive` config option in the Kubernetes provider relied on monkey-patching urllib3's default_socket_options. In urllib3 v2.x, the socket_options parameter in HTTPConnection.__init__ is evaluated as a default argument at import time, so post-import changes are never picked up by new connections. Fix by passing socket options through the Kubernetes client's Configuration.socket_options field, which is properly threaded through ApiClient -> RESTClientObject -> urllib3.PoolManager. Also includes TCP_NODELAY in the socket options to preserve the default urllib3 behavior of disabling Nagle's algorithm. For the KubernetesHook case, keepalive is applied in _TimeoutK8sApiClient.__init__ AFTER config loading so the configuration already has the correct host/credentials. Closes: apache#68396
anmolxlight
force-pushed
the
fix-tcp-keepalive-urllib3-v2
branch
from
27a9bca to
fa68e17
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
kubernetes_executor.enable_tcp_keepaliveconfig option was relying on monkey-patching urllib3'sdefault_socket_optionsafter module import. In urllib3 v2.x, thesocket_optionsparameter ofHTTPConnection.__init__is evaluated as a default argument at import time, so changing the class attribute afterwards has no effect on newly created connections. This meant the TCP keepalive configuration was silently a no-op.This PR fixes the issue by passing socket options through the Kubernetes client's
Configuration.socket_optionsfield, which is properly threaded through:ApiClient->RESTClientObject.__init__->urllib3.PoolManager->HTTPConnectionPool->HTTPConnection.__init__The
kubernetes-client/pythonlibrary already supports this:Configurationhas asocket_optionsattribute that is checked inRESTClientObject.__init__and forwarded tourllib3.PoolManageras a keyword argument.Changes
_enable_tcp_keepalive()now accepts aConfigurationobject and setsconfiguration.socket_optionsdirectly instead of monkey-patching urllib3.get_kube_client()calls it after obtaining the configuration object._enable_tcp_keepalivetest_enable_tcp_keepaliveto verifyconfiguration.socket_optionsis set correctlyNotes
TCP_NODELAYin the socket options to preserve the default urllib3 behavior of disabling Nagle's algorithmRelated issue
Closes: #68396
Reproduction
See https://github.com/jonminter-dojo/airflow-k8s-tcp-keepalive-repro for a minimal reproduction demonstrating the bug with the old approach.