-
Notifications
You must be signed in to change notification settings - Fork 43
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
chore(pkg/wireguard): replace bash script with native code #161
base: main
Are you sure you want to change the base?
Conversation
bc379e4
to
7946165
Compare
go.mod
Outdated
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.
whew - I ran go mod tidy
because of the new golang.org/x/sys
dependency 😅
pkg/wireguard/wireguard.go
Outdated
return nil | ||
|
||
if _, err := os.Stat("/dev/net/tun"); os.IsNotExist(err) { | ||
if err := unix.Mknod("/dev/net/tun", unix.S_IFIFO|0o600, int(unix.Mkdev(10, 100))); err != nil { |
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.
Would appreciate 👀 on this part @jodevsa - I think it's correct.
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.
LGTM; It would be useful to try it out. We have basic integration-tests that runs on each PR but it only tests creating the resources. It doesn't really test connecting to the VPN and making sure connectivity works as expected :( https://github.com/jodevsa/wireguard-operator/blob/main/internal/it/it_test.go#L39
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.
Also, the default behaviour is to use kernal implementation of WG. I guess the environment where the IT runs supports the kernal implementation of WG. we can use https://github.com/jodevsa/wireguard-operator/blob/main/cmd/agent/main.go#L26
--wg-use-userspace-implementation
to test this feature manually or write an IT for it. What are your thoughts on this?
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.
btw, you can easily manually test your changes by building a release from your branch. You can access the following workflow which would do all the building for you :). You only need to run this workflow https://github.com/jodevsa/wireguard-operator/actions/workflows/manual-dev-release-workflow.yaml
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.
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.
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.
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.
Thanks - I think that seems reasonable. Will try and test something this evening maybe.
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.
@jodevsa I tested this and it seems to work the same as the kernel version, such I can successfully connect to my router (192.168.1.1) from 5G through a wireguard tunnel.
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.
thanks a lot!
7396427
to
5e18011
Compare
There is no reason to use bash, or even to call out to the shell for most of what was happening. Replacing it allows for better error handling, clarity and reliability. Fixes: jodevsa#79
5e18011
to
026d943
Compare
There is no reason to use bash, or even to call out to the shell for most of what was happening. Replacing it allows for better error handling, clarity and reliability.
Fixes: #79