Skip to content

Commit 615bf44

Browse files
edimarlnxclaude
andcommitted
Separate deciding which firewall to drive from reaching for it
The detection tests stub what the host reports and then called Select, which also builds the backend — so on a machine with neither firewall installed they failed on the constructor rather than on the decision they were testing. That is CI, and it was right to complain: the tests were asserting through a step they did not mean to exercise. Resolve now answers "which firewall does this machine run", Select builds what Resolve named. The two questions are genuinely different, and only the first one can be answered on a machine that has neither. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0149Bf2QcorehfhYNNg27h5j
1 parent 2e081b0 commit 615bf44

2 files changed

Lines changed: 71 additions & 28 deletions

File tree

‎internal/backends/select.go‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -119,18 +119,30 @@ func unitIs(verb, unit string) bool {
119119
// rather than a supported setup, so the tie-break exists to be predictable
120120
// and explainable, not to be clever about it.
121121
func Select(cfg config.Config) (firewall.Backend, error) {
122+
name, err := Resolve(cfg)
123+
if err != nil {
124+
return nil, err
125+
}
126+
return build(name, cfg)
127+
}
128+
129+
// Resolve names the backend Select will build, without building it. It is
130+
// separate because deciding *which* firewall this machine runs and reaching
131+
// for its binary are two different questions, and only the first one can be
132+
// answered — or tested — on a machine that has neither.
133+
func Resolve(cfg config.Config) (string, error) {
122134
switch name := cfg.String(KeyBackend, BackendAuto); name {
123135
case BackendUFW, BackendFirewalld:
124-
return build(name, cfg)
136+
return name, nil
125137
case BackendAuto:
126-
return selectAuto(cfg)
138+
return resolveAuto()
127139
default:
128-
return nil, fmt.Errorf("unknown backend %q", name)
140+
return "", fmt.Errorf("unknown backend %q", name)
129141
}
130142
}
131143

132-
// selectAuto implements the detection described on Select.
133-
func selectAuto(cfg config.Config) (firewall.Backend, error) {
144+
// resolveAuto implements the detection described on Select.
145+
func resolveAuto() (string, error) {
134146
var installed, enabled []string
135147
for _, name := range preference {
136148
probe := probes[name]
@@ -139,7 +151,7 @@ func selectAuto(cfg config.Config) (firewall.Backend, error) {
139151
}
140152
installed = append(installed, name)
141153
if probe.Active() {
142-
return build(name, cfg)
154+
return name, nil
143155
}
144156
if probe.Enabled() {
145157
enabled = append(enabled, name)
@@ -148,14 +160,14 @@ func selectAuto(cfg config.Config) (firewall.Backend, error) {
148160
if len(enabled) > 0 {
149161
// Nothing is running, but systemd would start this one at boot: it is
150162
// the firewall this machine is configured to use.
151-
return build(enabled[0], cfg)
163+
return enabled[0], nil
152164
}
153165
if len(installed) > 0 {
154166
// Nothing is running or enabled: fall back to the first installed
155167
// backend so the user can at least inspect it.
156-
return build(installed[0], cfg)
168+
return installed[0], nil
157169
}
158-
return nil, fmt.Errorf(
170+
return "", fmt.Errorf(
159171
"no supported firewall found; install ufw " +
160172
"(apt install ufw / pacman -S ufw) or firewalld " +
161173
"(dnf install firewalld), pick one with `backend = \"...\"` " +

‎internal/backends/select_test.go‎

Lines changed: 50 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -37,57 +37,57 @@ func autoConfig() config.Config {
3737
return config.Config{Values: map[string]string{KeyBackend: BackendAuto}}
3838
}
3939

40-
func TestSelectAutoPrefersTheActiveBackend(t *testing.T) {
40+
func TestResolveAutoPrefersTheActiveBackend(t *testing.T) {
4141
// firewalld installed and running, ufw installed but stopped: the running
4242
// one wins even though ufw comes first in the preference order.
4343
stubProbes(t,
4444
probeState{installed: true},
4545
probeState{installed: true, active: true, enabled: true})
4646

47-
backend, err := Select(autoConfig())
47+
name, err := Resolve(autoConfig())
4848
if err != nil {
49-
t.Fatalf("Select: %v", err)
49+
t.Fatalf("Resolve: %v", err)
5050
}
51-
if backend.Name() != BackendFirewalld {
52-
t.Errorf("Name = %q, want firewalld", backend.Name())
51+
if name != BackendFirewalld {
52+
t.Errorf("Resolve = %q, want firewalld", name)
5353
}
5454
}
5555

56-
func TestSelectAutoPrefersTheEnabledUnit(t *testing.T) {
56+
func TestResolveAutoPrefersTheEnabledUnit(t *testing.T) {
5757
// Both installed, neither running: the one systemd would start at boot is
5858
// the one this machine is configured to use, even though ufw is first in
5959
// the preference order.
6060
stubProbes(t,
6161
probeState{installed: true},
6262
probeState{installed: true, enabled: true})
6363

64-
backend, err := Select(autoConfig())
64+
name, err := Resolve(autoConfig())
6565
if err != nil {
66-
t.Fatalf("Select: %v", err)
66+
t.Fatalf("Resolve: %v", err)
6767
}
68-
if backend.Name() != BackendFirewalld {
69-
t.Errorf("Name = %q, want firewalld", backend.Name())
68+
if name != BackendFirewalld {
69+
t.Errorf("Resolve = %q, want firewalld", name)
7070
}
7171
}
7272

73-
func TestSelectAutoFallsBackToInstalled(t *testing.T) {
73+
func TestResolveAutoFallsBackToInstalled(t *testing.T) {
7474
// Nothing is running and nothing is enabled: the first installed backend
7575
// is used so the user can look at it.
7676
stubProbes(t, probeState{}, probeState{installed: true})
7777

78-
backend, err := Select(autoConfig())
78+
name, err := Resolve(autoConfig())
7979
if err != nil {
80-
t.Fatalf("Select: %v", err)
80+
t.Fatalf("Resolve: %v", err)
8181
}
82-
if backend.Name() != BackendFirewalld {
83-
t.Errorf("Name = %q, want firewalld", backend.Name())
82+
if name != BackendFirewalld {
83+
t.Errorf("Resolve = %q, want firewalld", name)
8484
}
8585
}
8686

87-
func TestSelectAutoWithoutAnyFirewall(t *testing.T) {
87+
func TestResolveAutoWithoutAnyFirewall(t *testing.T) {
8888
stubProbes(t, probeState{}, probeState{})
8989

90-
_, err := Select(autoConfig())
90+
_, err := Resolve(autoConfig())
9191
if err == nil {
9292
t.Fatal("expected an error when no firewall is installed")
9393
}
@@ -98,14 +98,45 @@ func TestSelectAutoWithoutAnyFirewall(t *testing.T) {
9898
}
9999
}
100100

101-
func TestSelectUnknownBackend(t *testing.T) {
102-
if _, err := Select(config.Config{
101+
func TestResolveUnknownBackend(t *testing.T) {
102+
if _, err := Resolve(config.Config{
103103
Values: map[string]string{KeyBackend: "iptables"},
104104
}); err == nil {
105105
t.Error("expected an error for an unknown backend")
106106
}
107107
}
108108

109+
func TestResolveHonoursAnExplicitChoice(t *testing.T) {
110+
// An explicit choice skips detection entirely: the user said which
111+
// firewall this machine runs, and the tool does not argue.
112+
stubProbes(t, probeState{installed: true, active: true}, probeState{})
113+
name, err := Resolve(config.Config{
114+
Values: map[string]string{KeyBackend: BackendFirewalld},
115+
})
116+
if err != nil {
117+
t.Fatalf("Resolve: %v", err)
118+
}
119+
if name != BackendFirewalld {
120+
t.Errorf("Resolve = %q, want firewalld", name)
121+
}
122+
}
123+
124+
func TestSelectBuildsTheResolvedBackend(t *testing.T) {
125+
// Select reaches for the binary, which a test machine may not have; what
126+
// matters either way is that it reports the backend Resolve named, and
127+
// says so legibly when that binary is missing.
128+
stubProbes(t, probeState{installed: true, active: true}, probeState{})
129+
backend, err := Select(autoConfig())
130+
switch {
131+
case err != nil:
132+
if !strings.Contains(err.Error(), "ufw") {
133+
t.Errorf("error should name the backend it could not build: %v", err)
134+
}
135+
case backend.Name() != BackendUFW:
136+
t.Errorf("Name = %q, want ufw", backend.Name())
137+
}
138+
}
139+
109140
func TestInspectReportsEveryBackend(t *testing.T) {
110141
// The detector must describe the backend it did not pick, so `--check`
111142
// can say "firewalld is here but stopped" instead of staying silent.

0 commit comments

Comments
 (0)