diff --git a/app/pyproject.toml b/app/pyproject.toml index 386db293..2da3b106 100644 --- a/app/pyproject.toml +++ b/app/pyproject.toml @@ -3,7 +3,7 @@ [project] name = "github-runner-image-builder" -version = "0.12.0" +version = "0.12.1" authors = [ { name = "Canonical IS DevOps", email = "is-devops-team@canonical.com" }, ] diff --git a/app/tests/conftest.py b/app/tests/conftest.py index 884af93f..35cb4663 100644 --- a/app/tests/conftest.py +++ b/app/tests/conftest.py @@ -3,7 +3,6 @@ """Fixtures for github runner image builder app.""" - from pytest import Parser diff --git a/app/tests/integration/conftest.py b/app/tests/integration/conftest.py index 1c3fa53f..36ffebcc 100644 --- a/app/tests/integration/conftest.py +++ b/app/tests/integration/conftest.py @@ -2,6 +2,7 @@ # See LICENSE file for licensing details. """Fixtures for github runner image builder integration tests.""" + import logging import os import secrets diff --git a/app/tests/integration/helpers.py b/app/tests/integration/helpers.py index fb12dffc..38f11a49 100644 --- a/app/tests/integration/helpers.py +++ b/app/tests/integration/helpers.py @@ -382,8 +382,7 @@ def setup_aproxy(ssh_connection: SSHConnection, proxy: str) -> None: proxy: The hostname and port in the format of "hostname:port". """ ssh_connection.run(f"/usr/bin/sudo snap set aproxy proxy={proxy} listen=:8444") - ssh_connection.run( - """/usr/bin/sudo nft -f - << EOF + ssh_connection.run("""/usr/bin/sudo nft -f - << EOF define default-ip = $(ip route get $(ip route show 0.0.0.0/0 | grep -oP 'via \\K\\S+') \ | grep -oP 'src \\K\\S+') define private-ips = { 10.0.0.0/8, 127.0.0.1/8, 172.16.0.0/12, 192.168.0.0/16 } @@ -400,8 +399,7 @@ def setup_aproxy(ssh_connection: SSHConnection, proxy: str) -> None: } } EOF -""" - ) +""") # Wait for aproxy to become active. for _ in range(6): time.sleep(5) diff --git a/app/tests/integration/test_openstack_builder.py b/app/tests/integration/test_openstack_builder.py index 23195af1..a1976f74 100644 --- a/app/tests/integration/test_openstack_builder.py +++ b/app/tests/integration/test_openstack_builder.py @@ -9,6 +9,7 @@ import functools import itertools import logging +import secrets import typing import urllib.parse from datetime import datetime, timezone @@ -120,8 +121,8 @@ def image_ids_fixture( script_config=config.ScriptConfig( script_url=urllib.parse.urlparse(TESTDATA_TEST_SCRIPT_URL), script_secrets={ - "TEST_SECRET": "SHOULD_EXIST", - "TEST_NON_SECRET": "SHOULD_NOT_EXIST", + "TEST_SECRET": secrets.token_hex(16), + "TEST_NON_SECRET": secrets.token_hex(16), }, ), ), diff --git a/app/tests/unit/test_logging.py b/app/tests/unit/test_logging.py index d6ab7efc..326904fa 100644 --- a/app/tests/unit/test_logging.py +++ b/app/tests/unit/test_logging.py @@ -2,6 +2,7 @@ # See LICENSE file for licensing details. """Unit tests for logging module.""" + import logging as logging_module from pathlib import Path from unittest.mock import MagicMock diff --git a/app/tests/unit/test_openstack_builder.py b/app/tests/unit/test_openstack_builder.py index 73e71266..4fddc2ba 100644 --- a/app/tests/unit/test_openstack_builder.py +++ b/app/tests/unit/test_openstack_builder.py @@ -286,7 +286,9 @@ def test_run( if with_external_script else None ), - script_secrets={"TEST_SECRET_ONE": "HELLO"} if with_external_script else {}, + script_secrets=( + ({"TEST_SECRET_ONE": secrets.token_hex(16)}) if with_external_script else {} + ), ), ) @@ -694,7 +696,10 @@ def test__generate_cloud_init_script( name="test-image", script_config=openstack_builder.config.ScriptConfig( script_url=urllib.parse.urlparse("https://test-url.com/script.sh"), - script_secrets={"TEST_SECRET_ONE": "HELLO", "TEST_SECRET_TWO": "WORLD"}, + script_secrets={ + "TEST_SECRET_ONE": secrets.token_hex(16), + "TEST_SECRET_TWO": secrets.token_hex(16), + }, ), ), proxy="test.proxy.internal:3128", @@ -948,9 +953,13 @@ def test__wait_for_cloud_init_complete(): @pytest.mark.parametrize( "script_secrets", [ - pytest.param({"TEST_SECRET_ONE": "HELLO"}, id="single secret"), + pytest.param({"TEST_SECRET_ONE": secrets.token_hex(16)}, id="single secret"), pytest.param( - {"TEST_SECRET_ONE": "HELLO", "TEST_SECRET_TWO": "WORLD"}, id="multiple secrets" + { + "TEST_SECRET_ONE": secrets.token_hex(16), + "TEST_SECRET_TWO": secrets.token_hex(16), + }, + id="multiple secrets", ), pytest.param({}, id="no secrets"), ], @@ -1011,7 +1020,7 @@ def run_side_effect(*_, **__): with pytest.raises(ExternalScriptError) as exc: openstack_builder._execute_external_script( script_url="https://test-url.com/script.sh", - script_secrets={"TEST_SECRET_ONE": "HELLO"}, + script_secrets={"TEST_SECRET_ONE": secrets.token_hex(16)}, ssh_conn=mock_connection, ) assert "Unexpected exit code" in str(exc) diff --git a/docs/changelog.md b/docs/changelog.md index 603312f4..6ebb21fc 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -1,6 +1,9 @@ +## [#185 Remove aproxy installation and add proxy support in workload](https://github.com/canonical/github-runner-image-builder-operator/pull/185) (2026-01-20) +* Remove `aproxy` snap installation in the charm and inject proxy values from the model config into the workload process. + ## [#172 feat: apt upgrade](https://github.com/canonical/github-runner-image-builder-operator/pull/172) (2025-11-26) * Apply apt-update and apt-upgrade to GH runner images by applying them during cloud-init. diff --git a/src/builder.py b/src/builder.py index 83813f18..3e1ee2fc 100644 --- a/src/builder.py +++ b/src/builder.py @@ -52,7 +52,7 @@ OPENSTACK_CLOUDS_YAML_PATH = UBUNTU_HOME / "clouds.yaml" # Bandit thinks this is a hardcoded secret -IMAGE_BUILDER_SECRET_PREFIX = "IMAGE_BUILDER_SECRET_" # nosec: B105 +IMAGE_BUILDER_SECRET_PREFIX = "IMAGE_BUILDER_SECRET_" # nosec: hardcoded_password_string @dataclasses.dataclass @@ -348,10 +348,10 @@ class ExternalServiceConfig: """Builder run external service dependencies. Attributes: - proxy: The proxy to use to build the image. + proxy: The proxy configuration to use to build the image. """ - proxy: str | None + proxy: state.ProxyConfig | None @dataclasses.dataclass @@ -515,7 +515,11 @@ def _run(config: RunConfig) -> list[CloudImage]: script_secrets=config.image.script_config.script_secrets, ), service_options=_ServiceOptions( - proxy=config.external_service.proxy, + proxy=( + config.external_service.proxy.http or config.external_service.proxy.https + if config.external_service.proxy + else None + ), ), ) logger.info("Run build command: %s", run_command) @@ -528,6 +532,7 @@ def _run(config: RunConfig) -> list[CloudImage]: env={ "HOME": str(UBUNTU_HOME), **_transform_secrets(secrets=config.image.script_config.script_secrets), + **_get_proxy_env(proxy=config.external_service.proxy), }, ) # The return value of the CLI is "Image build success:\n" @@ -571,6 +576,42 @@ def _transform_secrets(secrets: dict[str, str] | None) -> dict[str, str]: ) +def _get_proxy_env(proxy: state.ProxyConfig | None) -> dict[str, str]: + """Transform proxy config to standard environment variables. + + Args: + proxy: The proxy configuration. + + Returns: + Dictionary of proxy environment variables. + """ + if not proxy: + return {} + env_vars = {} + if proxy.http: + env_vars.update( + { + "http_proxy": proxy.http, + "HTTP_PROXY": proxy.http, + } + ) + if proxy.https: + env_vars.update( + { + "https_proxy": proxy.https, + "HTTPS_PROXY": proxy.https, + } + ) + if proxy.no_proxy: + env_vars.update( + { + "no_proxy": proxy.no_proxy, + "NO_PROXY": proxy.no_proxy, + } + ) + return env_vars + + @dataclasses.dataclass class _RunArgs: """Builder application run arguments. diff --git a/src/charm.py b/src/charm.py index 51c22d9d..ab637af3 100755 --- a/src/charm.py +++ b/src/charm.py @@ -4,6 +4,7 @@ # See LICENSE file for licensing details. """Entrypoint for GithubRunnerImageBuilder charm.""" + import json import logging @@ -21,7 +22,6 @@ import builder import charm_utils import image -import proxy import state LOG_FILE_DIR = Path("/var/log/github-runner-image-builder") @@ -104,7 +104,6 @@ def _on_config_changed(self, _: ops.ConfigChangedEvent) -> None: if not self._is_any_image_relation_ready(cloud_config=builder_config_state.cloud_config): return # The following lines should be covered by integration tests. - proxy.configure_aproxy(proxy=state.ProxyConfig.from_env()) # pragma: no cover builder.install_clouds_yaml( # pragma: no cover cloud_config=builder_config_state.cloud_config.openstack_clouds_config ) @@ -131,7 +130,6 @@ def _on_image_relation_changed(self, evt: ops.RelationChangedEvent) -> None: evt.unit.name, ) return - proxy.configure_aproxy(proxy=state.ProxyConfig.from_env()) builder.install_clouds_yaml( cloud_config=builder_config_state.cloud_config.openstack_clouds_config ) @@ -178,7 +176,6 @@ def _on_run_action(self, event: ops.ActionEvent) -> None: def _setup_builder(self) -> None: """Set up the builder application.""" - proxy.setup(proxy=state.ProxyConfig.from_env()) builder_config_state = state.BuilderConfig.from_charm(charm=self) builder.initialize( app_init_config=builder.ApplicationInitializationConfig( @@ -193,8 +190,7 @@ def _setup_builder(self) -> None: def _setup_logrotate(self) -> None: """Set up the log rotation for image-builder application.""" APP_LOGROTATE_CONFIG_PATH.write_text( - dedent( - f"""\ + dedent(f"""\ {str(LOG_FILE_PATH.absolute())} {{ weekly rotate 3 @@ -202,8 +198,7 @@ def _setup_logrotate(self) -> None: delaycompress missingok }} - """ - ), + """), encoding="utf-8", ) try: @@ -322,7 +317,7 @@ def _get_static_config(self, builder_config: state.BuilderConfig) -> builder.Sta runner_version=builder_config.image_config.runner_version, ), service_config=builder.ExternalServiceConfig( - proxy=(builder_config.proxy.http if builder_config.proxy else None), + proxy=builder_config.proxy, ), ) diff --git a/src/exceptions.py b/src/exceptions.py index fc0db5b2..37afbc17 100644 --- a/src/exceptions.py +++ b/src/exceptions.py @@ -24,10 +24,6 @@ class BuilderRunError(BuilderBaseError): """Represents an error while running the image builder.""" -class ProxyInstallError(BuilderBaseError): - """Represents an error while installing proxy.""" - - class GetLatestImageError(BuilderBaseError): """Represents an error while fetching the latest image.""" diff --git a/src/pipx.py b/src/pipx.py index abc9f49e..9eef91d4 100644 --- a/src/pipx.py +++ b/src/pipx.py @@ -4,7 +4,6 @@ """Module handling interactions with pipx.""" - # Code is abstracting process interactions and is currently tested in integration tests. import logging diff --git a/src/proxy.py b/src/proxy.py deleted file mode 100644 index de8b03d5..00000000 --- a/src/proxy.py +++ /dev/null @@ -1,83 +0,0 @@ -# Copyright 2025 Canonical Ltd. -# See LICENSE file for licensing details. - -"""Module for interacting with proxy.""" - -# Ignore B404:blacklist since all subprocesses are run with predefined executables. -import subprocess # nosec - -from exceptions import ProxyInstallError -from state import ProxyConfig - -UBUNTU_USER = "ubuntu" - - -def setup(proxy: ProxyConfig | None) -> None: - """Install and configure aproxy. - - Args: - proxy: The charm proxy configuration. - - Raises: - ProxyInstallError: If there was an error setting up proxy. - """ - if not proxy: - return - try: - subprocess.run( # nosec: B603 - ["/usr/bin/sudo", "snap", "install", "aproxy", "--channel=latest/edge"], - timeout=5 * 60, - check=True, - user=UBUNTU_USER, - ) - configure_aproxy(proxy=proxy) - except subprocess.SubprocessError as exc: - raise ProxyInstallError from exc - - -def configure_aproxy(proxy: ProxyConfig | None) -> None: - """Configure aproxy. - - Args: - proxy: The charm proxy configuration. - - Raises: - ProxyInstallError: If there was an error configuring aproxy. - """ - if not proxy: - return - proxy_str = (proxy.http or proxy.https).replace("http://", "").replace("https://", "") - try: - subprocess.run( # nosec: B603 - ["/usr/bin/sudo", "snap", "set", "aproxy", f"proxy={proxy_str}"], - timeout=5 * 60, - check=True, - user=UBUNTU_USER, - ) - # Ignore shell=True rule since it is safe - subprocess.run( # nosec: B602, B603 - """/usr/bin/sudo nft -f - << EOF -define default-ip = $(ip route get $(ip route show 0.0.0.0/0 \ -| grep -oP 'via \\K\\S+') | grep -oP 'src \\K\\S+') -define private-ips = { 10.0.0.0/8, 127.0.0.1/8, 172.16.0.0/12, 192.168.0.0/16 } -table ip aproxy -flush table ip aproxy -table ip aproxy { - chain prerouting { - type nat hook prerouting priority dstnat; policy accept; - ip daddr != \\$private-ips tcp dport { 80, 443 } counter dnat to \\$default-ip:8443 - } - - chain output { - type nat hook output priority -100; policy accept; - ip daddr != \\$private-ips tcp dport { 80, 443 } counter dnat to \\$default-ip:8443 - } -} -EOF""", - timeout=5 * 60, - check=True, - shell=True, - user=UBUNTU_USER, - ) - except subprocess.SubprocessError as exc: - raise ProxyInstallError("Error insttalling ") from exc diff --git a/src/state.py b/src/state.py index ef9a656f..b2cc2aeb 100644 --- a/src/state.py +++ b/src/state.py @@ -30,7 +30,7 @@ EXTERNAL_BUILD_NETWORK_CONFIG_NAME = "build-network" OPENSTACK_AUTH_URL_CONFIG_NAME = "openstack-auth-url" # Bandit thinks this is a hardcoded password -OPENSTACK_PASSWORD_CONFIG_NAME = "openstack-password" # nosec: B105 +OPENSTACK_PASSWORD_CONFIG_NAME = "openstack-password" # nosec: hardcoded_password_string OPENSTACK_PROJECT_DOMAIN_CONFIG_NAME = "openstack-project-domain-name" OPENSTACK_PROJECT_CONFIG_NAME = "openstack-project-name" OPENSTACK_USER_DOMAIN_CONFIG_NAME = "openstack-user-domain-name" @@ -39,8 +39,8 @@ RUNNER_VERSION_CONFIG_NAME = "runner-version" SCRIPT_URL_CONFIG_NAME = "script-url" # Bandit thinks this is a hardcoded password -SCRIPT_SECRET_ID_CONFIG_NAME = "script-secret-id" # nosec: B105 -SCRIPT_SECRET_CONFIG_NAME = "script-secret" # nosec: B105 +SCRIPT_SECRET_ID_CONFIG_NAME = "script-secret-id" # nosec: hardcoded_password_string +SCRIPT_SECRET_CONFIG_NAME = "script-secret" # nosec: hardcoded_password_string IMAGE_RELATION = "image" diff --git a/tests/conftest.py b/tests/conftest.py index 3e7eccbc..96f5e0d2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,7 +3,6 @@ """Fixtures for github runner charm.""" - from pytest import Parser diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 2159c58d..96bc13c9 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -2,6 +2,7 @@ # See LICENSE file for licensing details. """Fixtures for github runner charm integration tests.""" + import functools import json import logging diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 55c42067..32665248 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -4,6 +4,7 @@ # See LICENSE file for licensing details. """Integration testing module.""" + import json import logging from contextlib import asynccontextmanager @@ -36,6 +37,7 @@ async def test_image_relation(app: Application, test_charm: Application): @pytest.mark.asyncio +@pytest.mark.abort_on_fail async def test_cos_agent_relation(app: Application): """ arrange: An active charm. @@ -54,6 +56,7 @@ async def test_cos_agent_relation(app: Application): @pytest.mark.asyncio +@pytest.mark.abort_on_fail async def test_build_image( openstack_connection: Connection, dispatch_time: datetime, @@ -69,6 +72,7 @@ async def test_build_image( # Ignore the "too many arguments" warning, as this is not significant for a test function where # the arguments are fixtures and the function is not expected to be called directly. +@pytest.mark.abort_on_fail async def test_charm_another_app_does_not_rebuild_image( # pylint: disable=R0913,R0917 app: Application, test_charm: Application, @@ -127,6 +131,7 @@ async def test_charm_another_app_does_not_rebuild_image( # pylint: disable=R091 @pytest.mark.asyncio +@pytest.mark.abort_on_fail async def test_periodic_rebuilt( app: Application, app_config: dict, @@ -178,6 +183,7 @@ async def _change_cronjob_to_minutes(unit: Unit, current_hour_interval: int): @pytest.mark.asyncio +@pytest.mark.abort_on_fail async def test_log_rotated(app: Application): """ arrange: A deployed active charm and manually write something to the log file. diff --git a/tests/unit/factories.py b/tests/unit/factories.py index d053f22c..9486e08f 100644 --- a/tests/unit/factories.py +++ b/tests/unit/factories.py @@ -3,6 +3,7 @@ """Factories for generating test data.""" +import secrets import typing from unittest.mock import MagicMock @@ -103,7 +104,7 @@ class CloudAuthFactory(factory.DictFactory): auth_url = "http://testing-auth/keystone" # We need to use known password for unit testing - password = "test-password" # nosec: B105:hardcoded_password_string + password = factory.LazyFunction(lambda: secrets.token_hex(16)) project_domain_name = "test-project-domain" project_name = "test-project-name" user_domain_name = "test-user-domain" @@ -174,7 +175,9 @@ class Meta: # pylint: disable=too-few-public-methods arch: state.Arch = state.Arch.ARM64 script_url: str | None = "https://test-url.com/script.sh" - script_secrets: dict[str, str] | None = {"test_secret": "test_value"} + script_secrets: dict[str, str] | None = factory.LazyFunction( + lambda: {"test_secret": secrets.token_hex(16)} + ) runner_version: str | None = "1.2.3" @@ -186,7 +189,11 @@ class Meta: # pylint: disable=too-few-public-methods model = builder.ExternalServiceConfig - proxy: str | None = "http://proxy.internal:3128" + proxy: state.ProxyConfig | None = state.ProxyConfig( + http="http://proxy.internal:3128", + https="http://proxy.internal:3128", + no_proxy="localhost,127.0.0.1", + ) class StaticConfigFactory(factory.Factory): @@ -211,7 +218,9 @@ class Meta: # pylint: disable=too-few-public-methods model = builder.ScriptConfig script_url: str | None = "https://test-url.com/script.sh" - script_secrets: dict[str, str] | None = {"test_secret": "test_value"} + script_secrets: dict[str, str] | None = factory.LazyFunction( + lambda: {"test_secret": secrets.token_hex(16)} + ) class ImageConfigFactory(factory.Factory): diff --git a/tests/unit/test_builder.py b/tests/unit/test_builder.py index aa11513f..229c0bcb 100644 --- a/tests/unit/test_builder.py +++ b/tests/unit/test_builder.py @@ -255,9 +255,7 @@ def test_install_clouds_yaml_not_exists(monkeypatch: pytest.MonkeyPatch, tmp_pat ) contents = test_path.read_text(encoding="utf-8") - assert ( - contents - == f"""clouds: + assert contents == f"""clouds: test: auth: auth_url: test-url @@ -267,7 +265,6 @@ def test_install_clouds_yaml_not_exists(monkeypatch: pytest.MonkeyPatch, tmp_pat user_domain_name: test_domain username: test-user """ - ) def test_install_clouds_yaml_unchanged(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): @@ -300,9 +297,7 @@ def test_install_clouds_yaml_unchanged(monkeypatch: pytest.MonkeyPatch, tmp_path builder.install_clouds_yaml(cloud_config=test_config) contents = test_path.read_text(encoding="utf-8") - assert ( - contents - == f"""clouds: + assert contents == f"""clouds: test: auth: auth_url: test-url @@ -312,7 +307,6 @@ def test_install_clouds_yaml_unchanged(monkeypatch: pytest.MonkeyPatch, tmp_path user_domain_name: test_domain username: test-user """ - ) @pytest.mark.parametrize( @@ -514,7 +508,7 @@ def test_run(monkeypatch: pytest.MonkeyPatch): prefix=TEST_STATIC_CONFIG.cloud_config.resource_prefix, script_config=builder.ScriptConfig( script_url="https://test-url.com/script.sh", - script_secrets={"test_secret": "test_value"}, + script_secrets={"test_secret": secrets.token_hex(16)}, ), runner_version="1.2.3", ), @@ -537,7 +531,9 @@ def test_run(monkeypatch: pytest.MonkeyPatch): prefix=TEST_STATIC_CONFIG.cloud_config.resource_prefix, script_config=builder.ScriptConfig( script_url="https://test-url.com/script.sh", - script_secrets={"test_secret": "test_value"}, + script_secrets={ + "test_secret": secrets.token_hex(16), + }, ), runner_version="1.2.3", ), @@ -560,7 +556,7 @@ def test_run(monkeypatch: pytest.MonkeyPatch): prefix=TEST_STATIC_CONFIG.cloud_config.resource_prefix, script_config=builder.ScriptConfig( script_url="https://test-url.com/script.sh", - script_secrets={"test_secret": "test_value"}, + script_secrets={"test_secret": secrets.token_hex(16)}, ), runner_version="1.2.3", ), @@ -1116,3 +1112,62 @@ def test__get_latest_image(monkeypatch: pytest.MonkeyPatch): cloud_id="test-cloud", image_id="test-image", ) + + +@pytest.mark.parametrize( + "proxy_config, expected_env", + [ + pytest.param( + None, + {}, + id="no proxy", + ), + pytest.param( + state.ProxyConfig(http="http://proxy:8080", https="", no_proxy=""), + { + "http_proxy": "http://proxy:8080", + "HTTP_PROXY": "http://proxy:8080", + }, + id="http proxy only", + ), + pytest.param( + state.ProxyConfig(http="", https="https://proxy:8080", no_proxy=""), + { + "https_proxy": "https://proxy:8080", + "HTTPS_PROXY": "https://proxy:8080", + }, + id="https proxy only", + ), + pytest.param( + state.ProxyConfig(http="", https="", no_proxy="localhost,127.0.0.1"), + { + "no_proxy": "localhost,127.0.0.1", + "NO_PROXY": "localhost,127.0.0.1", + }, + id="no_proxy only", + ), + pytest.param( + state.ProxyConfig( + http="http://proxy:8080", + https="https://proxy:8080", + no_proxy="localhost,127.0.0.1", + ), + { + "http_proxy": "http://proxy:8080", + "HTTP_PROXY": "http://proxy:8080", + "https_proxy": "https://proxy:8080", + "HTTPS_PROXY": "https://proxy:8080", + "no_proxy": "localhost,127.0.0.1", + "NO_PROXY": "localhost,127.0.0.1", + }, + id="all proxy settings", + ), + ], +) +def test__get_proxy_env(proxy_config: state.ProxyConfig | None, expected_env: dict[str, str]): + """ + arrange: given different proxy configurations. + act: when _get_proxy_env is called. + assert: expected environment variables are returned. + """ + assert builder._get_proxy_env(proxy=proxy_config) == expected_env diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index eb4eb18d..52c8c88d 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -2,6 +2,7 @@ # See LICENSE file for licensing details. """Unit tests for charm module.""" + import secrets # We're monkeypatching the subprocess module for testing @@ -15,7 +16,6 @@ import builder import charm as charm_module import image -import proxy import state from app.src.github_runner_image_builder.logging import LOG_FILE_PATH from charm import GithubRunnerImageBuilderCharm @@ -81,7 +81,6 @@ def test_hooks_that_trigger_run_for_all_clouds( act: when the hook is called. assert: the charm falls into ActiveStatus """ - monkeypatch.setattr(proxy, "configure_aproxy", MagicMock()) getattr(charm, hook)(MagicMock()) @@ -137,7 +136,6 @@ def test_installation( """ monkeypatch.setattr(state.BuilderConfig, "from_charm", MagicMock()) monkeypatch.setattr(image, "Observer", MagicMock()) - monkeypatch.setattr(proxy, "setup", MagicMock()) monkeypatch.setattr(builder, "initialize", (builder_setup_mock := MagicMock())) charm._setup_logrotate = (logrotate_setup_mock := MagicMock()) @@ -157,7 +155,6 @@ def test__on_image_relation_changed( act: when _on_image_relation_changed is called. assert: charm is in active status and run for the particular related unit is called. """ - monkeypatch.setattr(proxy, "configure_aproxy", MagicMock()) charm.image_observer = MagicMock() fake_clouds_auth_config = state.CloudsAuthConfig( auth_url="http://example.com", @@ -192,7 +189,6 @@ def test__on_image_relation_changed_image_already_in_cloud( act: when _on_image_relation_changed is called. assert: charm is in active status and no run is triggered but image data is updated """ - monkeypatch.setattr(proxy, "configure_aproxy", MagicMock()) charm.image_observer = MagicMock() fake_clouds_auth_config = state.CloudsAuthConfig( auth_url="http://example.com", @@ -235,7 +231,6 @@ def test__on_image_relation_changed_no_unit_auth_data( act: when _on_image_relation_changed is called. assert: charm is not building image """ - monkeypatch.setattr(proxy, "configure_aproxy", MagicMock()) charm.image_observer = MagicMock() monkeypatch.setattr( @@ -251,7 +246,7 @@ def test__on_image_relation_changed_no_unit_auth_data( evt.relation.data[evt.unit] = { "auth_url": "http://example.com", "username": "user", - "password": "pass", + "password": secrets.token_hex(16), "project_name": "project_name", "project_domain_name": "project_domain_name", "user_domain_name": "user_domain_name", diff --git a/tests/unit/test_image.py b/tests/unit/test_image.py index e1ff5c3a..b072da07 100644 --- a/tests/unit/test_image.py +++ b/tests/unit/test_image.py @@ -6,6 +6,7 @@ # Need access to protected functions for testing # pylint:disable=protected-access +import secrets from unittest.mock import MagicMock import pytest @@ -140,7 +141,7 @@ def test_update_image_data(harness: Harness, image_observer: image.Observer): app_or_unit="github-runner/0", key_values={ "auth_url": "test", - "password": "test", + "password": secrets.token_hex(16), "project_domain_name": "test", "project_name": "test", "user_domain_name": "test", @@ -153,7 +154,7 @@ def test_update_image_data(harness: Harness, image_observer: image.Observer): app_or_unit="github-runner/1", key_values={ "auth_url": "test", - "password": "test", + "password": secrets.token_hex(16), "project_domain_name": "test", "project_name": "test", "user_domain_name": "test", @@ -172,7 +173,7 @@ def test_update_image_data(harness: Harness, image_observer: image.Observer): app_or_unit="github-runner-two/0", key_values={ "auth_url": "test", - "password": "test", + "password": secrets.token_hex(16), "project_domain_name": "test", "project_name": "test", "user_domain_name": "test", @@ -187,7 +188,7 @@ def test_update_image_data(harness: Harness, image_observer: image.Observer): app_or_unit="github-runner-two/1", key_values={ "auth_url": "test", - "password": "test", + "password": secrets.token_hex(16), "project_domain_name": "test", "project_name": "test", "user_domain_name": "test", diff --git a/tests/unit/test_proxy.py b/tests/unit/test_proxy.py deleted file mode 100644 index 6a32fa3c..00000000 --- a/tests/unit/test_proxy.py +++ /dev/null @@ -1,166 +0,0 @@ -# Copyright 2025 Canonical Ltd. -# See LICENSE file for licensing details. - -"""Unit tests for proxy module.""" - -from unittest.mock import MagicMock, _Call, call - -import pytest - -import proxy -from proxy import ProxyConfig, ProxyInstallError, subprocess - - -def test_setup_error(monkeypatch: pytest.MonkeyPatch): - """ - arrange: given a monkeypatched subprocess.run that raises an error. - act: when proxy.setup is called. - assert: ProxyInstallError is raised. - """ - monkeypatch.setattr( - subprocess, - "run", - MagicMock(side_effect=subprocess.CalledProcessError(1, "", "", "Setup error")), - ) - - with pytest.raises(ProxyInstallError) as exc: - proxy.setup(MagicMock()) - - assert "Setup error" in str(exc.getrepr()) - - -def test_setup_no_proxy(monkeypatch: pytest.MonkeyPatch): - """ - arrange: given no proxy configuration. - act: when proxy.setup is called. - assert: no setup processes are called. - """ - monkeypatch.setattr(subprocess, "run", run_mock := MagicMock()) - - proxy.setup(None) - - run_mock.assert_not_called() - - -def test_setup(monkeypatch: pytest.MonkeyPatch): - """ - arrange: given a charm proxy configuration. - act: when setup is called. - assert: aproxy install command and nft table configuration command is called. - """ - monkeypatch.setattr(subprocess, "run", run_mock := MagicMock()) - monkeypatch.setattr(proxy, "configure_aproxy", configure_aproxy_mock := MagicMock()) - - proxy.setup(MagicMock()) - - configure_aproxy_mock.assert_called_once() - run_mock.assert_has_calls( - [ - call( - ["/usr/bin/sudo", "snap", "install", "aproxy", "--channel=latest/edge"], - timeout=300, - check=True, - user="ubuntu", - ) - ] - ) - - -def test_configure_aproxy_error(monkeypatch: pytest.MonkeyPatch): - """ - arrange: given monkeypatched subprocess run mocking aproxy setup. - act: when configure_aproxy is run. - assert: ProxyInstallError is raised. - """ - monkeypatch.setattr( - subprocess, - "run", - MagicMock(side_effect=subprocess.CalledProcessError(1, "", "", "Invalid proxy")), - ) - - with pytest.raises(ProxyInstallError) as exc: - proxy.configure_aproxy(MagicMock()) - - assert "Invalid proxy" in str(exc.getrepr()) - - -def test_configure_aproxy_none(monkeypatch: pytest.MonkeyPatch): - """ - arrange: given no proxy configuration. - act: when configure_aproxy is run. - assert: no setup calls are made. - """ - monkeypatch.setattr(subprocess, "run", run_mock := MagicMock()) - - proxy.configure_aproxy(None) - - run_mock.assert_not_called() - - -@pytest.mark.parametrize( - "proxy_config, expected_call", - [ - pytest.param( - ProxyConfig(http="http://proxy.internal:3128", https="", no_proxy=""), - call( - ["/usr/bin/sudo", "snap", "set", "aproxy", "proxy=proxy.internal:3128"], - timeout=300, - check=True, - user="ubuntu", - ), - id="http proxy", - ), - pytest.param( - ProxyConfig(http="", https="https://proxy.internal:3128", no_proxy=""), - call( - ["/usr/bin/sudo", "snap", "set", "aproxy", "proxy=proxy.internal:3128"], - timeout=300, - check=True, - user="ubuntu", - ), - id="https proxy", - ), - ], -) -def test_configure_aproxy( - monkeypatch: pytest.MonkeyPatch, proxy_config: ProxyConfig, expected_call: _Call -): - """ - arrange: given no proxy configuration. - act: when configure_aproxy is run. - assert: no setup calls are made. - """ - monkeypatch.setattr(subprocess, "run", run_mock := MagicMock()) - - proxy.configure_aproxy(proxy_config) - - run_mock.assert_has_calls( - [ - expected_call, - call( - """/usr/bin/sudo nft -f - << EOF -define default-ip = $(ip route get $(ip route show 0.0.0.0/0 \ -| grep -oP 'via \\K\\S+') | grep -oP 'src \\K\\S+') -define private-ips = { 10.0.0.0/8, 127.0.0.1/8, 172.16.0.0/12, 192.168.0.0/16 } -table ip aproxy -flush table ip aproxy -table ip aproxy { - chain prerouting { - type nat hook prerouting priority dstnat; policy accept; - ip daddr != \\$private-ips tcp dport { 80, 443 } counter dnat to \\$default-ip:8443 - } - - chain output { - type nat hook output priority -100; policy accept; - ip daddr != \\$private-ips tcp dport { 80, 443 } counter dnat to \\$default-ip:8443 - } -} -EOF""", - timeout=300, - check=True, - # Bandit thinks this is a real subprocess call. - shell=True, # nosec - user="ubuntu", - ), - ] - ) diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py index e451ace4..ea8f388d 100644 --- a/tests/unit/test_state.py +++ b/tests/unit/test_state.py @@ -160,6 +160,17 @@ def test_proxy_config(monkeypatch: pytest.MonkeyPatch): ) +def test_proxy_config_empty(monkeypatch: pytest.MonkeyPatch): + """ + arrange: given monkeypatched os.environ with empty value. + act: when ProxyConfig.from_env is called. + assert: None is returned. + """ + monkeypatch.setattr(os, "getenv", MagicMock(return_value="")) + + assert state.ProxyConfig.from_env() is None + + @pytest.mark.parametrize( "flavor, network, expected_config", [