Add Cloud Controller Manager addon #3630

wlan0 · 2017-10-14T18:36:09Z

This adds the CCM addon for the Kubernetes cluster.

This is a follow-up PR to #3408.

wlan0 · 2017-10-14T18:36:26Z

/assign chrislovecnm

wlan0 · 2017-10-14T18:45:17Z

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

+      - name: cloud-controller-manager
+        # for in-tree providers we use gcr.io/google_containers/cloud-controller-manager
+        # this can be replaced with any other image for out-of-tree providers
+        image: gcr.io/google_containers/cloud-controller-manager:v{{ .KubernetesVersion }}  # Reviewers: Will this work?


@chrislovecnm Is this the best way to set the version?

We should replace the whole image for out-of-tree providers?

I a master wondering if we should have the container images in code. Pondering

I am wondering ...

Any verdict on this?

The verdict is we need an issue open for tracking this.

jhorwit2 · 2017-10-14T18:49:44Z

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

+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: ClusterRole
+  name: cluster-admin


This should be system:cloud-controller-manager

andrewsykim · 2017-10-14T18:50:55Z

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

+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: ClusterRole
+  name: cluster-admin


system:cloud-controller-manager?

jhorwit2 · 2017-10-14T18:53:57Z

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

+  resources:
+  - serviceaccounts
+  verbs:
+  - create


This is missing persistent volumes access which GCE/AWS depend on for the labeler controller.

Thanks, I'll add it.

andrewsykim · 2017-10-14T18:54:03Z

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

+---
+
+apiVersion: extensions/v1beta1
+kind: Deployment


I think for kops, we can use daemonset on master cause we know that will be available. The original docs uses Deployment since it's not always guarenteed that is the case.

We just need one of these? Or do we need on on each master?

Deployment with the same tags as dns-controller. That will give us one, only on a master. We can follow same pattern. DS are great, but we would get three with HA master.

CCM supports leader-election so DS can still work :).

@chrislovecnm I spoke with @andrewsykim offline, and found that DS might be a better approach than Deployments.

DS makes it easy to scale the CCM as the number of master nodes scale. I'll update the implementation shortly

wlan0 · 2017-10-16T16:26:33Z

Added access for PVL Controller
Updated to DaemonSet

PTAL @chrislovecnm @andrewsykim @jhorwit2

jhorwit2 · 2017-10-16T17:02:04Z

lgtm

andrewsykim · 2017-10-17T03:29:25Z

pkg/apis/kops/validation/legacy.go

