Skip to content

Limit aproxy nftables redirection to egress traffic only #807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
weiiwang01 wants to merge 1 commit into
base: main
Choose a base branch
from
+12 −1
Open

Limit aproxy nftables redirection to egress traffic only #807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

This changelog documents user-relevant changes to the GitHub runner charm.

## 2026-07-14

- aproxy now only redirects traffic leaving the runner's default-route interface(s), so local traffic is no longer captured and private ranges no longer need to be added to `aproxy-exclude-addresses`.

## 2026-06-26

- Fixed runners whose cloud VM entered an error state being kept until the creation timeout (~23 minutes) before cleanup. Such VMs are now cleaned up immediately, freeing the slot for a replacement runner.
Expand Down
2 changes: 1 addition & 1 deletion github-runner-manager/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

[project]
name = "github-runner-manager"
version = "0.18.2"
version = "0.18.3"
authors = [
{ name = "Canonical IS DevOps", email = "is-devops-team@canonical.com" },
]
Expand Down
3 changes: 3 additions & 0 deletions github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ snap install aproxy --edge
snap set aproxy proxy={{ aproxy_address }} listen=:54969
cat << EOF > /etc/nftables.conf
define default-ipv4 = $(ip route get $(ip route show 0.0.0.0/0 | grep -oP 'via \K\S+') | grep -oP 'src \K\S+')

@weiiwang01 weiiwang01 Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This reply was written by an AI agent on behalf of the PR author.

I empirically tested this on Ubuntu 24.04 (iproute2 6.1.0) with two default routes configured. The pipeline does not fail under set -e: ip route get <gw1> <gw2> returns exit 0 and define default-ipv4 resolves to a valid address, so cloud-init is not broken.

That said, the underlying point is fair: with multiple default routes the current expression silently uses the last gateway, which is non-deterministic. Adding head -n1 (or sort-ing by metric) is a reasonable robustness/determinism improvement even though it isn't a correctness bug.

define nics = { $(ip route show 0.0.0.0/0 | grep -oP 'dev \K\S+' | sort -u | sed 's/.*/"&"/' | paste -sd,) }
table ip aproxy
flush table ip aproxy
table ip aproxy {
Expand All @@ -25,10 +26,12 @@ table ip aproxy {
}
chain prerouting {
type nat hook prerouting priority dstnat; policy accept;
fib daddr oifname != \$nics return
ip daddr != @exclude tcp dport { {{ aproxy_redirect_ports }} } counter dnat to \$default-ipv4:54969
}
chain output {
type nat hook output priority -100; policy accept;
oifname != \$nics return

@weiiwang01 weiiwang01 Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This reply was written by an AI agent on behalf of the PR author.

I tested this claim directly and it does not hold on the runner kernel (Ubuntu 24.04). Two findings:

  1. oifname is correctly populated in the nat output hook (priority -100). A probe rule oifname "vhost" ip daddr 10.200.0.2 counter matched (packets 1) for traffic egressing a non-uplink interface, and uplink traffic (egress enp5s0) was correctly DNAT'd while non-uplink traffic returned. If oifname were unset/empty, oifname != $nics would always be true and nothing would ever be redirected — but redirection works.

  2. The suggested replacement fib daddr oifname does not even load in the output hook: Error: Could not process rule: Operation not supported. It only works in prerouting.

This asymmetry is intentional: in prerouting the packet is not yet routed, so a FIB lookup (fib daddr oifname) is needed to predict egress; in output the packet is already routed, so plain oifname is available (and fib daddr oifname is EOPNOTSUPP there). The current implementation is correct as written.

ip daddr != @exclude tcp dport { {{ aproxy_redirect_ports }} } counter dnat to \$default-ipv4:54969
}
}
Expand Down
4 changes: 4 additions & 0 deletions github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,12 @@ def runner_metrics_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock:
}
chain prerouting {
type nat hook prerouting priority dstnat; policy accept;
fib daddr oifname != \\$nics return
ip daddr != @exclude tcp dport { 80, 443 } counter dnat to \\$default-ipv4:54969
}
chain output {
type nat hook output priority -100; policy accept;
oifname != \\$nics return

@weiiwang01 weiiwang01 Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This reply was written by an AI agent on behalf of the PR author.

This depends on the fib daddr oifname suggestion for the output chain (see the template thread). I verified that fib daddr oifname is Operation not supported in the nat output hook, and that plain oifname is correctly set there. So the expected test output matching oifname != $nics return reflects the correct, loadable ruleset — no change needed.

ip daddr != @exclude tcp dport { 80, 443 } counter dnat to \\$default-ipv4:54969
}
}
Expand All @@ -123,10 +125,12 @@ def runner_metrics_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock:
}
chain prerouting {
type nat hook prerouting priority dstnat; policy accept;
fib daddr oifname != \\$nics return
ip daddr != @exclude tcp dport { 80, 443 } counter dnat to \\$default-ipv4:54969
}
chain output {
type nat hook output priority -100; policy accept;
oifname != \\$nics return

@weiiwang01 weiiwang01 Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This reply was written by an AI agent on behalf of the PR author.

Same as the L108 thread: fib daddr oifname is not supported in the nat output hook (Operation not supported), while plain oifname is correctly populated there. The expected oifname != $nics return in the test matches the correct implementation, so no change is needed.

ip daddr != @exclude tcp dport { 80, 443 } counter dnat to \\$default-ipv4:54969
}
}
Expand Down
Loading