Change Cilium templates to standalone version #7474

nebril · 2019-08-26T13:51:34Z

Based on
https://github.com/cilium/cilium/blob/v1.6.0/install/kubernetes/quick-install.yaml

olemarkus · 2019-08-26T20:40:16Z

This is great!

It would be good to remove the code that opens the etcd ports to the nodes as well.

nebril · 2019-09-11T19:50:57Z

/retest

nebril · 2019-09-12T12:27:15Z

/test pull-kops-verify-bazel

This commit doesn't include any Cilium configuration, just takes the quick install yaml from https://github.com/cilium/cilium/blob/v1.6.0/install/kubernetes/quick-install.yaml Signed-off-by: Maciej Kwiek <[email protected]>

chrisz100 · 2019-09-16T12:56:06Z

k8s/crds/kops_v1alpha2_cluster.yaml

@@ -118,10 +122,26 @@ spec:
              properties:
                aws:
                  properties:
+                    cpuLimit:
+                      description: CPULimit CPU limit of AWS IAM Authenticator container.


Why are these limits required for cilium?

chrisz100 · 2019-09-16T12:56:26Z

k8s/crds/kops_v1alpha2_cluster.yaml

@@ -654,6 +674,9 @@ spec:
                  items:
                    type: string
                  type: array
+                admissionControlConfigFile:
+                  description: AdmissionControlConfigFile is the location of the admission-control-config-file


Is this really required to install cilium?

chrisz100 · 2019-09-16T12:56:44Z

k8s/crds/kops_v1alpha2_cluster.yaml

+                  description: The duration to cache authorized responses from the
+                    webhook token authorizer. Default is 30s. (default 30s)
+                  type: string
+                authorizationWebhookConfigFile:


And once again, looks like you mixed some upstream things into one PR?

chrisz100 · 2019-09-16T12:57:45Z

k8s/crds/kops_v1alpha2_cluster.yaml

+                  - bpfCTGlobalTCPMax
+                  - bpfCTGlobalAnyMax
+                  - preallocateBPFMaps
+                  - sidecarIstioProxyImage


istio for cilium? correct me if I am wrong but they don't belong together I feel?

Cilium can run alongside istio, docs are here: http://docs.cilium.io/en/v1.6/gettingstarted/istio

Signed-off-by: Maciej Kwiek <[email protected]>

nebril · 2019-09-16T14:52:11Z

@chrisz100 I did have some weird changes in this PR, thanks for pointing it out! Should be all good now.

nebril · 2019-09-17T09:03:19Z

/retest

chrisz100 · 2019-09-17T10:19:33Z

/lgtm
/assign @mikesplain

chrisz100 · 2019-09-18T20:14:05Z

It’s been long enough for people to also review.
/approve

k8s-ci-robot · 2019-09-18T20:14:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, nebril

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~~ [chrisz100]

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

mikesplain · 2019-09-18T21:08:37Z

I was planning to review this after all the current CVE's and Centos fixes were in but oh well.

mikesplain

I have a few concerns with this being merged since it's a pretty significant update in cilium and we're forcing it upon all users, even 1.7 and up? I personally would prefer this to be a new manifest that only works with 1.13 or 1.14 and up, that way we encourage people to move to new versions as they upgrade.

What's the ugprade path between 1.0 and 1.6?

Also for this to apply to existing clusters, this will also need to up updated accordingly

kops/upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

Lines 1091 to 1122 in 3b9821d

    
           if b.cluster.Spec.Networking.Cilium != nil { 
        
           	key := "networking.cilium.io" 
        
           	version := "v1.0-kops.2" 
        
           	{ 
        
           		id := "k8s-1.7" 
        
           		location := key + "/" + id + ".yaml" 
        
           		addons.Spec.Addons = append(addons.Spec.Addons, &channelsapi.AddonSpec{ 
        
           			Name:              fi.String(key), 
        
           			Version:           fi.String(version), 
        
           			Selector:          networkingSelector, 
        
           			Manifest:          fi.String(location), 
        
           			KubernetesVersion: ">=1.7.0 <1.12.0", 
        
           			Id:                id, 
        
           		}) 
        
           	} 
        
           	{ 
        
           		id := "k8s-1.12" 
        
           		location := key + "/" + id + ".yaml" 
        
           		addons.Spec.Addons = append(addons.Spec.Addons, &channelsapi.AddonSpec{ 
        
           			Name:              fi.String(key), 
        
           			Version:           fi.String(version), 
        
           			Selector:          networkingSelector, 
        
           			Manifest:          fi.String(location), 
        
           			KubernetesVersion: ">=1.12.0", 
        
           			Id:                id, 
        
           		}) 
        
           	} 
        
           }

