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

Move to netip and bump github.com/mdlayher/ndp #2376

Open
2 tasks done
fedepaol opened this issue Apr 17, 2024 · 5 comments
Open
2 tasks done

Move to netip and bump github.com/mdlayher/ndp #2376

fedepaol opened this issue Apr 17, 2024 · 5 comments

Comments

@fedepaol
Copy link
Member

Is your feature request related to a problem?

The ndp package we use moved to use netaddr. It would make sense for us to start using it all over metallb not only because it would allow us to bump ndp but because it's more efficient (more details on https://tailscale.com/blog/netaddr-new-ip-type-for-go )

Describe the solution you'd like

Replacing all the net.Something instance with the corresponding netaddr, add a translation only where an external library accepts net.Something.

Additional context

No response

I've read and agree with the following

  • I've checked all open and closed issues and my request is not there.
  • I've checked all open and closed pull requests and my request is not there.
@fulviodenza
Copy link

Hi I'd like to take this if possible. I will be looking at the code and at the library and will be back later.

@fedepaol
Copy link
Member Author

Hi I'd like to take this if possible. I will be looking at the code and at the library and will be back later.

cool! Assigning this to you, if you have any questions reach out on the #metallb-dev slack channel

@fulviodenza
Copy link

So, I've been giving a look at the library and at the ndp dependency and I have a few considerations:

  1. The library linked seems to be deprecated in favor of this library
  2. I've noticed that metallb is using a quite old version of ndp, while the newer versions of this library use net/netip. Shall we consider using that?
    Unless I didn't look in the right, place, it looks to me that we shall consider uysing the net/netip library

Let me know what do you think, thank you!

@fedepaol
Copy link
Member Author

So, I've been giving a look at the library and at the ndp dependency and I have a few considerations:

1. The library linked seems to be deprecated in favor of [this library](https://pkg.go.dev/go4.org/netipx)

2. I've noticed that metallb is using a quite old version of ndp, while [the newer versions of this library use net/netip](https://github.com/mdlayher/ndp/commit/3c95c205b9a485f6eeb29632df4c89504a2353dc). Shall we consider using that?
   Unless I didn't look in the right, place, it looks to me that we shall consider uysing the net/netip library

Let me know what do you think, thank you!

Yes, it was just me looking at the wrong place. I remembered ndp to use the new type in the go stdlib, remembered the post about tailscale (who contributed to the std lib) and went looking for it to find the name of the package (which eventually was the intermediate one).

So, TL;DR: we should use net/netip which comes with the stdlib, sorry again for the confusion 😅

I'll rename the issue.

@fedepaol fedepaol changed the title Move to netaddr and bump github.com/mdlayher/ndp Move to netip and bump github.com/mdlayher/ndp Apr 22, 2024
@fedepaol
Copy link
Member Author

@fulviodenza you might find this article useful

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

No branches or pull requests

2 participants