test(netfault): lock down Opts provider matrix; document Opts breaking change

- TestOptsProviderMatrix asserts which provider interfaces each of the six
  Opts types implements. Accidentally adding (or removing) one of
  ip/tc/iptables on the wrong type now fails this test instead of silently
  switching code paths in generateAndRunCommands.
- Remove the now-redundant TestTcpResetOpts_ImplementsOnlyIptables (the
  TcpReset case is covered by the matrix).
- CHANGELOG: also document the Opts interface breaking change — methods
  ipCommands/tcCommands are no longer required, and external code that
  consumed them needs a type assertion.

Author: joshiste

go/action_kit_commons/CHANGELOG.md

@@ -11,6 +11,13 @@
   after revert in that case.
 - **Breaking:** `netfault.Apply` now returns `([]string, error)` — the
   string slice contains preflight warnings to surface to the user.
+- **Breaking:** The `Opts` interface no longer requires `ipCommands` or
+  `tcCommands`. Subsystem behavior is now opt-in via two new optional
+  interfaces: `tcCommandProvider` (`tcCommands` + `tcRootQdiscInterfaces`)
+  and `ipCommandProvider` (`ipCommands`), mirroring the existing
+  `iptablesScriptProvider`. External `Opts` implementations that returned
+  `nil, nil` from these methods can simply remove them; external callers
+  that consumed those methods need a type assertion first.
 
 ## 1.6.1
 

go/action_kit_commons/network/netfault/netfault_test.go

@@ -104,6 +104,38 @@ qdisc cake 8001: dev eth1 root refcnt 2 bandwidth 1Gbit`,
 	}
 }
 
+// TestOptsProviderMatrix locks down which subsystem providers each Opts type
+// implements. Generated commands flow through type assertions in
+// generateAndRunCommands, so accidentally adding tcCommands to BlackholeOpts
+// (or any similar mistake) would silently start hitting a different code
+// path without any other test failing.
+func TestOptsProviderMatrix(t *testing.T) {
+	cases := []struct {
+		name        string
+		opts        Opts
+		hasIp       bool
+		hasTc       bool
+		hasIptables bool
+	}{
+		{"BlackholeOpts", &BlackholeOpts{}, true, false, false},
+		{"DelayOpts", &DelayOpts{}, false, true, true},
+		{"PackageLossOpts", &PackageLossOpts{}, false, true, false},
+		{"CorruptPackagesOpts", &CorruptPackagesOpts{}, false, true, false},
+		{"LimitBandwidthOpts", &LimitBandwidthOpts{}, false, true, false},
+		{"TcpResetOpts", &TcpResetOpts{}, false, false, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, isIp := tc.opts.(ipCommandProvider)
+			_, isTc := tc.opts.(tcCommandProvider)
+			_, isIptables := tc.opts.(iptablesScriptProvider)
+			assert.Equal(t, tc.hasIp, isIp, "ipCommandProvider")
+			assert.Equal(t, tc.hasTc, isTc, "tcCommandProvider")
+			assert.Equal(t, tc.hasIptables, isIptables, "iptablesScriptProvider")
+		})
+	}
+}
+
 func TestDelayOpts_IptablesScripts_FilterByFamily(t *testing.T) {
 	opts := &DelayOpts{
 		Filter: Filter{

go/action_kit_commons/network/netfault/tcp_reset_test.go

@@ -458,16 +458,6 @@ func TestTcpResetOpts_chainName(t *testing.T) {
 		(&TcpResetOpts{}).rstFilterChainName())
 }
 
-func TestTcpResetOpts_ImplementsOnlyIptables(t *testing.T) {
-	var opts Opts = &TcpResetOpts{}
-	_, isIp := opts.(ipCommandProvider)
-	assert.False(t, isIp, "TcpResetOpts must not implement ipCommandProvider")
-	_, isTc := opts.(tcCommandProvider)
-	assert.False(t, isTc, "TcpResetOpts must not implement tcCommandProvider")
-	_, isIptables := opts.(iptablesScriptProvider)
-	assert.True(t, isIptables, "TcpResetOpts must implement iptablesScriptProvider")
-}
-
 func TestTcpResetOpts_doesConflictWith(t *testing.T) {
 	filter := Filter{Include: network.NewNetWithPortRanges(network.NetAny, network.PortRangeAny)}