-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add extra parameter for dual stack (IPv4+IPv6) #407
Conversation
1c860cd
to
d1046d5
Compare
main.go
Outdated
@@ -186,10 +191,11 @@ func main() { | |||
// ******************************************************************************** | |||
log.FromContext(ctx).Infof("executing phase 3: creating icmp server ipam") | |||
// ******************************************************************************** | |||
_, ipnet, err := net.ParseCIDR(config.CidrPrefix) | |||
ipamChain, err := getIPAMChain(config.CidrPrefix, config.Ipv6Prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we simply have this take a list similar to how we handle it in cmd-forwarder-vpp ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwarnicke Sure, technically is not a big deal.
The reason to have this additional parameter is to be aligned with the cmd-nse-remote-vlan, where it is already used for dual-stack.
https://github.com/networkservicemesh/cmd-nse-remote-vlan/blob/51251b32c2c6be7f9f943dac4de17be9c5188f45/internal/pkg/config/config.go#L45
To deprecate that one would be a not backward compatible change. Maybe I should add the possibility on cmd-nse-remote-vlan to have a list in CidrPrefix parameter also and leave the Ipv6Prefix as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its also possible I'm just weird as I simply see dual stack as 'We have more than one IP, and they happen to be from different families'... :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwarnicke No, you are right - as usually :)
Updated the commit and the related PR: networkservicemesh/cmd-nse-remote-vlan#13
Also have a test update, waiting for implementation PRs to be merged: networkservicemesh/deployments-k8s#5569
Signed-off-by: Laszlo Kiraly <laszlo.kiraly@est.tech>
d1046d5
to
95a0779
Compare
…d-nse-icmp-responder@main PR link: networkservicemesh/cmd-nse-icmp-responder#407 Commit: 3b970e2 Author: Ed Warnicke Date: 2022-05-03 09:44:47 -0500 Message: - Merge pull request #407 from Nordix/imp-dual-stack Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
Signed-off-by: Laszlo Kiraly laszlo.kiraly@est.tech