Conversation
There was a problem hiding this comment.
Pull request overview
This PR merges development work (dev0107) into the main branch, introducing several major features and refactorings:
Changes:
- Adds a new RPKI validation stage with support for RTR protocol and file-based ROA loading
- Refactors variable naming from
opt_*prefix to simpler names across multiple stages - Consolidates utility functions from multiple locations into a new
pkg/utilpackage - Enhances error handling in file operations with proper error logging
- Updates dependency versions and adds new dependencies for RPKI functionality
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| stages/rpki/* | New RPKI validation stage with RTR client, file parser, validation logic, and comprehensive tests |
| stages/write.go | Refactored variable names (opt_every → every, etc.) and improved error handling in file operations |
| stages/read.go | Refactored variable names (opt_decompress → decompress) |
| stages/grep.go | Refactored to use flag retrieval in Attach() instead of direct field assignment |
| stages/limit.go | Changed type from nlri.NLRI to nlri.Prefix in map declarations |
| stages/websocket.go | Updated to use util package for Send/Close functions |
| stages/connect.go | Updated to use util package functions and added TLS support |
| stages/listen.go | Updated to use util package functions with capitalized names |
| pkg/util/* | New package consolidating utility functions (unsafe.go, net.go, tcpmd5*.go, stdlog.go) |
| pkg/extio/extio.go | Updated to use util package and enhanced with --skip flag support |
| core/bgpipe.go | Added signal.Reset() call after first signal received |
| core/stage.go | Changed context parent from b.Ctx to b.Pipe.Ctx and refactored filter fields |
| core/attach.go | Added rate limiting support for callbacks and inputs |
| go.mod/go.sum | Updated dependencies and enabled local replace directive |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
go.mod
Outdated
|
|
||
| // for dev: use the latest code in ../bgpfix | ||
| //replace github.com/bgpfix/bgpfix => ../bgpfix | ||
| replace github.com/bgpfix/bgpfix => ../bgpfix |
There was a problem hiding this comment.
The local replace directive for github.com/bgpfix/bgpfix is enabled, pointing to ../bgpfix. This should not be committed as it will break builds for other developers and CI/CD systems. This appears to be a development-only change that was accidentally included.
| case <-b.Ctx.Done(): | ||
| return | ||
| case sig := <-ch: | ||
| signal.Reset() // NB: reset to default behavior after first signal |
There was a problem hiding this comment.
signal.Reset() is called without arguments, but the os/signal package's Reset() function requires signal arguments (e.g., signal.Reset(syscall.SIGINT, syscall.SIGTERM)). This will cause a compilation error. The function should be called with the same signals that were registered with signal.Notify().
| // create new stage | ||
| s := &StageBase{} | ||
| s.Ctx, s.Cancel = context.WithCancelCause(b.Ctx) | ||
| s.Ctx, s.Cancel = context.WithCancelCause(b.Pipe.Ctx) |
There was a problem hiding this comment.
Changed context parent from b.Ctx to b.Pipe.Ctx. However, b.Pipe.Ctx is created from b.Ctx in NewBgpipe (line 59 of bgpipe.go: b.Pipe = pipe.NewPipe(b.Ctx)). This creates a different context hierarchy where stages are now children of the pipe's context instead of the bgpipe's context. This change affects shutdown behavior and may cause stages to not properly receive cancellation signals from the main bgpipe context.
| s.Ctx, s.Cancel = context.WithCancelCause(b.Pipe.Ctx) | |
| s.Ctx, s.Cancel = context.WithCancelCause(b.Ctx) |
| f.Bool("keep", false, "keep the message: run the filter but do not drop") | ||
| f.String("event-match", "", "emit event on match success") | ||
| f.String("event-fail", "", "emit event on match fail") | ||
| f.Bool("kill-match", false, "kill the process on match success") | ||
| f.Bool("kill-fail", false, "kill the process on match fail") |
There was a problem hiding this comment.
The code now uses f.Bool() and f.String() instead of f.BoolVar() and f.StringVar(), but then retrieves these values in Attach() using k.Bool() and k.String(). This is redundant - values are parsed twice. Consider using BoolVar/StringVar to bind directly to struct fields, or consistently use the k.* retrieval pattern without the intermediate struct fields.
core/attach.go
Outdated
| rs := k.Bool("limit-sample") | ||
| var rl *rate.Limiter | ||
| if rr > 0 { | ||
| rl = rate.NewLimiter(rate.Limit(rr), int(math.Ceil(rr))) |
There was a problem hiding this comment.
The variable rl is declared at line 217 and then reassigned at line 271 for input rate limiting. The comment at line 269 states 'NB: separate from the callback rate limit' but both use the same variable name and same rate value, which is confusing. Consider using different variable names (e.g., cbLimit and inputLimit) to make the separation clearer.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pkg/util/net.go:174
- The TLS configuration only sets
InsecureSkipVerifybut doesn't configure theServerNamefield. For proper TLS certificate validation, the ServerName should be extracted from the address parameter to ensure the certificate matches the expected hostname.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !send_safe(eio.Output, bb) { | ||
| mx.Callback.Drop() | ||
| if !util.Send(eio.Output, bb) { | ||
| mx.Callback.Disable() |
There was a problem hiding this comment.
The method was changed from Drop() to Disable(), but this change is not visible in the diff. Verify that the Disable() method exists in the callback type and behaves as expected, as this could cause a compilation error if the method doesn't exist.
| mx.Callback.Disable() | |
| mx.Callback.Drop() |
|
|
||
| // fix callbacks | ||
| if limit_rate > 0 { | ||
| rl = rate.NewLimiter(rate.Limit(limit_rate), int(math.Ceil(limit_rate))) |
There was a problem hiding this comment.
The math.Ceil import is used here but math is not imported in the visible imports section. Ensure that math is imported in this file.
No description provided.