Refuse an opening ufw would merge into a found rule that does other than a plain allow, and read log types in either place (hq ADR 0103)

internal/firewall/firewall.go

@@ -479,6 +479,7 @@ func words(rule string) []string {
 type ufwRule struct {
 	route              bool
 	action, in, out    string
+	log                string
 	from, fromPort, to string
 	port, proto, app   string
 	comment            string
@@ -488,8 +489,23 @@ type ufwRule struct {
 // in on mesh0 to any port 5432 proto tcp` — into the fields ufw compares. Not ok for anything it
 // does not recognise, which is then never taken to answer an opening.
 func parseRule(rule string) (ufwRule, bool) {
-	w := words(rule)
 	r := ufwRule{from: "any", to: "any", comment: comment(rule)}
+	// A log type may stand after the action or after the direction; either way it is a property
+	// of the rule, not of where it matches. The word after `comment` is the comment, whatever it
+	// says.
+	var w []string
+	all := words(rule)
+	for i := 0; i < len(all); i++ {
+		switch {
+		case all[i] == "comment" && i+1 < len(all):
+			w = append(w, all[i], all[i+1])
+			i++
+		case all[i] == "log" || all[i] == "log-all":
+			r.log = all[i]
+		default:
+			w = append(w, all[i])
+		}
+	}
 	i := 0
 	if i < len(w) && w[i] == "route" {
 		r.route = true
@@ -585,9 +601,13 @@ func isPorts(s string) bool {
 	return true
 }
 
-// sameAs is whether ufw would take two rules for one — everything but the comment equal.
+// sameAs is whether ufw would take two rules for one: everything equal but the comment, the
+// action and the log type. Adding one beside the other updates it in place — its comment, and its
+// action or log type — rather than adding a second.
 func (r ufwRule) sameAs(o ufwRule) bool {
 	r.comment, o.comment = "", ""
+	r.action, o.action = "", ""
+	r.log, o.log = "", ""
 	return r == o
 }
 
@@ -658,6 +678,7 @@ func Converge(ctx context.Context, run Runner, o *declaration.Opening) (Converge
 	present := false
 	var stale []string
 	satisfiedBy := ""
+	want, _ := parseRule(strings.Join(Rule(o), " "))
 	for _, rule := range rules {
 		c := comment(rule)
 		switch {
@@ -666,7 +687,21 @@ func Converge(ctx context.Context, run Runner, o *declaration.Opening) (Converge
 		case markedFor(c, o.ID):
 			stale = append(stale, rule)
 		default:
-			if parsed, ok := parseRule(rule); ok && satisfiedBy == "" && parsed.admits(o) {
+			parsed, ok := parseRule(rule)
+			if !ok {
+				continue
+			}
+			// ufw would take the mesh's rule for this one and rewrite its action or log type:
+			// an operator's refusal, or a limit, would silently become an allow — and removing the
+			// opening would then delete it. A plain allow answers the opening; anything else is a
+			// conflict the operator decides (novox/hq ADR 0103).
+			if parsed.sameAs(want) && (parsed.action != "allow" || parsed.log != "") {
+				return Converged{}, fmt.Errorf("ufw holds %q, which ufw takes for the same rule as the "+
+					"mesh's opening for %s, differing in what it does; adding the opening would change "+
+					"it, so nothing was added. Change or remove that rule, or have the mesh stop "+
+					"declaring the opening", rule, o.Target())
+			}
+			if satisfiedBy == "" && parsed.admits(o) {
 				satisfiedBy = rule
 			}
 		}

internal/firewall/firewall_test.go

@@ -658,7 +658,6 @@ func TestARuleThatDoesNotAnswerTheOpeningLeavesItToBeAdded(t *testing.T) {
 		"allow from 192.0.2.0/24 to any port 5671 proto tcp", // narrower: from one range
 		"allow in on eth0 to any port 5671 proto tcp",        // narrower: one interface
 		"allow 5671/udp",                         // another protocol
-		"deny 5671/tcp",                          // refuses
 		"route allow 5671/tcp",                   // another path
 		"allow to 192.0.2.1 port 5671 proto tcp", // one address
 	} {
@@ -681,3 +680,39 @@ func TestAnOpeningWhoseFoundRuleIsGoneIsAddedAgain(t *testing.T) {
 		t.Fatalf("the opening was not added once nothing answered it: %+v %v", done, err)
 	}
 }
+
+func TestARuleUfwWouldMergeThatDoesOtherThanAllowRefusesTheOpening(t *testing.T) {
+	// ufw takes two rules differing only in action or log type for one, and adding the mesh's
+	// would turn the operator's refusal into an allow (novox/hq ADR 0103).
+	for _, operators := range []string{
+		"deny 5671/tcp",
+		"reject 5671/tcp",
+		"limit 5671/tcp",
+		"allow log 5671/tcp",
+		"allow log-all proto tcp to any port 5671",
+		"deny in log to any port 5671 proto tcp comment 'operator note'",
+	} {
+		f := &fakeUFW{installed: true, active: true, rules: []string{operators}}
+		_, err := Converge(context.Background(), f.run, opening("adoption.bus", 5671, "everywhere", "incoming", 0))
+		if err == nil || !strings.Contains(err.Error(), operators) {
+			t.Errorf("%q: the conflict was not refused naming the rule: %v", operators, err)
+		}
+		if f.added() != 0 || len(f.rules) != 1 || f.rules[0] != operators {
+			t.Errorf("%q: something was added or changed: %v %v", operators, f.asked, f.rules)
+		}
+	}
+}
+
+func TestALogTypeIsReadInEitherPlace(t *testing.T) {
+	for rule, want := range map[string]string{
+		"allow log 22/tcp": "allow log 22 tcp in=",
+		"allow in log-all on mesh0 to any port 5432 proto tcp": "allow log-all 5432 tcp in=mesh0",
+		"route deny log in on mesh0 to any port 80 proto tcp":  "deny log 80 tcp in=mesh0",
+		"allow 22/tcp comment 'log'":                           "allow  22 tcp in=",
+	} {
+		r, ok := parseRule(rule)
+		if got := r.action + " " + r.log + " " + r.port + " " + r.proto + " in=" + r.in; !ok || got != want {
+			t.Errorf("%q read as %q (%v), want %q", rule, got, ok, want)
+		}
+	}
+}