Repository: scionproto/scion
Pull request #4343 (2023-05-11 08:02:46)

router: fix minimal SCMP checks in dataplane tests
There is a small number of of packet processing test cases that cover cases resulting in SCMPs.
The packet processor signals SCMPs by returning an `scmpError`. Previously, the test cases just asserted that any error is returned from processing and so they would pass even if an internal error occurred. In this instance there was an internal error while serializing the SCMP message, as the internal IP of the dataplane, the source address of the SCMP message, was not initialized in the test setup.

Add a dummy internal IP address to the test dataplane setup, and change the type of the dataplane's internal IP so that a missing initialization of the IP leads to a panic instead of a runtime error.

Assert that an `scmpError` is returned for the relevant test cases, and check the SCMP type/code.
The full SCMP message is still not checked.

<!-- Reviewable:start -->
- - -
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/4343)
<!-- Reviewable:end -->



Patch:
diff --git a/router/dataplane.go b/router/dataplane.go
index 6365b9784..341097968 100644
--- a/router/dataplane.go
+++ b/router/dataplane.go
@@ -99,7 +99,7 @@ type DataPlane struct {
 	linkTypes         map[uint16]topology.LinkType
 	neighborIAs       map[uint16]addr.IA
 	internal          BatchConn
-	internalIP        net.IP
+	internalIP        *net.IPAddr
 	internalNextHops  map[uint16]*net.UDPAddr
 	svc               *services
 	macFactory        func() hash.Hash
@@ -203,7 +203,7 @@ func (d *DataPlane) AddInternalInterface(conn BatchConn, ip net.IP) error {
 		return alreadySet
 	}
 	d.internal = conn
-	d.internalIP = ip
+	d.internalIP = &net.IPAddr{IP: ip}
 	return nil
 }
 
@@ -1624,7 +1624,7 @@ func (p *scionPacketProcessor) prepareSCMP(
 	if err := scionL.SetDstAddr(srcA); err != nil {
 		return nil, serrors.Wrap(cannotRoute, err, "details", "setting dest addr")
 	}
-	if err := scionL.SetSrcAddr(&net.IPAddr{IP: p.d.internalIP}); err != nil {
+	if err := scionL.SetSrcAddr(p.d.internalIP); err != nil {
 		return nil, serrors.Wrap(cannotRoute, err, "details", "setting src addr")
 	}
 	scionL.NextHdr = slayers.L4SCMP


Test patch:
diff --git a/router/dataplane_test.go b/router/dataplane_test.go
index 34ddf66aa..d68d95336 100644
--- a/router/dataplane_test.go
+++ b/router/dataplane_test.go
@@ -837,14 +837,10 @@ func TestProcessPkt(t *testing.T) {
 				dpath.Base.PathMeta.CurrHF = 2
 				dpath.HopFields[2].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[2])
 				ret := toMsg(t, spkt, dpath)
-				if afterProcessing {
-					ret.Addr = &net.IPAddr{IP: net.ParseIP("10.0.200.200").To4()}
-					ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil
-				}
 				return ret
 			},
 			srcInterface: 1,
-			assertFunc:   assert.Error,
+			assertFunc:   assertIsSCMPError(slayers.SCMPTypeDestinationUnreachable, 0),
 		},
 		"svc invalid": {
 			prepareDP: func(ctrl *gomock.Controller) *router.DataPlane {
@@ -864,14 +860,10 @@ func TestProcessPkt(t *testing.T) {
 				dpath.Base.PathMeta.CurrHF = 2
 				dpath.HopFields[2].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[2])
 				ret := toMsg(t, spkt, dpath)
-				if afterProcessing {
-					ret.Addr = &net.IPAddr{IP: net.ParseIP("10.0.200.200").To4()}
-					ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil
-				}
 				return ret
 			},
 			srcInterface: 1,
-			assertFunc:   assert.Error,
+			assertFunc:   assertIsSCMPError(slayers.SCMPTypeDestinationUnreachable, 0),
 		},
 		"onehop inbound": {
 			prepareDP: func(ctrl *gomock.Controller) *router.DataPlane {
@@ -1090,17 +1082,16 @@ func TestProcessPkt(t *testing.T) {
 					{ConsIngress: 31, ConsEgress: 404},
 					{ConsIngress: 1, ConsEgress: 0},
 				}
-				dpath.Base.PathMeta.CurrHF = 1
+				dpath.Base.PathMeta.CurrHF = 2
 				dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[1])
 				ret := toMsg(t, spkt, dpath)
-				if afterProcessing {
-					ret.Addr = &net.IPAddr{IP: net.ParseIP("10.0.200.200").To4()}
-					ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil
-				}
 				return ret
 			},
 			srcInterface: 1,
-			assertFunc:   assert.Error,
+			assertFunc: assertIsSCMPError(
+				slayers.SCMPTypeParameterProblem,
+				slayers.SCMPCodeInvalidDestinationAddress,
+			),
 		},
 		"epic inbound": {
 			prepareDP: func(ctrl *gomock.Controller) *router.DataPlane {
@@ -1325,3 +1316,14 @@ func bfd() control.BFD {
 		RequiredMinRxInterval: 25 * time.Millisecond,
 	}
 }
+
+// assertIsSCMPError returns an ErrorAssertionFunc asserting that error is an
+// scmpError with the specified type/code.
+func assertIsSCMPError(typ slayers.SCMPType, code slayers.SCMPCode) assert.ErrorAssertionFunc {
+	return func(t assert.TestingT, err error, args ...interface{}) bool {
+		target := router.SCMPError{}
+		return assert.ErrorAs(t, err, &target, args) &&
+			assert.Equal(t, typ, target.TypeCode.Type(), args) &&
+			assert.Equal(t, code, target.TypeCode.Code(), args)
+	}
+}
diff --git a/router/export_test.go b/router/export_test.go
index f09660947..3a9acc5c8 100644
--- a/router/export_test.go
+++ b/router/export_test.go
@@ -29,6 +29,8 @@ type ProcessResult struct {
 	processResult
 }
 
+type SCMPError = scmpError
+
 func NewDP(
 	external map[uint16]BatchConn,
 	linkTypes map[uint16]topology.LinkType,
@@ -47,6 +49,7 @@ func NewDP(
 		internalNextHops: internalNextHops,
 		svc:              &services{m: svc},
 		internal:         internal,
+		internalIP:       &net.IPAddr{IP: net.ParseIP("198.51.100.1")},
 	}
 	if err := dp.SetKey(key); err != nil {
 		panic(err)