mikesplain · 2019-09-18T21:12:16Z

pkg/apis/kops/networking.go

@@ -155,7 +155,7 @@ type AmazonVPCNetworkingSpec struct {
 	ImageName string `json:"imageName,omitempty"`
 }

-const CiliumDefaultVersion = "v1.0-stable"


Are we really upgrading from 1.0 to 1.6.1?

What’s more concerning to me: did we really not care about cilium for 6 minor versions?

Anyone is welcome to update at anytime, but especially when leaping versions we need to ensure a smooth upgrade path for all users.

mikesplain · 2019-09-18T21:14:14Z

upup/models/cloudup/resources/addons/networking.cilium.io/k8s-1.7.yaml.template

@@ -1,412 +1,136 @@
-{{- $etcd_scheme := EtcdScheme }}
-kind: ConfigMap
 apiVersion: v1


According to https://github.com/kubernetes/kops/pull/7474/files#diff-dec2adb888d136e5fcab51a2af69403dL425 Cilium doesn't support 1.7 so this shouldn't be updated? Also I really don't like forcing old users to this new version, especially when they really shouldn't be using k8s that's this old anyway.

I’m a little confused with that statement: people should use sth that old so we shouldn’t force people to update?

If a user is still running k8s 1.7 - k8s 1.12, forcing a cilium upgrade of this magnitude is likely aggressive and has a strong likelihood of causing instability.

mikesplain · 2019-09-18T21:49:49Z

@nebril Can you test an upgrade from before this PR to post this PR? We might need to break this into smaller upgrades:

According to https://docs.cilium.io/en/v1.5/install/upgrade/#upgrading-from-1-2-x-to-1-3-y

If you are running Cilium 1.0.x or 1.1.x, please upgrade to 1.2.x first. It is also possible to upgrade from 1.0 or 1.1 directly to 1.3 by combining the upgrade instructions for each minor release. See the documentation for said releases for further information.

olemarkus · 2019-09-19T07:05:26Z

The change with the most impact is probably changing state store from etcd to CRD. There will be network disruptions for the brief time cilium rebuilds the state.

I did a test upgrade from kops 1.13.0 to master now. The add-on version isn't bumped, so the manifests aren't applied. But even with the version bumped, nothing happens. So that is at least one thing to look into. What else needs to happen for the manifests to be applied?

olemarkus · 2019-09-19T07:55:11Z

Cilium resources also miss labels: role.kubernetes.io/networking: "1"

So that should be added to the manifests. But even when I label existing resources and then update, it doesn't change the daemonset.

nebril · 2019-09-19T09:06:21Z

