Implement reviewer's suggestions.

Mainly: use a separate, toml bound, definition of the dataplane BDF configuration, and redefine
the link BDF.disabled flag as a bool ptr, so we can tell the difference between false and unspecified.

Caveat: this makes possible a bug: the link's BFD config could be used while a nil BFD.disable pointer is
still not replaced by the default from the dataplane's config.

acceptance/router_benchmark/cases/topo.go

@@ -280,6 +280,9 @@ func MACAddr(ip netip.Addr) net.HardwareAddr {
 		return mac
 	}
 	as4 := ip.As4()
+
+	// This component makes no assumption regarding how the topology is used. We have to support all
+	// hosts that the topology describes, even fictional ones, should a test case refer to it.
 	return net.HardwareAddr{0xf0, 0x0d, 0xca, 0xfe, as4[2], as4[3]}
 }
 

private/topology/json/json.go

@@ -125,7 +125,7 @@ type Underlay struct {
 
 // BFD configuration.
 type BFD struct {
-	Disable               bool         `json:"disable,omitempty"`
+	Disable               *bool        `json:"disable,omitempty"`
 	DetectMult            uint8        `json:"detect_mult,omitempty"`
 	DesiredMinTxInterval  util.DurWrap `json:"desired_min_tx_interval,omitempty"`
 	RequiredMinRxInterval util.DurWrap `json:"required_min_rx_interval,omitempty"`

private/topology/topology.go

@@ -126,10 +126,10 @@ type (
 
 	// BFD is the configuration for a BFD session
 	// Disable can be set from two sources: the topology configuration for the link (here), and
-	// the dataplane's bfd global configuration. BFD is disabled if either of these is
-	// True.
+	// the dataplane's bfd global configuration. This is actually a pointer to boolean. nil
+	// means unspecified.
 	BFD struct {
-		Disable               bool
+		Disable               *bool
 		DetectMult            uint8
 		DesiredMinTxInterval  time.Duration
 		RequiredMinRxInterval time.Duration

router/BUILD.bazel

@@ -31,6 +31,7 @@ go_library(
         "//private/topology:go_default_library",
         "//private/underlay/conn:go_default_library",
         "//router/bfd:go_default_library",
+        "//router/config:go_default_library",
         "//router/control:go_default_library",
         "@com_github_google_gopacket//:go_default_library",
         "@com_github_google_gopacket//layers:go_default_library",

router/cmd/router/main.go

@@ -63,12 +63,7 @@ func realMain(ctx context.Context) error {
 		},
 		ReceiveBufferSize: globalCfg.Router.ReceiveBufferSize,
 		SendBufferSize:    globalCfg.Router.SendBufferSize,
-		BfdConfig: control.BFD{
-			Disable:               globalCfg.Router.Bfd.Disable,
-			DetectMult:            globalCfg.Router.Bfd.DetectMult,
-			DesiredMinTxInterval:  globalCfg.Router.Bfd.DesiredMinTxInterval.Duration,
-			RequiredMinRxInterval: globalCfg.Router.Bfd.RequiredMinRxInterval.Duration,
-		},
+		BFD:               globalCfg.Router.BFD,
 	}
 	iaCtx := &control.IACtx{
 		Config: controlConfig,

router/config/BUILD.bazel

@@ -15,7 +15,6 @@ go_library(
         "//private/config:go_default_library",
         "//private/env:go_default_library",
         "//private/mgmtapi:go_default_library",
-        "//private/topology/json:go_default_library",
     ],
 )
 

router/config/config.go

@@ -28,7 +28,6 @@ import (
 	"github.com/scionproto/scion/private/config"
 	"github.com/scionproto/scion/private/env"
 	api "github.com/scionproto/scion/private/mgmtapi"
-	"github.com/scionproto/scion/private/topology/json"
 )
 
 const idSample = "router-1"
@@ -43,12 +42,21 @@ type Config struct {
 }
 
 type RouterConfig struct {
-	ReceiveBufferSize     int      `toml:"receive_buffer_size,omitempty"`
-	SendBufferSize        int      `toml:"send_buffer_size,omitempty"`
-	NumProcessors         int      `toml:"num_processors,omitempty"`
-	NumSlowPathProcessors int      `toml:"num_slow_processors,omitempty"`
-	BatchSize             int      `toml:"batch_size,omitempty"`
-	Bfd                   json.BFD `toml:"bfd,omitempty"`
+	ReceiveBufferSize     int `toml:"receive_buffer_size,omitempty"`
+	SendBufferSize        int `toml:"send_buffer_size,omitempty"`
+	NumProcessors         int `toml:"num_processors,omitempty"`
+	NumSlowPathProcessors int `toml:"num_slow_processors,omitempty"`
+	BatchSize             int `toml:"batch_size,omitempty"`
+	BFD                   BFD `toml:"bfd,omitempty"`
+}
+
+// BFD configuration. Unfortunately cannot be shared with topology.BFD
+// as one is toml and the other json. Eventhough the semantics are identical.
+type BFD struct {
+	Disable               bool         `toml:"disable,omitempty"`
+	DetectMult            uint8        `toml:"detect_mult,omitempty"`
+	DesiredMinTxInterval  util.DurWrap `toml:"desired_min_tx_interval,omitempty"`
+	RequiredMinRxInterval util.DurWrap `toml:"required_min_rx_interval,omitempty"`
 }
 
 func (cfg *RouterConfig) ConfigName() string {
@@ -104,14 +112,14 @@ func (cfg *RouterConfig) InitDefaults() {
 	if cfg.BatchSize == 0 {
 		cfg.BatchSize = 256
 	}
-	if cfg.Bfd.DetectMult == 0 {
-		cfg.Bfd.DetectMult = 3
+	if cfg.BFD.DetectMult == 0 {
+		cfg.BFD.DetectMult = 3
 	}
-	if cfg.Bfd.DesiredMinTxInterval.Duration == 0 {
-		cfg.Bfd.DesiredMinTxInterval = util.DurWrap{Duration: 200 * time.Millisecond}
+	if cfg.BFD.DesiredMinTxInterval.Duration == 0 {
+		cfg.BFD.DesiredMinTxInterval = util.DurWrap{Duration: 200 * time.Millisecond}
 	}
-	if cfg.Bfd.RequiredMinRxInterval.Duration == 0 {
-		cfg.Bfd.RequiredMinRxInterval = util.DurWrap{Duration: 200 * time.Millisecond}
+	if cfg.BFD.RequiredMinRxInterval.Duration == 0 {
+		cfg.BFD.RequiredMinRxInterval = util.DurWrap{Duration: 200 * time.Millisecond}
 	}
 }
 

router/connector.go

@@ -25,6 +25,7 @@ import (
 	"github.com/scionproto/scion/pkg/private/serrors"
 	"github.com/scionproto/scion/private/topology"
 	"github.com/scionproto/scion/private/underlay/conn"
+	"github.com/scionproto/scion/router/config"
 	"github.com/scionproto/scion/router/control"
 )
 
@@ -41,7 +42,7 @@ type Connector struct {
 
 	ReceiveBufferSize int
 	SendBufferSize    int
-	BfdConfig         control.BFD
+	BFD               config.BFD
 }
 
 var errMultiIA = serrors.New("different IA not allowed")
@@ -89,8 +90,10 @@ func (c *Connector) AddExternalInterface(localIfID common.IFIDType, link control
 	log.Debug("Adding external interface", "interface", localIfID,
 		"local_isd_as", link.Local.IA, "local_addr", link.Local.Addr,
 		"remote_isd_as", link.Remote.IA, "remote_addr", link.Remote.Addr,
-		"owned", owned, "bfd", !link.BFD.Disable,
-		"dataplane_bfd_enabled", !c.BfdConfig.Disable)
+		"owned", owned,
+		"link_bfd_configured", link.BFD.Disable != nil,
+		"link_bfd_enabled", link.BFD.Disable == nil || !*link.BFD.Disable,
+		"dataplane_bfd_enabled", !c.BFD.Disable)
 
 	if !c.ia.Equal(link.Local.IA) {
 		return serrors.WithCtx(errMultiIA, "current", c.ia, "new", link.Local.IA)
@@ -102,7 +105,7 @@ func (c *Connector) AddExternalInterface(localIfID common.IFIDType, link control
 		return serrors.WrapStr("adding neighboring IA", err, "if_id", localIfID)
 	}
 
-	link.BFD = c.applyBFDdefaults(link.BFD)
+	link.BFD = c.applyBFDDefaults(link.BFD)
 	if owned {
 		if len(c.externalInterfaces) == 0 {
 			c.externalInterfaces = make(map[uint16]control.ExternalInterface)
@@ -208,19 +211,21 @@ func (c *Connector) ListSiblingInterfaces() ([]control.SiblingInterface, error)
 }
 
 // Apply the global BFD settings if required. Link-specific settings, if configured, prevail over
-// the global defaults.  Special case for BfdDisable, which can't be undefined: the link-specific
-// setting prevails if it is true. (ie. bfd is disabled as soon as either the link or the global
-// setting says so).
-func (c *Connector) applyBFDdefaults(cfg control.BFD) control.BFD {
-	cfg.Disable = cfg.Disable || c.BfdConfig.Disable
+// the global defaults. (cfg.Disable isn't a boolean, it is a pointer to boolean so we can have an
+// unspecified state - nil). After calling applyDefaults cfg.Disable is never nil.
+func (c *Connector) applyBFDDefaults(cfg control.BFD) control.BFD {
+	if cfg.Disable == nil {
+		disable := c.BFD.Disable
+		cfg.Disable = &disable
+	}
 	if cfg.DetectMult == 0 {
-		cfg.DetectMult = c.BfdConfig.DetectMult
+		cfg.DetectMult = c.BFD.DetectMult
 	}
 	if cfg.DesiredMinTxInterval == 0 {
-		cfg.DesiredMinTxInterval = c.BfdConfig.DesiredMinTxInterval
+		cfg.DesiredMinTxInterval = c.BFD.DesiredMinTxInterval.Duration
 	}
 	if cfg.RequiredMinRxInterval == 0 {
-		cfg.RequiredMinRxInterval = c.BfdConfig.RequiredMinRxInterval
+		cfg.RequiredMinRxInterval = c.BFD.RequiredMinRxInterval.Duration
 	}
 	return cfg
 }

router/dataplane.go

@@ -313,7 +313,7 @@ func (d *DataPlane) AddRemotePeer(local, remote uint16) error {
 func (d *DataPlane) addExternalInterfaceBFD(ifID uint16, conn BatchConn,
 	src, dst control.LinkEnd, cfg control.BFD) error {
 
-	if cfg.Disable {
+	if *cfg.Disable {
 		return nil
 	}
 	var m bfd.Metrics
@@ -448,7 +448,7 @@ func (d *DataPlane) AddNextHop(ifID uint16, src, dst *net.UDPAddr, cfg control.B
 func (d *DataPlane) addNextHopBFD(ifID uint16, src, dst *net.UDPAddr, cfg control.BFD,
 	sibling string) error {
 
-	if cfg.Disable {
+	if *cfg.Disable {
 		return nil
 	}
 	for k, v := range d.internalNextHops {

router/dataplane_test.go

@@ -118,7 +118,8 @@ func TestDataPlaneAddExternalInterface(t *testing.T) {
 		IA:   xtest.MustParseIA("1-ff00:0:3"),
 		Addr: &net.UDPAddr{IP: net.ParseIP("10.0.0.200")},
 	}
-	nobfd := control.BFD{Disable: true}
+	disable := true
+	nobfd := control.BFD{Disable: &disable}
 	t.Run("fails after serve", func(t *testing.T) {
 		ctrl := gomock.NewController(t)
 		defer ctrl.Finish()
@@ -197,7 +198,8 @@ func TestDataPlaneAddSVC(t *testing.T) {
 func TestDataPlaneAddNextHop(t *testing.T) {
 	l := &net.UDPAddr{}
 	r := &net.UDPAddr{}
-	nobfd := control.BFD{Disable: true}
+	disable := true
+	nobfd := control.BFD{Disable: &disable}
 
 	t.Run("fails after serve", func(t *testing.T) {
 		d := &router.DataPlane{}
@@ -302,7 +304,8 @@ func TestDataPlaneRun(t *testing.T) {
 					IA:   xtest.MustParseIA("1-ff00:0:3"),
 					Addr: &net.UDPAddr{IP: net.ParseIP("10.0.0.200")},
 				}
-				nobfd := control.BFD{Disable: true}
+				disable := true
+				nobfd := control.BFD{Disable: &disable}
 
 				_ = ret.AddExternalInterface(1, mExternal, l, r, nobfd)
 
@@ -1715,7 +1718,9 @@ func computeFullMAC(t *testing.T, key []byte, info path.InfoField, hf path.HopFi
 }
 
 func bfd() control.BFD {
+	no := false
 	return control.BFD{
+		Disable:               &no,
 		DetectMult:            3,
 		DesiredMinTxInterval:  1 * time.Millisecond,
 		RequiredMinRxInterval: 25 * time.Millisecond,

router/mgmtapi/api.go

@@ -99,7 +99,7 @@ func (s *Server) GetInterfaces(w http.ResponseWriter, r *http.Request) {
 			Bfd: BFD{
 				DesiredMinimumTxInterval: intf.Link.BFD.DesiredMinTxInterval.String(),
 				DetectionMultiplier:      int(intf.Link.BFD.DetectMult),
-				Enabled:                  !intf.Link.BFD.Disable,
+				Enabled:                  !*(intf.Link.BFD.Disable),
 				RequiredMinimumReceive:   intf.Link.BFD.RequiredMinRxInterval.String(),
 			},
 			InterfaceId:       int(intf.InterfaceID),

router/mgmtapi/api_test.go

@@ -149,6 +149,7 @@ func TestAPI(t *testing.T) {
 }
 
 func createExternalIntfs(t *testing.T) []control.ExternalInterface {
+	no := false
 	return []control.ExternalInterface{
 		{
 			InterfaceID: 1,
@@ -164,7 +165,7 @@ func createExternalIntfs(t *testing.T) []control.ExternalInterface {
 				Instance: "br1-ff00_0_110-1",
 				LinkTo:   topology.Core,
 				BFD: control.BFD{
-					Disable:               false,
+					Disable:               &no,
 					DetectMult:            3,
 					DesiredMinTxInterval:  200 * time.Millisecond,
 					RequiredMinRxInterval: 300 * time.Millisecond,
@@ -187,7 +188,7 @@ func createExternalIntfs(t *testing.T) []control.ExternalInterface {
 				Instance: "br1-ff00_0_110-1",
 				LinkTo:   topology.Child,
 				BFD: control.BFD{
-					Disable:               false,
+					Disable:               &no,
 					DetectMult:            3,
 					DesiredMinTxInterval:  200 * time.Millisecond,
 					RequiredMinRxInterval: 200 * time.Millisecond,
@@ -210,7 +211,7 @@ func createExternalIntfs(t *testing.T) []control.ExternalInterface {
 				Instance: "br1-ff00_0_111-1",
 				LinkTo:   topology.Child,
 				BFD: control.BFD{
-					Disable:               false,
+					Disable:               &no,
 					DetectMult:            3,
 					DesiredMinTxInterval:  150 * time.Millisecond,
 					RequiredMinRxInterval: 150 * time.Millisecond,
@@ -233,7 +234,7 @@ func createExternalIntfs(t *testing.T) []control.ExternalInterface {
 				Instance: "br1-ff00_0_112-1",
 				LinkTo:   topology.Child,
 				BFD: control.BFD{
-					Disable:               false,
+					Disable:               &no,
 					DetectMult:            3,
 					DesiredMinTxInterval:  150 * time.Millisecond,
 					RequiredMinRxInterval: 150 * time.Millisecond,