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

add support for tun/tap devices #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

pathcl
Copy link

@pathcl pathcl commented May 22, 2021

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 :)

@iximiuz
Copy link
Owner

iximiuz commented May 24, 2021

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
Copy link
Owner

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.

Copy link
Author

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`
Copy link
Owner

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.

Copy link
Author

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:
Copy link
Owner

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.

Copy link
Author

Choose a reason for hiding this comment

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

replaced

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.

2 participants