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.

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

Make BGP source address configurable #902

Merged
merged 2 commits into from Jul 7, 2021

Conversation

johananl
Copy link
Member

@johananl johananl commented Jul 2, 2021

This is a follow up for #672.

TODO:

  • Add docs
  • Add tests

NOTE: There doesn't seem to be an easy way to test the source address functionality. We would likely need to do some significant refactoring to the tests and possibly to the way we deal with BGP sessions in the code itself.

Fixes #670.

@johananl johananl marked this pull request as ready for review July 5, 2021 13:07
Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johananl Thanks! I had a cursory look, overall seems fine. I left some concerns I had with edge cases

internal/bgp/bgp.go Outdated Show resolved Hide resolved
internal/bgp/bgp.go Outdated Show resolved Hide resolved
internal/bgp/bgp.go Show resolved Hide resolved
internal/bgp/bgp.go Outdated Show resolved Hide resolved
@johananl johananl force-pushed the johananl/bgp-src-addr branch 2 times, most recently from d1b233a to 14801e7 Compare July 5, 2021 16:51
@johananl johananl requested a review from rata July 5, 2021 16:51
@johananl johananl force-pushed the johananl/bgp-src-addr branch 3 times, most recently from 58f225c to 86a9742 Compare July 6, 2021 14:27
Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! I've left two small comments (one is just a question), I don't think I need to review again even if you do one small change I asked in the comment (remove one sentence from docs).

I like the change to net.IP for the srcAddr param :)

internal/bgp/bgp.go Show resolved Hide resolved
internal/bgp/bgp.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@johananl johananl merged commit 9126567 into metallb:main Jul 7, 2021
@johananl johananl deleted the johananl/bgp-src-addr branch July 7, 2021 09:34
@flokli
Copy link

flokli commented Jul 26, 2021

Can you make a release containing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support selecting the source address when speaker sets up a bgp session
4 participants