Allow to use existing k8s cluster instead of minikube #442

alejandroEsc · 2018-07-24T17:38:16Z

What this PR does / why we need it:
Allows us to use an existing cluster with a valid kubeconfig to launch resources instead of using minikube

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

The command cluster create can now accept a kubeconfig file and launch on an existing k8s cluster. This allows us not to use minikube if we do not want to.

jessicaochen · 2018-07-24T20:14:50Z

Thanks for doing this! One of the reasons I wrote the logic with the ClusterProvisioner interface we could easily sub out minikube. I am glad it is being used.

I'm not going to jump on this review (unless someone wants me to). The only high level point you may want to think through is cleanup. As it is, after bootstrap is done, the external cluster will have a bunch of leftover controllers/objects sitting in it. We will want some cleanup logic if you plan on leaving the external cluster running. For minikube, the cleanup was handled in the deletion of the minikube cluster.

dims · 2018-07-24T20:49:25Z

@alejandroEsc can you please add some notes on what if any we need to run on the "external cluster"?

mkjelland

This is a great! Just added a few notes.

mkjelland · 2018-07-24T21:33:46Z

clusterctl/cmd/create_cluster.go

+			return err
+		}
+	} else {
+		exernalProvider = minikube.New(co.VmDriver)


Typo, should be externalProvider

mkjelland · 2018-07-24T21:35:46Z

clusterctl/cmd/create_cluster.go

@@ -112,6 +124,7 @@ func init() {
 	createClusterCmd.Flags().BoolVarP(&co.CleanupExternalCluster, "cleanup-external-cluster", "", true, "Whether to cleanup the external cluster after bootstrap")
 	createClusterCmd.Flags().StringVarP(&co.VmDriver, "vm-driver", "", "", "Which vm driver to use for minikube")
 	createClusterCmd.Flags().StringVarP(&co.KubeconfigOutput, "kubeconfig-out", "", "kubeconfig", "Where to output the kubeconfig for the provisioned cluster")
+	createClusterCmd.Flags().StringVarP(&co.ExistingClusterKubeconfigPath, "existing-cluster-kubeconfig", "k", "", "path to an existing cluster's kubeconfig (intead of using minikube)")


nit: Can you use uppercase for "path" to match the other flags values?

mkjelland · 2018-07-24T21:39:43Z

clusterctl/testdata/create-cluster-no-args-invalid-flag.golden

+  -a, --addon-components string              A yaml file containing cluster addons to apply to the internal cluster
+      --cleanup-external-cluster             Whether to cleanup the external cluster after bootstrap (default true)
+  -c, --cluster string                       A yaml file containing cluster object definition
+  -k, --existing-cluster-kubeconfig string   path to an existing cluster's kubeconfig (intead of using minikube)


Does this need to use the -k? Since it's not a required flag, it seems like we could force people to use the full "existing-cluster-kubeconfig" string

mkjelland · 2018-07-24T21:42:40Z

clusterctl/cmd/create_cluster.go

@@ -112,6 +124,7 @@ func init() {
 	createClusterCmd.Flags().BoolVarP(&co.CleanupExternalCluster, "cleanup-external-cluster", "", true, "Whether to cleanup the external cluster after bootstrap")
 	createClusterCmd.Flags().StringVarP(&co.VmDriver, "vm-driver", "", "", "Which vm driver to use for minikube")
 	createClusterCmd.Flags().StringVarP(&co.KubeconfigOutput, "kubeconfig-out", "", "kubeconfig", "Where to output the kubeconfig for the provisioned cluster")
+	createClusterCmd.Flags().StringVarP(&co.ExistingClusterKubeconfigPath, "existing-cluster-kubeconfig", "k", "", "path to an existing cluster's kubeconfig (intead of using minikube)")


This is a bit of a confusing concept for the external user, but it's unclear from the flag name what the existing cluster will be used for. Maybe it could be called "existing-bootstrap-cluster-kubeconfig" or change the explanation text to "path to an existing cluster's kubeconfig for bootstrapping (intead of using minikube)"

alejandroEsc · 2018-07-25T04:06:21Z

@jessicaochen would it be ok to review the cleanup process in another PR? Its definitely something im considering, including maybe adding an additional function to the interface that would do this. But, I want to be careful first and observe which items are safe to cleanup and which are not.

alejandroEsc · 2018-07-25T04:07:23Z

@dims Do you mean what are the min requirements for the external cluster? Where would you like to see the notes? Also would be ok to include that in another PR?

alejandroEsc · 2018-07-25T04:08:10Z

@jessicaochen @dims @mkjelland thank you so much for taking your time to review! I have made the changes requested for this PR at the moment. I renamed things a bit for clarity, let me know if thats ok or not.

jessicaochen · 2018-07-25T17:02:06Z

@alejandroEsc - It is fine to do cleanup in another PR. If you have not already, create an issue to track this cleanup work and add a comment in the code referencing the issue.

That way folks looking at the code will understand the situation till the cleanup is done.

If you want my 2-cents on how the cleanup should be - It should probably be in the ClusterDeployer. Since that class creates all the objects in the external cluster, it should have enough context to (optionally) clean them as well.

alejandroEsc · 2018-07-25T17:06:29Z

@jessicaochen agreed on all accounts. Issue: #448

…to be addressed soon.

mkjelland

lgtm, can someone else read through this and add the lgtm label?

dims · 2018-07-25T23:01:31Z

/lgtm

alejandroEsc · 2018-07-27T16:30:17Z

@jessicaochen @dims can i get this merged? or is there something missing?

jessicaochen · 2018-07-27T16:46:46Z

The box says you need still in an approval from at least one of your reviewers before it will automerge.

mkjelland · 2018-07-27T16:47:49Z

/approve

k8s-ci-robot · 2018-07-27T16:47:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alejandroEsc, mkjelland

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

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

dims · 2018-07-27T16:48:07Z

right, i am not an approver .. ah looks like @mkjelland took care of it

Cluster actuator needs delete permission for secret.

alejandroEsc added 3 commits July 24, 2018 10:06

created external cluster provisioner

c0e37ec

changes made here to support external provisioner that isnt minikube

7939f1a

added testing to the new external cluster provisioner

c9bf258

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 24, 2018

k8s-ci-robot requested review from mkjelland and rsdcastro July 24, 2018 17:38

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 24, 2018

alejandroEsc changed the title ~~Ae/externalcluster~~ Allow to use existing k8s cluster instead of minikube Jul 24, 2018

updated golden files to make sure tests run

e2fca82

mkjelland reviewed Jul 24, 2018

View reviewed changes

alejandroEsc added 2 commits July 24, 2018 21:02

pr changes.

5f39296

removed unused test function

8fab2a4

alejandroEsc mentioned this pull request Jul 25, 2018

External Bootstrap Cluster CleanUp #448

Closed

comment made to externalboostrapcluster referencing issue that needs …

a8a0f86

…to be addressed soon.

mkjelland approved these changes Jul 25, 2018

View reviewed changes

k8s-ci-robot assigned dims Jul 25, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2018

k8s-ci-robot merged commit f17f564 into kubernetes-sigs:master Jul 27, 2018

detiber mentioned this pull request Jul 27, 2018

clusterctl - Allow for disabling pivoting of cluster #463

Closed

jessicaochen mentioned this pull request Aug 20, 2018

Make the workflow for the creation of the external cluster pluggable #479

Closed

4 tasks

jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020

Merge pull request kubernetes-sigs#442 from akutz/feature/secret_rbac

74f274e

Cluster actuator needs delete permission for secret.

alejandroEsc deleted the ae/externalcluster branch December 29, 2021 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to use existing k8s cluster instead of minikube #442

Allow to use existing k8s cluster instead of minikube #442

alejandroEsc commented Jul 24, 2018 •

edited

Loading

jessicaochen commented Jul 24, 2018

dims commented Jul 24, 2018

mkjelland left a comment

mkjelland Jul 24, 2018

mkjelland Jul 24, 2018

mkjelland Jul 24, 2018

mkjelland Jul 24, 2018

alejandroEsc commented Jul 25, 2018

alejandroEsc commented Jul 25, 2018

alejandroEsc commented Jul 25, 2018

jessicaochen commented Jul 25, 2018 •

edited

Loading

alejandroEsc commented Jul 25, 2018

mkjelland left a comment

dims commented Jul 25, 2018

alejandroEsc commented Jul 27, 2018

jessicaochen commented Jul 27, 2018

mkjelland commented Jul 27, 2018

k8s-ci-robot commented Jul 27, 2018

dims commented Jul 27, 2018

Allow to use existing k8s cluster instead of minikube #442

Allow to use existing k8s cluster instead of minikube #442

Conversation

alejandroEsc commented Jul 24, 2018 • edited Loading

jessicaochen commented Jul 24, 2018

dims commented Jul 24, 2018

mkjelland left a comment

Choose a reason for hiding this comment

mkjelland Jul 24, 2018

Choose a reason for hiding this comment

mkjelland Jul 24, 2018

Choose a reason for hiding this comment

mkjelland Jul 24, 2018

Choose a reason for hiding this comment

mkjelland Jul 24, 2018

Choose a reason for hiding this comment

alejandroEsc commented Jul 25, 2018

alejandroEsc commented Jul 25, 2018

alejandroEsc commented Jul 25, 2018

jessicaochen commented Jul 25, 2018 • edited Loading

alejandroEsc commented Jul 25, 2018

mkjelland left a comment

Choose a reason for hiding this comment

dims commented Jul 25, 2018

alejandroEsc commented Jul 27, 2018

jessicaochen commented Jul 27, 2018

mkjelland commented Jul 27, 2018

k8s-ci-robot commented Jul 27, 2018

dims commented Jul 27, 2018

alejandroEsc commented Jul 24, 2018 •

edited

Loading

jessicaochen commented Jul 25, 2018 •

edited

Loading