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

feat(network): Configurable external address for Version network messages #8488

Merged
merged 3 commits into from
May 3, 2024

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to add an optional configurable external address to advertise in Version messages.

Close #1890

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?

Solution

I think this is the simplest change we can apply to support this feature. Having multiple external addresses and/or auto detection can be done in the context of #1893

Testing

Some basic test was added.

Review

Anyone can review.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

@oxarbitrage oxarbitrage added A-network Area: Network protocol updates or fixes C-feature Category: New features P-Low ❄️ labels May 1, 2024
@oxarbitrage oxarbitrage requested review from a team as code owners May 1, 2024 23:47
@oxarbitrage oxarbitrage requested review from arya2 and removed request for a team May 1, 2024 23:47
arya2
arya2 previously approved these changes May 2, 2024
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you. I left a few comments, none of them are blockers, feel free to resolve any or all of them without making changes.

It may be nice to add the new field to a test config in zebrad::tests::configs as well.

zebra-network/src/config.rs Outdated Show resolved Hide resolved
zebra-network/src/config.rs Outdated Show resolved Hide resolved
zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
zebrad/tests/acceptance.rs Outdated Show resolved Hide resolved
Co-authored-by: Arya <aryasolhi@gmail.com>
arya2
arya2 previously approved these changes May 2, 2024
@mergify mergify bot merged commit 239fcc8 into main May 3, 2024
137 checks passed
@mergify mergify bot deleted the issue1890 branch May 3, 2024 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes C-feature Category: New features P-Low ❄️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zebra should support separate local bind and external advertise addresses
2 participants