From 4bebd7a7f01765914d9d39907341faa85623f87d Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Thu, 19 Jun 2025 22:41:56 +0800 Subject: [PATCH 01/18] Forward arbitrary TCP traffic using aproxy --- .github/workflows/e2e_test_run.yaml | 16 +++- config.yaml | 20 +++- .../configuration/base.py | 5 + .../openstack_runner_manager.py | 23 ++++- .../templates/openstack-userdata.sh.j2 | 17 ++-- .../test_openstack_runner_manager.py | 93 ++++++++++++++++++- src/charm_state.py | 8 ++ src/factories.py | 2 + tests/integration/conftest.py | 2 + tests/unit/test_charm_state.py | 44 ++++++++- 10 files changed, 206 insertions(+), 24 deletions(-) diff --git a/.github/workflows/e2e_test_run.yaml b/.github/workflows/e2e_test_run.yaml index bebbc1a2fc..505b08687d 100644 --- a/.github/workflows/e2e_test_run.yaml +++ b/.github/workflows/e2e_test_run.yaml @@ -22,7 +22,7 @@ on: jobs: e2e-test: name: End-to-End Test Run - runs-on: [self-hosted, linux, "${{ inputs.runner-tag }}"] + runs-on: [ self-hosted, linux, "${{ inputs.runner-tag }}" ] steps: - name: Hostname is set to "github-runner" run: sudo hostnamectl hostname | grep github-runner @@ -87,3 +87,17 @@ jobs: # ~/.local/bin is added to path runner env through in scripts/env.j2 - name: test check-jsonschema run: check-jsonschema --version + - name: test network connectivity + run: | + timeout 60 curl --noproxy "*" http://example.com -svS -o /dev/null + timeout 60 curl --noproxy "*" https://example.com -svS -o /dev/null + [ $(timeout 60 echo hello | nc -q 0 tcpbin.com 4242) == "hello!" ] + - name: test aproxy logs + run: | + sudo snap logs aproxy.aproxy | grep -Fq "example.com:80" + sudo snap logs aproxy.aproxy | grep -Fq "example.com:443" + sudo snap logs aproxy.aproxy | grep -Eq "[0-9.]+:4242" + - name: show aproxy logs + if: failure() + run: | + sudo snap logs aproxy.aproxy -n=all diff --git a/config.yaml b/config.yaml index 30beec90b6..742bd3b70a 100644 --- a/config.yaml +++ b/config.yaml @@ -37,13 +37,25 @@ options: default: false description: >- (Experimental, may be removed) When set to true, aproxy (https://github.com/canonical/aproxy) - will be installed within the runners. It will forward all HTTP(S) traffic to standard ports - (80, 443, 11371) to a proxy server configured by the juju model config 'juju-http-proxy' + will be installed within the runners. It will forward TCP traffic matching the 'aproxy-exclude-addresses' + and 'aproxy-redirect-ports' settings to a proxy server configured by the Juju model config 'juju-http-proxy' (or, if this is not set, 'juju-https-proxy' will be used). This is useful when the charm is deployed in a network that requires a proxy to access the internet. - Note that you should not specify a proxy server listening on port 80 or 443, as all traffic - to these ports is relayed to aproxy, which would cause an infinite loop. + Note that you should carefully choose values for the 'aproxy-exclude-addresses' and + 'aproxy-redirect-ports' so that the network traffic from the runner to the HTTP proxy is not + captured by aproxy. The simplest way to achieve this is to add the IP address of the HTTP proxy + to 'aproxy-exclude-addresses' or exclude the HTTP proxy port from 'aproxy-redirect-ports'. + aproxy-exclude-addresses: + type: string + default: "10.0.0.0/8, 171.16.0.0/12, 192.168.0.0/16" + description: >- + A comma-separated list of IP addresses that should be excluded from redirection to aproxy. + aproxy-redirect-ports: + type: string + default: "80, 443" + description: >- + A comma-separated list of ports or port ranges that should be redirected to aproxy. group: type: string default: "default" diff --git a/github-runner-manager/src/github_runner_manager/configuration/base.py b/github-runner-manager/src/github_runner_manager/configuration/base.py index bf6263677c..09084b0d87 100644 --- a/github-runner-manager/src/github_runner_manager/configuration/base.py +++ b/github-runner-manager/src/github_runner_manager/configuration/base.py @@ -81,6 +81,9 @@ class SupportServiceConfig(BaseModel): proxy_config: The proxy configuration. runner_proxy_config: The proxy configuration for the runner. use_aproxy: Whether aproxy should be used for the runners. + aproxy_exclude_addresses: A comma-separated list of addresses to exclude from the aproxy + proxy. + aproxy_redirect_ports: A comma-separated list of ports to redirect to the aproxy proxy. dockerhub_mirror: The dockerhub mirror to use for runners. ssh_debug_connections: The information on the ssh debug services. repo_policy_compliance: The configuration of the repo policy compliance service. @@ -91,6 +94,8 @@ class SupportServiceConfig(BaseModel): proxy_config: "ProxyConfig | None" runner_proxy_config: "ProxyConfig | None" use_aproxy: bool + aproxy_exclude_addresses: str | None + aproxy_redirect_ports: str | None dockerhub_mirror: str | None ssh_debug_connections: "list[SSHDebugConnection]" repo_policy_compliance: "RepoPolicyComplianceConfig | None" diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index bab7672508..eeed06a6dc 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -282,15 +282,30 @@ def _generate_cloud_init(self, runner_context: RunnerContext) -> str: pre_job_contents = jinja.get_template("pre-job.j2").render(pre_job_contents_dict) - aproxy_address = ( - service_config.runner_proxy_config.proxy_address if service_config.use_aproxy else None - ) + use_aproxy = service_config.use_aproxy + if not service_config.runner_proxy_config.proxy_address: + use_aproxy = False + aproxy_redirect_ports = service_config.aproxy_redirect_ports + if not aproxy_redirect_ports: + use_aproxy = False + aproxy_exclude_addresses = service_config.aproxy_exclude_addresses + aproxy_exclude_ipv4_addresses = [] + if aproxy_exclude_addresses: + for address in aproxy_exclude_addresses.split(","): + address = address.strip() + if not address: + continue + if ":" not in address: + aproxy_exclude_ipv4_addresses.append(address) return jinja.get_template("openstack-userdata.sh.j2").render( run_script=runner_context.shell_run_script, env_contents=env_contents, pre_job_contents=pre_job_contents, metrics_exchange_path=str(METRICS_EXCHANGE_PATH), - aproxy_address=aproxy_address, + use_aproxy=use_aproxy, + aproxy_address=service_config.runner_proxy_config.proxy_address, + aproxy_exclude_ipv4_addresses=", ".join(aproxy_exclude_ipv4_addresses), + aproxy_redirect_ports=aproxy_redirect_ports, dockerhub_mirror=service_config.dockerhub_mirror, ssh_debug_info=ssh_debug_info, runner_proxy_config=service_config.runner_proxy_config, diff --git a/github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 b/github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 index f509b107af..4adbd0c95f 100644 --- a/github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 +++ b/github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 @@ -10,23 +10,28 @@ su - ubuntu -c 'cd ~/actions-runner && echo "{{ env_contents }}" > .env' snap refresh --hold=48h snap watch --last=auto-refresh? -{% if aproxy_address %} +{% if use_aproxy %} snap install aproxy --edge snap set aproxy proxy={{ aproxy_address }} listen=:54969 cat << EOF > /etc/nftables.conf -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 } +define default-ipv4 = $(ip route get $(ip route show 0.0.0.0/0 | grep -oP 'via \K\S+') | grep -oP 'src \K\S+') table ip aproxy flush table ip aproxy table ip aproxy { + set exclude { + type ipv4_addr; + flags interval; auto-merge; +{% if aproxy_exclude_ipv4_addresses %} + elements = { {{ aproxy_exclude_ipv4_addresses }} } +{% endif %} + } chain prerouting { type nat hook prerouting priority dstnat; policy accept; - ip daddr != \$private-ips tcp dport { 80, 443, 11371 } counter dnat to \$default-ip:54969 + ip daddr != @exclude tcp dport { {{ aproxy_redirect_ports }} } counter dnat to \$default-ipv4:54969 } - chain output { type nat hook output priority -100; policy accept; - ip daddr != \$private-ips tcp dport { 80, 443, 11371 } counter dnat to \$default-ip:54969 + ip daddr != @exclude tcp dport { {{ aproxy_redirect_ports }} } counter dnat to \$default-ipv4:54969 } } EOF diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py index bd3f71c2c0..aabfd74e13 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py @@ -3,6 +3,7 @@ """Module for unit-testing OpenStack runner manager.""" import logging +import textwrap from datetime import datetime, timezone from typing import Iterable from unittest.mock import MagicMock @@ -77,8 +78,87 @@ def runner_metrics_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock: return runner_metrics_mock +@pytest.mark.parametrize( + "aproxy_redirect_ports, aproxy_exclude_addresses, aproxy_used, except_aproxy_script", + [ + pytest.param( + "", "10.0.0.0/8", False, "", id="empty aproxy_redirect_ports disables aproxy" + ), + pytest.param( + "80, 443", + "10.0.0.0/8, 192.168.0.0/16", + True, + "10.0.0.0/8, 192.168.0.0/16", + id="aproxy with custom aproxy_exclude_addresses", + ), + pytest.param( + "0-3127, 3129-65535", + "10.0.0.0/8, 192.168.0.0/16", + True, + "0-3127, 3129-65535", + id="aproxy with custom aproxy_redirect_ports", + ), + pytest.param( + "80, 443", + "10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16", + True, + textwrap.dedent( + """\ + table ip aproxy { + set exclude { + type ipv4_addr; + flags interval; auto-merge; + + elements = { 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 } + + } + chain prerouting { + type nat hook prerouting priority dstnat; policy accept; + ip daddr != @exclude tcp dport { 80, 443 } counter dnat to \\$default-ipv4:54969 + } + chain output { + type nat hook output priority -100; policy accept; + ip daddr != @exclude tcp dport { 80, 443 } counter dnat to \\$default-ipv4:54969 + } + } + """ + ), + id="aproxy default config", + ), + pytest.param( + "80, 443", + "", + True, + textwrap.dedent( + """\ + table ip aproxy { + set exclude { + type ipv4_addr; + flags interval; auto-merge; + + } + chain prerouting { + type nat hook prerouting priority dstnat; policy accept; + ip daddr != @exclude tcp dport { 80, 443 } counter dnat to \\$default-ipv4:54969 + } + chain output { + type nat hook output priority -100; policy accept; + ip daddr != @exclude tcp dport { 80, 443 } counter dnat to \\$default-ipv4:54969 + } + } + """ + ), + id="aproxy with no aproxy_exclude_addresses", + ), + ], +) def test_create_runner_with_aproxy( - runner_manager: OpenStackRunnerManager, monkeypatch: pytest.MonkeyPatch + aproxy_redirect_ports: str, + aproxy_exclude_addresses: str, + aproxy_used: str, + except_aproxy_script: str, + runner_manager: OpenStackRunnerManager, + monkeypatch: pytest.MonkeyPatch, ): """ arrange: Prepare service config with aproxy enabled and a runner proxy config. @@ -88,6 +168,8 @@ def test_create_runner_with_aproxy( # Pending to pass service_config as a dependency instead of mocking it this way. service_config = runner_manager._config.service_config service_config.use_aproxy = True + service_config.aproxy_redirect_ports = aproxy_redirect_ports + service_config.aproxy_exclude_addresses = aproxy_exclude_addresses service_config.runner_proxy_config = ProxyConfig(http="http://proxy.example.com:3128") prefix = "test" @@ -101,10 +183,11 @@ def test_create_runner_with_aproxy( runner_manager.create_runner(identity, runner_context) openstack_cloud.launch_instance.assert_called_once() - assert ( - "snap set aproxy proxy=proxy.example.com:3128" - in openstack_cloud.launch_instance.call_args.kwargs["cloud_init"] - ) + + cloud_init = openstack_cloud.launch_instance.call_args.kwargs["cloud_init"] + assert ("snap set aproxy proxy=proxy.example.com:3128" in cloud_init) == aproxy_used + if aproxy_used: + assert except_aproxy_script in cloud_init def test_create_runner_without_aproxy( diff --git a/src/charm_state.py b/src/charm_state.py index 93e04d73b6..09fb0b4a5c 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -59,6 +59,8 @@ # bandit thinks this is a hardcoded password. TOKEN_CONFIG_NAME = "token" # nosec USE_APROXY_CONFIG_NAME = "experimental-use-aproxy" +APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME = "aproxy-exclude-addresses" +APROXY_REDIRECT_PORTS_CONFIG_NAME = "aproxy-redirect-ports" USE_RUNNER_PROXY_FOR_TMATE_CONFIG_NAME = "use-runner-proxy-for-tmate" VIRTUAL_MACHINES_CONFIG_NAME = "virtual-machines" CUSTOM_PRE_JOB_SCRIPT_CONFIG_NAME = "pre-job-script" @@ -340,6 +342,8 @@ class CharmConfig(BaseModel): token: GitHub personal access token for GitHub API. manager_proxy_command: ProxyCommand for the SSH connection from the manager to the runner. use_aproxy: Whether to use aproxy in the runner. + aproxy_exclude_addresses: a comma-separated list of addresses to exclude from the aproxy proxy. + aproxy_redirect_ports: a comma-separated list of ports to redirect to the aproxy proxy. custom_pre_job_script: Custom pre-job script to run before the job. jobmanager_url: Base URL of the job manager service. """ @@ -353,6 +357,8 @@ class CharmConfig(BaseModel): token: str | None manager_proxy_command: str | None use_aproxy: bool + aproxy_exclude_addresses: str | None + aproxy_redirect_ports: str | None custom_pre_job_script: str | None jobmanager_url: AnyHttpUrl | None @@ -514,6 +520,8 @@ def from_charm(cls, charm: CharmBase) -> "CharmConfig": token=github_config.token if github_config else None, manager_proxy_command=manager_proxy_command, use_aproxy=use_aproxy, + aproxy_exclude_addresses=charm.config.get(APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME), + aproxy_redirect_ports=charm.config.get(APROXY_REDIRECT_PORTS_CONFIG_NAME), custom_pre_job_script=custom_pre_job_script, jobmanager_url=jobmanager_config.url if jobmanager_config else None, ) diff --git a/src/factories.py b/src/factories.py index aad9c55781..afa2f6a3b6 100644 --- a/src/factories.py +++ b/src/factories.py @@ -86,6 +86,8 @@ def create_application_configuration( ssh_debug_connections=state.ssh_debug_connections, repo_policy_compliance=state.charm_config.repo_policy_compliance, use_aproxy=state.charm_config.use_aproxy, + aproxy_exclude_addresses=state.charm_config.aproxy_exclude_addresses, + aproxy_redirect_ports=state.charm_config.aproxy_redirect_ports, custom_pre_job_script=state.charm_config.custom_pre_job_script, ) non_reactive_configuration = _get_non_reactive_configuration(state) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 795ed3af14..da33aaddac 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -36,6 +36,7 @@ OPENSTACK_NETWORK_CONFIG_NAME, PATH_CONFIG_NAME, USE_APROXY_CONFIG_NAME, + APROXY_REDIRECT_PORTS_CONFIG_NAME, ) from tests.integration.helpers.common import ( MONGODB_APP_NAME, @@ -461,6 +462,7 @@ async def app_openstack_runner_fixture( OPENSTACK_NETWORK_CONFIG_NAME: network_name, OPENSTACK_FLAVOR_CONFIG_NAME: flavor_name, USE_APROXY_CONFIG_NAME: bool(openstack_http_proxy), + APROXY_REDIRECT_PORTS_CONFIG_NAME: "1-3127,3129-65535", LABELS_CONFIG_NAME: app_name, }, wait_idle=False, diff --git a/tests/unit/test_charm_state.py b/tests/unit/test_charm_state.py index f43ecd77b7..1e8d84f750 100644 --- a/tests/unit/test_charm_state.py +++ b/tests/unit/test_charm_state.py @@ -33,6 +33,8 @@ RUNNER_HTTP_PROXY_CONFIG_NAME, TOKEN_CONFIG_NAME, USE_APROXY_CONFIG_NAME, + APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME, + APROXY_REDIRECT_PORTS_CONFIG_NAME, USE_RUNNER_PROXY_FOR_TMATE_CONFIG_NAME, VIRTUAL_MACHINES_CONFIG_NAME, Arch, @@ -1053,11 +1055,12 @@ def test_charm_state__log_prev_state_redacts_sensitive_information( @pytest.mark.parametrize( - "juju_http, juju_https, juju_no_proxy, runner_http, use_aproxy," - "expected_proxy, expected_runner_proxy", + "juju_http, juju_https, juju_no_proxy, runner_http, use_aproxy, " + "aproxy_exclude_addresses, aproxy_redirect_ports, expected_proxy, " + "expected_runner_proxy", [ pytest.param( - "", "", "", "", False, ProxyConfig(), ProxyConfig(), id="No proxy. No aproxy" + "", "", "", "", False, "", "", ProxyConfig(), ProxyConfig(), id="No proxy. No aproxy" ), pytest.param( "", @@ -1065,6 +1068,8 @@ def test_charm_state__log_prev_state_redacts_sensitive_information( "localhost", "", False, + "", + "", ProxyConfig(), ProxyConfig(), id="No proxy with only no_proxy. No aproxy", @@ -1075,6 +1080,8 @@ def test_charm_state__log_prev_state_redacts_sensitive_information( "", "", False, + "", + "", ProxyConfig(http="http://example.com:3128"), ProxyConfig(http="http://example.com:3128"), id="Only proxy from juju. No aproxy.", @@ -1085,6 +1092,8 @@ def test_charm_state__log_prev_state_redacts_sensitive_information( "", "http://runner.example.com:3128", False, + "", + "", ProxyConfig(http="http://manager.example.com:3128"), ProxyConfig(http="http://runner.example.com:3128"), id="Both juju and runner proxy. No aproxy.", @@ -1095,6 +1104,8 @@ def test_charm_state__log_prev_state_redacts_sensitive_information( "", "http://runner.example.com:3128", True, + "", + "", ProxyConfig(), ProxyConfig(http="http://runner.example.com:3128"), id="Only proxy in runner. aproxy configured.", @@ -1105,6 +1116,8 @@ def test_charm_state__log_prev_state_redacts_sensitive_information( "127.0.0.1", "http://runner.example.com:3128", True, + "", + "", ProxyConfig( http="http://manager.example.com:3128", https="http://securemanager.example.com:3128", @@ -1115,6 +1128,24 @@ def test_charm_state__log_prev_state_redacts_sensitive_information( ), id="Proxy in juju and the runner. aproxy configured.", ), + pytest.param( + "http://manager.example.com:3128", + "http://securemanager.example.com:3128", + "127.0.0.1", + "http://runner.example.com:3128", + True, + "10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16", + "80, 443", + ProxyConfig( + http="http://manager.example.com:3128", + https="http://securemanager.example.com:3128", + no_proxy="127.0.0.1", + ), + ProxyConfig( + http="http://runner.example.com:3128", + ), + id="Proxy in juju and the runner. aproxy configured with exclude addresses and redirect ports.", + ), ], ) def test_proxy_config( @@ -1124,6 +1155,8 @@ def test_proxy_config( juju_no_proxy: str, runner_http: str, use_aproxy: bool, + aproxy_exclude_addresses: str, + aproxy_redirect_ports: str, expected_proxy: ProxyConfig, expected_runner_proxy: ProxyConfig, ): @@ -1140,7 +1173,8 @@ def test_proxy_config( monkeypatch.setenv("JUJU_CHARM_NO_PROXY", juju_no_proxy) mock_charm.config[USE_APROXY_CONFIG_NAME] = use_aproxy mock_charm.config[RUNNER_HTTP_PROXY_CONFIG_NAME] = runner_http - + mock_charm.config[APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME] = aproxy_exclude_addresses + mock_charm.config[APROXY_REDIRECT_PORTS_CONFIG_NAME] = aproxy_redirect_ports mock_charm.model.relations[IMAGE_INTEGRATION_NAME] = [] mock_database = MagicMock(spec=DatabaseRequires) mock_database.relations = [] @@ -1148,5 +1182,7 @@ def test_proxy_config( charm_state = CharmState.from_charm(mock_charm, mock_database) assert charm_state.charm_config.use_aproxy == use_aproxy + assert charm_state.charm_config.aproxy_exclude_addresses == aproxy_exclude_addresses + assert charm_state.charm_config.aproxy_redirect_ports == aproxy_redirect_ports assert charm_state.proxy_config == expected_proxy assert charm_state.runner_proxy_config == expected_runner_proxy From 8e46f4681ddb52cf35efcc24c73af205db127f0b Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Fri, 20 Jun 2025 13:53:05 +0800 Subject: [PATCH 02/18] Update integration tests and fix linting tests --- .github/workflows/e2e_test_run.yaml | 29 +++++++++++++++-------------- src/charm_state.py | 3 ++- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/.github/workflows/e2e_test_run.yaml b/.github/workflows/e2e_test_run.yaml index 505b08687d..e693e732e9 100644 --- a/.github/workflows/e2e_test_run.yaml +++ b/.github/workflows/e2e_test_run.yaml @@ -47,6 +47,21 @@ jobs: run: | [[ -z "${http_proxy}" && -z "${HTTP_PROXY}" ]] \ || cat /home/ubuntu/.docker/config.json | grep httpProxy + - name: test network connectivity + run: | + timeout 60 curl --noproxy "*" http://example.com -svS -o /dev/null + timeout 60 curl --noproxy "*" https://example.com -svS -o /dev/null + [ $(timeout 60 echo hello | nc -q 0 tcpbin.com 4242) == "hello!" ] + - name: test aproxy logs + run: | + sudo snap logs aproxy.aproxy | grep -Fq "example.com:80" + sudo snap logs aproxy.aproxy | grep -Fq "example.com:443" + sudo snap logs aproxy.aproxy | grep -Eq "[0-9.]+:4242" + - name: show aproxy logs + if: failure() + run: | + sudo snap logs aproxy.aproxy -n=all + sudo nft list ruleset - name: Install microk8s run: sudo snap install microk8s --classic - name: Wait for microk8s @@ -87,17 +102,3 @@ jobs: # ~/.local/bin is added to path runner env through in scripts/env.j2 - name: test check-jsonschema run: check-jsonschema --version - - name: test network connectivity - run: | - timeout 60 curl --noproxy "*" http://example.com -svS -o /dev/null - timeout 60 curl --noproxy "*" https://example.com -svS -o /dev/null - [ $(timeout 60 echo hello | nc -q 0 tcpbin.com 4242) == "hello!" ] - - name: test aproxy logs - run: | - sudo snap logs aproxy.aproxy | grep -Fq "example.com:80" - sudo snap logs aproxy.aproxy | grep -Fq "example.com:443" - sudo snap logs aproxy.aproxy | grep -Eq "[0-9.]+:4242" - - name: show aproxy logs - if: failure() - run: | - sudo snap logs aproxy.aproxy -n=all diff --git a/src/charm_state.py b/src/charm_state.py index 09fb0b4a5c..1a1c2d4bb4 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -342,7 +342,8 @@ class CharmConfig(BaseModel): token: GitHub personal access token for GitHub API. manager_proxy_command: ProxyCommand for the SSH connection from the manager to the runner. use_aproxy: Whether to use aproxy in the runner. - aproxy_exclude_addresses: a comma-separated list of addresses to exclude from the aproxy proxy. + aproxy_exclude_addresses: a comma-separated list of addresses to exclude from the + aproxy proxy. aproxy_redirect_ports: a comma-separated list of ports to redirect to the aproxy proxy. custom_pre_job_script: Custom pre-job script to run before the job. jobmanager_url: Base URL of the job manager service. From 1ffe059a3a872765037ea54060c39cb0db914d82 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Fri, 20 Jun 2025 15:25:30 +0800 Subject: [PATCH 03/18] Fix linting issue and update integration issues --- .github/workflows/e2e_test_run.yaml | 2 +- docs/changelog.md | 2 ++ tests/integration/conftest.py | 2 +- tests/unit/test_charm_state.py | 4 ++-- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/e2e_test_run.yaml b/.github/workflows/e2e_test_run.yaml index e693e732e9..196a5cd697 100644 --- a/.github/workflows/e2e_test_run.yaml +++ b/.github/workflows/e2e_test_run.yaml @@ -51,7 +51,7 @@ jobs: run: | timeout 60 curl --noproxy "*" http://example.com -svS -o /dev/null timeout 60 curl --noproxy "*" https://example.com -svS -o /dev/null - [ $(timeout 60 echo hello | nc -q 0 tcpbin.com 4242) == "hello!" ] + [ "$(timeout 60 echo hello | nc -q 0 tcpbin.com 4242)" == "hello" ] - name: test aproxy logs run: | sudo snap logs aproxy.aproxy | grep -Fq "example.com:80" diff --git a/docs/changelog.md b/docs/changelog.md index 11b369d9fb..a5a543bd4e 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,6 +2,8 @@ This changelog documents user-relevant changes to the GitHub runner charm. +### 2025-06-20 +- New configuration options aproxy-exclude-addresses and aproxy-redirect-ports for allowing aproxy to redirect arbitrary TCP traffic ### 2025-06-17 diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index da33aaddac..4e62ade169 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -29,6 +29,7 @@ from pytest_operator.plugin import OpsTest from charm_state import ( + APROXY_REDIRECT_PORTS_CONFIG_NAME, BASE_VIRTUAL_MACHINES_CONFIG_NAME, LABELS_CONFIG_NAME, OPENSTACK_CLOUDS_YAML_CONFIG_NAME, @@ -36,7 +37,6 @@ OPENSTACK_NETWORK_CONFIG_NAME, PATH_CONFIG_NAME, USE_APROXY_CONFIG_NAME, - APROXY_REDIRECT_PORTS_CONFIG_NAME, ) from tests.integration.helpers.common import ( MONGODB_APP_NAME, diff --git a/tests/unit/test_charm_state.py b/tests/unit/test_charm_state.py index 1e8d84f750..b565a7d390 100644 --- a/tests/unit/test_charm_state.py +++ b/tests/unit/test_charm_state.py @@ -16,6 +16,8 @@ import charm_state from charm_state import ( + APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME, + APROXY_REDIRECT_PORTS_CONFIG_NAME, BASE_VIRTUAL_MACHINES_CONFIG_NAME, CUSTOM_PRE_JOB_SCRIPT_CONFIG_NAME, DEBUG_SSH_INTEGRATION_NAME, @@ -33,8 +35,6 @@ RUNNER_HTTP_PROXY_CONFIG_NAME, TOKEN_CONFIG_NAME, USE_APROXY_CONFIG_NAME, - APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME, - APROXY_REDIRECT_PORTS_CONFIG_NAME, USE_RUNNER_PROXY_FOR_TMATE_CONFIG_NAME, VIRTUAL_MACHINES_CONFIG_NAME, Arch, From d1a474d43fc7fd56ca142b6a8b27b148ef5fe71c Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Fri, 20 Jun 2025 15:45:37 +0800 Subject: [PATCH 04/18] Fix linting issues --- src/charm_state.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/charm_state.py b/src/charm_state.py index 1a1c2d4bb4..e2ca4f681f 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -521,8 +521,12 @@ def from_charm(cls, charm: CharmBase) -> "CharmConfig": token=github_config.token if github_config else None, manager_proxy_command=manager_proxy_command, use_aproxy=use_aproxy, - aproxy_exclude_addresses=charm.config.get(APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME), - aproxy_redirect_ports=charm.config.get(APROXY_REDIRECT_PORTS_CONFIG_NAME), + aproxy_exclude_addresses=( + cast(str | None, charm.config.get(APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME)) + ), + aproxy_redirect_ports=( + cast(str | None, charm.config.get(APROXY_REDIRECT_PORTS_CONFIG_NAME)) + ), custom_pre_job_script=custom_pre_job_script, jobmanager_url=jobmanager_config.url if jobmanager_config else None, ) From 08f9e8f995d9be52ecb53f3bd98ab49a513234d5 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Mon, 23 Jun 2025 13:42:49 +0800 Subject: [PATCH 05/18] Update e2e test --- .github/workflows/e2e_test_run.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/e2e_test_run.yaml b/.github/workflows/e2e_test_run.yaml index 196a5cd697..0bea15774a 100644 --- a/.github/workflows/e2e_test_run.yaml +++ b/.github/workflows/e2e_test_run.yaml @@ -51,7 +51,7 @@ jobs: run: | timeout 60 curl --noproxy "*" http://example.com -svS -o /dev/null timeout 60 curl --noproxy "*" https://example.com -svS -o /dev/null - [ "$(timeout 60 echo hello | nc -q 0 tcpbin.com 4242)" == "hello" ] + printf "" | timeout 60 nc github.com 22 | head -n 1 | grep SSH - name: test aproxy logs run: | sudo snap logs aproxy.aproxy | grep -Fq "example.com:80" From 19778e6289e5ea74527f6215f023635fa02d74d6 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Mon, 23 Jun 2025 19:29:25 +0800 Subject: [PATCH 06/18] Add validator to aproxy_redirect_ports and aproxy_exclude_addresses --- .../configuration/base.py | 9 +- src/charm_state.py | 111 ++++++++++++++++-- tests/unit/test_charm_state.py | 4 +- 3 files changed, 108 insertions(+), 16 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/configuration/base.py b/github-runner-manager/src/github_runner_manager/configuration/base.py index 09084b0d87..978cfb4b8e 100644 --- a/github-runner-manager/src/github_runner_manager/configuration/base.py +++ b/github-runner-manager/src/github_runner_manager/configuration/base.py @@ -81,9 +81,8 @@ class SupportServiceConfig(BaseModel): proxy_config: The proxy configuration. runner_proxy_config: The proxy configuration for the runner. use_aproxy: Whether aproxy should be used for the runners. - aproxy_exclude_addresses: A comma-separated list of addresses to exclude from the aproxy - proxy. - aproxy_redirect_ports: A comma-separated list of ports to redirect to the aproxy proxy. + aproxy_exclude_addresses: A list of addresses to exclude from the aproxy proxy. + aproxy_redirect_ports: A list of ports to redirect to the aproxy proxy. dockerhub_mirror: The dockerhub mirror to use for runners. ssh_debug_connections: The information on the ssh debug services. repo_policy_compliance: The configuration of the repo policy compliance service. @@ -94,8 +93,8 @@ class SupportServiceConfig(BaseModel): proxy_config: "ProxyConfig | None" runner_proxy_config: "ProxyConfig | None" use_aproxy: bool - aproxy_exclude_addresses: str | None - aproxy_redirect_ports: str | None + aproxy_exclude_addresses: list[str] = [] + aproxy_redirect_ports: list[str] = [] dockerhub_mirror: str | None ssh_debug_connections: "list[SSHDebugConnection]" repo_policy_compliance: "RepoPolicyComplianceConfig | None" diff --git a/src/charm_state.py b/src/charm_state.py index e2ca4f681f..506253859d 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -3,7 +3,10 @@ """State of the Charm.""" +# pylint: disable=too-many-lines + import dataclasses +import ipaddress import json import logging import platform @@ -342,9 +345,8 @@ class CharmConfig(BaseModel): token: GitHub personal access token for GitHub API. manager_proxy_command: ProxyCommand for the SSH connection from the manager to the runner. use_aproxy: Whether to use aproxy in the runner. - aproxy_exclude_addresses: a comma-separated list of addresses to exclude from the - aproxy proxy. - aproxy_redirect_ports: a comma-separated list of ports to redirect to the aproxy proxy. + aproxy_exclude_addresses: a list of addresses to exclude from the aproxy proxy. + aproxy_redirect_ports: a list of ports to redirect to the aproxy proxy. custom_pre_job_script: Custom pre-job script to run before the job. jobmanager_url: Base URL of the job manager service. """ @@ -358,8 +360,8 @@ class CharmConfig(BaseModel): token: str | None manager_proxy_command: str | None use_aproxy: bool - aproxy_exclude_addresses: str | None - aproxy_redirect_ports: str | None + aproxy_exclude_addresses: list[str] = [] + aproxy_redirect_ports: list[str] = [] custom_pre_job_script: str | None jobmanager_url: AnyHttpUrl | None @@ -457,6 +459,96 @@ def check_reconcile_interval(cls, reconcile_interval: int) -> int: return reconcile_interval + @validator("aproxy_exclude_addresses", pre=True) + @classmethod + def check_aproxy_exclude_addresses( + cls, aproxy_exclude_addresses: list[str] | str | None + ) -> list[str]: + """Parse and validate aproxy_exclude_addresses config value. + + Args: + aproxy_exclude_addresses: The aproxy_exclude_addresses configuration input. + + Raises: + CharmConfigInvalidError: invalid aproxy_exclude_addresses configuration input. + + Returns: + Parsed aproxy_exclude_addresses configuration input. + """ + if aproxy_exclude_addresses is None: + aproxy_exclude_addresses = [] + if isinstance(aproxy_exclude_addresses, str): + aproxy_exclude_addresses = aproxy_exclude_addresses.split(",") + result = [] + for address_range in aproxy_exclude_addresses: + address_range = address_range.strip() + if not address_range: + continue + if "-" in address_range: + start, end = address_range.split("-") + try: + ipaddress.ip_address(start) + ipaddress.ip_address(end) + except ValueError as exc: + raise CharmConfigInvalidError( + f"Invalid {APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} config" + ) from exc + else: + try: + ipaddress.ip_network(address_range, strict=False) + except ValueError as exc: + raise CharmConfigInvalidError( + f"Invalid {APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} config" + ) from exc + result.append(address_range) + return result + + @validator("aproxy_redirect_ports", pre=True) + @classmethod + def check_aproxy_redirect_ports( + cls, aproxy_redirect_ports: list[str] | str | None + ) -> list[str]: + """Parse and validate check_aproxy_redirect_ports config value. + + Args: + aproxy_redirect_ports: The aproxy_exclude_addresses configuration input. + + Raises: + CharmConfigInvalidError: invalid check_aproxy_redirect_ports configuration input. + + Returns: + Parsed check_aproxy_redirect_ports configuration input. + """ + if aproxy_redirect_ports is None: + aproxy_redirect_ports = [] + if isinstance(aproxy_redirect_ports, str): + aproxy_redirect_ports = aproxy_redirect_ports.split(",") + result = [] + for port_range in aproxy_redirect_ports: + port_range = port_range.strip() + if not port_range: + continue + if "-" in port_range: + start, end = port_range.split("-") + try: + start_num = int(start) + end_num = int(end) + except ValueError as exc: + raise CharmConfigInvalidError( + f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config" + ) from exc + if start_num < 0 or start_num > 65535 or end_num < 0 or end_num > 65535: + raise CharmConfigInvalidError( + f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config" + ) + else: + if not port_range.isdecimal() or int(port_range) < 0 or int(port_range) > 65535: + raise CharmConfigInvalidError( + f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config" + ) + result.append(port_range) + return result + @classmethod def from_charm(cls, charm: CharmBase) -> "CharmConfig": """Initialize the config from charm. @@ -521,11 +613,12 @@ def from_charm(cls, charm: CharmBase) -> "CharmConfig": token=github_config.token if github_config else None, manager_proxy_command=manager_proxy_command, use_aproxy=use_aproxy, - aproxy_exclude_addresses=( - cast(str | None, charm.config.get(APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME)) + # mypy doesn't know about the validator + aproxy_exclude_addresses=charm.config.get( # type: ignore + APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME ), - aproxy_redirect_ports=( - cast(str | None, charm.config.get(APROXY_REDIRECT_PORTS_CONFIG_NAME)) + aproxy_redirect_ports=charm.config.get( # type: ignore + APROXY_REDIRECT_PORTS_CONFIG_NAME ), custom_pre_job_script=custom_pre_job_script, jobmanager_url=jobmanager_config.url if jobmanager_config else None, diff --git a/tests/unit/test_charm_state.py b/tests/unit/test_charm_state.py index b565a7d390..e050de96a2 100644 --- a/tests/unit/test_charm_state.py +++ b/tests/unit/test_charm_state.py @@ -1182,7 +1182,7 @@ def test_proxy_config( charm_state = CharmState.from_charm(mock_charm, mock_database) assert charm_state.charm_config.use_aproxy == use_aproxy - assert charm_state.charm_config.aproxy_exclude_addresses == aproxy_exclude_addresses - assert charm_state.charm_config.aproxy_redirect_ports == aproxy_redirect_ports + assert ", ".join(charm_state.charm_config.aproxy_exclude_addresses) == aproxy_exclude_addresses + assert ", ".join(charm_state.charm_config.aproxy_redirect_ports) == aproxy_redirect_ports assert charm_state.proxy_config == expected_proxy assert charm_state.runner_proxy_config == expected_runner_proxy From 2898d1a53ea90b747d7a6a962e35557b2806ae54 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Mon, 23 Jun 2025 19:49:03 +0800 Subject: [PATCH 07/18] Use partition instead of split --- src/charm_state.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/charm_state.py b/src/charm_state.py index 506253859d..2f8dce130c 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -529,7 +529,7 @@ def check_aproxy_redirect_ports( if not port_range: continue if "-" in port_range: - start, end = port_range.split("-") + start, _, end = port_range.partition("-") try: start_num = int(start) end_num = int(end) From a3f8de511db8b3d53c7dafcbc775610e266a3b03 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Mon, 23 Jun 2025 21:30:23 +0800 Subject: [PATCH 08/18] Simplify _generate_cloud_init --- .../openstack_cloud/openstack_runner_manager.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index eeed06a6dc..74ba4ff749 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -290,13 +290,9 @@ def _generate_cloud_init(self, runner_context: RunnerContext) -> str: use_aproxy = False aproxy_exclude_addresses = service_config.aproxy_exclude_addresses aproxy_exclude_ipv4_addresses = [] - if aproxy_exclude_addresses: - for address in aproxy_exclude_addresses.split(","): - address = address.strip() - if not address: - continue - if ":" not in address: - aproxy_exclude_ipv4_addresses.append(address) + for address in aproxy_exclude_addresses: + if ":" not in address: + aproxy_exclude_ipv4_addresses.append(address) return jinja.get_template("openstack-userdata.sh.j2").render( run_script=runner_context.shell_run_script, env_contents=env_contents, From eef137b1d6a548ef9c71c336def5525bd1d0bf1c Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Mon, 23 Jun 2025 21:30:58 +0800 Subject: [PATCH 09/18] Fix _generate_cloud_init --- .../openstack_cloud/openstack_runner_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index 74ba4ff749..8597081eed 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -301,7 +301,7 @@ def _generate_cloud_init(self, runner_context: RunnerContext) -> str: use_aproxy=use_aproxy, aproxy_address=service_config.runner_proxy_config.proxy_address, aproxy_exclude_ipv4_addresses=", ".join(aproxy_exclude_ipv4_addresses), - aproxy_redirect_ports=aproxy_redirect_ports, + aproxy_redirect_ports=", ".join(aproxy_redirect_ports), dockerhub_mirror=service_config.dockerhub_mirror, ssh_debug_info=ssh_debug_info, runner_proxy_config=service_config.runner_proxy_config, From 9bfb7079a9807f623b9b25fa20fb6479e4767ea6 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Mon, 23 Jun 2025 21:34:37 +0800 Subject: [PATCH 10/18] Simplify _generate_cloud_init --- .../openstack_cloud/openstack_runner_manager.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index 8597081eed..3a690c64c0 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -288,11 +288,9 @@ def _generate_cloud_init(self, runner_context: RunnerContext) -> str: aproxy_redirect_ports = service_config.aproxy_redirect_ports if not aproxy_redirect_ports: use_aproxy = False - aproxy_exclude_addresses = service_config.aproxy_exclude_addresses - aproxy_exclude_ipv4_addresses = [] - for address in aproxy_exclude_addresses: - if ":" not in address: - aproxy_exclude_ipv4_addresses.append(address) + aproxy_exclude_ipv4_addresses = [ + address for address in service_config.aproxy_exclude_addresses if ":" not in address + ] return jinja.get_template("openstack-userdata.sh.j2").render( run_script=runner_context.shell_run_script, env_contents=env_contents, From 37bac79cec345c1397fe9affed9ea322a96a1316 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Wed, 25 Jun 2025 17:21:56 +0800 Subject: [PATCH 11/18] Fix e2e test --- .github/workflows/e2e_test_run.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/e2e_test_run.yaml b/.github/workflows/e2e_test_run.yaml index 0bea15774a..09a0eaa59f 100644 --- a/.github/workflows/e2e_test_run.yaml +++ b/.github/workflows/e2e_test_run.yaml @@ -56,7 +56,7 @@ jobs: run: | sudo snap logs aproxy.aproxy | grep -Fq "example.com:80" sudo snap logs aproxy.aproxy | grep -Fq "example.com:443" - sudo snap logs aproxy.aproxy | grep -Eq "[0-9.]+:4242" + sudo snap logs aproxy.aproxy | grep -Eq "[0-9.]+:22" - name: show aproxy logs if: failure() run: | From 63049590dd00c22d87298f3d86dc6a54b564a08e Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Thu, 26 Jun 2025 18:50:24 +0800 Subject: [PATCH 12/18] Apply suggestions from review comments --- src/charm_state.py | 65 +++++++++++++++++++++++++--------- tests/unit/test_charm_state.py | 35 ++++++++++++++++++ 2 files changed, 83 insertions(+), 17 deletions(-) diff --git a/src/charm_state.py b/src/charm_state.py index 2f8dce130c..41eb7e53c0 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -459,6 +459,22 @@ def check_reconcile_interval(cls, reconcile_interval: int) -> int: return reconcile_interval + @staticmethod + def _parse_list(input_: str | list[str] | None) -> list[str]: + """Split a comma-separated list of strings into a list of strings. + + Args: + input_: The comma-separated list of strings. + + Returns: + A list of strings. + """ + if input_ is None: + return [] + if isinstance(input_, str): + input_ = input_.split(",") + return [i.strip() for i in input_ if i.strip()] + @validator("aproxy_exclude_addresses", pre=True) @classmethod def check_aproxy_exclude_addresses( @@ -475,23 +491,30 @@ def check_aproxy_exclude_addresses( Returns: Parsed aproxy_exclude_addresses configuration input. """ - if aproxy_exclude_addresses is None: - aproxy_exclude_addresses = [] - if isinstance(aproxy_exclude_addresses, str): - aproxy_exclude_addresses = aproxy_exclude_addresses.split(",") + aproxy_exclude_addresses = cls._parse_list(aproxy_exclude_addresses) result = [] for address_range in aproxy_exclude_addresses: - address_range = address_range.strip() if not address_range: continue if "-" in address_range: - start, end = address_range.split("-") + start, _, end = address_range.partition("-") + if not start: + raise CharmConfigInvalidError( + f"Invalid {APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} config, " + f"in {repr(address_range)}, missing start in range" + ) + if not end: + raise CharmConfigInvalidError( + f"Invalid {APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} config, " + f"in {repr(address_range)}, missing end in range" + ) try: ipaddress.ip_address(start) ipaddress.ip_address(end) except ValueError as exc: raise CharmConfigInvalidError( - f"Invalid {APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} config" + f"Invalid {APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} config, " + f"in {repr(address_range)}, not an IP address" ) from exc else: try: @@ -499,6 +522,7 @@ def check_aproxy_exclude_addresses( except ValueError as exc: raise CharmConfigInvalidError( f"Invalid {APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME} config" + f"in {repr(address_range)}, not an IP address" ) from exc result.append(address_range) return result @@ -519,32 +543,39 @@ def check_aproxy_redirect_ports( Returns: Parsed check_aproxy_redirect_ports configuration input. """ - if aproxy_redirect_ports is None: - aproxy_redirect_ports = [] - if isinstance(aproxy_redirect_ports, str): - aproxy_redirect_ports = aproxy_redirect_ports.split(",") + aproxy_redirect_ports = cls._parse_list(aproxy_redirect_ports) result = [] for port_range in aproxy_redirect_ports: - port_range = port_range.strip() - if not port_range: - continue if "-" in port_range: start, _, end = port_range.partition("-") + if not start: + raise CharmConfigInvalidError( + f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config, " + f"in {repr(port_range)}, missing start in range" + ) + if not end: + raise CharmConfigInvalidError( + f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config, " + f"in {repr(port_range)}, missing end in range" + ) try: start_num = int(start) end_num = int(end) except ValueError as exc: raise CharmConfigInvalidError( - f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config" + f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config, " + f"in {repr(port_range)}, not a number" ) from exc if start_num < 0 or start_num > 65535 or end_num < 0 or end_num > 65535: raise CharmConfigInvalidError( - f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config" + f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config, " + f"in {repr(port_range)}, invalid port number" ) else: if not port_range.isdecimal() or int(port_range) < 0 or int(port_range) > 65535: raise CharmConfigInvalidError( - f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config" + f"Invalid {APROXY_REDIRECT_PORTS_CONFIG_NAME} config," + f"in {repr(port_range)}, port is not a number or invalid port number" ) result.append(port_range) return result diff --git a/tests/unit/test_charm_state.py b/tests/unit/test_charm_state.py index e050de96a2..94db5b4f41 100644 --- a/tests/unit/test_charm_state.py +++ b/tests/unit/test_charm_state.py @@ -1186,3 +1186,38 @@ def test_proxy_config( assert ", ".join(charm_state.charm_config.aproxy_redirect_ports) == aproxy_redirect_ports assert charm_state.proxy_config == expected_proxy assert charm_state.runner_proxy_config == expected_runner_proxy + + +@pytest.mark.parametrize( + "aproxy_exclude_addresses, aproxy_redirect_ports", + [ + ["256.0.0.0/8", ""], + ["10.0.0.0-", ""], + ["-192.168.0.0", ""], + ["-", ""], + ["foobar", ""], + ["", "foobar"], + ["", "99999"], + ["", "-1"], + ["", "80-"], + ["", "-"], + ], +) +def test_invalid_aproxy_config_in_charm_state( + monkeypatch, aproxy_exclude_addresses: str, aproxy_redirect_ports: str +): + """ + arrange: Mock CharmBase and necessary methods to raise the specified exceptions. + act: Call CharmState.from_charm with invalid aproxy related configurations. + assert: Ensure CharmConfigInvalidError is raised. + """ + mock_charm = MockGithubRunnerCharmFactory() + mock_charm.config[USE_APROXY_CONFIG_NAME] = True + mock_charm.config[APROXY_EXCLUDE_ADDRESSES_CONFIG_NAME] = aproxy_exclude_addresses + mock_charm.config[APROXY_REDIRECT_PORTS_CONFIG_NAME] = aproxy_redirect_ports + mock_charm.model.relations[IMAGE_INTEGRATION_NAME] = [] + mock_database = MagicMock(spec=DatabaseRequires) + mock_database.relations = [] + + with pytest.raises(CharmConfigInvalidError): + CharmState.from_charm(mock_charm, mock_database) From 06d9007084724280bb503645ad52ecea4615ba3a Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Thu, 26 Jun 2025 19:31:00 +0800 Subject: [PATCH 13/18] Small refactor over the charm_state.py --- src/charm_state.py | 102 +-------------------------------- src/models.py | 89 ++++++++++++++++++++++++++++ tests/unit/conftest.py | 5 +- tests/unit/test_charm_state.py | 28 --------- 4 files changed, 94 insertions(+), 130 deletions(-) create mode 100644 src/models.py diff --git a/src/charm_state.py b/src/charm_state.py index 41eb7e53c0..729407ea2d 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -2,18 +2,14 @@ # See LICENSE file for licensing details. """State of the Charm.""" - -# pylint: disable=too-many-lines - import dataclasses import ipaddress import json import logging import platform import re -from enum import Enum from pathlib import Path -from typing import TypedDict, cast +from typing import cast from urllib.parse import urlsplit import yaml @@ -31,6 +27,7 @@ ) from errors import MissingMongoDBError +from models import AnyHttpsUrl, Arch, FlavorLabel, OpenStackCloudsYAML from utilities import get_env_var logger = logging.getLogger(__name__) @@ -75,16 +72,6 @@ MONGO_DB_INTEGRATION_NAME = "mongodb" -class AnyHttpsUrl(AnyHttpUrl): - """Represents an HTTPS URL. - - Attributes: - allowed_schemes: Allowed schemes for the URL. - """ - - allowed_schemes = {"https"} - - @dataclasses.dataclass class GithubConfig: """Charm configuration related to GitHub. @@ -171,18 +158,6 @@ def from_charm(cls, charm: CharmBase) -> "JobManagerConfig | None": return None -class Arch(str, Enum): - """Supported system architectures. - - Attributes: - ARM64: Represents an ARM64 system architecture. - X64: Represents an X64/AMD64 system architecture. - """ - - ARM64 = "arm64" - X64 = "x64" - - class CharmConfigInvalidError(Exception): """Raised when charm config is invalid. @@ -199,21 +174,6 @@ def __init__(self, msg: str): self.msg = msg -def _valid_storage_size_str(size: str) -> bool: - """Validate the storage size string. - - Args: - size: Storage size string. - - Return: - Whether the string is valid. - """ - # Checks whether the string confirms to using the KiB, MiB, GiB, TiB, PiB, - # EiB suffix for storage size as specified in config.yaml. - valid_suffixes = {"KiB", "MiB", "GiB", "TiB", "PiB", "EiB"} - return size[-3:] in valid_suffixes and size[:-3].isdigit() - - WORD_ONLY_REGEX = re.compile("^[\\w\\-]+$") @@ -285,50 +245,6 @@ def from_charm(cls, charm: CharmBase) -> "RepoPolicyComplianceConfig": return cls(url=url, token=token) # type: ignore -class _OpenStackAuth(TypedDict): - """The OpenStack cloud connection authentication info. - - Attributes: - auth_url: The OpenStack authentication URL (keystone). - password: The OpenStack project user's password. - project_domain_name: The project domain in which the project belongs to. - project_name: The OpenStack project to connect to. - user_domain_name: The user domain in which the user belongs to. - username: The user to authenticate as. - """ - - auth_url: str - password: str - project_domain_name: str - project_name: str - user_domain_name: str - username: str - - -class _OpenStackCloud(TypedDict): - """The OpenStack cloud connection info. - - See https://docs.openstack.org/python-openstackclient/pike/configuration/index.html. - - Attributes: - auth: The connection authentication info. - region_name: The OpenStack region to authenticate to. - """ - - auth: _OpenStackAuth - region_name: str - - -class OpenStackCloudsYAML(TypedDict): - """The OpenStack clouds YAML dict mapping. - - Attributes: - clouds: The map of cloud name to cloud connection info. - """ - - clouds: dict[str, _OpenStackCloud] - - class CharmConfig(BaseModel): """General charm configuration. @@ -694,20 +610,6 @@ def from_charm(cls, charm: CharmBase) -> "OpenstackImage | None": return OpenstackImage(id=None, tags=None) -@dataclasses.dataclass -class FlavorLabel: - """Combination of flavor and label. - - Attributes: - flavor: Flavor for the VM. - label: Label associated with the flavor. - """ - - flavor: str - # Remove the None when several FlavorLabel combinations are supported. - label: str | None - - class OpenstackRunnerConfig(BaseModel): """Runner configuration for OpenStack Instances. diff --git a/src/models.py b/src/models.py new file mode 100644 index 0000000000..151a37f479 --- /dev/null +++ b/src/models.py @@ -0,0 +1,89 @@ +# Copyright 2025 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Data classes and type definitions.""" +import dataclasses +from enum import Enum +from typing import TypedDict + +from pydantic import AnyHttpUrl + + +class AnyHttpsUrl(AnyHttpUrl): + """Represents an HTTPS URL. + + Attributes: + allowed_schemes: Allowed schemes for the URL. + """ + + allowed_schemes = {"https"} + + +class Arch(str, Enum): + """Supported system architectures. + + Attributes: + ARM64: Represents an ARM64 system architecture. + X64: Represents an X64/AMD64 system architecture. + """ + + ARM64 = "arm64" + X64 = "x64" + + +class _OpenStackAuth(TypedDict): + """The OpenStack cloud connection authentication info. + + Attributes: + auth_url: The OpenStack authentication URL (keystone). + password: The OpenStack project user's password. + project_domain_name: The project domain in which the project belongs to. + project_name: The OpenStack project to connect to. + user_domain_name: The user domain in which the user belongs to. + username: The user to authenticate as. + """ + + auth_url: str + password: str + project_domain_name: str + project_name: str + user_domain_name: str + username: str + + +class _OpenStackCloud(TypedDict): + """The OpenStack cloud connection info. + + See https://docs.openstack.org/python-openstackclient/pike/configuration/index.html. + + Attributes: + auth: The connection authentication info. + region_name: The OpenStack region to authenticate to. + """ + + auth: _OpenStackAuth + region_name: str + + +class OpenStackCloudsYAML(TypedDict): + """The OpenStack clouds YAML dict mapping. + + Attributes: + clouds: The map of cloud name to cloud connection info. + """ + + clouds: dict[str, _OpenStackCloud] + + +@dataclasses.dataclass +class FlavorLabel: + """Combination of flavor and label. + + Attributes: + flavor: Flavor for the VM. + label: Label associated with the flavor. + """ + + flavor: str + # Remove the None when several FlavorLabel combinations are supported. + label: str | None diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index fd0f9251bf..5a10c9afca 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -12,6 +12,7 @@ import charm_state import utilities +from models import FlavorLabel, OpenStackCloudsYAML @pytest.fixture(name="exec_command") @@ -132,7 +133,7 @@ def complete_charm_state_fixture(): charm_config=charm_state.CharmConfig( dockerhub_mirror="https://docker.example.com", labels=("label1", "label2"), - openstack_clouds_yaml=charm_state.OpenStackCloudsYAML( + openstack_clouds_yaml=OpenStackCloudsYAML( clouds={ "microstack": { "auth": { @@ -161,7 +162,7 @@ def complete_charm_state_fixture(): base_virtual_machines=1, max_total_virtual_machines=2, flavor_label_combinations=[ - charm_state.FlavorLabel( + FlavorLabel( flavor="flavor", label="flavorlabel", ) diff --git a/tests/unit/test_charm_state.py b/tests/unit/test_charm_state.py index 94db5b4f41..d8f1d1ae82 100644 --- a/tests/unit/test_charm_state.py +++ b/tests/unit/test_charm_state.py @@ -154,34 +154,6 @@ def test_parse_github_path( assert getattr(result, attr) == value -@pytest.mark.parametrize( - "size, expected_result", - [ - ("100KiB", True), - ("10MiB", True), - ("1GiB", True), - ("0TiB", True), - ("1000PiB", True), - ("10000EiB", True), - ("100KB", False), # Invalid suffix - ("100GB", False), # Invalid suffix - ("abc", False), # Non-numeric characters - ("100", False), # No suffix - ("100Ki", False), # Incomplete suffix - ("100.5MiB", False), # Non-integer size - ], -) -def test_valid_storage_size_str(size: str, expected_result: bool): - """ - arrange: Provide storage size string. - act: Call _valid_storage_size_str with the provided storage size string. - assert: Verify that the function returns the expected result. - """ - result = charm_state._valid_storage_size_str(size) - - assert result == expected_result - - def test_parse_labels_invalid(): """ arrange: Provide labels string with an invalid label. From 8f7a3bc7d12a41abe4d694798a60f9dfa4c6c358 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Fri, 27 Jun 2025 13:05:00 +0800 Subject: [PATCH 14/18] Fix unit tests --- .../test_openstack_runner_manager.py | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py index aabfd74e13..371684dca9 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py @@ -82,25 +82,29 @@ def runner_metrics_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock: "aproxy_redirect_ports, aproxy_exclude_addresses, aproxy_used, except_aproxy_script", [ pytest.param( - "", "10.0.0.0/8", False, "", id="empty aproxy_redirect_ports disables aproxy" + [], + ["10.0.0.0/8"], + False, + "", + id="empty aproxy_redirect_ports disables aproxy", ), pytest.param( - "80, 443", - "10.0.0.0/8, 192.168.0.0/16", + ["80", "443"], + ["10.0.0.0/8", "192.168.0.0/16"], True, "10.0.0.0/8, 192.168.0.0/16", id="aproxy with custom aproxy_exclude_addresses", ), pytest.param( - "0-3127, 3129-65535", - "10.0.0.0/8, 192.168.0.0/16", + ["0-3127", "3129-65535"], + ["10.0.0.0/8", "192.168.0.0/16"], True, "0-3127, 3129-65535", id="aproxy with custom aproxy_redirect_ports", ), pytest.param( - "80, 443", - "10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16", + ["80", "443"], + ["10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"], True, textwrap.dedent( """\ @@ -126,8 +130,8 @@ def runner_metrics_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock: id="aproxy default config", ), pytest.param( - "80, 443", - "", + ["80", "443"], + [], True, textwrap.dedent( """\ @@ -153,8 +157,8 @@ def runner_metrics_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock: ], ) def test_create_runner_with_aproxy( - aproxy_redirect_ports: str, - aproxy_exclude_addresses: str, + aproxy_redirect_ports: list[str], + aproxy_exclude_addresses: list[str], aproxy_used: str, except_aproxy_script: str, runner_manager: OpenStackRunnerManager, From 1f78857578e1bb08f9f5f30404ace3c1fe984a02 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Wed, 2 Jul 2025 13:45:21 +0800 Subject: [PATCH 15/18] remove test for TCP proxy --- .github/workflows/e2e_test_run.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/e2e_test_run.yaml b/.github/workflows/e2e_test_run.yaml index 09a0eaa59f..7f81d7c4ff 100644 --- a/.github/workflows/e2e_test_run.yaml +++ b/.github/workflows/e2e_test_run.yaml @@ -51,12 +51,10 @@ jobs: run: | timeout 60 curl --noproxy "*" http://example.com -svS -o /dev/null timeout 60 curl --noproxy "*" https://example.com -svS -o /dev/null - printf "" | timeout 60 nc github.com 22 | head -n 1 | grep SSH - name: test aproxy logs run: | sudo snap logs aproxy.aproxy | grep -Fq "example.com:80" sudo snap logs aproxy.aproxy | grep -Fq "example.com:443" - sudo snap logs aproxy.aproxy | grep -Eq "[0-9.]+:22" - name: show aproxy logs if: failure() run: | From 8d38c4513e92e99db3c49ad1e1ff63a0525010c9 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Wed, 2 Jul 2025 18:55:07 +0800 Subject: [PATCH 16/18] Update e2e tests --- .github/workflows/e2e_test_run.yaml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/e2e_test_run.yaml b/.github/workflows/e2e_test_run.yaml index 7f81d7c4ff..dd37ee4194 100644 --- a/.github/workflows/e2e_test_run.yaml +++ b/.github/workflows/e2e_test_run.yaml @@ -55,11 +55,6 @@ jobs: run: | sudo snap logs aproxy.aproxy | grep -Fq "example.com:80" sudo snap logs aproxy.aproxy | grep -Fq "example.com:443" - - name: show aproxy logs - if: failure() - run: | - sudo snap logs aproxy.aproxy -n=all - sudo nft list ruleset - name: Install microk8s run: sudo snap install microk8s --classic - name: Wait for microk8s @@ -100,3 +95,9 @@ jobs: # ~/.local/bin is added to path runner env through in scripts/env.j2 - name: test check-jsonschema run: check-jsonschema --version + - name: show aproxy logs + if: always() + run: | + sudo snap get aproxy + sudo snap logs aproxy.aproxy -n=all + sudo nft list ruleset From 6cb3d6b62d5c006b0138c112c03ca1e788c3e202 Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Thu, 3 Jul 2025 14:43:02 +0800 Subject: [PATCH 17/18] Always exclude 127.0.0.0/8 --- config.yaml | 1 + .../templates/openstack-userdata.sh.j2 | 4 +--- .../unit/openstack_cloud/test_openstack_runner_manager.py | 6 ++---- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/config.yaml b/config.yaml index 742bd3b70a..07388aea67 100644 --- a/config.yaml +++ b/config.yaml @@ -51,6 +51,7 @@ options: default: "10.0.0.0/8, 171.16.0.0/12, 192.168.0.0/16" description: >- A comma-separated list of IP addresses that should be excluded from redirection to aproxy. + 127.0.0.0/8 are always excluded so you can omit if from the configuration. aproxy-redirect-ports: type: string default: "80, 443" diff --git a/github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 b/github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 index 4adbd0c95f..696de182a4 100644 --- a/github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 +++ b/github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 @@ -21,9 +21,7 @@ table ip aproxy { set exclude { type ipv4_addr; flags interval; auto-merge; -{% if aproxy_exclude_ipv4_addresses %} - elements = { {{ aproxy_exclude_ipv4_addresses }} } -{% endif %} + elements = { 127.0.0.0/8, {{ aproxy_exclude_ipv4_addresses }} } } chain prerouting { type nat hook prerouting priority dstnat; policy accept; diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py index 371684dca9..444645c684 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py @@ -112,9 +112,7 @@ def runner_metrics_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock: set exclude { type ipv4_addr; flags interval; auto-merge; - - elements = { 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 } - + elements = { 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 } } chain prerouting { type nat hook prerouting priority dstnat; policy accept; @@ -139,7 +137,7 @@ def runner_metrics_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock: set exclude { type ipv4_addr; flags interval; auto-merge; - + elements = { 127.0.0.0/8, } } chain prerouting { type nat hook prerouting priority dstnat; policy accept; From af5a7dcf7d3a645407ef3dcb764ee064bfb7fa0c Mon Sep 17 00:00:00 2001 From: Weii Wang Date: Fri, 4 Jul 2025 16:59:54 +0800 Subject: [PATCH 18/18] Increase timeout for runner_manager_with_one_runner_fixture --- tests/integration/test_runner_manager_openstack.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_runner_manager_openstack.py b/tests/integration/test_runner_manager_openstack.py index 311f4f0d31..2058a7d2fe 100644 --- a/tests/integration/test_runner_manager_openstack.py +++ b/tests/integration/test_runner_manager_openstack.py @@ -243,7 +243,7 @@ async def runner_manager_with_one_runner_fixture(runner_manager: RunnerManager) try: await wait_for( lambda: runner_manager.get_runners()[0].platform_state == PlatformRunnerState.IDLE, - timeout=120, + timeout=1200, check_interval=10, ) except TimeoutError as err: