Conversation
…nnect, quickstart)
Two holes in graceful stop could silently drop trailing messages of finite pipelines (eg. read file -- rov -- stdout), exit code 0: 1. auto-added stages (--stdout/--stdin) were not in b.Stages, so Run() never stopped them: process exited without draining the stdout extio Output buffer - track them in b.Autos and stop last; 2. a Run() goroutine scheduled after normal pipe completion saw ErrPipeFinished and skipped Run() entirely, abandoning the whole output - still run Run() on ErrPipeFinished so write-side stages drain, bounded by the usual StopTimeout. Regression test: TestBgpipe_AutoStdoutDrained. Remove the FIFO workarounds from integration tests 60/62.
There was a problem hiding this comment.
Pull request overview
Adds a Docker-based integration test suite for bgpipe (real BGP daemons + RTR/RPKI fixtures) and includes several correctness/robustness fixes in core stages, lifecycle handling, metrics registration, and documentation.
Changes:
- Introduce
test/integration/end-to-end tests (GoBGP, BIRD, FRR, StayRTR, rtrtr) with fixtures and a runner script. - Fix/adjust pipeline behavior: withdrawal handling in
update, listener lifecycle, limit-stage withdrawal counting, idempotent RPKI metrics, and ensure auto-added--stdoutdrains before exit. - Documentation updates for stage options/behavior and add CI workflow to run integration tests.
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/testdata/updates.json | Adds small UPDATE fixtures for integration tests. |
| test/integration/testdata/rtrtr.conf | Adds rtrtr config for serving local RPKI JSON. |
| test/integration/testdata/rpki.json | Adds ROA/ASPA fixture data for RTR/ROV/ASPA tests. |
| test/integration/testdata/gobgp-passive.conf | Adds GoBGP passive (dynamic neighbor) config for tests. |
| test/integration/testdata/gobgp-active.conf.in | Adds templated GoBGP active config for tests. |
| test/integration/testdata/frr.conf | Adds FRR config for standard BGP integration scenarios. |
| test/integration/testdata/frr-vtysh.conf | Enables integrated vtysh config for FRR container tests. |
| test/integration/testdata/frr-replay.conf | Adds FRR config tuned for MRT replay acceptance checks. |
| test/integration/testdata/frr-daemons | Enables required FRR daemons for integration testing. |
| test/integration/testdata/bird.Dockerfile | Adds minimal BIRD image for integration tests. |
| test/integration/testdata/bird.conf.in | Adds templated BIRD config for active-dial tests. |
| test/integration/testdata/aspa-updates.json | Adds UPDATE fixtures with peer tags for ASPA tests. |
| test/integration/run.sh | Adds an integration test runner that builds bgpipe and runs NN-*.sh. |
| test/integration/README.md | Documents integration suite purpose, requirements, and layout. |
| test/integration/lib.sh | Adds shared test helpers (docker lifecycle, waits, assertions). |
| test/integration/80-replay-frr.sh | Adds MRT replay scenario validating FRR accepts marshaled UPDATEs. |
| test/integration/70-proxy.sh | Adds transparent proxy scenario (GoBGP ↔ bgpipe ↔ FRR). |
| test/integration/65-rtr-resync.sh | Adds RTR resync test covering ROA revocation propagation. |
| test/integration/62-aspa-rtrtr.sh | Adds ASPA-over-RTRv2 test using rtrtr. |
| test/integration/60-rov-stayrtr.sh | Adds ROV-over-RTR test using StayRTR. |
| test/integration/50-connect-frr.sh | Adds scenario where bgpipe connects to FRR and receives routes. |
| test/integration/40-listen-bird.sh | Adds scenario where BIRD connects to bgpipe listener. |
| test/integration/30-listen-gobgp.sh | Adds scenario where GoBGP connects to bgpipe listener. |
| test/integration/20-connect-gobgp.sh | Adds scenario where bgpipe connects to GoBGP passive listener. |
| test/integration/10-smoke.sh | Adds non-docker smoke test for JSON round-trip. |
| test/integration/.gitignore | Ignores integration test cache directory. |
| stages/update.go | Prevents update-stage attribute modifications on pure withdrawals. |
| stages/rv-live/openbmp.go | Simplifies AS_PATH stripping logic in RouteViews live ingest. |
| stages/listen.go | Ensures listener is closed after first accepted connection via defer. |
| stages/limit.go | Fixes min-length error message and preserves withdrawal counting for drop decision in permanent mode. |
| README.md | Links to the new integration test suite. |
| pkg/extio/extio.go | Returns pooled buffer on marshal/write error to avoid leaks. |
| LICENSE | Updates copyright years/holders and normalizes quotes. |
| docs/stages/update.md | Documents --add-com-ext JSON format and withdrawal pass-through behavior. |
| docs/stages/speaker.md | Clarifies passive vs active speaker behavior for ASN/ID inference. |
| docs/stages/rv-live.md | Updates documented tag meanings for RouteViews live ingest. |
| docs/stages/connect.md | Documents platform constraints for --ttl/--md5. |
| docs/quickstart.md | Adds --transparent and --ttl to quickstart options listing. |
| docs/filters.md | Clarifies existence-testing limitations for prefix filters. |
| core/run.go | Allows late stage Run() start after normal pipe completion to drain writers. |
| core/rpki.go | Makes metrics registration idempotent via GetOrCreateGauge. |
| core/lifecycle_test.go | Adds regression test for auto-stdout drain on shutdown. |
| core/http.go | Allows --pprof <addr> without --http, and validates --pprof http requires --http. |
| core/bgpipe.go | Tracks auto-added stages separately and stops them last for clean shutdown. |
| core/attach.go | Records auto-added stdin/stdout stages for later stopping/drain. |
| .github/workflows/test.yml | Adds CI job to run integration tests on pushes/PRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| d := &dummyStage{StageBase: base} | ||
| base.Options.IsProducer = true | ||
| d.runFn = func() error { | ||
| for range N { |
Comment on lines
101
to
+105
| if seg.IsSet || len(seg.List) < 2 || seg.List[0] == peer_as || seg.List[1] != peer_as { | ||
| return | ||
| } | ||
|
|
||
| // remove the first ASN from the first segment | ||
| if len(seg.List) > 1 { | ||
| seg.List = seg.List[1:] | ||
| } else if len(asp.Segments) > 1 { | ||
| asp.Segments = asp.Segments[1:] | ||
| } else { | ||
| return // AS_PATH would become empty, leave it as is | ||
| } | ||
| // remove the first ASN from the first segment (NB: len >= 2 checked above) |
Comment on lines
+46
to
+47
| // for dev: use the latest code in ../bgpfix | ||
| // replace github.com/bgpfix/bgpfix => ../bgpfix | ||
| replace github.com/bgpfix/bgpfix => ../bgpfix |
Comment on lines
+6
to
+8
| run_daemon gobgp 1790 \ | ||
| -v "$TESTDATA/gobgp-passive.conf:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest |
Comment on lines
+13
to
+16
| run_daemon gobgp - \ | ||
| --add-host=hostgw:host-gateway \ | ||
| -v "$WORK/gobgp.toml:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest |
Comment on lines
+24
to
+29
| render gobgp-active.conf.in gobgp.toml | ||
| run_daemon gobgp - \ | ||
| --add-host=hostgw:host-gateway \ | ||
| -v "$WORK/gobgp.toml:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest | ||
| GOBGP="$DAEMON" |
Comment on lines
+7
to
+9
| run_daemon stayrtr 8282 \ | ||
| -v "$TESTDATA/rpki.json:/rpki.json:ro" \ | ||
| rpki/stayrtr:latest -bind :8282 -cache /rpki.json -checktime=false |
Comment on lines
+12
to
+14
| run_daemon stayrtr 8282 \ | ||
| -v "$WORK:/data" \ | ||
| rpki/stayrtr:latest -bind :8282 -cache /data/rpki.json -checktime=false -refresh 1 |
Comment on lines
+8
to
+11
| run_daemon rtrtr 3323 \ | ||
| -v "$TESTDATA/rtrtr.conf:/rtrtr.conf:ro" \ | ||
| -v "$TESTDATA/rpki.json:/rpki.json:ro" \ | ||
| nlnetlabs/rtrtr:latest -c /rtrtr.conf |
Comment on lines
723
to
726
| if err != nil { | ||
| eio.Warn().Err(err).Msg("extio write error") | ||
| eio.Pool.Put(bb) | ||
| return keep |
…-version instead of local replace
Comment on lines
+48
to
+51
| if [ -n "$PIDS" ]; then kill $PIDS 2>/dev/null || true; fi | ||
| docker ps -aq -f "label=$RUN_ID" 2>/dev/null | while read -r c; do | ||
| docker rm -f "$c" >/dev/null 2>&1 || true | ||
| done |
Comment on lines
+34
to
+38
| if [ $rc -ne 0 ] && [ $rc -ne 77 ]; then | ||
| for c in $(docker ps -aq -f "label=$RUN_ID" 2>/dev/null); do | ||
| msg "--- docker logs $c (tail) ---" | ||
| docker logs "$c" 2>&1 | tail -30 >&2 || true | ||
| done |
Comment on lines
+4
to
+6
| set -eu | ||
| cd "$(dirname "$0")" | ||
|
|
Comment on lines
+7
to
+9
| run_daemon stayrtr 8282 \ | ||
| -v "$TESTDATA/rpki.json:/rpki.json:ro" \ | ||
| rpki/stayrtr:latest -bind :8282 -cache /rpki.json -checktime=false |
Comment on lines
+12
to
+14
| run_daemon stayrtr 8282 \ | ||
| -v "$WORK:/data" \ | ||
| rpki/stayrtr:latest -bind :8282 -cache /data/rpki.json -checktime=false -refresh 1 |
Comment on lines
+8
to
+11
| run_daemon rtrtr 3323 \ | ||
| -v "$TESTDATA/rtrtr.conf:/rtrtr.conf:ro" \ | ||
| -v "$TESTDATA/rpki.json:/rpki.json:ro" \ | ||
| nlnetlabs/rtrtr:latest -c /rtrtr.conf |
Comment on lines
+6
to
+8
| run_daemon gobgp 1790 \ | ||
| -v "$TESTDATA/gobgp-passive.conf:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest |
Comment on lines
+13
to
+16
| run_daemon gobgp - \ | ||
| --add-host=hostgw:host-gateway \ | ||
| -v "$WORK/gobgp.toml:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest |
Comment on lines
+25
to
+28
| run_daemon gobgp - \ | ||
| --add-host=hostgw:host-gateway \ | ||
| -v "$WORK/gobgp.toml:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest |
Comment on lines
+399
to
+407
| d.runFn = func() error { | ||
| for range N { | ||
| m := d.P.GetMsg().Switch(msg.KEEPALIVE) | ||
| if err := d.inputs[0].WriteMsg(m); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
Comment on lines
99
to
104
| // check we should remove the first hop | ||
| seg := &asp.Segments[0] | ||
| if seg.IsSet || len(seg.List) < 2 || seg.List[0] == peer_as || seg.List[1] != peer_as { | ||
| return | ||
| } | ||
|
|
Comment on lines
+6
to
+8
| run_daemon gobgp 1790 \ | ||
| -v "$TESTDATA/gobgp-passive.conf:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest |
Comment on lines
+13
to
+16
| run_daemon gobgp - \ | ||
| --add-host=hostgw:host-gateway \ | ||
| -v "$WORK/gobgp.toml:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest |
Comment on lines
+25
to
+28
| run_daemon gobgp - \ | ||
| --add-host=hostgw:host-gateway \ | ||
| -v "$WORK/gobgp.toml:/config/gobgp.toml:ro" \ | ||
| jauderho/gobgp:latest |
Comment on lines
+7
to
+9
| run_daemon stayrtr 8282 \ | ||
| -v "$TESTDATA/rpki.json:/rpki.json:ro" \ | ||
| rpki/stayrtr:latest -bind :8282 -cache /rpki.json -checktime=false |
Comment on lines
+12
to
+14
| run_daemon stayrtr 8282 \ | ||
| -v "$WORK:/data" \ | ||
| rpki/stayrtr:latest -bind :8282 -cache /data/rpki.json -checktime=false -refresh 1 |
Comment on lines
+8
to
+11
| run_daemon rtrtr 3323 \ | ||
| -v "$TESTDATA/rtrtr.conf:/rtrtr.conf:ro" \ | ||
| -v "$TESTDATA/rpki.json:/rpki.json:ro" \ | ||
| nlnetlabs/rtrtr:latest -c /rtrtr.conf |
- extio: flush pending aggregated MRT table-dump UPDATEs at EOF - extio: autodetect rib.*/bview.* RouteViews and RIS RIB filenames - write: drain queued output buffers when the context is cancelled - on normal completion the cancel arrives after all messages are queued, so returning early dropped an arbitrary tail of the output (data loss visible with any file read, worst for table dumps flushed at EOF) - docs: read stage TABLE_DUMP support Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.