Support for CNCF containernetworking plugins #281

Arvinderpal · 2019-02-08T01:21:22Z

Add support for CNCF containernetworking plugins; in particular, bridge and host-local ipam.
This approach extends KinD's Config with 3 additional fields: CNI, Config File Name and Config File Body.
If these fields are specified then KinD will copy the config file to the /etc/cni/net.d directory under the desired name.
In theory, this approach should work with other plugins that only require a configuration file and their binaries have already been placed in the /opt/cni/bin directory.
** WIP **

Addresses the following issue: #280

bridge and host-local ipam. This approach extends KinD's Config with 3 additional fields: CNI, Config File Name and Config File Body. If these fields are specified then KinD will copy the config file to the /etc/cni/net.d directory under the desired name. In theory this approach should work with other plugins that only require a configuration file and their binaries have already been placed in the /opt/cni/bin directory. ** WIP **

k8s-ci-robot · 2019-02-08T01:21:23Z

Welcome @Arvinderpal! It looks like this is your first PR to kubernetes-sigs/kind 🎉

k8s-ci-robot · 2019-02-08T01:21:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Arvinderpal
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: bentheelder

If they are not already assigned, you can assign the PR to them by writing /assign @bentheelder in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2019-02-08T01:21:34Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2019-02-08T01:21:36Z

Hi @Arvinderpal. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Arvinderpal · 2019-02-08T01:22:39Z

You can try out the bridge+host-local plugin as so:
kind create cluster --config docs/user/kind-bridge-ipv4-config.yaml

neolit123

thanks for the PR @Arvinderpal
added some comments
we probably need to make some decision of how to handle different CNIs first and figure out IPV6 at the same time.

pkg/cluster/config/types.go

neolit123 · 2019-02-08T01:33:28Z

pkg/cluster/config/types.go

@@ -43,6 +43,12 @@ type Node struct {
 	// Role defines the role of the nodw in the in the Kubernetes cluster managed by `kind`
 	// Defaults to "control-plane"
 	Role NodeRole
+	// CNI is the name of the desired network plugin
+	CNI string


if we are going to add these they should be under a CNI object and here it should be a pointer so that we can use omitifempty with conjunction with the types.go from v1alpha2

That's a good idea. So something like?
type CNI struct{
CNIType string
CNIConfigFileName string
CNIConfig []string
}

neolit123 · 2019-02-08T01:35:26Z

docs/user/kind-bridge-ipv4-config.yaml

@@ -0,0 +1,41 @@
+# this config file contains all config fields with comments


if this file is added should it be linked from the docs?
possibly a separate PR should add this file and link to docs?

Perhaps an "examples" folder would be more appropriate. We will likely have additional configurations for different scenarios -- e.g. IPv4/IPv6 single/multi node with various cni plugins.

neolit123 · 2019-02-08T02:10:38Z

you can sign the CLA @Arvinderpal
just follow the instructions the bot gave and after you are done call this command /check-cla

neolit123 · 2019-02-08T02:11:04Z

you can sign the CLA @Arvinderpal

ok, looks like you signed it already. :)

BenTheElder · 2019-02-08T02:33:10Z

pkg/cluster/nodes/node.go

@@ -96,6 +96,10 @@ func (n *Node) CopyFrom(source, dest string) error {
 	return docker.CopyFrom(n.nameOrID, source, dest)
 }

