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 CNI bridge network provider #4352

Merged
merged 1 commit into from
Jan 10, 2024
Merged

Conversation

tonistiigi
Copy link
Member

fixes #3864

This adds a new network configuration based on CNI bridge. Unlike the existing CNI provider this does not require the user to provide CNI configuration and plugins externally.

The minimal set of plugins required to use the network mode is provided together with buildkit. Currently, bridge mode is opt-in, but the intention is to make it default after a testing period of one release cycle.

Unlike the CNI provider, if the bridge interface was created by buildkitd, it is cleaned up automatically when buildkitd is shut down.

@tonistiigi tonistiigi force-pushed the cni-bridge branch 2 times, most recently from 16c853b to e895f73 Compare October 19, 2023 17:10
@@ -71,6 +71,8 @@ type NetworkConfig struct {
CNIConfigPath string `toml:"cniConfigPath"`
CNIBinaryPath string `toml:"cniBinaryPath"`
CNIPoolSize int `toml:"cniPoolSize"`
BridgeName string `toml:"bridgeName"`
// BridgeSubnet string `toml:"bridgeSubnet"`
Copy link
Member Author

Choose a reason for hiding this comment

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

@AkihiroSuda @jedevc If we add #4099 then I think we need to add some reserved IP ranges that the tunnels created via LLB should use. If this BridgeSubnet collides with such ranges we could give an error and buildkitd would not start. Otherwise, if user would use an IP that collides with the configured bridge then it would not be possible to create such tunnels. Does this make sense?

]
}
}
]
Copy link
Member

Choose a reason for hiding this comment

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

Can we have ingressPolicy": "same-bridge" ?
https://www.cni.dev/plugins/current/meta/firewall/#ingress-policy

Copy link
Member Author

Choose a reason for hiding this comment

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

So this would be another plugin we would need to include?

Copy link
Member

Choose a reason for hiding this comment

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

Yes (firewall plugin)

"type": "%s",
"ranges": [
[
{ "subnet": "10.10.0.0/16" }
Copy link
Member

Choose a reason for hiding this comment

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

This should be customizable

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see #4352 (comment)

@tonistiigi
Copy link
Member Author

@gabriel-samfira What approach we should take for wcow with this? Just disallow for now? I see there is also https:/containernetworking/plugins/tree/main/plugins/main/windows/win-bridge but I don't know much about it.

@tonistiigi tonistiigi force-pushed the cni-bridge branch 2 times, most recently from ee0c9fd to feaf181 Compare December 5, 2023 06:40
@tonistiigi tonistiigi marked this pull request as ready for review December 5, 2023 06:40
@gabriel-samfira
Copy link
Collaborator

@gabriel-samfira What approach we should take for wcow with this? Just disallow for now? I see there is also https:/containernetworking/plugins/tree/main/plugins/main/windows/win-bridge but I don't know much about it.

Sorry for the late reply.

Windows container networking lives here:

https:/microsoft/windows-container-networking

The L2Bridge CNI is the sdnbridge one located here: https:/microsoft/windows-container-networking/tree/master/plugins/sdnbridge

But if the goal is to permit basic network connectivity, I'd recommend using the NAT CNI on Windows:

https://gist.github.com/gabriel-samfira/6e56238ad11c24f490ac109bdd378471#file-setup_buildkitd_on_windows-ps1-L45-L64

@tonistiigi
Copy link
Member Author

@gabriel-samfira I think we can cover these in a follow-up if necessary as looks like completely separate code would be needed anyway. You can take a look at #4099 as well to see what the approach for wcow could be for that.

nc.BridgeName = "buildkit0"
}
if nc.BridgeSubnet == "" {
nc.BridgeSubnet = "10.10.0.0/16"
Copy link
Member

Choose a reason for hiding this comment

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

In a follow up PR, could you add a documentation to explain this default CIDR and other configuration?

Dockerfile Outdated
COPY --link --from=cni-plugins /opt/cni/bin/bridge /buildkit-cni-bridge
COPY --link --from=cni-plugins /opt/cni/bin/loopback /buildkit-cni-loopback
COPY --link --from=cni-plugins /opt/cni/bin/host-local /buildkit-cni-host-local
COPY --link --from=cni-plugins /opt/cni/bin/firewall /buildkit-firewall
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
COPY --link --from=cni-plugins /opt/cni/bin/firewall /buildkit-firewall
COPY --link --from=cni-plugins /opt/cni/bin/firewall /buildkit-cni-firewall

for consistency

Copy link
Member

Choose a reason for hiding this comment

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

@AkihiroSuda AkihiroSuda added this to the v0.13 milestone Jan 7, 2024
This adds a new network configuration based on CNI bridge.
Unlike the existing CNI provider this does not require
user to provide CNI configuration and plugins externally.

The minimal set of plugins required to use the network mode
is provided together with buildkit. Currently, bridge
mode is opt-in but intention is to make it default after
a testing period of one release cycle.

Signed-off-by: Tonis Tiigi <[email protected]>
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.

path to CNI bridge by default
3 participants