Add an option for CNI shim to create and configure the pod interface #824

winsopc · 2019-09-05T21:02:00Z

Currently we use client/server design to create and configure the pod
interface. The client being the ovn-k8s-cni-overlay and server
being the ovnkube-node container. The ovnkube-node creates and
configures the pod interface. This requires It to be running with
SYS_ADMIN capability and this is undesirable.

To make ovnkube-node to run with least capabilities as possible, the idea
is to create and configure the pod interface in the client itself (i.e., in
ovn-k8s-cni-overlay running on the host). In this approach, the deployer
explicity asks for unpriivelged ovnkube-node using a CLI option. The
server returns to client all the pod interface information (ip, mac, gateway,
MTU, ingresss, egress bandwidth, and so on). The client then creates
and sets up the pod interface.

Signed-off-by: Zhen Wang zhewang@nvidia.com

girishmg · 2019-09-09T17:33:01Z

@dcbw @danwinship PTAL

danwinship · 2019-09-09T19:53:50Z

ovn-kubernetes has way too many modes. Is there any reason to not just always do it this way?

girishmg · 2019-09-09T20:48:38Z

@danwinship it was implemented using a knob for two reasons.

In the last community call, I got the impression that we might need to keep the current default
behavior as it works out of the box (see bullet (2) below). Furthermore, I also got the
impression that Redhat wants everything to be containerized and that it doesn't want to run
anything on the host (see bullet (2) below).
This implementation requires that the ovs-* utilities are available on the host so that the
ovn-k8s-cni-overlay binary in the host can add one end of the VETH pair to br-int bridge.
In everything needs to be containerized model, this will not work. In our case, we run
ovs-vswitchd and ovsdb-server on the host directly. As a result, ovs-* utilities are easily
available.

dcbw · 2019-09-10T14:17:38Z

