Skip to content

Limit aproxy nftables redirection to egress traffic only#807

Open
weiiwang01 wants to merge 1 commit into
mainfrom
feat/aproxy-nftables
Open

Limit aproxy nftables redirection to egress traffic only#807
weiiwang01 wants to merge 1 commit into
mainfrom
feat/aproxy-nftables

Conversation

@weiiwang01

Copy link
Copy Markdown
Collaborator

What this PR does

Update the aproxy transparent proxy nftables rules so they only redirect traffic leaving the runner's default-route ("physical") interfaces.

Why we need it

This can simplify the aproxy-exclude-addresses configuration, as we don't need to include the private IP ranges by default, and we only need to include the actual IP addresses that we need to exclude. Also, this would help reduce the chances of IP range conflicts between internal services, like MicroK8s, Docker, and LXD, and external services within the infrastructure using private IP addresses.

Checklist

  • I followed the contributing guide
  • I added or updated the documentation (if applicable)
  • I updated docs/changelog.md with user-relevant changes
  • I used AI to assist with preparing this PR
  • I added or updated tests as needed (unit and integration)
  • If this is a Grafana dashboard: I added a screenshot of the dashboard
  • If this is Terraform: terraform fmt passes and tflint reports no errors
  • If the github-runner-manager application has been changed: The application version number is updated in github-runner-manager/pyproject.toml.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the OpenStack runner cloud-init template to scope aproxy’s transparent-proxy nftables DNAT rules to traffic that will egress via the runner’s default-route interface(s), reducing the need to exclude broad private IP ranges and avoiding local/overlay traffic being captured.

Changes:

  • Add default-route interface filtering to aproxy nftables prerouting and output chains.
  • Update unit-test expected nftables snippets to match the new rules.
  • Bump github-runner-manager version and document the user-facing change in docs/changelog.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
github-runner-manager/src/github_runner_manager/templates/openstack-userdata.sh.j2 Adds default-route NIC detection and applies interface-based filtering to aproxy nftables redirection.
github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py Updates expected nftables rules in unit tests to match the new template output.
github-runner-manager/pyproject.toml Version bump from 0.18.2 to 0.18.3.
docs/changelog.md Adds a 2026-07-14 entry describing the aproxy redirection behavior change.

@@ -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+')

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.

}
chain output {
type nat hook output priority -100; policy accept;
oifname != \\$nics return

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.

}
chain output {
type nat hook output priority -100; policy accept;
oifname != \\$nics return

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.

}
chain output {
type nat hook output priority -100; policy accept;
oifname != \$nics return

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants