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

Tap plugin #784

Merged
merged 1 commit into from Feb 13, 2023
Merged

Tap plugin #784

merged 1 commit into from Feb 13, 2023

Conversation

mmirecki
Copy link
Contributor

@mmirecki mmirecki commented Nov 17, 2022

This PR adds a plugin to create tap devices.
The plugin adds a tap device to the container.

The plugin has a workaround for a golang netlink library
which does not allow for tap devices with no owner/group
to be created. When no tap owner/group is requested, the
plugin will fall back to using the ip tool for creating
the tap device. A fix to the golang netlink lib is pending:
vishvananda/netlink#835

@mmirecki mmirecki force-pushed the tap-plugin branch 4 times, most recently from ac1e7ca to 20b47b6 Compare November 21, 2022 11:28
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
@mmirecki mmirecki force-pushed the tap-plugin branch 6 times, most recently from 55fa761 to 2c65844 Compare November 23, 2022 13:41
@dougbtv
Copy link

dougbtv commented Nov 23, 2022

cc: @mccv1r0 too

@mmirecki mmirecki force-pushed the tap-plugin branch 2 times, most recently from 2fc8ec3 to 14ac542 Compare November 25, 2022 10:34
@dougbtv
Copy link

dougbtv commented Nov 30, 2022

Conceptually it seems good to me!

plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap_suite_test.go Outdated Show resolved Hide resolved
plugins/main/tap/tap_test.go Outdated Show resolved Hide resolved
@mmirecki mmirecki force-pushed the tap-plugin branch 5 times, most recently from 32a31ff to 5d37272 Compare December 6, 2022 12:29
@EmilyShepherd
Copy link
Contributor

NB: The test failure on this PR appears to be unrelated to these changes.

Please see #793 for a potential fix.

@mmirecki
Copy link
Contributor Author

mmirecki commented Dec 7, 2022

NB: The test failure on this PR appears to be unrelated to these changes.

Please see #793 for a potential fix.

Thanks for the tip
Added the fix on top of my changes. Will remove it if the fix gets in first.

Copy link
Contributor

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

nice work!

two small nits

plugins/main/tap/tap_suite_test.go Outdated Show resolved Hide resolved
plugins/meta/portmap/portmap_integ_test.go Outdated Show resolved Hide resolved
@mmirecki mmirecki force-pushed the tap-plugin branch 3 times, most recently from fa2eccd to 6a616b0 Compare January 19, 2023 12:01
@squeed
Copy link
Member

squeed commented Jan 23, 2023

Summarizing some discussion we had in the maintainer's meeting:

  • we can't be distro specific
  • the SELinux labels should just be a configuration option; the maintainer can supply it.
  • due to an issue in netlink you currently cannot specify file-less TAPs, so we need to fallback to the ip tool
  • Just like the SELinux label, the uid and gid of the tap device should be a *int32 in the plugin configuration

@mmirecki
Copy link
Contributor Author

Summarizing some discussion we had in the maintainer's meeting:

  • we can't be distro specific
  • the SELinux labels should just be a configuration option; the maintainer can supply it.
  • due to an issue in netlink you currently cannot specify file-less TAPs, so we need to fallback to the ip tool
  • Just like the SELinux label, the uid and gid of the tap device should be a *int32 in the plugin configuration

@squeed Could you please take a look again?

Copy link
Contributor

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

small comments

plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

Really nice work.

Most of what I have are opinionated nits, but I would like to see a README with info on how to use this particular plugin, and I question the proposed API (providing the MAC address of the tap via something named IPAMArgs .

plugins/main/tap/tap_suite_test.go Outdated Show resolved Hide resolved
plugins/main/tap/tap_test.go Outdated Show resolved Hide resolved
plugins/main/tap/tap_test.go Outdated Show resolved Hide resolved
plugins/main/tap/tap_test.go Show resolved Hide resolved
plugins/main/tap/tap_test.go Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
plugins/main/tap/tap.go Show resolved Hide resolved
plugins/main/tap/tap.go Outdated Show resolved Hide resolved
@mmirecki mmirecki force-pushed the tap-plugin branch 2 times, most recently from f8bb33f to 81307ea Compare February 6, 2023 16:29
@dcbw
Copy link
Member

dcbw commented Feb 6, 2023

I'm OK with the updates WRT selinux stuff and the /sbin/ip vs. netlink issues.

@dcbw
Copy link
Member

dcbw commented Feb 6, 2023

@squeed how about now?

plugins/main/tap/tap.go Outdated Show resolved Hide resolved
Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

Still missing the README, but I'm OK with having it as a follow-up.

Great work, thank you.

@mmirecki
Copy link
Contributor Author

mmirecki commented Feb 8, 2023

@squeed Can you please have a look again?

This PR adds a plugin to create tap devices.
The plugin adds a tap device to the container.

The plugin has a workaround for a golang netlink library
which does not allow for tap devices with no owner/group
to be created. When no tap owner/group is requested, the
plugin will fall back to using the ip tool for creating
the tap device. A fix to the golang netlink lib is pending.

Signed-off-by: mmirecki <mmirecki@redhat.com>
@squeed squeed merged commit fb92605 into containernetworking:main Feb 13, 2023
}
}

func createTap(conf *NetConf, ifName string, netns ns.NetNS) (*current.Interface, error) {
Copy link

@fahedouch fahedouch May 10, 2023

Choose a reason for hiding this comment

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

Hi @mmirecki,
thank you for your greate job. Is it possible to export this func we may need to refacto container rootlesskit code. If LGTY I can take care thanks

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