Merge branch 'master' into fix4415

.github/workflows/pr-title.yml

@@ -0,0 +1,31 @@
+name: Check the pull request title
+
+on:
+  pull_request:
+    types: [opened, edited, reopened, synchronize]
+
+jobs:
+  check:
+    runs-on: ubuntu-latest
+    steps:
+    - name : Check the PR title
+      env:
+        TITLE: ${{ github.event.pull_request.title }}
+      run: |
+        # Check that PR is of the form `<subsystem>: <lowercase message>`
+
+        url='https://docs.scion.org/en/latest/dev/git.html#good-commit-messages'
+        if [[ ! "$TITLE" =~ ^[a-z0-9,/]*:[[:space:]] ]]; then
+          echo '::error::The PR title should start with `<substystem>: `. See '"$url"
+          exit 1
+        fi
+        # Title should be lower case; initialisms and identifiers still occur occasionally and should be allowed.
+        # -> enforce only the first word
+        if [[ ! "$TITLE" =~ ^[a-z0-9,/]*:[[:space:]][a-z] ]]; then
+          echo '::error::The PR title should be lower case (enforced on first letter). See '"$url"
+          exit 1
+        fi
+        if [[ $TITLE =~ \.[[:space:]]*$ ]]; then
+          echo '::error::The PR title should not end with a ".". See '"$url"
+          exit 1
+        fi

BUILD.bazel

@@ -63,16 +63,6 @@ config_setting(
     },
 )
 
