-
Notifications
You must be signed in to change notification settings - Fork 94
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 support for tun/tap devices #14
base: master
Are you sure you want to change the base?
Conversation
Hi Luis! Thank you very much for your contribution! Overall, it looks good to me and I'll be happy to merge this PR. Would you mind making some minor adjustments before I merge it? I left a few comments there. |
Makefile
Outdated
@@ -93,3 +93,10 @@ clean-docker-images: | |||
echo "<noop>";\ | |||
fi | |||
|
|||
net: | |||
tunctl -t tap0 -u `whoami` | |||
brctl addif docker0 tap0 |
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.
Probably docker0
could be replaced with a dynamic lookup of the bridge interface name docker network inspect -f {{.Options}} bridge
. Although it still wouldn't be a fully universal solution, it'd make this target more universal.
Additionally, the bridge's interface name could become a variable with the default name derived from Docker's default bridge interface. In such case, a user would be able to override it via an environment variable.
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.
replaced for ip and a variable
Makefile
Outdated
@@ -93,3 +93,10 @@ clean-docker-images: | |||
echo "<noop>";\ | |||
fi | |||
|
|||
net: | |||
tunctl -t tap0 -u `whoami` |
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.
Both tunctl
and brctl
are deprecated at this time. I'd ask you to use ip
instead. It'd also decrease the needed dependencies for this project.
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.
didnt know they were going to deprecate it :(
Makefile
Outdated
brctl addif docker0 tap0 | ||
ip link set tap0 up | ||
|
||
run-debian: |
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.
This target doesn't really mention debian
anywhere in the command. Probably, run-qemu-with-net
or something like that would be a more suitable name. Also, it de facto depends on the linux.img
and net
targets. I think you could just reflect this fact in the target line.
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.
replaced
Hello there,
Thanks for making this project opensource. Its really useful for me already!
Idea of this PR is to add network support. Not sure if we want all of them (nat/bridged/etc). For now already works if you have a 'docker0' interface which is pretty common I'd say.
Any ideas/feedback are appreciated :)