Skip to content

Commit

Permalink
Remove proxy vars when using aproxy (#223)
Browse files Browse the repository at this point in the history 
* Remove other env vars when using aproxy

* Refactor proxy config

* Fix check_fields

* Add ret code check for repo policy uninstall

* Add docs

* lint

* add and adapt integration tests

* fix runner_manager

* Enhance integration test

* Add missing docstring

* Fix integration test

* Fix integration test

* Fix integration test

* Fix environment files

* Use su - ubuntu to ensure the use of env

* Restructure test

* Cleanup

* Lint

* Lint

* Lint

* Remove proxy settings for request and fastcore

* Set Docker proxy only if defined

* Use f-string
  • Loading branch information
@cbartz
cbartz authored Feb 20, 2024
1 parent e487546 commit 3ef9b0e
Show file tree
Hide file tree
Showing 20 changed files with 560 additions and 195 deletions.
24 changes: 12 additions & 12 deletions config.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,22 @@ options:
The private docker registry configured as dockerhub mirror to be used by the runners. If set
a message will be displayed prior to job execution on self-hosted runner informing users to
use the provided registry.
experimental-openstack-clouds-yaml:
type: string
default: ""
description: |
(Experimental, under development) The openstack clouds.yaml
(https://docs.openstack.org/python-openstackclient/pike/configuration/index.html#clouds-yaml).
Setting the openstack-clouds-yaml would enable spawning runners on OpenStack, instead of LXD
vms. This feature is not yet ready and is under development.
experimental-use-aproxy:
type: boolean
default: false
description: |
(Experimental, may be removed) When set to true, aproxy (https://github.com/canonical/aproxy)
will be installed inside the runners and will forward all http/s traffic 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).
(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) 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
Expand All @@ -34,14 +42,6 @@ options:
description: |
The organization runner group to register the self-hosted runner under. This has no effect on
runners under a repository.
experimental-openstack-clouds-yaml:
type: string
default: ""
description: |
(Experimental, under development) The openstack clouds.yaml
(https://docs.openstack.org/python-openstackclient/pike/configuration/index.html#clouds-yaml).
Setting the openstack-clouds-yaml would enable spawning runners on OpenStack, instead of LXD
vms. This feature is not yet ready and is under development.
path:
type: string
default: ""
Expand Down
25 changes: 22 additions & 3 deletions docs/explanation/charm-architecture.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,28 @@ The configurations applied in the image include:

## Network configuration

The charm respects the HTTP(S) proxy configuration of the model configuration of Juju. The configuration can be set with [`juju model-config`](https://juju.is/docs/juju/juju-model-config) using the following keys: `juju-http-proxy`, `juju-https-proxy`, `juju-no-proxy`. The GitHub self-hosted runner applications are configured to use the proxy configuration.

If an HTTP(S) proxy is used, all HTTP(S) requests in the GitHub workflow will be transparently routed to the proxy with [aproxy](https://github.com/canonical/aproxy). Iptables are set up to route network traffic to the destination on ports 80 and 443 to the aproxy. The aproxy will route received packets to the configured HTTP(S) proxy. The service is installed on each runner virtual machine and configured according to the proxy configuration from the Juju model.
The charm respects the HTTP(S) proxy configuration of the model configuration of Juju. The configuration can be set with [`juju model-config`](https://juju.is/docs/juju/juju-model-config) using the following keys: `juju-http-proxy`, `juju-https-proxy`, `juju-no-proxy`.
The GitHub self-hosted runner applications will be configured to utilise the proxy configuration.
This involves setting environment variables such as `http_proxy`, `https_proxy`, `no_proxy`, `HTTP_PROXY`, `HTTPS_PROXY`, and `NO_PROXY`
in various locations within the runner environment, such as `/etc/environment`.

However, employing this approach with environment variables has its drawbacks.
Not all applications within a workflow may adhere to these variables as they
[lack standardisation](https://about.gitlab.com/blog/2021/01/27/we-need-to-talk-no-proxy/).
This inconsistency can result in failed workflows, prompting the introduction of aproxy, as detailed in the subsection below.

### aproxy
If the proxy configuration is utilised and [aproxy](https://github.com/canonical/aproxy) is specified through the charm's configuration option,
all HTTP(S) requests to standard ports (80, 443) within the GitHub workflow will be automatically directed
to the specified HTTP(s) proxy. Network traffic destined for ports 80 and 443 is redirected to aproxy using iptables.
aproxy then forwards received packets to the designated HTTP(S) proxy.
Beyond that, the environment variables (`http_proxy`, `https_proxy`, `no_proxy`, `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY`)
will no longer be defined in the runner environment.
It's worth noting that this setup deviates from the behaviour when not using aproxy,
where these variables are set in the runner environment. In that scenario, traffic to non-standard ports
would also be directed to the HTTP(s) proxy, unlike when using aproxy.

### denylist

The nftables on the Juju machine are configured to deny traffic from the runner virtual machine to IPs on the [`denylist` configuration](https://charmhub.io/github-runner/configure#denylist). The runner will always have access to essential services such as DHCP and DNS, regardless of the denylist configuration.

Expand Down
4 changes: 2 additions & 2 deletions src-docs/runner.py.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ Construct the runner instance.

---

<a href="../src/runner.py#L133"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner.py#L125"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `create`

Expand All @@ -91,7 +91,7 @@ Create the runner instance on LXD and register it on GitHub.

---

<a href="../src/runner.py#L169"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner.py#L161"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `remove`

Expand Down
22 changes: 10 additions & 12 deletions src-docs/runner_manager.py.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,15 @@ Runner Manager manages the runners on LXD and GitHub.
## <kbd>class</kbd> `RunnerManager`
Manage a group of runners according to configuration.

<a href="../src/runner_manager.py#L56"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_manager.py#L55"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `__init__`

```python
__init__(
app_name: str,
unit: int,
runner_manager_config: RunnerManagerConfig,
proxies: Optional[ProxySetting] = None
runner_manager_config: RunnerManagerConfig
) → None
```

Expand All @@ -39,14 +38,13 @@ Construct RunnerManager object for creating and managing runners.
- <b>`app_name`</b>: An name for the set of runners.
- <b>`unit`</b>: Unit number of the set of runners.
- <b>`runner_manager_config`</b>: Configuration for the runner manager.
- <b>`proxies`</b>: HTTP proxy settings.




---

<a href="../src/runner_manager.py#L726"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_manager.py#L742"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `build_runner_image`

Expand All @@ -66,7 +64,7 @@ Build container image in test mode, else virtual machine image.

---

<a href="../src/runner_manager.py#L117"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_manager.py#L107"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `check_runner_bin`

Expand All @@ -83,7 +81,7 @@ Check if runner binary exists.

---

<a href="../src/runner_manager.py#L555"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_manager.py#L580"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `flush`

Expand All @@ -106,7 +104,7 @@ Remove existing runners.

---

<a href="../src/runner_manager.py#L218"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_manager.py#L208"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `get_github_info`

Expand All @@ -123,7 +121,7 @@ Get information on the runners from GitHub.

---

<a href="../src/utilities.py#L125"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/utilities.py#L115"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `get_latest_runner_bin_url`

Expand All @@ -148,7 +146,7 @@ The runner binary URL changes when a new version is available.

---

<a href="../src/runner_manager.py#L450"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_manager.py#L475"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `reconcile`

Expand All @@ -172,7 +170,7 @@ Bring runners in line with target.

---

<a href="../src/runner_manager.py#L736"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_manager.py#L752"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `schedule_build_runner_image`

Expand All @@ -184,7 +182,7 @@ Install cron job for building runner image.

---

<a href="../src/utilities.py#L151"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/utilities.py#L141"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `update_runner_bin`

Expand Down
4 changes: 2 additions & 2 deletions src-docs/runner_type.py.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Represent GitHub organization.

---

<a href="../src/runner_type.py#L50"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_type.py#L51"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `path`

Expand All @@ -38,7 +38,7 @@ Represent GitHub repository.

---

<a href="../src/runner_type.py#L38"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
<a href="../src/runner_type.py#L39"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>

### <kbd>function</kbd> `path`

Expand Down
37 changes: 12 additions & 25 deletions src/charm.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
from runner import LXD_PROFILE_YAML
from runner_manager import RunnerManager, RunnerManagerConfig
from runner_manager_type import FlushMode
from runner_type import GithubOrg, GithubRepo, ProxySetting, VirtualMachineResources
from runner_type import GithubOrg, GithubRepo, VirtualMachineResources
from utilities import bytes_with_unit_to_kib, execute_command, retry

RECONCILE_RUNNERS_EVENT = "reconcile-runners"
Expand Down Expand Up @@ -181,19 +181,6 @@ def __init__(self, *args, **kargs) -> None:
runner_bin_url=None,
)

self.proxies: ProxySetting = {}
if proxy_config := self._state.proxy_config:
if http_proxy := proxy_config.http_proxy:
self.proxies["http"] = str(http_proxy)
if https_proxy := proxy_config.https_proxy:
self.proxies["https"] = str(https_proxy)
# there's no need for no_proxy if there's no http_proxy or https_proxy
no_proxy = proxy_config.no_proxy
if (https_proxy or http_proxy) and no_proxy:
self.proxies["no_proxy"] = no_proxy
if proxy_config.use_aproxy:
self.proxies["aproxy_address"] = proxy_config.aproxy_address

self.service_token = None

self.on.define_event("reconcile_runners", ReconcileRunnersEvent)
Expand Down Expand Up @@ -366,7 +353,6 @@ def _get_runner_manager(
charm_state=self._state,
dockerhub_mirror=dockerhub_mirror,
),
proxies=self.proxies,
)

@catch_charm_errors
Expand Down Expand Up @@ -776,15 +762,16 @@ def _install_repo_policy_compliance(self) -> bool:
"""
# Prepare environment for pip subprocess
env = {}
if "http" in self.proxies:
env["HTTP_PROXY"] = self.proxies["http"]
env["http_proxy"] = self.proxies["http"]
if "https" in self.proxies:
env["HTTPS_PROXY"] = self.proxies["https"]
env["https_proxy"] = self.proxies["https"]
if "no_proxy" in self.proxies:
env["NO_PROXY"] = self.proxies["no_proxy"]
env["no_proxy"] = self.proxies["no_proxy"]
proxy_config = self._state.proxy_config
if http_proxy := proxy_config.http:
env["HTTP_PROXY"] = http_proxy
env["http_proxy"] = http_proxy
if https_proxy := proxy_config.https:
env["HTTPS_PROXY"] = https_proxy
env["https_proxy"] = https_proxy
if no_proxy := proxy_config.no_proxy:
env["NO_PROXY"] = no_proxy
env["no_proxy"] = no_proxy

old_version = execute_command(
[
Expand Down Expand Up @@ -903,7 +890,7 @@ def _start_services(self) -> None:
working_directory=str(self.repo_check_web_service_path),
charm_token=self.service_token,
github_token=self.config["token"],
proxies=self.proxies,
proxies=self._state.proxy_config,
)
self.repo_check_systemd_service.write_text(service_content, encoding="utf-8")

Expand Down
41 changes: 28 additions & 13 deletions src/charm_state.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,16 +121,16 @@ class ProxyConfig(BaseModel):
"""Proxy configuration.
Attributes:
http_proxy: HTTP proxy address.
https_proxy: HTTPS proxy address.
http: HTTP proxy address.
https: HTTPS proxy address.
no_proxy: Comma-separated list of hosts that should not be proxied.
use_aproxy: Whether aproxy should be used.
use_aproxy: Whether aproxy should be used for the runners.
"""

http_proxy: Optional[AnyHttpUrl]
https_proxy: Optional[AnyHttpUrl]
http: Optional[AnyHttpUrl]
https: Optional[AnyHttpUrl]
no_proxy: Optional[str]
use_aproxy: bool
use_aproxy: bool = False

@classmethod
def from_charm(cls, charm: CharmBase) -> "ProxyConfig":
Expand All @@ -147,9 +147,13 @@ def from_charm(cls, charm: CharmBase) -> "ProxyConfig":
https_proxy = get_env_var("JUJU_CHARM_HTTPS_PROXY") or None
no_proxy = get_env_var("JUJU_CHARM_NO_PROXY") or None

# there's no need for no_proxy if there's no http_proxy or https_proxy
if not (https_proxy or http_proxy) and no_proxy:
no_proxy = None

return cls(
http_proxy=http_proxy,
https_proxy=https_proxy,
http=http_proxy,
https=https_proxy,
no_proxy=no_proxy,
use_aproxy=use_aproxy,
)
Expand All @@ -158,7 +162,7 @@ def from_charm(cls, charm: CharmBase) -> "ProxyConfig":
def aproxy_address(self) -> Optional[str]:
"""Return the aproxy address."""
if self.use_aproxy:
proxy_address = self.http_proxy or self.https_proxy
proxy_address = self.http or self.https
# assert is only used to make mypy happy
assert proxy_address is not None # nosec for [B101:assert_used]
aproxy_address = f"{proxy_address.host}:{proxy_address.port}"
Expand All @@ -170,13 +174,24 @@ def aproxy_address(self) -> Optional[str]:
@classmethod
def check_fields(cls, values: dict) -> dict:
"""Validate the proxy configuration."""
if values.get("use_aproxy") and not (
values.get("http_proxy") or values.get("https_proxy")
):
raise ValueError("aproxy requires http_proxy or https_proxy to be set")
if values.get("use_aproxy") and not (values.get("http") or values.get("https")):
raise ValueError("aproxy requires http or https to be set")

return values

def __bool__(self):
"""Return whether we have a proxy config."""
return bool(self.http or self.https)

class Config: # pylint: disable=too-few-public-methods
"""Pydantic model configuration.
Attributes:
allow_mutation: Whether the model is mutable.
"""

allow_mutation = False


class UnsupportedArchitectureError(Exception):
"""Raised when given machine charm architecture is unsupported.
Expand Down
22 changes: 9 additions & 13 deletions src/runner.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,6 @@ def __init__(

self._shared_fs: Optional[shared_fs.SharedFilesystem] = None

# If the proxy setting are set, then add NO_PROXY local variables.
if self.config.proxies.get("http") or self.config.proxies.get("https"):
if self.config.proxies.get("no_proxy"):
self.config.proxies["no_proxy"] += ","
else:
self.config.proxies["no_proxy"] = ""
self.config.proxies["no_proxy"] += f"{self.config.name},.svc"

def create(self, config: CreateRunnerConfig):
"""Create the runner instance on LXD and register it on GitHub.
Expand Down Expand Up @@ -630,9 +622,13 @@ def _configure_docker_proxy(self):
docker_client_proxy = {
"proxies": {
"default": {
"httpProxy": self.config.proxies["http"],
"httpsProxy": self.config.proxies["https"],
"noProxy": self.config.proxies["no_proxy"],
key: value
for key, value in (
("httpProxy", self.config.proxies.http),
("httpsProxy", self.config.proxies.https),
("noProxy", self.config.proxies.no_proxy),
)
if value
}
}
}
Expand Down Expand Up @@ -708,8 +704,8 @@ def _configure_runner(self) -> None:
self.instance.execute(["systemctl", "restart", "docker"])

if self.config.proxies:
if self.config.proxies.get("aproxy_address"):
self._configure_aproxy(self.config.proxies["aproxy_address"])
if aproxy_address := self.config.proxies.aproxy_address:
self._configure_aproxy(aproxy_address)
else:
self._configure_docker_proxy()

Expand Down
Loading

0 comments on commit 3ef9b0e

Please sign in to comment.