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

network device injector plugin #82

Merged
merged 1 commit into from
May 23, 2024

Conversation

aojea
Copy link
Contributor

@aojea aojea commented May 12, 2024

The network device inject plugin allow to inject network interfaces that are present in the host to the Pods.

The network interface can be renamed and a network configuration can be passed.

It is important to differentiate between network interface injection and CNI, as today, CNI is used for container runtimes to provider the network configuration, it performs also the creation and the configuration of the interfaces that are injected into the Pod namespace and provide as a result some properties like the assiged IPs that are consumed later by the upstream projects like Kubernetes.

@aojea
Copy link
Contributor Author

aojea commented May 12, 2024

/assign @samuelkarp

Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

This is a neat proof of concept! Just took a brief pass through. I'd love if we can model netdevs in the runtime spec (opencontainers/runtime-spec#1239), but in the meantime it's great to see that this approach works too.

@aojea aojea force-pushed the network-device-injector branch 2 times, most recently from 849aa8f to 0b32005 Compare May 14, 2024 10:34
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.31%. Comparing base (53d3371) to head (4d4784d).
Report is 2 commits behind head on main.

❗ Current head 4d4784d differs from pull request most recent head 0b32005. Consider uploading reports for the commit 0b32005 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   64.55%   57.31%   -7.24%     
==========================================
  Files          10        4       -6     
  Lines        1845     1305     -540     
==========================================
- Hits         1191      748     -443     
+ Misses        503      430      -73     
+ Partials      151      127      -24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

Very nice proof of concept.. see comments :)

plugins/network-device-injector/README.md Outdated Show resolved Hide resolved
plugins/network-device-injector/README.md Outdated Show resolved Hide resolved
plugins/network-device-injector/network-device-injector.go Outdated Show resolved Hide resolved
plugins/network-device-injector/network-device-injector.go Outdated Show resolved Hide resolved
plugins/network-device-injector/network-device-injector.go Outdated Show resolved Hide resolved
plugins/network-device-injector/README.md Show resolved Hide resolved
@aojea aojea force-pushed the network-device-injector branch 2 times, most recently from 04c0169 to ca24037 Compare May 23, 2024 12:52
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

plugins/network-device-injector/README.md Outdated Show resolved Hide resolved
plugins/network-device-injector/README.md Outdated Show resolved Hide resolved
plugins/network-device-injector/README.md Outdated Show resolved Hide resolved
plugins/network-device-injector/README.md Outdated Show resolved Hide resolved
plugins/network-device-injector/README.md Outdated Show resolved Hide resolved
plugins/network-device-injector/network-device-injector.go Outdated Show resolved Hide resolved
plugins/network-device-injector/network-device-injector.go Outdated Show resolved Hide resolved
plugins/network-device-injector/network-device-injector.go Outdated Show resolved Hide resolved
plugins/network-device-injector/network-device-injector.go Outdated Show resolved Hide resolved
Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

Can you squash the commits? I don't think there's value in preserving these intermediates.

plugins/network-device-injector/host-device.go Outdated Show resolved Hide resolved
The network device inject plugin allow to inject network interfaces
that are present in the host to the Pods.

The network interface can be renamed and a network configuration can
be passed.

It is important to differentiate between network interface injection and
CNI, as today, CNI is used for container runtimes to provider the
network configuration, it performs also the creation and the
configuration of the interfaces that are injected into the Pod
namespace and provide as a result some properties like the assiged IPs
that are consumed later by the upstream projects like Kubernetes.

Signed-off-by: Antonio Ojea <[email protected]>
@aojea
Copy link
Contributor Author

aojea commented May 23, 2024

Can you squash the commits? I don't think there's value in preserving these intermediates.

done, I just kept them to simplify the review, so you can see the changes :) squasshed

Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

LGTM!

@samuelkarp samuelkarp merged commit 7977ced into containerd:main May 23, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants