fix: Use HTTP_PROXY and HTTPS_PROXY environment variables for proxy (#2397)

Author: myakove

README.md

@@ -96,19 +96,16 @@ export OPENSHIFT_PYTHON_WRAPPER_LOG_LEVEL=<LOG_LEVEL> # can be: "DEBUG", "INFO",
 ## Proxy Enablement
 
 This configuration allows the client to route traffic through a specified proxy server.
-It can be enabled via the environment variable `OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY`.
 
 To enable proxy configuration for the client:
 
-1. Set the environment variable `OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY=<any value>`
+1. Define either `HTTPS_PROXY` or `HTTP_PROXY` environment variable with your proxy URL:
 
-2. Define either `HTTPS_PROXY` or `HTTP_PROXY` environment variable with your proxy URL:
 ```bash
 export HTTPS_PROXY="http://proxy.example.com:8080"
 # or
 export HTTP_PROXY="http://proxy.example.com:8080"
 ```
-If neither variable is set when proxy is enabled, a `ValueError` will be raised.
 
 ## Code check
 

ocp_resources/resource.py

@@ -77,10 +77,13 @@ def _get_api_version(dyn_client: DynamicClient, api_group: str, kind: str) -> st
 
 
 def get_client(
-    config_file: str = "",
+    config_file: str | None = None,
     config_dict: dict[str, Any] | None = None,
-    context: str = "",
-    **kwargs: Any,
+    context: str | None = None,
+    client_configuration: kubernetes.client.Configuration | None = None,
+    persist_config: bool = True,
+    temp_file_path: str | None = None,
+    try_refresh_token: bool = True,
 ) -> DynamicClient:
     """
     Get a kubernetes client.
@@ -97,19 +100,31 @@ def get_client(
         config_file (str): path to a kubeconfig file.
         config_dict (dict): dict with kubeconfig configuration.
         context (str): name of the context to use.
+        persist_config (bool): whether to persist config file.
+        temp_file_path (str): path to a temporary kubeconfig file.
+        try_refresh_token (bool): try to refresh token
 
     Returns:
         DynamicClient: a kubernetes client.
     """
+    proxy = os.environ.get("HTTPS_PROXY") or os.environ.get("HTTP_PROXY")
+
+    client_configuration = client_configuration or kubernetes.client.Configuration()
+
+    if not client_configuration.proxy and proxy:
+        LOGGER.info(f"Setting proxy from environment variable: {proxy}")
+        client_configuration.proxy = proxy
+
     # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/kube_config.py
     if config_dict:
-        return kubernetes.dynamic.DynamicClient(
-            client=kubernetes.config.new_client_from_config_dict(
-                config_dict=config_dict, context=context or None, **kwargs
-            )
+        _client = kubernetes.config.new_client_from_config_dict(
+            config_dict=config_dict,
+            context=context,
+            client_configuration=client_configuration,
+            persist_config=persist_config,
+            temp_file_path=temp_file_path,
         )
-    client_configuration = kwargs.get("client_configuration", kubernetes.client.Configuration())
-    try:
+    else:
         # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/__init__.py
         LOGGER.info("Trying to get client via new_client_from_config")
 
@@ -118,36 +133,22 @@ def get_client(
         # is populated during import which comes before setting the variable in code.
         config_file = config_file or os.environ.get("KUBECONFIG", "~/.kube/config")
 
-        if os.environ.get("OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY"):
-            proxy = os.environ.get("HTTPS_PROXY") or os.environ.get("HTTP_PROXY")
-            if not proxy:
-                raise ValueError(
-                    "Proxy configuration is enabled but neither HTTPS_PROXY nor HTTP_PROXY environment variables are set."
-                )
-            if client_configuration.proxy and client_configuration.proxy != proxy:
-                raise ValueError(
-                    f"Conflicting proxy settings: client_configuration.proxy={client_configuration.proxy}, "
-                    f"but the environment variable 'HTTPS_PROXY/HTTP_PROXY' defines proxy as {proxy}."
-                )
-            client_configuration.proxy = proxy
-
-        kwargs["client_configuration"] = client_configuration
-
-        return kubernetes.dynamic.DynamicClient(
-            client=kubernetes.config.new_client_from_config(
-                config_file=config_file,
-                context=context or None,
-                **kwargs,
-            )
+        _client = kubernetes.config.new_client_from_config(
+            config_file=config_file,
+            context=context,
+            client_configuration=client_configuration,
+            persist_config=persist_config,
         )
+
+    try:
+        return kubernetes.dynamic.DynamicClient(client=_client)
     except MaxRetryError:
         # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/incluster_config.py
         LOGGER.info("Trying to get client via incluster_config")
         return kubernetes.dynamic.DynamicClient(
             client=kubernetes.config.incluster_config.load_incluster_config(
-                client_configuration=client_configuration,
-                try_refresh_token=kwargs.get("try_refresh_token", True),
-            )
+                client_configuration=client_configuration, try_refresh_token=try_refresh_token
+            ),
         )
 
 
@@ -429,9 +430,9 @@ def __init__(
         dry_run: bool = False,
         node_selector: dict[str, Any] | None = None,
         node_selector_labels: dict[str, str] | None = None,
-        config_file: str = "",
+        config_file: str | None = None,
         config_dict: dict[str, Any] | None = None,
-        context: str = "",
+        context: str | None = None,
         label: dict[str, str] | None = None,
         annotations: dict[str, str] | None = None,
         api_group: str = "",
@@ -486,11 +487,6 @@ def __init__(
         self.node_selector = node_selector
         self.node_selector_labels = node_selector_labels
         self.config_file = config_file
-        if not isinstance(self.config_file, str):
-            # If we pass config_file which isn't a string, get_client will fail and it will be very hard to know why.
-            # Better fail here and let the user know.
-            raise ValueError("config_file must be a string")
-
         self.config_dict = config_dict or {}
         self.context = context
         self.label = label
@@ -967,10 +963,10 @@ def retry_cluster_exceptions(
     def get(
         cls,
         config_file: str = "",
-        context: str = "",
         singular_name: str = "",
         exceptions_dict: dict[type[Exception], list[str]] = DEFAULT_CLUSTER_RETRY_EXCEPTIONS,
         raw: bool = False,
+        context: str | None = None,
         dyn_client: DynamicClient | None = None,
         *args: Any,
         **kwargs: Any,
@@ -1196,7 +1192,7 @@ def events(
     def get_all_cluster_resources(
         client: DynamicClient | None = None,
         config_file: str = "",
-        context: str = "",
+        context: str | None = None,
         config_dict: dict[str, Any] | None = None,
         *args: Any,
         **kwargs: Any,
@@ -1333,10 +1329,10 @@ def __init__(
     def get(
         cls,
         config_file: str = "",
-        context: str = "",
         singular_name: str = "",
         exceptions_dict: dict[type[Exception], list[str]] = DEFAULT_CLUSTER_RETRY_EXCEPTIONS,
         raw: bool = False,
+        context: str | None = None,
         dyn_client: DynamicClient | None = None,
         *args: Any,
         **kwargs: Any,

tests/test_resources.py

@@ -1,7 +1,8 @@
+from __future__ import annotations
+
 import pytest
 import yaml
 from docker.errors import DockerException
-import kubernetes
 from testcontainers.k3s import K3SContainer
 
 from ocp_resources.exceptions import ResourceTeardownError
@@ -19,13 +20,18 @@ def clean_up(self, wait: bool = True, timeout: int | None = None) -> bool:
         return False
 
 
-@pytest.fixture(scope="session")
-def client():
+@pytest.fixture(scope="class")
+def k3scontainer_config():
     try:
         with K3SContainer() as k3s:
-            yield get_client(config_dict=yaml.safe_load(k3s.config_yaml()))
-    except DockerException:
-        pytest.skip("K3S container not available")
+            yield yaml.safe_load(k3s.config_yaml())
+    except DockerException as ex:
+        pytest.skip(f"K3S container not available. {ex}")
+
+
+@pytest.fixture(scope="class")
+def client(k3scontainer_config):
+    yield get_client(config_dict=k3scontainer_config)
 
 
 @pytest.fixture(scope="class")
@@ -109,25 +115,21 @@ def test_resource_context_manager_exit(self, client):
             with SecretTestExit(name="test-context-manager-exit", namespace="default", client=client):
                 pass
 
-    def test_proxy_enabled_but_no_proxy_set(self, monkeypatch):
-        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.",
-        ):
-            get_client()
+class TestClient:
+    def test_client_with_proxy(self, monkeypatch, k3scontainer_config):
+        http_proxy = "http://env-http-proxy.com"
+        monkeypatch.setenv(name="HTTPS_PROXY", value=http_proxy)
 
-    def test_proxy_conflict_raises_value_error(self, monkeypatch):
-        monkeypatch.setenv(name="OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY", value="1")
-        monkeypatch.setenv(name="HTTPS_PROXY", value="http://env-proxy.com")
+        client = get_client(config_dict=k3scontainer_config)
+        assert client.configuration.proxy == http_proxy
 
-        client_configuration = kubernetes.client.Configuration()
-        client_configuration.proxy = "http://not-env-proxy.com"
+    def test_proxy_precedence(self, monkeypatch, k3scontainer_config):
+        https_proxy = "https://env-https-proxy.com"
+        http_proxy = "http://env-http-proxy.com"
+        monkeypatch.setenv(name="HTTPS_PROXY", value=https_proxy)
+        monkeypatch.setenv(name="HTTP_PROXY", value=http_proxy)
 
-        with pytest.raises(
-            ValueError,
-            match="Conflicting proxy settings: client_configuration.proxy=http://not-env-proxy.com, "
-            "but the environment variable 'HTTPS_PROXY/HTTP_PROXY' defines proxy as http://env-proxy.com.",
-        ):
-            get_client(client_configuration=client_configuration)
+        client = get_client(config_dict=k3scontainer_config)
+        # Verify HTTPS_PROXY takes precedence over HTTP_PROXY
+        assert client.configuration.proxy == https_proxy