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

allow specifying --wg-iface and --wg-port #659

Closed
wants to merge 1 commit into from

Conversation

nazarewk
Copy link
Contributor

@nazarewk nazarewk commented Jan 20, 2023

Describe your changes

I am working on setting up multiple instances of Netbird on same machine, I need a way to tell the deamon to input different interfaces/ports to those without modifying files by hand.

I got it to work at nazarewk-iac/nix-configs@aeefbf5c

Issue ticket number and link

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@nazarewk
Copy link
Contributor Author

I guess adding entries to interfaces blacklist would be handy too

@mlsmaycon
Copy link
Collaborator

hello @nazarewk, thanks for the contribution.

The Pr is still with a Draft status. I have a few comments to add and some requests as well, but I wonder if you want to add more to it.

@nazarewk nazarewk marked this pull request as ready for review February 1, 2023 13:21
@nazarewk
Copy link
Contributor Author

nazarewk commented Feb 1, 2023

hello @nazarewk, thanks for the contribution.

The Pr is still with a Draft status. I have a few comments to add and some requests as well, but I wonder if you want to add more to it.

I have what I personally need in the PR, but I'm up for comments.

Copy link
Collaborator

@mlsmaycon mlsmaycon left a comment

Choose a reason for hiding this comment

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

thanks @nazarewk. I left some comments regarding the input; we are moving things a bit to be configured through the up command to prevent reinstallation of the daemon.

client/cmd/root.go Outdated Show resolved Hide resolved
client/server/server.go Outdated Show resolved Hide resolved
client/internal/config.go Show resolved Hide resolved
client/cmd/login.go Outdated Show resolved Hide resolved
client/cmd/root.go Outdated Show resolved Hide resolved
@nazarewk nazarewk changed the title allow specifying --wg-iface and --wg-port on start allow specifying --wg-iface and --wg-port Feb 1, 2023
@nazarewk nazarewk force-pushed the feature/custom-iface-port branch 2 times, most recently from ccb33b5 to 3bdbc96 Compare February 1, 2023 15:20
client/internal/config.go Show resolved Hide resolved
client/internal/config.go Show resolved Hide resolved
@nazarewk
Copy link
Contributor Author

nazarewk commented Feb 2, 2023

what is the difference between netbird up and netbird service run/start/stop? After moving flags to up I'm not getting those reflected in my service file.

@mlsmaycon
Copy link
Collaborator

mlsmaycon commented Feb 2, 2023

what is the difference between netbird up and netbird service run/start/stop? After moving flags to up I'm not getting those reflected in my service file.

The service start and stop only controls the OS system start and stop of a service. The run starts netbird as a daemon process, which listens for commands and updates the process execution.

I saw that you updated the flags to be added with the service run, but the problem there is that to update a configuration you need to reinstall the client for most installations, this is due because the netbird service install generates a static service configuration and not updating it will make every restart restore any update made with up

Comment on lines +241 to +288
if !slices.Contains(config.IFaceBlackList, config.WgIface) {
var newBlacklist []string
replaced := false
for _, ifc := range config.IFaceBlackList {
if ifc == oldIface {
ifc = config.WgIface
replaced = true
}
newBlacklist = append(newBlacklist, ifc)
}
if !replaced {
newBlacklist = append(newBlacklist, config.WgIface)
}
log.Infof("prepending Wireguard Interface Name to interface blacklist %#v, updated to %v (old value %v)", config.WgIface, newBlacklist, config.IFaceBlackList)
config.IFaceBlackList = newBlacklist
refresh = true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if !slices.Contains(config.IFaceBlackList, config.WgIface) {
var newBlacklist []string
replaced := false
for _, ifc := range config.IFaceBlackList {
if ifc == oldIface {
ifc = config.WgIface
replaced = true
}
newBlacklist = append(newBlacklist, ifc)
}
if !replaced {
newBlacklist = append(newBlacklist, config.WgIface)
}
log.Infof("prepending Wireguard Interface Name to interface blacklist %#v, updated to %v (old value %v)", config.WgIface, newBlacklist, config.IFaceBlackList)
config.IFaceBlackList = newBlacklist
refresh = true
}
if oldIface != config.WgIface {
for i, ifc := range config.IFaceBlackList {
if ifc == oldIface {
ifc = config.WgIface
config.IFaceBlackList[i] = config.WgIface
log.Infof("replaced Wireguard Interface Name in the interface blacklist. old name %s, new name", oldIface, config.WgIface)
refresh = true
break
}
}
}

Comment on lines +235 to +270
if input.WgPort != 0 && input.WgPort != config.WgPort {
log.Infof("new Wireguard Port provided, updated to %#v (old value %#v)", input.WgPort, config.WgPort)
config.WgPort = input.WgPort
refresh = true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if input.WgPort != 0 && input.WgPort != config.WgPort {
log.Infof("new Wireguard Port provided, updated to %#v (old value %#v)", input.WgPort, config.WgPort)
config.WgPort = input.WgPort
refresh = true
}

Comment on lines +215 to 250
if config.WgPort == 0 && input.WgPort == 0 {
log.Infof("using default Wireguard Port: %#v ", iface.DefaultWgPort)
config.WgPort = iface.DefaultWgPort
refresh = true
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if config.WgPort == 0 && input.WgPort == 0 {
log.Infof("using default Wireguard Port: %#v ", iface.DefaultWgPort)
config.WgPort = iface.DefaultWgPort
refresh = true
}
if input.WgPort != 0 && input.WgPort != config.WgPort {
log.Infof("new Wireguard Port provided, updated to %d (old value %d)", input.WgPort, config.WgPort)
config.WgPort = input.WgPort
refresh = true
}
// previous version config upgrade check because WgPort didn't exist
if config.WgPort == 0 {
log.Infof("using default Wireguard Port: %d", iface.DefaultWgPort)
config.WgPort = iface.DefaultWgPort
refresh = true
}

client/internal/config.go Show resolved Hide resolved
@mlsmaycon
Copy link
Collaborator

@nazarewk, my apologies, I forgot about the protocol update required to pass the new flags from the client to the daemon.

You need to update the protocol file here /client/proto/daemon.proto#L31 and then you can run the generate.sh

Once that is done, you need to retrieve them in the https://github.com/netbirdio/netbird/blob/main/client/server/server.go#L158 and pass to the configuration calls

With that updating the port will look like this:

netbird down
netbird up --wg-port 5000

@mlsmaycon
Copy link
Collaborator

@nazarewk let us know if the requests were too much. I would be happy to join the effort and help you in your branch

@nazarewk
Copy link
Contributor Author

nazarewk commented Feb 8, 2023

@mlsmaycon I can't continue this right now (maybe next week) and I'm not planning to become Netbird architecture expert so feel free to help and fix the things you mentioned :)

@mlsmaycon
Copy link
Collaborator

@nazarewk, thanks for the contribution. We have decided to move forward with the flag support in the following PR: #1467

For now, I will close this one, and we can discuss further improvements on a separate ticket/issue.

@mlsmaycon mlsmaycon closed this Jan 15, 2024
@nazarewk nazarewk deleted the feature/custom-iface-port branch January 15, 2024 14:06
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.

None yet

2 participants