Add romana to built-in CNI options #3290

cgilmour · 2017-08-28T01:18:16Z

This PR adds romana as a networking option for kops.

It installs the latest "preview" release of Romana v2.0, which provides the expected features in terms of IP allocations and route configuration. Network policy features are being ported to 2.0 and will be in the final release. (We intend to submit a followup PR for kops as part of that rolling out that release.)

Note: in this setup, we're using the etcd cluster that kops deploys for k8s. This isn't ideal, but some possibilities (eg: StatefulSets) aren't practical for the CNI itself, and creating a parallel etcd cluster via manifests seemed to be a more-intrusive approach than using the existing one.
If this is a concern or problem, then I'm very open to discussing and implementing it based on your suggestions.

Also, some functionality is exclusive to AWS environments. Other cloud platforms are on Romana's roadmap but not developed yet. Let me know that restriction needs to be enforced in code or directly documented.

k8s-ci-robot · 2017-08-28T01:18:23Z

Hi @cgilmour. Thanks for your PR.

I'm waiting for a 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.

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. I understand the commands that are listed here.

chrislovecnm · 2017-08-28T03:08:26Z

/ok-to-test

You mind squashing your commits?

chrislovecnm

Thanks for the PR! Some questions and changes.

chrislovecnm · 2017-08-28T03:10:11Z

pkg/apis/kops/networking.go

