Conversation
…/installer/extfw Pre-PR-23 blocker #2 of 5 remaining. Replaces two divergent detection surfaces with one canonical `extfw.Detect(exec, log)` shared across install, update, and uninstall lifecycle paths. **Option A resolution locked** (per authorization 2026-04-20): `/etc/csf/csf.conf` is a valid CSF signal for ALL callers — not just uninstall. This intentionally makes install-side detection stricter for hosts with CSF config remnants so install/update/uninstall share one external-firewall truth surface. **Locked precedence** (frozen): ufw → firewalld → iptables → csf. This order is used ONLY when collapsing to a single Authoritative answer; when multiple firewalls are observed active simultaneously, the result is Ambiguous and the caller is responsible for handling that state explicitly. No silent collapse. ## New package: internal/installer/extfw - `Name` enum (ufw / firewalld / iptables / csf + none sentinel) - `Precedence` frozen order constant - `Observation` (Name / Source / Unit / Detail) — one signal per entry - `DetectionResult` (Observations / Active / Authoritative / Ambiguous) - `Detect(exec, log)` canonical shared detection - `Observation.DisplayName()` preserves legacy "UFW"/"CSF"/"iptables-nft" display strings so existing consumers don't drift ## Signal set (union of pre-unification signals — no new heuristics) | Firewall | Signals | |---|---| | UFW | `ufw.service` active | | firewalld | `firewalld.service` OR ghost nft table containing "firewalld" | | iptables | `iptables.service` OR iptables-save ≥3 non-comment rule lines OR ghost nft table named filter/nat/mangle | | CSF | `csf.service` OR `lfd.service` OR `/etc/csf/csf.conf` present | Nothing added. Nothing removed. Union. ## Migration - `internal/installer/detect/conflicts.go` → thin adapter over `extfw.Detect`. `DetectConflicts()` / `ConflictNames()` / `Conflict` struct API preserved; CSF still emits two conflict entries (one per unit) for takeover-time stop+disable+mask. - `internal/installer/uninstall/authority.go` → removed local `detectExternalAuthority` + `hasActiveIptables` + `countNonCommentLines` helpers. `Classify` now calls `extfw.Detect` and routes ambiguous multi-external hosts through the new `AuthorityAmbiguous` branch rather than silently picking one via precedence. Test fixtures updated: bare `mock.Services["ufw"]` → `ufw.service` (canonical systemd unit name). - Existing consumers (`phases.go`, `switchop.DisableConflicts`, `lifecycle_bridge`) unchanged — they see the same `[]detect.Conflict` shape. ## Tests - `extfw/detect_test.go` — single-firewall cases (4), no-firewall case, multi-firewall ambiguous case with "no silent collapse" assertion, all-four-active case, same-firewall-multi-signal-not- ambiguous case, precedence-order-frozen regression guard, DisplayName legacy-shape tests. - `extfw/consistency_test.go` — PR-P2-2 cross-caller regression guard. For each fixture (clean / ufw / firewalld / iptables / csf-services / csf-config-only / ufw+csf-ambiguous), asserts `extfw.Detect`, `detect.DetectConflicts`, and `uninstall.Classify` all see the same external-firewall truth. One dedicated test locks the Option A resolution specifically — csf-config-only remnant must be seen by install side as well. ## Explicit non-goals (scope-lock) - NO new firewall types - NO heuristics beyond current signals - NO behavior expansion - NO silent conflict resolution ## Test plan - `Build & Test` green — new extfw unit tests + consistency tests pass; existing detect/conflicts_test.go + uninstall tests pass under adapter semantics - Install / Update / Uninstall Canonization matrices green — no CI-gate regressions - Cross-caller consistency proven by `extfw.consistency_test.go` Refs: internal/installer/uninstall/contract.md §"Pre-PR-23 blockers" Authorization: locked Option A resolution (2026-04-20) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
…iling colon
MockExecutor.lookupResult builds its key as `name + ":" + strings.Join(args, ":")`.
For exec.Run("iptables-save") with no args, that's "iptables-save:"
(with trailing colon). My fixtures used "iptables-save" without the
colon — lookups missed, fell through to the default
`Result{ExitCode: 0, Stdout: ""}`, so hasActiveIptablesRules returned
false and the detector reported Authoritative="".
TestDetect_None happened to pass because "no firewall" is the default
anyway; TestDetect_Iptables_AllThreeSignals (rule-count case) and
TestConsistency_InstallAndUninstallAgree/iptables-save_rules_present
both failed against empty stdout.
Fix: update every `RunResults["iptables-save"]` → `RunResults["iptables-save:"]`
in detect_test.go + consistency_test.go. No production code change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
itcmsgr
added a commit
that referenced
this pull request
Apr 20, 2026
Pre-PR-23 assurance blocker #3 of 5 remaining. Adds system-level before/after truth checks to every dry-run CI path so a future regression that mutates nftables tables or firewall service states without touching tracked files is caught at gate time. ## Scope (locked per authorization 2026-04-20) - Before/after `nft list tables` sorted diff — hard-assert equal - Before/after `systemctl is-active` for the 6 lifecycle-relevant units (nftband + 5 external firewalls) — hard-assert equal - No `|| true` on the diff checks - Covered paths: install dry-run refusal, update dry-run, uninstall dry-run (explicit + implicit) ## Implementation - NEW: scripts/ci-snapshot-kernel-service.sh — reusable helper that emits a stable, sorted snapshot. Degrades gracefully (both sides return the same placeholder) when nft or systemctl aren't available (e.g. almalinux-9 container without systemd). Contract is: * purely read-only probes * never invokes nft/systemctl with mutation verbs * never writes to the filesystem * exit 0 always — caller decides whether differences fail - EXTENDED: all 3 canonization workflows * ci-install-canonization.yml / G3-IN-REFUSE-DRY-RUN * ci-update-canonization.yml / G3-U3 * ci-uninstall-canonization.yml / G3-UN-PLAN-RENDERS Each takes a snapshot before the dry-run invocation and hard- asserts byte-identical equality after. ## Monitored units (must match extfw.Detect's signal set) nftband.service ufw.service firewalld.service csf.service lfd.service iptables.service Kept in lockstep with internal/installer/extfw/detect.go so CI and production code agree on "what counts as a firewall service." ## Non-goals (scope-lock) - NO code-path redesign - NO strace/exec tracing yet (deferred to PR-P2-4) - NO mutation behavior changes - NO new firewall-unit additions to the signal set ## Also: tracking update Marks blocker #2 (external-firewall detection unification, PR #486 / 49d98fc) as LANDED in the contract blocker table. Remaining: 4 Phase 2 PRs before PR-23. Refs: internal/installer/uninstall/contract.md §"Pre-PR-23 blockers" Authorization: locked Phase 2 sequencing (2026-04-20) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
itcmsgr
added a commit
that referenced
this pull request
Apr 20, 2026
…OT) (#487) Pre-PR-23 assurance blocker #3 of 5 remaining. Adds system-level before/after truth checks to every dry-run CI path so a future regression that mutates nftables tables or firewall service states without touching tracked files is caught at gate time. ## Scope (locked per authorization 2026-04-20) - Before/after `nft list tables` sorted diff — hard-assert equal - Before/after `systemctl is-active` for the 6 lifecycle-relevant units (nftband + 5 external firewalls) — hard-assert equal - No `|| true` on the diff checks - Covered paths: install dry-run refusal, update dry-run, uninstall dry-run (explicit + implicit) ## Implementation - NEW: scripts/ci-snapshot-kernel-service.sh — reusable helper that emits a stable, sorted snapshot. Degrades gracefully (both sides return the same placeholder) when nft or systemctl aren't available (e.g. almalinux-9 container without systemd). Contract is: * purely read-only probes * never invokes nft/systemctl with mutation verbs * never writes to the filesystem * exit 0 always — caller decides whether differences fail - EXTENDED: all 3 canonization workflows * ci-install-canonization.yml / G3-IN-REFUSE-DRY-RUN * ci-update-canonization.yml / G3-U3 * ci-uninstall-canonization.yml / G3-UN-PLAN-RENDERS Each takes a snapshot before the dry-run invocation and hard- asserts byte-identical equality after. ## Monitored units (must match extfw.Detect's signal set) nftband.service ufw.service firewalld.service csf.service lfd.service iptables.service Kept in lockstep with internal/installer/extfw/detect.go so CI and production code agree on "what counts as a firewall service." ## Non-goals (scope-lock) - NO code-path redesign - NO strace/exec tracing yet (deferred to PR-P2-4) - NO mutation behavior changes - NO new firewall-unit additions to the signal set ## Also: tracking update Marks blocker #2 (external-firewall detection unification, PR #486 / 49d98fc) as LANDED in the contract blocker table. Remaining: 4 Phase 2 PRs before PR-23. Refs: internal/installer/uninstall/contract.md §"Pre-PR-23 blockers" Authorization: locked Phase 2 sequencing (2026-04-20) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-PR-23 blocker #2 of 5 remaining. Replaces two divergent detection surfaces with one canonical `extfw.Detect(exec, log)` shared across install, update, and uninstall lifecycle paths.
Option A resolution (locked per authorization 2026-04-20)
Unified CSF signal (used by ALL callers): `csf.service` active OR `lfd.service` active OR `/etc/csf/csf.conf` present.
Before PR-P2-2: install side ignored `/etc/csf/csf.conf`; uninstall side used it. Two lifecycle modes saw different ground truth for the same host — the exact drift this PR exists to eliminate.
Locked precedence
ufw → firewalld → iptables → csf (frozen). Used ONLY when collapsing to a single Authoritative answer. Multiple active firewalls → Ambiguous, never silent collapse.
New package
`internal/installer/extfw/detect.go` — canonical detector with `Name` enum, `Precedence` constant, `Observation` / `DetectionResult` structs, `Detect(exec, log)` function.
Signal set (union of pre-unification signals — no new heuristics)
Migration
Tests
`extfw/detect_test.go` — single-firewall cases (4), no-firewall, multi-firewall ambiguous with "no silent collapse" assertion, all-four-active, same-firewall-multi-signal-not-ambiguous, precedence-frozen regression guard, DisplayName legacy-shape.
`extfw/consistency_test.go` — PR-P2-2 cross-caller regression guard. For each fixture (clean / ufw / firewalld / iptables / csf-services / csf-config-only / ufw+csf-ambiguous), asserts `extfw.Detect`, `detect.DetectConflicts`, and `uninstall.Classify` all see the same external-firewall truth. One dedicated test (`TestConsistency_OptionA_CSFConfigFileOnly`) locks the Option A resolution: config-file-only CSF remnant must be seen by install side as well.
Reviewer trap checklist
Explicit non-goals
Test plan
🤖 Generated with Claude Code
DirectAdmin-specific commands — reviewed, intentionally out of scope
During PR-P2-2 design the following DA-specific CSF commands surfaced and were explicitly reviewed:
/usr/local/directadmin/custombuild/build set csf nonftban_remove_csf(shell function incli/lib/nftban/core/nftban_firewall_conflicts.sh)options.conf csf=post-takeover verificationAll four are mutation / remediation surfaces that change system state. PR-P2-2 is detection-only — it unifies how install / update / uninstall lifecycle paths observe external-firewall presence. It does NOT perform CSF removal or any DirectAdmin panel action.
These DA-specific commands therefore stay in their existing location (
internal/installer/switchop/takeover.go, gated onpd.panel == PanelDirectAdmin) and will be revisited by the later PR that owns takeover / removal semantics (PR-25 artifact-removal, or a future DA-panel-aware uninstall path if PR-24 restore logic needs it).Separation rule going forward:
extfw.Detectanswers "is CSF present?" with one canonical signal set, shared across all lifecycle callers.