@mikesplain the biggest issue is that Cilium addon was actually broken for several versions since kops closed access to etcd clusters for addons (I don't remember which exact kops version it happened).

I was trying to make a PR with etcd operator, but etcd operator is still hardly production ready, so instead I waited for Cilium to support being backed by CRDs, which landed in 1.6.

…4-origin-release-1.15 Automated cherry pick of #7474: Change Cilium templates to standalone version

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

k8s-ci-robot requested review from granular-ryanbonham and zetaab August 26, 2019 13:51

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 26, 2019

nebril force-pushed the cilium-standalone branch 2 times, most recently from b6435d3 to 8889400 Compare September 11, 2019 11:48

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

nebril force-pushed the cilium-standalone branch from 24d16e2 to 0db3311 Compare September 11, 2019 12:21

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

nebril force-pushed the cilium-standalone branch 3 times, most recently from 6bbdeba to 5caf4b7 Compare September 11, 2019 17:36

nebril force-pushed the cilium-standalone branch 6 times, most recently from c80742a to a38d09f Compare September 12, 2019 12:23

nebril force-pushed the cilium-standalone branch 2 times, most recently from a38d09f to 6617f6c Compare September 12, 2019 13:28

Change Cilium templates to standalone version

74e10da

This commit doesn't include any Cilium configuration, just takes the quick install yaml from https://github.com/cilium/cilium/blob/v1.6.0/install/kubernetes/quick-install.yaml Signed-off-by: Maciej Kwiek <[email protected]>

nebril force-pushed the cilium-standalone branch from 6617f6c to d2ebd0f Compare September 12, 2019 15:24

This was referenced Sep 13, 2019

Cannot change networking plugin due wrong detected etcd version #7575

Closed

Cilium with etcd-manager does not work #7046

Closed

Use a dedicated etcd cluster for Cilium CNI #6496

Closed

chrisz100 reviewed Sep 16, 2019

View reviewed changes

Add configuration options for Cilium 1.6

004cc3c

Signed-off-by: Maciej Kwiek <[email protected]>

nebril and others added 4 commits September 16, 2019 16:49

Generate apimachinery for Cilium config

8feca9a

Signed-off-by: Maciej Kwiek <[email protected]>

Mount BPF for cilium as part of nodeup instead of using a daemonset

e7ff69a

Sync the cilium-operator ClusterRole with quick-start.yaml

2ad6963

Fix remaining Cilium manifests and update hashes

7fd24b9

Signed-off-by: Maciej Kwiek <[email protected]>

nebril force-pushed the cilium-standalone branch from 09fba19 to 7fd24b9 Compare September 16, 2019 14:49

k8s-ci-robot assigned mikesplain and chrisz100 Sep 17, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2019

k8s-ci-robot merged commit 3b9821d into kubernetes:master Sep 18, 2019

mikesplain reviewed Sep 18, 2019

View reviewed changes

This was referenced Sep 21, 2019

Cilium standalone continuation #7646

Merged

Automated cherry pick of #7474: Change Cilium templates to standalone version #7695

Merged

k8s-ci-robot added a commit that referenced this pull request Oct 1, 2019

Merge pull request #7695 from olemarkus/automated-cherry-pick-of-#747…

fcb4e76

…4-origin-release-1.15 Automated cherry pick of #7474: Change Cilium templates to standalone version

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Cilium templates to standalone version #7474

Change Cilium templates to standalone version #7474

nebril commented Aug 26, 2019 •

edited

Loading

olemarkus commented Aug 26, 2019

nebril commented Sep 11, 2019

nebril commented Sep 12, 2019

chrisz100 Sep 16, 2019

chrisz100 Sep 16, 2019

chrisz100 Sep 16, 2019

chrisz100 Sep 16, 2019

nebril Sep 16, 2019

nebril commented Sep 16, 2019

nebril commented Sep 17, 2019

chrisz100 commented Sep 17, 2019

chrisz100 commented Sep 18, 2019

k8s-ci-robot commented Sep 18, 2019

mikesplain commented Sep 18, 2019

mikesplain left a comment

mikesplain Sep 18, 2019

chrisz100 Sep 18, 2019 •

edited

Loading

mikesplain Sep 18, 2019

mikesplain Sep 18, 2019

chrisz100 Sep 18, 2019

mikesplain Sep 18, 2019

mikesplain commented Sep 18, 2019

olemarkus commented Sep 19, 2019

olemarkus commented Sep 19, 2019

nebril commented Sep 19, 2019

	if b.cluster.Spec.Networking.Cilium != nil {
	key := "networking.cilium.io"
	version := "v1.0-kops.2"

	{
	id := "k8s-1.7"
	location := key + "/" + id + ".yaml"

	addons.Spec.Addons = append(addons.Spec.Addons, &channelsapi.AddonSpec{
	Name: fi.String(key),
	Version: fi.String(version),
	Selector: networkingSelector,
	Manifest: fi.String(location),
	KubernetesVersion: ">=1.7.0 <1.12.0",
	Id: id,
	})
	}

	{
	id := "k8s-1.12"
	location := key + "/" + id + ".yaml"

	addons.Spec.Addons = append(addons.Spec.Addons, &channelsapi.AddonSpec{
	Name: fi.String(key),
	Version: fi.String(version),
	Selector: networkingSelector,
	Manifest: fi.String(location),
	KubernetesVersion: ">=1.12.0",
	Id: id,
	})
	}
	}

Change Cilium templates to standalone version #7474

Change Cilium templates to standalone version #7474

Conversation

nebril commented Aug 26, 2019 • edited Loading

olemarkus commented Aug 26, 2019

nebril commented Sep 11, 2019

nebril commented Sep 12, 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

nebril commented Sep 16, 2019

nebril commented Sep 17, 2019

chrisz100 commented Sep 17, 2019

chrisz100 commented Sep 18, 2019

k8s-ci-robot commented Sep 18, 2019

mikesplain commented Sep 18, 2019

mikesplain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisz100 Sep 18, 2019 • 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

mikesplain commented Sep 18, 2019

olemarkus commented Sep 19, 2019

olemarkus commented Sep 19, 2019

nebril commented Sep 19, 2019

nebril commented Aug 26, 2019 •

edited

Loading

chrisz100 Sep 18, 2019 •

edited

Loading