@@ -117,6 +117,11 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error {
 	if strict && c.Spec.KubeControllerManager == nil {
 		return field.Required(fieldSpec.Child("KubeControllerManager"), "KubeControllerManager not configured")
 	}
+	if kubernetesRelease.LT(semver.MustParse("1.7.0")) {


nit:

if kubernetesRelease.LT(semver.MustParse("1.7.0")) && c.Spec.ExternalCloudControllerManager != nil

please file a PR to move this out of legacy. validation.go is the new home.

andrewsykim · 2017-10-17T03:30:20Z

pkg/apis/kops/validation/legacy.go

@@ -279,23 +284,31 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error {
 		}

 		if c.Spec.Kubelet != nil && (strict || c.Spec.Kubelet.CloudProvider != "") {
-			if k8sCloudProvider != c.Spec.Kubelet.CloudProvider {
-				return field.Invalid(fieldSpec.Child("Kubelet", "CloudProvider"), c.Spec.Kubelet.CloudProvider, "Did not match cluster CloudProvider")
+			if c.Spec.Kubelet.CloudProvider != "external" {


nit:

if c.Spec.Kubelet.CloudProvider != "external" && k8sCloudProvider != c.Spec.Kubelet.CloudProvider

andrewsykim · 2017-10-17T03:30:30Z

pkg/apis/kops/validation/legacy.go

 			}
 		}
 		if c.Spec.MasterKubelet != nil && (strict || c.Spec.MasterKubelet.CloudProvider != "") {
-			if k8sCloudProvider != c.Spec.MasterKubelet.CloudProvider {
-				return field.Invalid(fieldSpec.Child("MasterKubelet", "CloudProvider"), c.Spec.MasterKubelet.CloudProvider, "Did not match cluster CloudProvider")
+			if c.Spec.MasterKubelet.CloudProvider != "external" {


andrewsykim · 2017-10-17T03:30:43Z

pkg/apis/kops/validation/legacy.go

 			}
 		}
 		if c.Spec.KubeAPIServer != nil && (strict || c.Spec.KubeAPIServer.CloudProvider != "") {
-			if k8sCloudProvider != c.Spec.KubeAPIServer.CloudProvider {
-				return field.Invalid(fieldSpec.Child("KubeAPIServer", "CloudProvider"), c.Spec.KubeAPIServer.CloudProvider, "Did not match cluster CloudProvider")
+			if c.Spec.KubeAPIServer.CloudProvider != "external" {


andrewsykim · 2017-10-17T03:30:49Z

pkg/apis/kops/validation/legacy.go

 			}
 		}
 		if c.Spec.KubeControllerManager != nil && (strict || c.Spec.KubeControllerManager.CloudProvider != "") {
-			if k8sCloudProvider != c.Spec.KubeControllerManager.CloudProvider {
-				return field.Invalid(fieldSpec.Child("KubeControllerManager", "CloudProvider"), c.Spec.KubeControllerManager.CloudProvider, "Did not match cluster CloudProvider")
+			if c.Spec.KubeControllerManager.CloudProvider != "external" {


andrewsykim · 2017-10-17T03:32:14Z

pkg/model/components/apiserver.go

@@ -115,6 +115,10 @@ func (b *KubeAPIServerOptionsBuilder) BuildOptions(o interface{}) error {
 		return fmt.Errorf("unknown cloudprovider %q", clusterSpec.CloudProvider)
 	}

+	if clusterSpec.ExternalCloudControllerManager != nil {


Adding this here so we can discuss later. I don't think setting this flag is actually required for apiserver, leaving the cloudprovider blank is sufficient, same for KCM. No change required for this PR just a note for the future

@wlan0 please file an issue and make a note in the code

andrewsykim · 2017-10-17T03:34:47Z

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

+        - --use-service-account-credentials
+        # these flags will vary for every cloud provider
+        - --allocate-node-cidrs=true
+        - --configure-cloud-routes=true


If these vary for every cloud provider should we have them be configurable?

That is what I am struggling with, do we abstract this or make it so that we have a manifest for every cloud provider. Thoughts?

I don't think we need a separate manifest for every cloud provider yet.
Edit:
I think it would suffice to just make them configurable.

Have we addressed this? I think this is a blocker.

chrislovecnm

Need to get issues filed. Looking good. My call is that we cleanup @andrewsykim's nit picks, and iterate.

chrislovecnm · 2017-10-18T18:01:15Z

pkg/apis/kops/validation/legacy.go

@@ -117,6 +117,11 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error {
 	if strict && c.Spec.KubeControllerManager == nil {
 		return field.Required(fieldSpec.Child("KubeControllerManager"), "KubeControllerManager not configured")
 	}
+	if kubernetesRelease.LT(semver.MustParse("1.7.0")) {


please file a PR to move this out of legacy. validation.go is the new home.

chrislovecnm · 2017-10-18T18:01:45Z

pkg/model/components/apiserver.go

@@ -115,6 +115,10 @@ func (b *KubeAPIServerOptionsBuilder) BuildOptions(o interface{}) error {
 		return fmt.Errorf("unknown cloudprovider %q", clusterSpec.CloudProvider)
 	}

+	if clusterSpec.ExternalCloudControllerManager != nil {


@wlan0 please file an issue and make a note in the code

chrislovecnm · 2017-10-18T18:05:29Z

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

+      - name: cloud-controller-manager
+        # for in-tree providers we use gcr.io/google_containers/cloud-controller-manager
+        # this can be replaced with any other image for out-of-tree providers
+        image: gcr.io/google_containers/cloud-controller-manager:v{{ .KubernetesVersion }}  # Reviewers: Will this work?


The verdict is we need an issue open for tracking this.

wlan0 · 2017-10-19T18:11:54Z

@andrewsykim @chrislovecnm I've fixed the conditional statement to address andrew's comments.

PTAL.

jhorwit2 · 2017-10-19T18:39:02Z

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

+  - ""
+  resources:
+  - persistentvolumes
+  verbs:


This also needs secret list access since the service account credential flag is enabled.

fyi @andrewsykim

Thanks. I updated the PR

chrislovecnm · 2017-10-19T19:46:25Z

@andrewsykim can I get a LGTM?

andrewsykim · 2017-10-20T03:44:26Z

@wlan0 my previous comment about the CCM flags not being configurable (https://github.com/kubernetes/kops/pull/3630/files#r145873454) wasn't addressed which I think is a requirement for this.

wlan0 · 2017-10-20T04:03:43Z

@andrewsykim Sorry I didn't provide more context before asking for a review. I should've told you that I believe the configuration doesn't change when using the CCM for just GCE and AWS.

In the context of present day kops, those are the only two existing clouds that kops supports. This PR was meant as a step to use CCM in kops to run existing functionality, so that we can run e2e tests asap.

In order to address the larger concern, which your accurately pointed out in your comment, I have added another issue - #3669, which would deal with the intricacies of supporting vendor CCMs (such as DO) in the right manner and its configurability.

chrislovecnm · 2017-10-20T12:36:21Z

@wlan0 we will need another e2e run for ccm as well ;) It will not happen automatically with addons.

andrewsykim · 2017-10-20T12:37:20Z

/lgtm

@wlan0 thanks for clarifying!

wlan0 · 2017-10-20T12:58:54Z

@wlan0 we will need another e2e run for ccm as well ;) It will not happen automatically with addons.

Yes, I'm aware

chrislovecnm · 2017-10-21T01:32:16Z

/approve

k8s-github-robot · 2017-10-21T01:32:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, 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-10-21T01:33:03Z

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

k8s-github-robot · 2017-10-21T02:07:50Z

Automatic merge from submit-queue.

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

k8s-ci-robot assigned chrislovecnm Oct 14, 2017

wlan0 commented Oct 14, 2017

View reviewed changes

jhorwit2 suggested changes Oct 14, 2017

View reviewed changes

andrewsykim reviewed Oct 14, 2017

View reviewed changes

jhorwit2 reviewed Oct 14, 2017

View reviewed changes

andrewsykim reviewed Oct 14, 2017

View reviewed changes

wlan0 force-pushed the api branch 3 times, most recently from 97f27b3 to 69eb97c Compare October 16, 2017 16:24

andrewsykim reviewed Oct 17, 2017

View reviewed changes

chrislovecnm reviewed Oct 18, 2017

View reviewed changes

wlan0 force-pushed the api branch from 69eb97c to cf72658 Compare October 19, 2017 18:10

wlan0 mentioned this pull request Oct 19, 2017

Enabling other cloud support in Kops #3669

Open

3 tasks

jhorwit2 suggested changes Oct 19, 2017

View reviewed changes

add Cloud Controller Manager addon

d3143ec

wlan0 force-pushed the api branch from cf72658 to d3143ec Compare October 19, 2017 19:33

k8s-ci-robot assigned andrewsykim Oct 20, 2017

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

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

k8s-github-robot merged commit 3315d29 into kubernetes:master Oct 21, 2017

wlan0 deleted the api branch October 21, 2017 17:56

fred-vogt mentioned this pull request Jun 27, 2020

Add support for using cloud-controller-manager (external cloud provider) in kops clusters #2778

Open

Add Cloud Controller Manager addon #3630

Add Cloud Controller Manager addon #3630

Conversation

wlan0 commented Oct 14, 2017 • edited Loading

wlan0 commented Oct 14, 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

jhorwit2 Oct 14, 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

Choose a reason for hiding this comment

wlan0 Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

wlan0 commented Oct 16, 2017

jhorwit2 commented Oct 16, 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

wlan0 Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

wlan0 commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Oct 19, 2017

andrewsykim commented Oct 20, 2017

wlan0 commented Oct 20, 2017 • edited Loading

chrislovecnm commented Oct 20, 2017

andrewsykim commented Oct 20, 2017

wlan0 commented Oct 20, 2017

chrislovecnm commented Oct 21, 2017

k8s-github-robot commented Oct 21, 2017

k8s-github-robot commented Oct 21, 2017

k8s-github-robot commented Oct 21, 2017

wlan0 commented Oct 14, 2017 •

edited

Loading

jhorwit2 Oct 14, 2017 •

edited

Loading

wlan0 Oct 16, 2017 •

edited

Loading

wlan0 Oct 17, 2017 •

edited

Loading

wlan0 commented Oct 20, 2017 •

edited

Loading