From 1b45ed26d40ce6269b5789e296bc2ec486249bbf Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 8 May 2025 14:01:55 +0300 Subject: [PATCH 1/8] Use `HTTP_PROXY` and `HTTPS_PROXY` environment variables for proxy Use the same proxy configuration as the `http` and `https` protocols. --- README.md | 5 ++--- ocp_resources/resource.py | 47 +++++++++++++++++---------------------- tests/test_resources.py | 42 +++++++++++++--------------------- 3 files changed, 38 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index f77d95cc6f..2c68e5c3cc 100644 --- a/README.md +++ b/README.md @@ -96,18 +96,17 @@ export OPENSHIFT_PYTHON_WRAPPER_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=` +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 diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index 9fe83241c8..ef6c72ab21 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -101,15 +101,22 @@ def get_client( Returns: DynamicClient: a kubernetes client. """ + client_configuration = kwargs.get("client_configuration", kubernetes.client.Configuration()) + + # If the proxy is not set, set it from the environment + if not client_configuration.proxy: + proxy = os.environ.get("HTTPS_PROXY") or os.environ.get("HTTP_PROXY") + if proxy: + client_configuration.proxy = proxy + + kwargs["client_configuration"] = client_configuration + # 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 or None, **kwargs ) - 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,28 +125,14 @@ 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 or None, + **kwargs, ) + + 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") diff --git a/tests/test_resources.py b/tests/test_resources.py index 110cfeb874..6ca49a0d81 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -1,7 +1,7 @@ import pytest import yaml from docker.errors import DockerException -import kubernetes +from kubernetes.config.config_exception import ConfigException from testcontainers.k3s import K3SContainer from ocp_resources.exceptions import ResourceTeardownError @@ -19,13 +19,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 +114,10 @@ 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() - - 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_configuration = kubernetes.client.Configuration() - client_configuration.proxy = "http://not-env-proxy.com" +def test_client_with_proxy(monkeypatch, k3scontainer_config): + proxy = "http://env-proxy.com" + monkeypatch.setenv(name="HTTPS_PROXY", value=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) + with pytest.raises(ConfigException, match=r"Service host/port is not set."): + get_client(config_dict=k3scontainer_config) From b1d56e5e6949f9c7ff378d74298d5c9aad90ab1a Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 8 May 2025 14:50:09 +0300 Subject: [PATCH 2/8] Set proxy on the right client --- ocp_resources/resource.py | 44 ++++++++++++++++++++++----------------- tests/test_resources.py | 5 ++--- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index ef6c72ab21..dc7ed588dc 100644 --- a/ocp_resources/resource.py +++ b/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,24 +100,21 @@ 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. """ - client_configuration = kwargs.get("client_configuration", kubernetes.client.Configuration()) - - # If the proxy is not set, set it from the environment - if not client_configuration.proxy: - proxy = os.environ.get("HTTPS_PROXY") or os.environ.get("HTTP_PROXY") - if proxy: - client_configuration.proxy = proxy - - kwargs["client_configuration"] = client_configuration - # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/kube_config.py if config_dict: _client = kubernetes.config.new_client_from_config_dict( - config_dict=config_dict, context=context or None, **kwargs + config_dict=config_dict, + context=context, + client_configuration=client_configuration, + persist_config=persist_config, + temp_file_path=temp_file_path, ) else: # Ref: https://github.com/kubernetes-client/python/blob/v26.1.0/kubernetes/base/config/__init__.py @@ -127,10 +127,17 @@ def get_client( _client = kubernetes.config.new_client_from_config( config_file=config_file, - context=context or None, - **kwargs, + context=context, + client_configuration=client_configuration, + persist_config=persist_config, ) + proxy = os.environ.get("HTTPS_PROXY") or os.environ.get("HTTP_PROXY") + + if not _client.configuration.proxy and proxy: + LOGGER.info(f"Setting proxy from environment variable: {proxy}") + _client.configuration.proxy = proxy + try: return kubernetes.dynamic.DynamicClient(client=_client) except MaxRetryError: @@ -138,9 +145,8 @@ def get_client( 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 + ), ) diff --git a/tests/test_resources.py b/tests/test_resources.py index 6ca49a0d81..162d35274f 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -1,7 +1,6 @@ import pytest import yaml from docker.errors import DockerException -from kubernetes.config.config_exception import ConfigException from testcontainers.k3s import K3SContainer from ocp_resources.exceptions import ResourceTeardownError @@ -119,5 +118,5 @@ def test_client_with_proxy(monkeypatch, k3scontainer_config): proxy = "http://env-proxy.com" monkeypatch.setenv(name="HTTPS_PROXY", value=proxy) - with pytest.raises(ConfigException, match=r"Service host/port is not set."): - get_client(config_dict=k3scontainer_config) + client = get_client(config_dict=k3scontainer_config) + assert client.configuration.proxy == proxy From 5aa6380a24fd85510290cc6b43644da8a159184c Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 8 May 2025 15:06:15 +0300 Subject: [PATCH 3/8] remove valueerror from the readme --- README.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/README.md b/README.md index 2c68e5c3cc..0e927c6482 100644 --- a/README.md +++ b/README.md @@ -107,8 +107,6 @@ export HTTPS_PROXY="http://proxy.example.com:8080" export HTTP_PROXY="http://proxy.example.com:8080" ``` -If neither variable is set when proxy is enabled, a `ValueError` will be raised. - ## Code check We use pre-commit for code check. From 5ff873159eac9354f7c5061c10204899428fd4ab Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 8 May 2025 15:14:09 +0300 Subject: [PATCH 4/8] add from __future__ import annotations --- tests/test_resources.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_resources.py b/tests/test_resources.py index 162d35274f..78d21ae8e1 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import pytest import yaml from docker.errors import DockerException From 35a47f7427b05a280c217434e47358b0ad6f657d Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 8 May 2025 15:40:37 +0300 Subject: [PATCH 5/8] Add another test for the client --- tests/test_resources.py | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tests/test_resources.py b/tests/test_resources.py index 78d21ae8e1..4f9bd34f69 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -116,9 +116,20 @@ def test_resource_context_manager_exit(self, client): pass -def test_client_with_proxy(monkeypatch, k3scontainer_config): - proxy = "http://env-proxy.com" - monkeypatch.setenv(name="HTTPS_PROXY", value=proxy) - - client = get_client(config_dict=k3scontainer_config) - assert client.configuration.proxy == proxy +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) + + client = get_client(config_dict=k3scontainer_config) + assert client.configuration.proxy == http_proxy + + 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) + + client = get_client(config_dict=k3scontainer_config) + # Verify HTTPS_PROXY takes precedence over HTTP_PROXY + assert client.configuration.proxy == https_proxy From 233696c49e2153dfb2381066d62993df60fa4a1b Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 8 May 2025 16:07:18 +0300 Subject: [PATCH 6/8] context should be None by default --- ocp_resources/resource.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index dc7ed588dc..8ef6708eff 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -428,9 +428,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 = "", @@ -966,10 +966,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, @@ -1195,7 +1195,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, @@ -1332,10 +1332,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, From 06faf24b83a33dec0cd228b9b690a1d440f240f2 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Thu, 8 May 2025 16:10:46 +0300 Subject: [PATCH 7/8] Do not force config_file to be string --- ocp_resources/resource.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index 8ef6708eff..ddd5b16ca4 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -485,11 +485,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 From 2272482ab4683c524977e71f36327b7b9c47d67b Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Sun, 11 May 2025 11:29:29 +0300 Subject: [PATCH 8/8] set proxy first --- ocp_resources/resource.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index ddd5b16ca4..fe87aef0e8 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -107,6 +107,14 @@ def get_client( 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: _client = kubernetes.config.new_client_from_config_dict( @@ -132,12 +140,6 @@ def get_client( persist_config=persist_config, ) - proxy = os.environ.get("HTTPS_PROXY") or os.environ.get("HTTP_PROXY") - - if not _client.configuration.proxy and proxy: - LOGGER.info(f"Setting proxy from environment variable: {proxy}") - _client.configuration.proxy = proxy - try: return kubernetes.dynamic.DynamicClient(client=_client) except MaxRetryError: