Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: generalize bgp server to allow any net.Listener #398

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

feat: generalize bgp server to allow any net.Listener #398

wants to merge 4 commits into from

Conversation

tobikris
Copy link
Contributor

This allows to implement in-memory bgp servers using the same code.
They can be used for testing bio-routing as well as some other use cases.

@tobikris
fix: nil pointer for local address with ebgp
c5d25a6
@tobikris
feat: generalize bgp server to allow any net.Listener
2f79afa 
this allows to implement in-memory bgp servers using the same code.
they can be used for testing bio-routing as well as some other use cases.
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2022 •
edited
Loading

Codecov Report

Merging #398 (b554c27) into master (91dfd06) will increase coverage by 3.40%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   55.14%   58.55%   +3.40%     
==========================================
  Files         147      147              
  Lines        7672     7702      +30     
==========================================
+ Hits         4231     4510     +279     
+ Misses       3245     2967     -278     
- Partials      196      225      +29
Flag Coverage Δ
unittests 58.55% <50.00%> (+3.40%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
protocols/bgp/server/tcplistener.go 0.00% <0.00%> (ø)
protocols/bgp/server/server.go 54.96% <60.00%> (+37.68%) ⬆️
protocols/bgp/server/peer.go 41.23% <71.42%> (+39.10%) ⬆️
protocols/bgp/server/fsm.go 67.59% <100.00%> (+39.66%) ⬆️
protocols/bgp/server/fsm_established.go 45.80% <0.00%> (+2.29%) ⬆️
protocols/bgp/server/fsm_address_family.go 78.04% <0.00%> (+2.43%) ⬆️
protocols/bgp/server/fsm_open_sent.go 52.09% <0.00%> (+11.37%) ⬆️
protocols/bgp/server/fsm_open_confirm.go 16.90% <0.00%> (+14.08%) ⬆️
protocols/bgp/server/peer_role.go 28.57% <0.00%> (+28.57%) ⬆️
protocols/bgp/server/fsm_active.go 28.57% <0.00%> (+28.57%) ⬆️
... and 6 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tobikris
Copy link
Contributor Author

tobikris commented Dec 1, 2022 •
edited
Loading

I would like to add the router ID to all log lines originating from a BGP server instance. The direct calls are trivial, do you think I should do something similar for the deeper calls like the FSM logging?

go run ./examples/bgp-in-memory
INFO[0000] Starting memory server: 172.17.0.3           
INFO[0000] Starting memory server: 169.254.200.0        
DEBU[0000] setTCPMD5 called on dummyListener, ignoring...  router_id=172.17.0.3
INFO[0000] Added BGP peer                                local_address=172.17.0.3 local_as=65100 peer_address=169.254.200.0 peer_as=65101 router_id=172.17.0.3
DEBU[0000] setTCPMD5 called on dummyListener, ignoring...  router_id=169.254.200.0
INFO[0000] Added BGP peer                                local_address=169.254.200.0 local_as=65101 peer_address=172.17.0.3 peer_as=65100 router_id=169.254.200.0
DEBU[0000] AddPath to locRIB                             Prefix=10.0.0.0/8 Route="Local Pref: 3, Origin: IGP, AS Path: , BGP type: external, NEXT HOP: 172.17.0.3, MED: 0, Path ID: 10, Source: 172.17.0.3, "
INFO[0003] FSM: Neighbor state change                    last_state=idle new_state=connect peer=169.254.200.0 reason="Received AutomaticStart event"
INFO[0003] Incoming TCP connection                       router_id=169.254.200.0 source="172.17.0.3:0"
DEBU[0003] Sending incoming TCP connection to fsm for peer  peer=172.17.0.3 router_id=169.254.200.0
INFO[0003] FSM: Neighbor state change                    last_state=active new_state=openSent peer=172.17.0.3 reason="Sent OPEN message"
INFO[0003] FSM: Neighbor state change                    last_state=connect new_state=openSent peer=169.254.200.0 reason="TCP connection succeeded"
INFO[0003] FSM: Neighbor state change                    last_state=openSent new_state=openConfirm peer=172.17.0.3 reason="Received OPEN message"
INFO[0003] FSM: Neighbor state change                    last_state=openConfirm new_state=established peer=172.17.0.3 reason="Received KEEPALIVE"
INFO[0003] FSM: Neighbor state change                    last_state=openSent new_state=openConfirm peer=169.254.200.0 reason="Received OPEN message"
INFO[0003] FSM: Neighbor state change                    last_state=openConfirm new_state=established peer=169.254.200.0 reason="Received KEEPALIVE"
DEBU[0003] AddPath to locRIB                             Prefix=10.0.0.0/8 Route="Local Pref: 0, Origin: IGP, AS Path: 65100, BGP type: external, NEXT HOP: 172.17.0.3, MED: 0, Path ID: 0, Source: 172.17.0.3, "

I would probably also like to be able to override the logger for each BGP server manually from the caller side.
This would allow for example to mute server logging for some servers completely or redirect them to separate files.

@tobikris
Copy link
Contributor Author

tobikris commented Dec 6, 2022

I have added a testcase to verify the correctness of multiple BGP speakers in one process.

The test case is currently failing:

--- FAIL: TestInMemoryServerMinimalPaths (2.10s)
    server_test.go:122: 
                Error Trace:    /home/tobikris/dev/git.exaring.net/network/bio-rd/protocols/bgp/server/server_test.go:122
                                                        /home/tobikris/dev/git.exaring.net/network/bio-rd/protocols/bgp/server/asm_amd64.s:1594
                Error:          Not equal: 
                                expected: 1
                                actual  : 2
                Test:           TestInMemoryServerMinimalPaths
                Messages:       Expected 1 path, got 2
    server_test.go:123: [Local Pref: 0, Origin: IGP, AS Path: 65101, BGP type: external, NEXT HOP: 172.16.0.2, MED: 0, Path ID: 0, Source: 172.16.0.1,  Local Pref: 0, Origin: IGP, AS Path: 65101, BGP type: external, NEXT HOP: 172.16.0.1, MED: 0, Path ID: 0, Source: 172.16.0.1, ]
    server_test.go:126: 
                Error Trace:    /home/tobikris/dev/git.exaring.net/network/bio-rd/protocols/bgp/server/server_test.go:126
                                                        /home/tobikris/dev/git.exaring.net/network/bio-rd/protocols/bgp/server/asm_amd64.s:1594
                Error:          Should be true
                Test:           TestInMemoryServerMinimalPaths
                Messages:       Expected next hop to be 172.16.0.1, got 172.16.0.2
    server_test.go:133: 
                Error Trace:    /home/tobikris/dev/git.exaring.net/network/bio-rd/protocols/bgp/server/server_test.go:133
                                                        /home/tobikris/dev/git.exaring.net/network/bio-rd/protocols/bgp/server/asm_amd64.s:1594
                Error:          Not equal: 
                                expected: 1
                                actual  : 2
                Test:           TestInMemoryServerMinimalPaths
                Messages:       Expected 1 path, got 2
    server_test.go:134: [Local Pref: 0, Origin: IGP, AS Path: 65102 65101, BGP type: external, NEXT HOP: 172.16.0.2, MED: 0, Path ID: 0, Source: 172.16.0.2,  Local Pref: 0, Origin: IGP, AS Path: 65102 65101, BGP type: external, NEXT HOP: 172.16.0.2, MED: 0, Path ID: 0, Source: 172.16.0.2, ]
    server_test.go:137: 
                Error Trace:    /home/tobikris/dev/git.exaring.net/network/bio-rd/protocols/bgp/server/server_test.go:137
                                                        /home/tobikris/dev/git.exaring.net/network/bio-rd/protocols/bgp/server/asm_amd64.s:1594
                Error:          Should be true
                Test:           TestInMemoryServerMinimalPaths
                Messages:       Expected next hop to be 172.16.0.1, got 172.16.0.2
FAIL
FAIL    github.com/bio-routing/bio-rd/protocols/bgp/server      3.208s
FAIL

However, I do not find the relevant code path responsible for this.

@taktv6 @BarbarossaTM: Is my test premise valid in the first place? Only one path is expected, correct? The incorrect next-hop is probably caused by the singular cache structure being reused by multiple servers?

@taktv6
Copy link
Member

taktv6 commented Jul 21, 2024

Looking into this I'm wondering how this is caused. Even with just two instances it happens.

@taktv6
Copy link
Member

taktv6 commented Jul 21, 2024

Could it be both instance 1 and 2 initiate connections to each other and don't clean up the collision?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tobikris @codecov-commenter @taktv6