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.

Sign up for GitHub

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

Jump to bottom

tap: allow for a tap device to be created as a bridge port #832

Merged
merged 1 commit into from May 22, 2023
Merged

tap: allow for a tap device to be created as a bridge port #832

merged 1 commit into from May 22, 2023

Conversation

maiqueb
Copy link
Contributor

@maiqueb maiqueb commented Feb 17, 2023 •
edited

This extends the tap plugin API enabling the user to instruct the CNI plugin the created tap device must be set as a port of an existing linux bridge on the pod network namespace.

This is helpful for KubeVirt, allowing network connectivity to be extended from the pod's interface into the Virtual Machine running inside the pod.

@tjjh89017
Copy link

@maiqueb Hi
I'm curious to know about the usage of this TAP CNI.
IIRC, tap device will be inside pod.
So this PR will ask CNI to set Tap's master to bridge inside the same pod.
Is it correct?
thanks a lot.

@maiqueb
Copy link
Contributor Author

maiqueb commented Feb 20, 2023

@maiqueb Hi I'm curious to know about the usage of this TAP CNI. IIRC, tap device will be inside pod. So this PR will ask CNI to set Tap's master to bridge inside the same pod. Is it correct? thanks a lot.

Yes, both the tap device and the bridge must be inside the same network namespace (e.g. the pod).

mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 20, 2023
@mlguerrero12
Add tap test
a64c5bf 
Test for the following PR:
containernetworking/plugins#832
@mlguerrero12 mlguerrero12 mentioned this pull request Feb 20, 2023
Merged
mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 20, 2023
@mlguerrero12
Add tap plugin test
f1a8bc9 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 20, 2023
@mlguerrero12
Add tap plugin test
d83eb5d 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
@squeed
Copy link
Member

squeed commented Feb 20, 2023

This looks good, but could you rebase real quick?

@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from bd8eaed to 9a43e72 Compare February 21, 2023 10:11
@maiqueb
Copy link
Contributor Author

maiqueb commented Feb 21, 2023

@squeed

This looks good, but could you rebase real quick?

Done. Thanks for the quick review / approval.

mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 22, 2023
@mlguerrero12
Add tap plugin test
95f7e41 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 22, 2023
@mlguerrero12
Add tap plugin test
a4925f6 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 23, 2023
@mlguerrero12
Add tap plugin test
67c2551 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 23, 2023
@mlguerrero12
Add tap plugin test
12114da 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 23, 2023
@mlguerrero12
Add tap plugin test
e37ba3f 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
mlguerrero12 added a commit to mlguerrero12/origin that referenced this pull request Feb 27, 2023
@mlguerrero12
Add tap plugin test
b461281 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from 9a43e72 to 2cbb041 Compare March 28, 2023 11:04
@maiqueb
Copy link
Contributor Author

maiqueb commented Mar 28, 2023

@mlguerrero12 IIUC this PR is failing on the lint stage for something unrelated to it.

Should I rebase #871 to address those ?

@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from 2cbb041 to 1f8c7fb Compare March 28, 2023 11:21
@maiqueb
Copy link
Contributor Author

maiqueb commented Mar 28, 2023

@dcbw / @squeed can we proceed with a merge for this PR ?

... I now need #871 in before, to address the lint errors.

@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from 1f8c7fb to 57d2aae Compare April 4, 2023 13:45
@maiqueb
Copy link
Contributor Author

maiqueb commented Apr 4, 2023

@squeed I think this could be merged now that #871 was merged.

@mlguerrero12
Copy link
Member

gingko-linter was just merged and this introduced new errors

@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from 57d2aae to 66ac4ac Compare April 4, 2023 13:51
@maiqueb
Copy link
Contributor Author

maiqueb commented Apr 4, 2023

gingko-linter was just merged and this introduced new errors

wow 😅 I usually point these ones out ... Should not have blindly copied an existing test .

Let's see if it is now OK.

@maiqueb
Copy link
Contributor Author

maiqueb commented Apr 4, 2023

OK, now I am not introducing any errors, but seems we now pull an additional linter that frowns upon the way ginkgo is used.

... don't get me wrong, I think the linter is actually right.

... but I also think it should complain exclusively of code introduced in this PR, not existing code.

@mlguerrero12
Copy link
Member

Correct, I was not referring to the new test of this pr, but just to let you know that we have now new issues in general.

@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch 2 times, most recently from 4dd73d0 to 616cec6 Compare April 4, 2023 14:24
@maiqueb
Copy link
Contributor Author

maiqueb commented Apr 4, 2023

Is this somehow a flake @squeed ?

Summarizing 1 Failure:
  [FAIL] chain tests [It] deletes chains idempotently in parallel
  /home/runner/work/plugins/plugins/plugins/meta/portmap/chain_test.go:224

I find it very strange that the contents of this PR (main tap plugin) somehow impact the meta portmap plugin ...

I'm not touching any library.

tjungblu pushed a commit to tjungblu/origin that referenced this pull request Apr 11, 2023
@mlguerrero12 @tjungblu
Add tap plugin test
5c5b844 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
tjungblu pushed a commit to tjungblu/origin that referenced this pull request Apr 11, 2023
@mlguerrero12 @tjungblu
Add tap plugin test
ef1609a 
Test for the following PR:
containernetworking/plugins#832

Signed-off-by: Marcelo Guerrero Viveros <marguerr@redhat.com>
@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from 616cec6 to b35e889 Compare May 16, 2023 12:40
@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from b35e889 to 4722336 Compare May 19, 2023 13:14
@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from 4722336 to 86cd865 Compare May 19, 2023 14:26
@maiqueb
tap: allow for a tap device to be created as a bridge port
edab9ef 
This extends the tap plugin API enabling the user to instruct the CNI
plugin the created tap device must be set as a port of an *existing*
linux bridge on the pod network namespace.

This is helpful for KubeVirt, allowing network connectivity to be
extended from the pod's interface into the Virtual Machine running
inside the pod.

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
@maiqueb maiqueb force-pushed the tap-plugin-set-as-bridge-port branch from 86cd865 to edab9ef Compare May 19, 2023 14:26
@dcbw
Copy link
Member

dcbw commented May 22, 2023

LGTM

@dcbw dcbw merged commit 6265f4e into containernetworking:main May 22, 2023
5 checks passed
@dcbw
Copy link
Member

dcbw commented May 22, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed approved these changes

@mlguerrero12 mlguerrero12 mlguerrero12 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@maiqueb @tjjh89017 @squeed @mlguerrero12 @dcbw