+func (n *Node) MkDirPath(dirPath string) error {


this seems awfully specific?

BenTheElder · 2019-02-08T02:33:26Z

pkg/cluster/config/v1alpha2/types.go

@@ -42,6 +42,12 @@ type Node struct {
 	// Role defines the role of the nodw in the in the Kubernetes cluster managed by `kind`
 	// Defaults to "control-plane"
 	Role NodeRole `json:"role,omitempty"`
+	// CNI is the name of the desired network plugin
+	CNI string `json:"cni,omitempty"`


are we going to put different CNI per node?

That's a good point.
I don't think that's a very likely scenario, but at the same time I don't believe there is any strict requirement that all nodes in a cluster use the same CNI plugin. For example, a particular set of nodes may have specialized NICs for which a different CNI plugin may be desirable.

we should definitely move the CNI object outside of the Node and into the main Config object.
CNI should be per cluster.

@neolit123 CNIConfig can contain node specific data. For example, the host-local ipam consists of fields (e.g. subnet, gateway, routes) that are node specific (https://github.com/containernetworking/plugins/tree/master/plugins/ipam/host-local)
I suspect that this will not play well with Replicas since nodes will not be exact copies of each other. However, this is something that can be documented.

For example, a particular set of nodes may have specialized NICs for which a different CNI plugin may be desirable.

But these are not on real NICs.

I also think having CNI config is perhaps not the right level of complexity for kind. Most CNI are deployable as a k8s manifest, I think we can support that or not installing a CNI at all and leaving it to the end user. Ref #278

Arvinderpal · 2019-02-08T15:41:56Z

Thanks for the feedback guys!
I would love to help out with this effort. On that note, are there any periodic syncs/meetings where such topics are discussed?

Arvinderpal · 2019-02-08T15:46:04Z

I accidentally closed the PR. My bad!

BenTheElder · 2019-02-14T13:38:21Z

Sorry I missed this:

I would love to help out with this effort. On that note, are there any periodic syncs/meetings where such topics are discussed?

There's a weekly SIG-Testing subproject meeting.
https://github.com/kubernetes/community/blob/master/sig-list.md

BenTheElder · 2019-02-14T13:46:30Z

Apologies for the delay
/priority backlog
I'd prefer to let this come after the next release. So far we're testing Kubernetes / kubeadm, and apps on top. Testing CNIs with kind is neat but originally unplanned territory and we're trying to stabilize the config in particular so I'd like to think a bit more before adding anything to it.

Arvinderpal · 2019-02-15T19:36:23Z

Hi, no worries. This does bloat the Config so if there is a cleaner solution that would be preferred. Most if not all major CNIs provide manifests. With these smaller CNIs however, there are no such manifests available but in theory it should be possible to create them. I'll look into it. :)

k8s-ci-robot · 2019-02-21T23:15:27Z

@Arvinderpal: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

BenTheElder · 2019-02-24T23:46:20Z

SGTM, let's do a new PR moving this into EG the node image build, or override the manifest in config (and just depend on pulling the images) ref: #340.

Going forward the manifest is now not pulled from weave's site except as a fallback for old images, it is pulled from the node instead.

k8s-ci-robot requested review from krzyzacy and munnerz February 8, 2019 01:21

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 8, 2019

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 8, 2019

Arvinderpal mentioned this pull request Feb 8, 2019

IPv6 Support #280

Closed

neolit123 reviewed Feb 8, 2019

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 8, 2019

BenTheElder reviewed Feb 8, 2019

View reviewed changes

Arvinderpal closed this Feb 8, 2019

Arvinderpal reopened this Feb 8, 2019

k8s-ci-robot added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Feb 14, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2019

BenTheElder closed this Feb 24, 2019

stg-0 added a commit to stg-0/kind that referenced this pull request Sep 11, 2023

Add warning for keep-mgmt (kubernetes-sigs#281)

d4e2279

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for CNCF containernetworking plugins #281

Support for CNCF containernetworking plugins #281

Arvinderpal commented Feb 8, 2019 •

edited

Loading

k8s-ci-robot commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

Arvinderpal commented Feb 8, 2019

neolit123 left a comment

neolit123 Feb 8, 2019

Arvinderpal Feb 8, 2019

neolit123 Feb 8, 2019

Arvinderpal Feb 8, 2019

neolit123 commented Feb 8, 2019

neolit123 commented Feb 8, 2019

BenTheElder Feb 8, 2019

BenTheElder Feb 8, 2019

Arvinderpal Feb 8, 2019

neolit123 Feb 8, 2019

Arvinderpal Feb 8, 2019

BenTheElder Feb 14, 2019

Arvinderpal commented Feb 8, 2019 •

edited

Loading

Arvinderpal commented Feb 8, 2019

BenTheElder commented Feb 14, 2019

BenTheElder commented Feb 14, 2019

Arvinderpal commented Feb 15, 2019

k8s-ci-robot commented Feb 21, 2019

BenTheElder commented Feb 24, 2019

		@@ -0,0 +1,41 @@
		# this config file contains all config fields with comments

Support for CNCF containernetworking plugins #281

Support for CNCF containernetworking plugins #281

Conversation

Arvinderpal commented Feb 8, 2019 • edited Loading

k8s-ci-robot commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

k8s-ci-robot commented Feb 8, 2019

Arvinderpal commented Feb 8, 2019

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Feb 8, 2019

neolit123 commented Feb 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arvinderpal commented Feb 8, 2019 • edited Loading

Arvinderpal commented Feb 8, 2019

BenTheElder commented Feb 14, 2019

BenTheElder commented Feb 14, 2019

Arvinderpal commented Feb 15, 2019

k8s-ci-robot commented Feb 21, 2019

BenTheElder commented Feb 24, 2019

Arvinderpal commented Feb 8, 2019 •

edited

Loading

Arvinderpal commented Feb 8, 2019 •

edited

Loading