@danwinship if we also ran OVS on the host, then we could use this mode too :( And this is mostly what we do in openshift-sdn except that we do the OVS operation inside our container because that's where our ovs-vsctl lives.

I guess we could switch to this mode if we map our ovs-vsctl binary and the vsctl socket onto the host's filesystem and call it from there?

girishmg · 2019-09-13T19:01:56Z

@dcbw @danwinship PTAL. This doesn't use the cniShimConfig.json file anymore, so it is very simple now.

danwinship · 2019-09-13T20:52:14Z

Hm... it seems like if we split setupInterface out of ConfigureInterface then we could do this just like we do in openshift-sdn and not need a configuration option; the CNI plugin would call setupInterface and do the privileged netlink operations, and then it would pass the request on to the daemon, which would run the rest of ConfigureInterface to do the OVS operations. And then everyone wins. 🎉 🕺

girishmg · 2019-09-13T21:15:16Z

Hm... it seems like if we split setupInterface out of ConfigureInterface then we could do this just like we do in openshift-sdn and not need a configuration option; the CNI plugin would call setupInterface and do the privileged netlink operations, and then it would pass the request on to the daemon, which would run the rest of ConfigureInterface to do the OVS operations. And then everyone wins. 🎉 🕺

Except that the CNI plugin doesn't have any K8s credentials and OVN DB endpoint information. So,
it cannot do setupInterface. To do whatever you are saying, we will need to do to and fro communication between CNI plugin and the CNI server.

CNI plugin to server to get pod interface info
CNI plugin does setupinterface and calls the server
Server configures the interface and returns the result in json
CNI plugin prints the results.

danwinship · 2019-09-14T19:47:47Z

ah... ok

danwinship

mostly looks good then

danwinship · 2019-09-14T19:49:59Z

go-controller/pkg/config/config.go

@@ -71,6 +71,9 @@ var (

 	// NbctlDaemon enables ovn-nbctl to run in daemon mode
 	NbctlDaemonMode bool
+
+	// PrivilegedMode needs ovnkube-node container to run with SYS_ADMIN capability by default.
+	PrivilegedMode = true


If this was UnprivilegedMode instead then it would match the config flag, and wouldn't need to be initialized here, and could be set directly from the cli.BoolFlag declaration below rather than needing separate code in ovnkube.go.

As for the comment, assuming you go with UnprivilegedMode:

// UnprivilegedMode allows ovnkube-node to run without SYS_ADMIN capability, by performing interface setup in the CNI plugin

(except don't you mean NET_ADMIN anyway?)

Thanks for reviewing it.
I have updated the code.

danwinship · 2019-09-14T19:53:27Z

go-controller/pkg/cni/cni.go

+		IPAddress:  ipAddress,
+		GatewayIP:  gatewayIP,
+		Ingress:    ingress,
+		Egress:     egress}


style nit: put the } on the next line

danwinship · 2019-09-14T19:56:16Z

go-controller/pkg/cni/cni.go

-		},
-	}
-
+	podIntfaceInfo := &PodIntfaceInfo{


"Intface" is weird... Usually people abbreviate "interface" to "iface", but you could also just not abbreviate it

Currently we use client/server design to create and configure the pod interface. The client being the ovn-k8s-cni-overlay and server being the ovnkube-node container. The ovnkube-node creates and configures the pod interface. This requires It to be running with SYS_ADMIN capability and this is undesirable. To make ovnkube-node to run with least capabilities as possible, the idea is to create and configure the pod interface in the client itself (i.e., in ovn-k8s-cni-overlay running on the host). In this approach, the deployer explicity asks for unpriivelged ovnkube-node using a CLI option. The server returns to client all the pod interface information (ip, mac, gateway, MTU, ingresss, egress bandwidth, and so on). The client then creates and sets up the pod interface. Signed-off-by: Zhen Wang <zhewang@nvidia.com>

winsopc · 2019-09-15T04:53:16Z

@danwinship Please review my latest code, thanks!

danwinship · 2019-09-17T18:21:36Z

lgtm

winsopc force-pushed the sdn231 branch from 8468087 to 45ad4e3 Compare September 5, 2019 21:35

dcbw changed the title ~~Add an option for host to create and configure the pod interface~~ Add an option for CNI shim to create and configure the pod interface Sep 10, 2019

winsopc force-pushed the sdn231 branch from 45ad4e3 to 6a8cd5c Compare September 13, 2019 18:57

danwinship reviewed Sep 14, 2019

View reviewed changes

winsopc force-pushed the sdn231 branch from 6a8cd5c to 7812e22 Compare September 15, 2019 04:31

girishmg merged commit 8133024 into ovn-org:master Sep 17, 2019

winsopc deleted the sdn231 branch September 17, 2019 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option for CNI shim to create and configure the pod interface #824

Add an option for CNI shim to create and configure the pod interface #824

winsopc commented Sep 5, 2019

girishmg commented Sep 9, 2019

danwinship commented Sep 9, 2019

girishmg commented Sep 9, 2019 •

edited

dcbw commented Sep 10, 2019 •

edited

girishmg commented Sep 13, 2019

danwinship commented Sep 13, 2019 •

edited

girishmg commented Sep 13, 2019

danwinship commented Sep 14, 2019

danwinship left a comment

danwinship Sep 14, 2019

winsopc Sep 15, 2019

danwinship Sep 14, 2019

winsopc Sep 15, 2019

danwinship Sep 14, 2019

winsopc Sep 15, 2019

winsopc commented Sep 15, 2019 •

edited

danwinship commented Sep 17, 2019

Add an option for CNI shim to create and configure the pod interface #824

Add an option for CNI shim to create and configure the pod interface #824

Conversation

winsopc commented Sep 5, 2019

girishmg commented Sep 9, 2019

danwinship commented Sep 9, 2019

girishmg commented Sep 9, 2019 • edited

dcbw commented Sep 10, 2019 • edited

girishmg commented Sep 13, 2019

danwinship commented Sep 13, 2019 • edited

girishmg commented Sep 13, 2019

danwinship commented Sep 14, 2019

danwinship left a comment

Choose a reason for hiding this comment

danwinship Sep 14, 2019

Choose a reason for hiding this comment

winsopc Sep 15, 2019

Choose a reason for hiding this comment

danwinship Sep 14, 2019

Choose a reason for hiding this comment

winsopc Sep 15, 2019

Choose a reason for hiding this comment

danwinship Sep 14, 2019

Choose a reason for hiding this comment

winsopc Sep 15, 2019

Choose a reason for hiding this comment

winsopc commented Sep 15, 2019 • edited

danwinship commented Sep 17, 2019

girishmg commented Sep 9, 2019 •

edited

dcbw commented Sep 10, 2019 •

edited

danwinship commented Sep 13, 2019 •

edited

winsopc commented Sep 15, 2019 •

edited