forgejo_admin/pal-e-platform

Fork 0

fix: reload nftables on tailscaled restart via PartOf= directive #216

Merged

forgejo_admin merged 1 commit from 181-nftables-reload-after-tailscale into main

2026-03-28 05:28:37 +00:00

forgejo_admin commented

2026-03-28 04:58:00 +00:00

Owner

Summary

nftables resolves iif "tailscale0" to a numeric ifindex at load time; when tailscaled restarts mid-uptime the interface gets a new index but nftables keeps the stale one, silently dropping all Tailscale traffic
Adds PartOf=tailscaled.service to the existing systemd drop-in so nftables automatically restarts whenever tailscaled restarts, re-resolving the interface name

Changes

salt/states/firewall/init.sls: Added PartOf=tailscaled.service to the nftables systemd drop-in. Updated comments to document both failure modes (boot ordering and mid-uptime restart) and how each directive addresses them.

Test Plan

Apply highstate: sudo salt-call state.apply firewall
Verify drop-in contents: cat /etc/systemd/system/nftables.service.d/after-tailscale.conf shows all three directives
Verify systemd parsed it: systemctl show nftables | grep PartOf shows PartOf=tailscaled.service
Mid-uptime restart test: sudo systemctl restart tailscaled then sudo nft list ruleset | grep iif shows iif "tailscale0" (not a stale numeric index)
Boot test: reboot, then verify same nft list ruleset | grep iif result

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #181
Discovered during Mac agent setup (#174)
Prior boot-ordering fix: branch 80-feat-fix-nftables-boot-ordering-start-af

## Summary - nftables resolves `iif "tailscale0"` to a numeric ifindex at load time; when tailscaled restarts mid-uptime the interface gets a new index but nftables keeps the stale one, silently dropping all Tailscale traffic - Adds `PartOf=tailscaled.service` to the existing systemd drop-in so nftables automatically restarts whenever tailscaled restarts, re-resolving the interface name ## Changes - `salt/states/firewall/init.sls`: Added `PartOf=tailscaled.service` to the nftables systemd drop-in. Updated comments to document both failure modes (boot ordering and mid-uptime restart) and how each directive addresses them. ## Test Plan - [ ] Apply highstate: `sudo salt-call state.apply firewall` - [ ] Verify drop-in contents: `cat /etc/systemd/system/nftables.service.d/after-tailscale.conf` shows all three directives - [ ] Verify systemd parsed it: `systemctl show nftables | grep PartOf` shows `PartOf=tailscaled.service` - [ ] Mid-uptime restart test: `sudo systemctl restart tailscaled` then `sudo nft list ruleset | grep iif` shows `iif "tailscale0"` (not a stale numeric index) - [ ] Boot test: reboot, then verify same `nft list ruleset | grep iif` result ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #181 - Discovered during Mac agent setup (#174) - Prior boot-ordering fix: branch `80-feat-fix-nftables-boot-ordering-start-af`

forgejo_admin added 1 commit

2026-03-28 04:58:00 +00:00

fix: reload nftables on tailscaled restart via PartOf= directive

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

ci/woodpecker/pull_request_closed/woodpecker Pipeline was successful

Details

e9d3f7f335

nftables resolves iif "tailscale0" to a numeric ifindex at load time.
When tailscaled restarts mid-uptime, it gets a new ifindex but nftables
still has the stale one cached -- Tailscale traffic silently drops.

Add PartOf=tailscaled.service to the existing systemd drop-in so
nftables is automatically restarted whenever tailscaled restarts,
re-resolving the interface name to the current index.

Closes #181

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-28 04:58:39 +00:00

Author

Owner

QA Review

Files reviewed: salt/states/firewall/init.sls (1 file, +13/-4)

Correctness

PartOf=tailscaled.service is the correct systemd directive for this problem. It causes nftables to restart when tailscaled restarts. Combined with the existing After=tailscaled.service, the restart ordering is correct: tailscaled finishes starting first (interface exists with new ifindex), then nftables restarts (re-resolves iif "tailscale0" to current ifindex).
No Salt state logic changed beyond the drop-in contents and comments. Existing requisite chains (daemon-reload, service.enabled, cmd.wait) remain intact.

Edge case noted (informational, not blocking)

PartOf= means nftables also stops when tailscaled stops (systemctl stop tailscaled). This leaves the host with no firewall. Acceptable because: (a) stopping tailscaled is a deliberate operator action, (b) the nftables rules referencing tailscale0 are meaningless without the interface, and (c) the next systemctl start tailscaled will bring nftables back via Wants=.

SOP compliance

Single focused change, no scope creep
No secrets in diff
PR body follows template with all required sections
Closes #181 present for auto-close
Test plan covers both Salt apply and systemd runtime verification
Comments explain the "why" clearly

Nits

None.

VERDICT: APPROVE

## QA Review **Files reviewed:** `salt/states/firewall/init.sls` (1 file, +13/-4) ### Correctness - `PartOf=tailscaled.service` is the correct systemd directive for this problem. It causes nftables to restart when tailscaled restarts. Combined with the existing `After=tailscaled.service`, the restart ordering is correct: tailscaled finishes starting first (interface exists with new ifindex), then nftables restarts (re-resolves `iif "tailscale0"` to current ifindex). - No Salt state logic changed beyond the drop-in contents and comments. Existing requisite chains (`daemon-reload`, `service.enabled`, `cmd.wait`) remain intact. ### Edge case noted (informational, not blocking) `PartOf=` means nftables also stops when tailscaled stops (`systemctl stop tailscaled`). This leaves the host with no firewall. Acceptable because: (a) stopping tailscaled is a deliberate operator action, (b) the nftables rules referencing `tailscale0` are meaningless without the interface, and (c) the next `systemctl start tailscaled` will bring nftables back via `Wants=`. ### SOP compliance - [x] Single focused change, no scope creep - [x] No secrets in diff - [x] PR body follows template with all required sections - [x] `Closes #181` present for auto-close - [x] Test plan covers both Salt apply and systemd runtime verification - [x] Comments explain the "why" clearly ### Nits None. --- **VERDICT: APPROVE**

forgejo_admin commented

2026-03-28 05:00:18 +00:00

Author

Owner

PR #216 Review

DOMAIN REVIEW

Tech stack: Salt (SaltStack state files) + systemd drop-in configuration.

Salt compliance:

State file structure is correct: file.managed with contents block for the drop-in.
No pillar changes needed -- the drop-in is a static systemd unit override, not data-driven.
The existing cmd.wait on nftables-daemon-reload already watches nftables-after-tailscale, so adding a new line to the drop-in contents will trigger systemctl daemon-reload automatically. Requisite chain is correct.

systemd semantics:

PartOf=tailscaled.service is the correct directive. It creates a restart/stop coupling: when tailscaled is restarted or stopped, nftables will also be restarted or stopped. This is exactly the desired behavior for re-resolving the ifindex.
The three directives work together correctly: After= (ordering), Wants= (soft dependency pull-in), PartOf= (lifecycle coupling). No conflicts or redundancy.
Verified that iif "tailscale0" is rendered by nftables.conf.j2 via allowed_interfaces in salt/pillar/firewall.sls, confirming the ifindex caching risk described in the PR.

Comment quality:

The updated block comment documents both failure modes (boot ordering and mid-uptime restart) with clear cause-and-fix pairings. Excellent operational documentation for future maintainers.

BLOCKERS

None.

The "test coverage" BLOCKER criterion does not apply here. This is a 1-line systemd directive addition to a Salt state. There is no test infrastructure for Salt states in this repo (validation is manual highstate + systemctl verification, as documented in the Test Plan). The PR's Test Plan provides 5 concrete manual verification steps covering the exact behavior change.

NITS

Stale header comment (line 3 on main): The file header references Phase 3 of plan-2026-02-26-salt-host-management. Plans are a deprecated concept per project conventions. Not introduced by this PR, so not blocking, but noted for future cleanup.

SOP COMPLIANCE

Branch named after issue: 181-nftables-reload-after-tailscale references issue #181
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
Related section references parent issue: Closes #181, cross-references #174 (discovery context) and prior boot-ordering branch
No secrets committed: single file change, no credentials
No unnecessary file changes: 1 file, 1 concern, zero scope creep
Commit messages are descriptive: fix: reload nftables on tailscaled restart via PartOf= directive

Minor SOP note: The PR body uses ## Related Notes instead of ## Related, and ## Review Checklist is not part of the standard template. Neither is blocking -- the substance is all there.

PROCESS OBSERVATIONS

Change failure risk: Low. The PartOf= directive is additive to existing behavior. If tailscaled does not restart, this directive has zero effect. The existing boot-ordering behavior is unchanged.
MTTR impact: Positive. This prevents a silent failure mode where firewall rules stop matching Tailscale traffic after a tailscaled restart, which would be extremely difficult to diagnose without knowledge of the ifindex caching behavior.
Deployment: Requires sudo salt-call state.apply firewall on the target host. Not CI-deployed. Test plan is thorough.

VERDICT: APPROVED

## PR #216 Review ### DOMAIN REVIEW **Tech stack**: Salt (SaltStack state files) + systemd drop-in configuration. **Salt compliance:** - State file structure is correct: `file.managed` with `contents` block for the drop-in. - No pillar changes needed -- the drop-in is a static systemd unit override, not data-driven. - The existing `cmd.wait` on `nftables-daemon-reload` already watches `nftables-after-tailscale`, so adding a new line to the drop-in contents will trigger `systemctl daemon-reload` automatically. Requisite chain is correct. **systemd semantics:** - `PartOf=tailscaled.service` is the correct directive. It creates a restart/stop coupling: when tailscaled is restarted or stopped, nftables will also be restarted or stopped. This is exactly the desired behavior for re-resolving the ifindex. - The three directives work together correctly: `After=` (ordering), `Wants=` (soft dependency pull-in), `PartOf=` (lifecycle coupling). No conflicts or redundancy. - Verified that `iif "tailscale0"` is rendered by `nftables.conf.j2` via `allowed_interfaces` in `salt/pillar/firewall.sls`, confirming the ifindex caching risk described in the PR. **Comment quality:** - The updated block comment documents both failure modes (boot ordering and mid-uptime restart) with clear cause-and-fix pairings. Excellent operational documentation for future maintainers. ### BLOCKERS None. The "test coverage" BLOCKER criterion does not apply here. This is a 1-line systemd directive addition to a Salt state. There is no test infrastructure for Salt states in this repo (validation is manual highstate + systemctl verification, as documented in the Test Plan). The PR's Test Plan provides 5 concrete manual verification steps covering the exact behavior change. ### NITS 1. **Stale header comment** (line 3 on `main`): The file header references `Phase 3 of plan-2026-02-26-salt-host-management`. Plans are a deprecated concept per project conventions. Not introduced by this PR, so not blocking, but noted for future cleanup. ### SOP COMPLIANCE - [x] Branch named after issue: `181-nftables-reload-after-tailscale` references issue #181 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] Related section references parent issue: `Closes #181`, cross-references #174 (discovery context) and prior boot-ordering branch - [x] No secrets committed: single file change, no credentials - [x] No unnecessary file changes: 1 file, 1 concern, zero scope creep - [x] Commit messages are descriptive: `fix: reload nftables on tailscaled restart via PartOf= directive` **Minor SOP note**: The PR body uses `## Related Notes` instead of `## Related`, and `## Review Checklist` is not part of the standard template. Neither is blocking -- the substance is all there. ### PROCESS OBSERVATIONS - **Change failure risk**: Low. The `PartOf=` directive is additive to existing behavior. If tailscaled does not restart, this directive has zero effect. The existing boot-ordering behavior is unchanged. - **MTTR impact**: Positive. This prevents a silent failure mode where firewall rules stop matching Tailscale traffic after a tailscaled restart, which would be extremely difficult to diagnose without knowledge of the ifindex caching behavior. - **Deployment**: Requires `sudo salt-call state.apply firewall` on the target host. Not CI-deployed. Test plan is thorough. ### VERDICT: APPROVED