Add network provider option and Calico support #714

dghubble · 2017-09-19T21:57:40Z

Support choosing Flannel or Calico networking (--network-provider)
On Jenkins, I've added a new test to provision a cluster with calico and perform the usual e2e tests.

This is based on poseidon/bootkube-terraform, which uses Calico or flannel as the network provider on AWS, GCE, and bare-metal clusters (Digital Ocean w caveats). I figured it would be nice to have in the upstream project in case others want to use it.

rel #405
Closes #407

aaronlevy · 2017-09-19T22:19:05Z

cmd/bootkube/render.go

@@ -50,6 +50,8 @@ var (
 		serviceCIDR         string
 		selfHostKubelet     bool
 		cloudProvider       string
+		networkProvider     string
+		networkMTU          int


Can we get away with a sane default here rather than a flag? I'd really like to avoid this

Agreed, I was on the fence about this. An appropriate default is 1440. Real users would pick the right value depending on the environment or network performance suffers. Since bootkube render just produces valid example manifests (not ideal ones), perhaps we leave it to projects to generate assets that use the correct MTU. In my cases, I generate the right assets for the platform separately, so I'm not opinionated which way bootkube render goes.

Removed. flannel and Calico use different defaults, so setting it in one place breaks tests anyway. This means bootkube render will create clusters that work using the default settings, but aren't necessarily getting optimal throughput. I think that's ok given that higher level tools do handle these cases.

aaronlevy · 2017-09-19T22:20:51Z

cmd/bootkube/render.go

+		return errors.New("Must specify --network-provider flannel or calico")
+	}
+	if renderOpts.networkProvider == asset.NetworkCalico && renderOpts.calicoNetworkPolicy {
+		return errors.New("Cannot specify --experimental-calico-network-policy with --network-provider=calico, Calico supports network policy automatically.")


Maybe we just drop this altogether and put this under network provider instead?

--network-provider={flannel,experimental-calico-network-policy, experimiental-calico-bgp}?

I'm leaning toward the experimental because if we don't have tests - we could break it at any time.

Experimental prefixes work for me. I prefer we say "calico" rather than "calico-bgp", since bgp is redundant. I'd also prefer to keep "canal-style" policy-only projects separate from CNI providers, long term it doesn't really belong here since its not (supposed to be) providing networking or really essential to bringing up a minimal control plane. Suppose some other network policy app comes along. Its an addon.

--network-provider={flannel,experimental-calico}

klausenbusk · 2017-09-20T10:34:07Z

Should we add a brief pros/cons (/use-case) of flannel vs calico? For a newcomer I think it can be a bit hard to know what is "best".

dghubble · 2017-09-20T17:25:13Z

coreosbot run e2e calico

dghubble · 2017-09-20T17:51:53Z

coreosbot run e2e

dghubble · 2017-09-20T21:22:49Z

@klausenbusk to a degree, though maybe we just link to the relevant projects. So kubernetes-incubator projects stay clear of providing recommendations and leave it to users or to higher level tools to choose what's right for them.

Practically speaking, this Calico support in bootkube is experimental so flannel is still the supported option.

dghubble · 2017-09-21T23:04:10Z

Re-enabling broken tests (#716) finds some issues in this PR :) I'll investigate.

dghubble · 2017-09-26T20:14:03Z

Squashed and rebased

aaronlevy

Overall this is looking good - but I'd like to move the "calico network policy" under the same umbrella (as "canal")

aaronlevy · 2017-09-27T01:19:16Z

cmd/bootkube/render.go

@@ -72,6 +73,7 @@ func init() {
 	cmdRender.Flags().StringVar(&renderOpts.serviceCIDR, "service-cidr", "10.3.0.0/24", "The CIDR range of cluster services.")
 	cmdRender.Flags().BoolVar(&renderOpts.selfHostKubelet, "experimental-self-hosted-kubelet", false, "(Experimental) Create a self-hosted kubelet daemonset.")
 	cmdRender.Flags().StringVar(&renderOpts.cloudProvider, "cloud-provider", "", "The provider for cloud services.  Empty string for no provider")
+	cmdRender.Flags().StringVar(&renderOpts.networkProvider, "network-provider", "flannel", "CNI network provider (flannel or experimental-calico).")
 	cmdRender.Flags().BoolVar(&renderOpts.selfHostedEtcd, "experimental-self-hosted-etcd", false, "(Experimental) Create self-hosted etcd assets.")
 	cmdRender.Flags().BoolVar(&renderOpts.calicoNetworkPolicy, "experimental-calico-network-policy", false, "(Experimental) Add network policy support by calico.")


We should drop this right?

Or really this should be moved under networkProvider={flannel, experimental-calico, experimental-canal}?

We could do this and change the testing setup/scripts to work that way (Jenkins jobs pass enable_network_policy to terraform to scripts to bootkube render so the jobs need changes). It may be worth a separate PR.

Concerning whether we should, imo flannel with policy-only Calico (i.e. "canal") probably shouldn't be conflated with a top level CNI network provider. Policy-only Calico manifests can be deployed on top of a flannel cluster afterward, as an addon, no reason it needs to be part of control plane bootstrapping or a bootkube concern. I'd prefer we eventually remove --experimental-calico-network-policy and just point to the upstream plain-old-manifests that work fine. Its the same logic that governs why bootkube clusters don't come with Ingress or CLUO or other optional addons.

I'd never recommend installing network policy after the fact (even if you could probably make it work). Essentially you'd end up relying on the name ordering of CNI configuration, (and that already makes me nervous), as well as deployed CNI plugin versions / compatibility. Basically -- not worth the risk (nor would we try and support that here). Networking IMO should only be considered an install-time choice.

To that end - I would much prefer that we have a single flag for the network provider rather than separating those concepts.

What about a separate PR (that comes before this) that changes the flag from:

--experimental-calico-network-policy=true

to

--network-provider=experimental-canal

Then this as a follow-up is merely adding a new provider?

Might also consider a minor version bump for this given the flag change. But given that it's experimental (and breaking changes should be expected), I could go either way.

I'd never recommend installing network policy after the fact... not worth the risk

Calico policy-only manifests are jumbled up with control plane manifests and get created in random order. Their install-cni sidecar races with flannel's sidecar. Creating these manifests early shouldn't provide any sense of safety. In my view, any of these setups that involve flannel with policy-only calico are racy.

nor would we try and support that here

If anything, I want the opposite. Less support responsibility for bootkube. I'd prefer we point to upstream to (try to) handle policy-only mode since their docs cover adding it to a flannel cluster. Elevating it to a top level network provider in bootkube means we continue supporting it. That's fine if we wanna keep trying, its just not a mode I'd encourage users to choose myself.

dghubble · 2017-09-28T23:01:46Z

Basically just gonna re-do this

dghubble · 2017-09-28T23:30:42Z

Force override old PR.

dghubble · 2017-09-29T22:51:29Z

coreosbot run e2e

aaronlevy

Overall, LGTM. One question about a bash debug option.

aaronlevy · 2017-10-02T16:47:40Z

hack/quickstart/init-master.sh

@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
-set -euo pipefail
+set -euox pipefail


Do we want to leave the -x option, or was that during debugging? Or if it's generally useful should we turn it on for init-node.sh also?

It was for debugging, I'll remove, though its a bit useful

* Add network_provider to e2e scripts / Terraform setup

dghubble · 2017-10-02T17:58:56Z

Failed was related to the Jenkins testing log collection setup, which was different on the e2e-calico test.

dghubble · 2017-10-02T18:18:40Z

coreosbot run e2e

dghubble · 2017-10-02T18:19:24Z

coreosbot run e2e calico

diegs

LGTM

ozdanborne · 2017-10-05T23:28:06Z

pkg/asset/internal/templates.go

+              name: var-run-calico
+              readOnly: false
+        - name: install-cni
+          image: quay.io/calico/cni:v1.10.0


I think this should this be {{ .Images.CalicoCNI }}

Should have been addressed by #726

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 19, 2017

aaronlevy reviewed Sep 19, 2017

View reviewed changes

dghubble closed this Sep 20, 2017

dghubble reopened this Sep 20, 2017

dghubble mentioned this pull request Sep 25, 2017

Add --network-provider option for Calico support #717

Closed

dghubble mentioned this pull request Sep 26, 2017

Use CNI pod to deploy CNI plugins to host filesystem #405

Closed

dghubble requested review from diegs and squat September 26, 2017 21:09

aaronlevy reviewed Sep 27, 2017

View reviewed changes

dghubble mentioned this pull request Sep 28, 2017

pkg: Move experimental-network-policy to network-provider canal #723

Merged

dghubble mentioned this pull request Sep 28, 2017

pkg/asset: calico: Enable hostPort for calico networking & update cniVersion to 0.3.1 #711

Merged

aaronlevy approved these changes Oct 2, 2017

View reviewed changes

Add --network-provider Calico support

24143d6

* Add network_provider to e2e scripts / Terraform setup

diegs approved these changes Oct 2, 2017

View reviewed changes

dghubble merged commit 347605b into kubernetes-retired:master Oct 2, 2017

dghubble deleted the calico-networking branch October 2, 2017 19:00

ozdanborne reviewed Oct 5, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network provider option and Calico support #714

Add network provider option and Calico support #714

dghubble commented Sep 19, 2017 •

edited

Loading

aaronlevy Sep 19, 2017

dghubble Sep 19, 2017 •

edited

Loading

dghubble Sep 20, 2017

dghubble Sep 26, 2017

aaronlevy Sep 19, 2017

dghubble Sep 19, 2017

dghubble Sep 20, 2017

klausenbusk commented Sep 20, 2017

dghubble commented Sep 20, 2017

dghubble commented Sep 20, 2017

dghubble commented Sep 20, 2017

dghubble commented Sep 21, 2017 •

edited

Loading

dghubble commented Sep 26, 2017

aaronlevy left a comment

aaronlevy Sep 27, 2017

aaronlevy Sep 27, 2017

dghubble Sep 27, 2017

aaronlevy Sep 27, 2017

dghubble Sep 28, 2017

dghubble commented Sep 28, 2017

dghubble commented Sep 28, 2017

dghubble commented Sep 29, 2017

aaronlevy left a comment

aaronlevy Oct 2, 2017

dghubble Oct 2, 2017

dghubble commented Oct 2, 2017

dghubble commented Oct 2, 2017

dghubble commented Oct 2, 2017

diegs left a comment

ozdanborne Oct 5, 2017

dghubble Oct 5, 2017

Add network provider option and Calico support #714

Add network provider option and Calico support #714

Conversation

dghubble commented Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

dghubble Sep 19, 2017 • edited Loading

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

klausenbusk commented Sep 20, 2017

dghubble commented Sep 20, 2017

dghubble commented Sep 20, 2017

dghubble commented Sep 20, 2017

dghubble commented Sep 21, 2017 • edited Loading

dghubble commented Sep 26, 2017

aaronlevy 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

dghubble commented Sep 28, 2017

dghubble commented Sep 28, 2017

dghubble commented Sep 29, 2017

aaronlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dghubble commented Oct 2, 2017

dghubble commented Oct 2, 2017

dghubble commented Oct 2, 2017

diegs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dghubble commented Sep 19, 2017 •

edited

Loading

dghubble Sep 19, 2017 •

edited

Loading

dghubble commented Sep 21, 2017 •

edited

Loading