Add proxy support to Kubernetes client configuration in get_client #2291
Conversation
WalkthroughThe changes involve modifying the Changes
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
|
/verified |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
124-129: LGTM! Proxy configuration implementation is robust.The implementation correctly handles proxy configuration through environment variables, following standard practices. Consider adding a debug log when no proxy is found to improve observability.
client_configuration = client_configuration or client.Configuration() proxy = os.environ.get("HTTPS_PROXY") or os.environ.get("HTTP_PROXY") +if not proxy: + LOGGER.debug("No proxy configuration found in environment variables") if proxy: LOGGER.info(f"Trying to get client using proxy {proxy}") client_configuration.proxy = proxy
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/resource.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (2)
ocp_resources/resource.py (2)
83-87: LGTM! Function signature changes are well-structured.The new
client_configurationparameter is properly typed and documented, allowing for flexible client configuration injection.Also applies to: 103-103
132-134: LGTM! Client creation properly utilizes the configuration.The updated client creation correctly passes the configuration object to
new_client_from_config.
sbahar619
force-pushed
the
CNV-46351/add-proxy-support-to-k8s-client-configuration
branch
from
043a0f7 to
e6d27e6
Compare
|
/verified |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ocp_resources/resource.py (1)
102-102: Enhance the docstring for better clarity.The documentation for
client_configurationshould be more descriptive to help users understand its purpose and usage.- client_configuration (kubernetes.client.Configuration): The kubernetes.client.Configuration to set configs to. + client_configuration (kubernetes.client.Configuration): Optional custom Kubernetes client configuration. + Use this to provide pre-configured settings such as proxy configuration, SSL certificates, + API keys, etc. If not provided, a new configuration will be created. + Example: + config = kubernetes.client.Configuration() + config.proxy = "http://proxy.example.com:8080" + client = get_client(client_configuration=config)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ocp_resources/resource.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (2)
ocp_resources/resource.py (2)
82-86: LGTM! Function signature changes are well-structured.The new
client_configurationparameter is properly typed and maintains backward compatibility.
131-133: LGTM! Client configuration is properly utilized.The
client_configurationis correctly passed tonew_client_from_config, ensuring consistent configuration throughout the client creation process.
sbahar619
force-pushed
the
CNV-46351/add-proxy-support-to-k8s-client-configuration
branch
from
e6d27e6 to
791e1b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_resources.py (1)
108-120: Add documentation and improve readability.The test implementation is solid, using
monkeypatchfor safe environment variable handling and flexible error message assertions. However, a few improvements would make it even better:
- Add a docstring explaining the test's purpose
- Add comments to group the logical sections
Apply this diff to improve documentation and readability:
def test_proxy_conflict_raises_value_error(self, monkeypatch): + """Test that get_client raises ValueError when proxy settings conflict. + + When both environment variables and client configuration define different proxy + settings, get_client should raise a ValueError indicating the conflict. + """ + # Setup proxy via environment variables monkeypatch.setenv(name="OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY", value="1") monkeypatch.setenv(name="HTTPS_PROXY", value="http://env-proxy.com") + # Create conflicting client configuration client_configuration = kubernetes.client.Configuration() client_configuration.proxy = "http://not-env-proxy.com" + # Verify that ValueError is raised with appropriate message with pytest.raises(ValueError) as exc_info: get_client(client_configuration=client_configuration) assert "Conflicting proxy settings" in str(exc_info.value) assert "http://not-env-proxy.com" in str(exc_info.value) assert "http://env-proxy.com" in str(exc_info.value)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_resources.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (1)
tests/test_resources.py (1)
3-3: LGTM!The
kubernetesimport is correctly placed and necessary for creating the Configuration object in the new test.
|
/verified |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_resources.py (1)
108-115: Add docstring and make error message assertion more maintainable.Good use of
monkeypatchfor environment variable management. Consider these improvements:def test_proxy_enabled_but_no_proxy_set(self, monkeypatch): + """Test that get_client raises ValueError when proxy is enabled but not configured. + + When OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY is set but neither HTTPS_PROXY + nor HTTP_PROXY are defined, get_client should raise a ValueError. + """ monkeypatch.setenv(name="OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY", value="1") with pytest.raises( ValueError, - match="Proxy configuration is enabled but neither HTTPS_PROXY nor HTTP_PROXY environment variables are set.", + match=r".*neither HTTPS_PROXY nor HTTP_PROXY.*", ): get_client()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_resources.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (2)
tests/test_resources.py (2)
3-3: LGTM!The
kubernetesimport is correctly placed and necessary for the new test functionality.
117-129: Improve test structure and maintainability.Good use of
monkeypatchfor environment variable management. Consider these improvements:def test_proxy_conflict_raises_value_error(self, monkeypatch): + """Test that get_client raises ValueError when proxy settings conflict. + + When both environment variables and client configuration define different proxy + settings, get_client should raise a ValueError indicating the conflict. + """ + # Setup environment proxy monkeypatch.setenv(name="OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY", value="1") monkeypatch.setenv(name="HTTPS_PROXY", value="http://env-proxy.com") + # Setup conflicting client proxy client_configuration = kubernetes.client.Configuration() client_configuration.proxy = "http://not-env-proxy.com" + # Verify error is raised with appropriate message with pytest.raises( ValueError, - match="Conflicting proxy settings: client_configuration.proxy=http://not-env-proxy.com, " - "but the environment variable 'OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY' defines proxy as http://env-proxy.com.", + match=r".*Conflicting proxy settings.*http://not-env-proxy\.com.*http://env-proxy\.com.*", ): get_client(client_configuration=client_configuration)
|
/verified |
Short description:
Add proxy support to Kubernetes client configuration in get_client
More details:
This update modifies the get_client function to support setting a proxy for the Kubernetes client configuration. The client_configuration argument is introduced to allow passing a client.Configuration object directly, and if not provided, it defaults to creating a new configuration instance. Additionally, if the HTTPS_PROXY or HTTP_PROXY environment variables are set, the proxy is applied to the Kubernetes client configuration.
Introduced dynamic proxy enablement by reading the OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY environment variable.
Updated README.md with instructions on using the new proxy enablement feature.
What this PR does / why we need it:
This solution is preferred over requiring the caller to set the client.Configuration for each client instance creation. Doing so would necessitate updating every instance where the client is created, which would be less efficient and error-prone.
Which issue(s) this PR fixes:
https://issues.redhat.com/browse/CNV-46351
Special notes for reviewer:
A similar solution was previously proposed in PR #2066 , but it was not accepted at the time. This PR aims to provide additional reasoning and demonstrate why this approach is the most effective.
This change is especially useful in environments where proxies are required, and it simplifies the client setup process while minimizing the chances of errors.
Note:
We can also promote this upstream PR in kubernetes-client, which is a good solution:
kubernetes-client/python#2182
Bug:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests