diff --git a/docs/changelog.md b/docs/changelog.md index 8d00d65c..1c87110d 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. + ## [#195 feat: otel-collector snap](https://github.com/canonical/github-runner-image-builder-operator/pull/195) (2026-02-11) * Install opentelemetry collector snap in the runner image. diff --git a/src/builder.py b/src/builder.py index 83813f18..8aa51d91 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 @@ -188,6 +188,7 @@ def _build_init_command( """ cmd = [ "/usr/bin/sudo", + "--preserve-env=http_proxy,https_proxy,no_proxy,HTTP_PROXY,HTTPS_PROXY,NO_PROXY", str(GITHUB_RUNNER_IMAGE_BUILDER_PATH), "--os-cloud", cloud_name, @@ -528,6 +529,7 @@ def _run(config: RunConfig) -> list[CloudImage]: env={ "HOME": str(UBUNTU_HOME), **_transform_secrets(secrets=config.image.script_config.script_secrets), + **{k: v for (k, v) in os.environ.items() if "proxy" in k.lower()}, }, ) # The return value of the CLI is "Image build success:\n" diff --git a/src/charm.py b/src/charm.py index 035f6e93..cdecae48 100755 --- a/src/charm.py +++ b/src/charm.py @@ -7,6 +7,7 @@ import json import logging +import os # We ignore low severity security warning for importing subprocess module import subprocess # nosec B404 @@ -22,7 +23,6 @@ import builder import charm_utils import image -import proxy import state LOG_FILE_DIR = Path("/var/log/github-runner-image-builder") @@ -102,10 +102,10 @@ def _on_upgrade_charm(self, _: ops.UpgradeCharmEvent) -> None: def _on_config_changed(self, _: ops.ConfigChangedEvent) -> None: """Handle charm configuration change events.""" builder_config_state = state.BuilderConfig.from_charm(charm=self) + self._setup_proxy_environment(builder_config_state.proxy) 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 ) @@ -119,6 +119,7 @@ def _on_config_changed(self, _: ops.ConfigChangedEvent) -> None: def _on_image_relation_changed(self, evt: ops.RelationChangedEvent) -> None: """Handle charm image relation changed event.""" builder_config_state = state.BuilderConfig.from_charm(charm=self) + self._setup_proxy_environment(builder_config_state.proxy) if not evt.unit: logger.info("No unit in image relation changed event. Skipping image building.") return @@ -132,7 +133,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 ) @@ -158,6 +158,7 @@ def _on_image_relation_changed(self, evt: ops.RelationChangedEvent) -> None: def _on_run(self, _: RunEvent) -> None: """Handle the run event.""" builder_config_state = state.BuilderConfig.from_charm(charm=self) + self._setup_proxy_environment(builder_config_state.proxy) if not self._is_any_image_relation_ready(cloud_config=builder_config_state.cloud_config): return # The following line should be covered by the integration test. @@ -171,16 +172,33 @@ def _on_run_action(self, event: ops.ActionEvent) -> None: event: The run action event. """ builder_config_state = state.BuilderConfig.from_charm(charm=self) + self._setup_proxy_environment(builder_config_state.proxy) if not self._is_any_image_relation_ready(cloud_config=builder_config_state.cloud_config): event.fail("Image relation not yet ready.") return # The following line should be covered by the integration test. self._run() # pragma: nocover + def _setup_proxy_environment(self, proxy_config: state.ProxyConfig | None) -> None: + """Set up proxy environment variables. + + Args: + proxy_config: The proxy configuration to apply to environment variables. + """ + if proxy_config: + os.environ["http_proxy"] = proxy_config.http + os.environ["https_proxy"] = proxy_config.https + os.environ["no_proxy"] = proxy_config.no_proxy + os.environ["HTTP_PROXY"] = proxy_config.http + os.environ["HTTPS_PROXY"] = proxy_config.https + os.environ["NO_PROXY"] = proxy_config.no_proxy + 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) + + self._setup_proxy_environment(builder_config_state.proxy) + builder.initialize( app_init_config=builder.ApplicationInitializationConfig( cloud_config=builder_config_state.cloud_config, 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/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/integration/test_charm.py b/tests/integration/test_charm.py index 7a7236a9..32665248 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -37,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. @@ -55,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, @@ -70,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, @@ -128,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, @@ -179,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 7c1637d6..153480a0 100644 --- a/tests/unit/factories.py +++ b/tests/unit/factories.py @@ -104,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 = "test-password" # nosec: hardcoded_password_string project_domain_name = "test-project-domain" project_name = "test-project-name" user_domain_name = "test-user-domain" diff --git a/tests/unit/test_builder.py b/tests/unit/test_builder.py index 1370a474..57e16e15 100644 --- a/tests/unit/test_builder.py +++ b/tests/unit/test_builder.py @@ -186,6 +186,7 @@ def test__initialize_image_builder_error( None, [ "/usr/bin/sudo", + "--preserve-env=http_proxy,https_proxy,no_proxy,HTTP_PROXY,HTTPS_PROXY,NO_PROXY", "/home/ubuntu/.local/bin/github-runner-image-builder", "--os-cloud", "test-cloud-name", @@ -199,6 +200,7 @@ def test__initialize_image_builder_error( "test-prefix", [ "/usr/bin/sudo", + "--preserve-env=http_proxy,https_proxy,no_proxy,HTTP_PROXY,HTTPS_PROXY,NO_PROXY", "/home/ubuntu/.local/bin/github-runner-image-builder", "--os-cloud", "test-cloud-name", diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index ca34280e..67117818 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -3,6 +3,7 @@ """Unit tests for charm module.""" +import os import secrets # We're monkeypatching the subprocess module for testing @@ -16,7 +17,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 @@ -38,7 +38,7 @@ def mock_builder_fixture(monkeypatch: pytest.MonkeyPatch): monkeypatch.setattr( state.BuilderConfig, "from_charm", - MagicMock(return_value=MagicMock(image_config=image_config)), + MagicMock(return_value=MagicMock(image_config=image_config, proxy=None)), ) monkeypatch.setattr(builder, "install_clouds_yaml", MagicMock()) monkeypatch.setattr(builder, "get_latest_images", MagicMock(return_value=[])) @@ -82,7 +82,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()) @@ -136,9 +135,10 @@ def test_installation( act: when _on_install is called. assert: setup_builder is called. """ - monkeypatch.setattr(state.BuilderConfig, "from_charm", MagicMock()) + monkeypatch.setattr( + state.BuilderConfig, "from_charm", MagicMock(return_value=MagicMock(proxy=None)) + ) 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()) @@ -158,7 +158,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", @@ -193,7 +192,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", @@ -236,7 +234,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( @@ -309,3 +306,30 @@ def test__setup_logrotate(monkeypatch, tmp_path, charm: GithubRunnerImageBuilder mock_check_call.assert_called_once_with( ["/usr/sbin/logrotate", str(logrotate_path), "--debug"] ) + + +def test_setup_proxy_environment_with_proxy_config( + monkeypatch: pytest.MonkeyPatch, charm: GithubRunnerImageBuilderCharm +): + """ + arrange: given a ProxyConfig with http, https, and no_proxy values. + act: when _setup_proxy_environment is called. + assert: environment variables are set correctly. + """ + for key in ["http_proxy", "https_proxy", "no_proxy", "HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY"]: + monkeypatch.delenv(key, raising=False) + + proxy_config = state.ProxyConfig( + http="http://proxy.example.com:8080", + https="https://proxy.example.com:8443", + no_proxy="localhost,127.0.0.1", + ) + + charm._setup_proxy_environment(proxy_config) + + assert os.environ["http_proxy"] == "http://proxy.example.com:8080" + assert os.environ["https_proxy"] == "https://proxy.example.com:8443" + assert os.environ["no_proxy"] == "localhost,127.0.0.1" + assert os.environ["HTTP_PROXY"] == "http://proxy.example.com:8080" + assert os.environ["HTTPS_PROXY"] == "https://proxy.example.com:8443" + assert os.environ["NO_PROXY"] == "localhost,127.0.0.1" 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", - ), - ] - )