node: Install individual per node routes #1888

tgraf · 2017-10-29T17:40:36Z

The route pointing packets from the host to IPs managed by other nodes
has been covered by a single cluster wide route prefix which covered all
nodes. This did not allow to have any non Cilium IPs unless the IPs were
associated to the local host.

Merge node and nodeaddress package together as they depend on each
other.
Move route and address addition from the init.sh script to golang
native code in pkg/node to allow tiggering route additions based
on events.
Add a per node route for each node discovered in the cluster

Fixes: #1846

Signed-off-by: Thomas Graf [email protected]

aanm · 2017-10-30T09:10:05Z

pkg/node/logfields.go

+package node
+
+const (
+	fieldRoute = "route"


So, which package will have its logfields defined? CC @raybejjani

log fields which are specific to a package don't need to be global but I'm open to a discussion

Good question. fieldRoute is used in an Error message, and it is likely to be common in logs. I'd vote to put it in logfields.

Moved to logfields

aanm · 2017-10-30T09:11:43Z

pkg/node/manager.go

+}
+
+func replaceNodeRoute(ip *net.IPNet) {
+	link, err := netlink.LinkByName("cilium_host")


Use a const for cilium_host

aanm · 2017-10-30T09:13:49Z

pkg/node/manager.go

+	if err := netlink.RouteAdd(&route); err != nil {
+		scopedLog.WithError(err).Error("Unable to add node route")
+	} else {
+		scopedLog.Info("Installed node route")


If I was a user, I wouldn't understand which node route is this. Can you add the specific route that was installed?

The scopedLog contains the full route description as field.

aanm · 2017-10-30T09:14:09Z

pkg/node/manager.go

+	return nil
+}
+
+func replaceNodeRoute(ip *net.IPNet) {


Can you add a description to that function of what it does?

raybejjani · 2017-10-30T10:00:29Z

bpf/init.sh

-IP4_SVC_RANGE=$8
-IP6_SVC_RANGE=$9
-MODE=${10}
+IP4_HOST=$3


Not too relevant, but we could use shift to avoid renumbering args on refactor. This doesn't happen too often, though.

@scanf is working on removing init.sh entirely in favor of a golang native implementation.

raybejjani · 2017-10-30T10:09:16Z

daemon/daemon.go

 		if err != nil {
 			log.WithError(err).WithField(logfields.V6Prefix, v6ServicePrefix).Fatal("Invalid IPv6 service prefix")
 		}
+
+		node.AddAuxPrefix(ipnet)


This should be the else case for if err != nil since we don't exit/return on error.

oh, nevermind, it's a fatal

raybejjani · 2017-10-30T10:09:43Z

daemon/daemon.go

 		if err != nil {
 			log.WithError(err).WithField(logfields.V4Prefix, v4ServicePrefix).Fatal("Invalid IPv4 service prefix")
 		}
+
+		node.AddAuxPrefix(ipnet)


This should be the else case for if err != nil since we don't exit/return on error.

oh, nevermind, it's a fatal

raybejjani · 2017-10-30T10:15:47Z

pkg/k8s/init.go


 		k8sNode, err := GetNode(Client(), nodeName)
 		if err != nil {
 			return fmt.Errorf("unable to retrieve k8s node information: %s", err)
 		}

-		node := ParseNode(k8sNode)
+		n := ParseNode(k8sNode)
+		log.Infof("Retrieved node's %s information from kubernetes", n.Name)


log.WithField(logfields.NodeName, n.Name).Info("Retrieved node information from kubernetes") to be consistent with our logrus usage.

raybejjani · 2017-10-30T10:19:26Z

pkg/node/logfields.go

+package node
+
+const (
+	fieldRoute = "route"


Good question. fieldRoute is used in an Error message, and it is likely to be common in logs. I'd vote to put it in logfields.

raybejjani · 2017-10-30T10:21:10Z

pkg/node/manager.go

+	nodes                 = map[Identity]*Node{}
+	ciliumHostInitialized = false
+	usePerNodeRoutes      = false
+	auxPrefixes           = []*net.IPNet{}


Since these are package variables (which is probably unintentional) and file variables, could you comment what/when/how they're to be used. In particular, what is mutex protecting?

raybejjani · 2017-10-30T10:35:09Z

pkg/node/manager.go

+		log.WithError(err).Error("Unable to add nexthop route")
+	}
+
+	route := netlink.Route{LinkIndex: link.Attrs().Index, Dst: ip, Gw: via, Src: local}


We delete then add the same route object. Does this work because netlink.RouteDel matches only on some of the fields?

I was worried that RouteReplace does not have the correct semantics. I double checked and it does so I will be replacing this with a single netlink.RouteReplace() call

raybejjani · 2017-10-30T10:38:43Z

pkg/node/manager.go

+func installHostRoutes() {
+	if !ciliumHostInitialized {
+		return
+	}


Since we went to all the effort to guard this (in InstallHostRoutes and here in installHostRoutes) we should log something here, or return an error that can be logged at the callers.

The case where we might hit this is if the Kubernetes watcher is installed before we put the cilium_host device in place in which case we will start collecting node information without being able to install the routes. They will be installed when we add the device. I'll add a debug statement to indicate that installation was deferred.

manalibhutiyani

Looks good.

The route pointing packets from the host to IPs managed by other nodes has been covered by a single cluster wide route prefix which covered all nodes. This did not allow to have any non Cilium IPs unless the IPs were associated to the local host. * Merge node and nodeaddress package together as they depend on each other. * Move route and address addition from the init.sh script to golang native code in pkg/node to allow tiggering route additions based on events. * Add a per node route for each node discovered in the cluster * Per node routes are enabled by default when running on Kubernetes but can be disabled with --single-cluster-route. Fixes: #1846 Signed-off-by: Thomas Graf <[email protected]>

tgraf added the wip label Oct 29, 2017

tgraf requested review from a team October 29, 2017 17:40

tgraf requested a review from a team as a code owner October 29, 2017 17:40

tgraf requested a review from a team October 29, 2017 17:40

tgraf requested a review from a team as a code owner October 29, 2017 17:40

cilium deleted a comment from houndci-bot Oct 29, 2017

tgraf force-pushed the individual-routes branch 2 times, most recently from 0f7da8d to 535d68d Compare October 29, 2017 21:06

tgraf force-pushed the individual-routes branch 2 times, most recently from 6b75af5 to 24e9319 Compare October 30, 2017 08:32

aanm requested changes Oct 30, 2017

View reviewed changes

tgraf force-pushed the individual-routes branch from 24e9319 to 79c1314 Compare October 30, 2017 10:26

raybejjani suggested changes Oct 30, 2017

View reviewed changes

aanm approved these changes Oct 30, 2017

View reviewed changes

tgraf force-pushed the individual-routes branch 2 times, most recently from 98afafe to c28d66d Compare October 30, 2017 12:35

raybejjani approved these changes Oct 30, 2017

View reviewed changes

aalemayhu approved these changes Oct 30, 2017

View reviewed changes

manalibhutiyani approved these changes Oct 30, 2017

View reviewed changes

tgraf force-pushed the individual-routes branch from c28d66d to fcd2e39 Compare October 30, 2017 19:45

tgraf added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Oct 30, 2017

tgraf merged commit 2b3301f into master Oct 30, 2017

tgraf deleted the individual-routes branch October 30, 2017 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: Install individual per node routes #1888

node: Install individual per node routes #1888

tgraf commented Oct 29, 2017

aanm Oct 30, 2017

tgraf Oct 30, 2017

raybejjani Oct 30, 2017

tgraf Oct 30, 2017

aanm Oct 30, 2017

tgraf Oct 30, 2017

aanm Oct 30, 2017

tgraf Oct 30, 2017

aanm Oct 30, 2017

tgraf Oct 30, 2017

raybejjani Oct 30, 2017

tgraf Oct 30, 2017

raybejjani Oct 30, 2017

raybejjani Oct 30, 2017

raybejjani Oct 30, 2017

raybejjani Oct 30, 2017

raybejjani Oct 30, 2017

tgraf Oct 30, 2017

raybejjani Oct 30, 2017

raybejjani Oct 30, 2017

tgraf Oct 30, 2017

raybejjani Oct 30, 2017

tgraf Oct 30, 2017

raybejjani Oct 30, 2017

tgraf Oct 30, 2017

manalibhutiyani left a comment

node: Install individual per node routes #1888

node: Install individual per node routes #1888

Conversation

tgraf commented Oct 29, 2017

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

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

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

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

Choose a reason for hiding this comment

manalibhutiyani left a comment

Choose a reason for hiding this comment