-# This is a dummy target so Make can "blaze build --announce_rc <something>
-# Where something truly does nothing that we may care about.
-
-config_setting(
-    name = "dummy_setting",
-    define_values = {
-        "whatever": "whatever",
-    },
-)
-
 gazelle(
     name = "gazelle",
     build_tags = select({

Makefile

@@ -115,7 +115,7 @@ lint-go-bazel:
 	$(info ==> $@)
 	@tools/quiet bazel test --config lint //...
 
-GO_BUILD_TAGS_ARG=$(shell bazel build --ui_event_filters=-stdout,-stderr --announce_rc --noshow_progress :dummy_setting 2>&1 | grep "'build' options" | sed -n "s/^.*--define gotags=\(\S*\).*/--build-tags \1/p" )
+GO_BUILD_TAGS_ARG=$(shell bazel info --ui_event_filters=-stdout,-stderr --announce_rc --noshow_progress 2>&1 | grep "'build' options" | sed -n "s/^.*--define gotags=\(\S*\).*/--build-tags \1/p" )
 
 lint-go-golangci:
 	$(info ==> $@)

pkg/slayers/path/scion/decoded.go

@@ -64,7 +64,7 @@ func (s *Decoded) SerializeTo(b []byte) error {
 		return serrors.New("buffer too small to serialize path.", "expected", s.Len(),
 			"actual", len(b))
 	}
-	if err := s.PathMeta.SerializeTo(b[:MetaLen]); err != nil {
+	if err := s.PathMeta.SerializeTo(b); err != nil {
 		return err
 	}
 	offset := MetaLen

pkg/slayers/path/scion/raw.go

@@ -51,7 +51,10 @@ func (s *Raw) SerializeTo(b []byte) error {
 	}
 	// XXX(roosd): This modifies the underlying buffer. Consider writing to data
 	// directly.
-	if err := s.PathMeta.SerializeTo(s.Raw[:MetaLen]); err != nil {
+	// TODO(matzf, jiceatscion): it is not clear whether updating pathmeta in s.Raw is desirable
+	// or not. It migh be best to make that question moot by not keeping the path meta header as
+	// raw bytes at all. However that's a viral change.
+	if err := s.PathMeta.SerializeTo(s.Raw); err != nil {
 		return err
 	}
 	copy(b, s.Raw)
@@ -82,7 +85,7 @@ func (s *Raw) Reverse() (path.Path, error) {
 // ToDecoded transforms a scion.Raw to a scion.Decoded.
 func (s *Raw) ToDecoded() (*Decoded, error) {
 	// Serialize PathMeta to ensure potential changes are reflected Raw.
-	if err := s.PathMeta.SerializeTo(s.Raw[:MetaLen]); err != nil {
+	if err := s.PathMeta.SerializeTo(s.Raw); err != nil {
 		return nil, err
 	}
 	decoded := &Decoded{}
@@ -97,7 +100,7 @@ func (s *Raw) IncPath() error {
 	if err := s.Base.IncPath(); err != nil {
 		return err
 	}
-	return s.PathMeta.SerializeTo(s.Raw[:MetaLen])
+	return s.PathMeta.SerializeTo(s.Raw)
 }
 
 // GetInfoField returns the InfoField at a given index.

private/env/BUILD.bazel

@@ -19,6 +19,7 @@ go_library(
         "//pkg/scrypto:go_default_library",
         "//private/config:go_default_library",
         "@com_github_opentracing_opentracing_go//:go_default_library",
+        "@com_github_prometheus_client_golang//prometheus:go_default_library",
         "@com_github_prometheus_client_golang//prometheus/promhttp:go_default_library",
         "@com_github_uber_jaeger_client_go//:go_default_library",
         "@com_github_uber_jaeger_client_go//config:go_default_library",

private/env/env.go

@@ -34,6 +34,7 @@ import (
 	"time"
 
 	opentracing "github.com/opentracing/opentracing-go"
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	jaeger "github.com/uber/jaeger-client-go"
 	jaegercfg "github.com/uber/jaeger-client-go/config"
@@ -61,6 +62,10 @@ const (
 	// ShutdownGraceInterval is the time applications wait after issuing a
 	// clean shutdown signal, before forcerfully tearing down the application.
 	ShutdownGraceInterval = 5 * time.Second
+
+	// HandlerTimeout is the time after which the http handler gives up on a request and
+	// returns an error instead.
+	HandlerTimeout = time.Minute
 )
 
 var sighupC chan os.Signal
@@ -185,7 +190,14 @@ func (cfg *Metrics) ServePrometheus(ctx context.Context) error {
 	if cfg.Prometheus == "" {
 		return nil
 	}
-	http.Handle("/metrics", promhttp.Handler())
+	handler := promhttp.InstrumentMetricHandler(
+		prometheus.DefaultRegisterer,
+		promhttp.HandlerFor(
+			prometheus.DefaultGatherer,
+			promhttp.HandlerOpts{Timeout: HandlerTimeout},
+		),
+	)
+	http.Handle("/metrics", handler)
 	log.Info("Exporting prometheus metrics", "addr", cfg.Prometheus)
 
 	server := &http.Server{Addr: cfg.Prometheus}

router/dataplane.go

@@ -27,6 +27,7 @@ import (
 	"net"
 	"net/netip"
 	"sync"
+	"sync/atomic"
 	"time"
 	"unsafe"
 
@@ -184,7 +185,7 @@ type DataPlane struct {
 	bfdSessions         map[uint16]bfdSession
 	localIA             addr.IA
 	mtx                 sync.Mutex
-	running             bool
+	running             atomic.Bool
 	Metrics             *Metrics
 	forwardingMetrics   map[uint16]interfaceMetrics
 	dispatchedPortStart uint16
@@ -245,11 +246,39 @@ type drkeyProvider interface {
 	) (drkey.ASHostKey, error)
 }
 
+// setRunning() Configures the running state of the data plane to true. setRunning() is called once
+// the dataplane is finished initializing and is ready to process packets.
+func (d *DataPlane) setRunning() {
+	d.running.Store(true)
+}
+
+// setStopping() Configures the running state of the data plane to false. This should not be called
+// during the dataplane initialization. Calling this before initialization starts has no effect.
+func (d *DataPlane) setStopping() {
+	d.running.Store(false)
+}
+
+// IsRunning() Indicates the running state of the data plane. If true, the dataplane is initialized
+// and ready to process or already processing packets. In this case some configuration changes are
+// not permitted. If false, the data plane is not ready to process packets yet, or is shutting
+// down.
+func (d *DataPlane) IsRunning() bool {
+	return d.running.Load()
+}
+
+// Shutdown() causes the dataplane to stop accepting packets and then terminate. Note that
+// in that case the router is committed to shutting down. There is no mechanism to restart it.
+func (d *DataPlane) Shutdown() {
+	d.mtx.Lock() // make sure we're nor racing with initialization.
+	defer d.mtx.Unlock()
+	d.setStopping()
+}
+
 // SetIA sets the local IA for the dataplane.
 func (d *DataPlane) SetIA(ia addr.IA) error {
 	d.mtx.Lock()
 	defer d.mtx.Unlock()
-	if d.running {
+	if d.IsRunning() {
 		return modifyExisting
 	}
 	if ia.IsZero() {
@@ -267,7 +296,7 @@ func (d *DataPlane) SetIA(ia addr.IA) error {
 func (d *DataPlane) SetKey(key []byte) error {
 	d.mtx.Lock()
 	defer d.mtx.Unlock()
-	if d.running {
+	if d.IsRunning() {
 		return modifyExisting
 	}
 	if len(key) == 0 {
@@ -299,7 +328,7 @@ func (d *DataPlane) SetPortRange(start, end uint16) {
 func (d *DataPlane) AddInternalInterface(conn BatchConn, ip netip.Addr) error {
 	d.mtx.Lock()
 	defer d.mtx.Unlock()
-	if d.running {
+	if d.IsRunning() {
 		return modifyExisting
 	}
 	if conn == nil {
@@ -326,7 +355,7 @@ func (d *DataPlane) AddExternalInterface(ifID uint16, conn BatchConn,
 	d.mtx.Lock()
 	defer d.mtx.Unlock()
 
-	if d.running {
+	if d.IsRunning() {
 		return modifyExisting
 	}
 	if conn == nil || !src.Addr.IsValid() || !dst.Addr.IsValid() {
@@ -356,7 +385,7 @@ func (d *DataPlane) AddExternalInterface(ifID uint16, conn BatchConn,
 func (d *DataPlane) AddNeighborIA(ifID uint16, remote addr.IA) error {
 	d.mtx.Lock()
 	defer d.mtx.Unlock()
-	if d.running {
+	if d.IsRunning() {
 		return modifyExisting
 	}
 	if remote.IsZero() {
@@ -517,7 +546,7 @@ func (d *DataPlane) AddNextHop(ifID uint16, src, dst netip.AddrPort, cfg control
 	d.mtx.Lock()
 	defer d.mtx.Unlock()
 
-	if d.running {
+	if d.IsRunning() {
 		return modifyExisting
 	}
 	if !dst.IsValid() || !src.IsValid() {
@@ -587,7 +616,7 @@ type RunConfig struct {
 
 func (d *DataPlane) Run(ctx context.Context, cfg *RunConfig) error {
 	d.mtx.Lock()
-	d.running = true
+	d.setRunning()
 	d.initMetrics()
 
 	processorQueueSize := max(
@@ -720,7 +749,7 @@ func (d *DataPlane) runReceiver(ifID uint16, conn BatchConn, cfg *RunConfig,
 		}
 	}
 
-	for d.running {
+	for d.IsRunning() {
 		// collect packets.
 
 		// Give a new buffer to the msgs elements that have been used in the previous loop.
@@ -780,7 +809,7 @@ func (d *DataPlane) runProcessor(id int, q <-chan *packet,
 
 	log.Debug("Initialize processor with", "id", id)
 	processor := newPacketProcessor(d)
-	for d.running {
+	for d.IsRunning() {
 		p, ok := <-q
 		if !ok {
 			continue
@@ -837,7 +866,7 @@ func (d *DataPlane) runSlowPathProcessor(id int, q <-chan *packet,
 
 	log.Debug("Initialize slow-path processor with", "id", id)
 	processor := newSlowPathProcessor(d)
-	for d.running {
+	for d.IsRunning() {
 		p, ok := <-q
 		if !ok {
 			continue
@@ -1014,7 +1043,7 @@ func (d *DataPlane) runForwarder(ifID uint16, conn BatchConn, cfg *RunConfig, c
 	metrics := d.forwardingMetrics[ifID]
 
 	toWrite := 0
-	for d.running {
+	for d.IsRunning() {
 		toWrite += readUpTo(c, cfg.BatchSize-toWrite, toWrite == 0, pkts[toWrite:])
 
 		// Turn the packets into underlay messages that WriteBatch can send.

router/dataplane_internal_test.go

@@ -71,7 +71,7 @@ func TestReceiver(t *testing.T) {
 	).Times(2)
 	mInternal.EXPECT().ReadBatch(gomock.Any()).DoAndReturn(
 		func(m underlayconn.Messages) (int, error) {
-			dp.running = false
+			dp.setStopping()
 			done <- true
 			return 0, nil
 		},
@@ -86,7 +86,7 @@ func TestReceiver(t *testing.T) {
 	dp.initPacketPool(runConfig, 64)
 	procCh, _, _ := initQueues(runConfig, dp.interfaces, 64)
 	initialPoolSize := len(dp.packetPool)
-	dp.running = true
+	dp.setRunning()
 	dp.initMetrics()
 	go func() {
 		dp.runReceiver(0, dp.internal, runConfig, procCh)
@@ -109,7 +109,7 @@ func TestReceiver(t *testing.T) {
 			// make sure that the processing routine received exactly 20 messages
 			if i != 20 {
 				t.Fail()
-				dp.running = false
+				dp.setStopping()
 			}
 		}
 	}
@@ -160,7 +160,7 @@ func TestForwarder(t *testing.T) {
 					}
 				}
 				if totalCount == 255 {
-					ret.running = false
+					ret.setStopping()
 					done <- struct{}{}
 				}
 				if len(ms) == 0 {
@@ -180,7 +180,7 @@ func TestForwarder(t *testing.T) {
 	dp.initPacketPool(runConfig, 64)
 	_, fwCh, _ := initQueues(runConfig, dp.interfaces, 64)
 	initialPoolSize := len(dp.packetPool)
-	dp.running = true
+	dp.setRunning()
 	dp.initMetrics()
 	go dp.runForwarder(0, dp.internal, runConfig, fwCh[0])
 
@@ -211,7 +211,7 @@ func TestForwarder(t *testing.T) {
 		assert.Equal(t, initialPoolSize, len(dp.packetPool))
 	case <-time.After(100 * time.Millisecond):
 		t.Fail()
-		dp.running = false
+		dp.setStopping()
 	}
 }
 

router/export_test.go

@@ -96,7 +96,7 @@ func NewDP(
 }
 
 func (d *DataPlane) FakeStart() {
-	d.running = true
+	d.setRunning()
 }
 
 func (d *DataPlane) ProcessPkt(pkt *Packet) Disposition {

tools/update_testdata.sh

@@ -10,7 +10,7 @@ folders=$(grep \
             --exclude-dir=bazel-\* \
             "xtest.UpdateGoldenFiles()" . | xargs dirname  | sort  | uniq )
 
-GO_BUILD_TAGS_ARG=$(bazel build --ui_event_filters=-stdout,-stderr --announce_rc --noshow_progress :dummy_setting 2>&1 | grep "'build' options" | sed -n "s/^.*--define gotags=\(\S*\).*/-tags \1/p")
+GO_BUILD_TAGS_ARG=$(bazel info --ui_event_filters=-stdout,-stderr --announce_rc --noshow_progress 2>&1 | grep "'build' options" | sed -n "s/^.*--define gotags=\(\S*\).*/-tags \1/p")
 
 echo $folders -update | xargs go test ${GO_BUILD_TAGS_ARG}
 echo $folders -count=1 | xargs go test ${GO_BUILD_TAGS_ARG}