+
+// Romana declares that we want Romana networking
+type RomanaNetworkingSpec struct {
+	DaemonServiceIP string `json:"daemonServiceIP,omitempty"`


Can we get a description on the sctruct members?

chrislovecnm · 2017-08-28T03:10:47Z

docs/networking.md

+
+#### Installing Romana on a new Cluster
+
+The following command sets up a cluster with Kube-router as the CNI, service proxy and networking policy provider


Can we document that etcd has to be open to the nodes?

chrislovecnm · 2017-08-28T03:12:01Z

upup/models/cloudup/resources/addons/networking.romana/k8s-1.6.yaml.template

+        effect: NoSchedule
+      containers:
+      - name: romana-agent
+        image: quay.io/romana/agent:v2.0-preview.2


Is this released in GA? If not may want to add a note to the docs

Added a note into the Romana section of networking doc.

chrislovecnm · 2017-08-28T03:12:45Z

upup/models/cloudup/resources/addons/networking.romana/k8s-1.6.yaml.template

+        args:
+        - --service-cluster-ip-range={{ .ServiceClusterIPRange }}
+        securityContext:
+          privileged: true


Does it need full privileged?

This one does, yes.

chrislovecnm · 2017-08-28T03:13:07Z

upup/models/cloudup/resources/addons/networking.romana/k8s-1.6.yaml.template

+      tolerations:
+      - key: node-role.kubernetes.io/master
+        effect: NoSchedule
+      containers:


Can I add CPU and mem limits please?

CPU and memory limits added for all containers in the spec.

chrislovecnm · 2017-08-28T03:15:10Z

upup/models/cloudup/resources/addons/networking.romana/k8s-1.6.yaml.template

+        imagePullPolicy: Always
+        args:
+        - --etcd_use_v2
+        - --etcd_addr={{ .Networking.Romana.EtcdServiceIP }}


How is calico setting this? Can we do hostname(s) instead of IP address?

This component, romana-vpcrouter is involved in creating route entries so that pods can be reached, and runs on the host network instead of as a plain pod.
We'd originally configured it as a regular pod and using a DNS address for etcd, but it didn't work. It provides the routing to pods, such as kube-dns. When the routes don't exist yet, it can't find etcd to get the information to add those routes.

For those reasons, it uses hostNetworking and a predeclared service ip for the etcd address.

chrislovecnm · 2017-08-28T03:15:42Z

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

@@ -468,6 +468,27 @@ func (b *BootstrapChannelBuilder) buildManifest() (*channelsapi.Addons, map[stri
 		}
 	}

+	if b.cluster.Spec.Networking.Romana != nil {


Should we test that k8s version is 1.6 or greater?

I'm fine adding this. Is there an example to base it on?

Probably best to add it to cluster validation and a similar example (where a networking mode is not supported on particular k8s versions) is here https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/validation/legacy.go#L400

(And I'm sure you could make romana work with k8s 1.5, but I agree that it probably isn't worth it :-) )

@cgilmour did you make this change?

chrislovecnm · 2017-08-28T03:16:43Z

upup/models/cloudup/resources/addons/networking.romana/k8s-1.6.yaml.template

+      tolerations:
+      - key: node-role.kubernetes.io/master
+        effect: NoSchedule
+      containers:


CPU and mem limits please

cgilmour · 2017-08-28T05:52:07Z

Commits have been squashed, and updates made for the requested changes.

justinsb

Generally LGTM. I'm happy to merge and iterate on these!

justinsb · 2017-08-28T12:18:34Z

pkg/model/firewall.go

+		if b.Cluster.Spec.Networking.Romana != nil {
+			// Romana needs to access etcd
+			glog.Warningf("Opening etcd port on masters for access from the nodes, for romana.  This is unsafe in untrusted environments.")
+			tcpPorts = append(tcpPorts, 4001)


It looks like you open etcd on 12379, not 4001.

Kind of. The service port is 12379, but that gets translated by kube-proxy's iptables rules to a specific IP and port 4001. So the security group sees connections to port 4001.

justinsb · 2017-08-28T12:22:41Z

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

@@ -468,6 +468,27 @@ func (b *BootstrapChannelBuilder) buildManifest() (*channelsapi.Addons, map[stri
 		}
 	}

+	if b.cluster.Spec.Networking.Romana != nil {


Probably best to add it to cluster validation and a similar example (where a networking mode is not supported on particular k8s versions) is here https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/validation/legacy.go#L400

(And I'm sure you could make romana work with k8s 1.5, but I agree that it probably isn't worth it :-) )

justinsb · 2017-08-28T12:27:58Z

pkg/model/components/networking.go

@@ -69,5 +69,18 @@ func (b *NetworkingOptionsBuilder) BuildOptions(o interface{}) error {
 		}
 	}

+	if networking.Romana != nil {
+		daemonIP, err := WellKnownServiceIP(clusterSpec, 99)


You may want to set these only if they aren't already set, to allow the user to override them if they want to. That said I suspect we don't want to allow the user to override them?

"Our" needs for the address is mainly that it's static and reserved, because in this "layer" we can't depend on kube-dns pods being reachable.
I don't think it'd be an issue if users changed them, because they'd get substituted in all the necessary places, but it's not really a valuable thing to do.

@justinsb thoughts?

justinsb · 2017-08-28T12:29:09Z

upup/models/cloudup/resources/addons/networking.romana/k8s-1.6.yaml.template

+  name: romana
+  namespace: kube-system
+spec:
+  clusterIP: {{ .Networking.Romana.DaemonServiceIP }}


Is this actually changeable? I note that the clients aren't templated with this value, so I'm wondering how they reach the service.

Most of the containers take the address from the environment variables that get added to the pod automatically for services. Those ones don't need to have the value passed in explicitly via flags.

justinsb · 2017-08-28T12:32:09Z

pkg/model/firewall.go

+	if b.Cluster.Spec.Networking.Romana != nil {
+		// Romana needs to access etcd
+		glog.Warningf("Opening etcd port on masters for access from the nodes, for romana.  This is unsafe in untrusted environments.")
+		tcpRanges = []portRange{{From: 1, To: 4001}, {From: 4003, To: 65535}}


This is inconsistent with the above opened ports (e.g. 9600 is missing)

This was based on what another CNI does. Port 9600 is covered by the second range (4003-65535).
I'm OK with making this more specific in both places.

cgilmour · 2017-08-28T23:45:05Z

pkg/apis/kops/validation/legacy.go

@@ -393,6 +393,12 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error {
 		}
 	}

+	if kubernetesRelease.LT(semver.MustParse("1.6.0")) {


This adds the version check to require v1.6.0 or higher for romana networking.

cgilmour · 2017-08-29T21:18:03Z

Are there any other changes that you'd like in this PR, or other questions that need an answer?

k8s-github-robot · 2017-09-13T22:09:47Z

@cgilmour PR needs rebase

chrislovecnm · 2017-09-15T00:51:31Z

/lgtm

k8s-github-robot · 2017-09-15T00:51:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-15T03:05:44Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-15T03:43:04Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 28, 2017

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 28, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 28, 2017

chrislovecnm suggested changes Aug 28, 2017

View reviewed changes

cgilmour force-pushed the romana-networking-option branch from eb32ca1 to 57bf416 Compare August 28, 2017 05:44

justinsb reviewed Aug 28, 2017

View reviewed changes

cgilmour force-pushed the romana-networking-option branch from 57bf416 to 558eb62 Compare August 28, 2017 23:26

cgilmour commented Aug 28, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2017

Add support for Romana as a networking option

79d331e

cgilmour force-pushed the romana-networking-option branch from 558eb62 to 79d331e Compare September 13, 2017 22:48

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2017

k8s-ci-robot assigned chrislovecnm Sep 15, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2017

k8s-github-robot merged commit 5cb443d into kubernetes:master Sep 15, 2017


		#### Installing Romana on a new Cluster

		The following command sets up a cluster with Kube-router as the CNI, service proxy and networking policy provider

Add romana to built-in CNI options #3290

Add romana to built-in CNI options #3290

Conversation

cgilmour commented Aug 28, 2017

k8s-ci-robot commented Aug 28, 2017

chrislovecnm commented Aug 28, 2017

chrislovecnm 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

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

cgilmour commented Aug 28, 2017

justinsb 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

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

cgilmour commented Aug 29, 2017

k8s-github-robot commented Sep 13, 2017

chrislovecnm commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017

k8s-github-robot commented Sep 15, 2017