change clientset of service controller to versioned #26694

mfanjie · 2016-06-02T09:45:11Z

create a public function BuildClusterConfig() to build cluster config so that service controller can reuse
Current implement always requires POD_NAMESPACE to be set and always require federation control plane be placed in k8s cluster. Per my understanding if use did not provide secretRef and want to use unsecure connection, then we should not call KubeconfigGetterForCluster(), instead of we should create the clientset by clientcmd.BuildConfigFromFlags().

@nikhiljindal please correct me if I am wrong for bullet 2.

k8s-bot · 2016-06-02T09:46:15Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-06-02T09:46:33Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-06-02T09:47:15Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

mfanjie · 2016-06-03T05:29:11Z

added further changes:

moved the function BuildClusterConfig() and KubeconfigGetterForCluster() to federation/pkg/federation-controller/util/cluster_util.go
changed clientset and objects in service controller to versioned.
fixed a panic in dns.go#L284 when zone is not configured on the zones is not properly configured
changed the expect object in clustercontroller to versioned.
changed copyright of info cluster controller code to 2016
@quinton-hoole, please help to add 1.3 milestone, it should be merged to 1.3. Thanks.

ghost · 2016-06-04T00:06:03Z

cc @nikhiljindal

ghost · 2016-06-04T00:06:48Z

@kubernetes/sig-cluster-federation

ghost · 2016-06-04T00:08:05Z

@k8s-bot ok to test

ghost · 2016-06-04T00:11:33Z

federation/pkg/federation-controller/service/dns.go

@@ -254,6 +254,9 @@ func (s *ServiceController) ensureDnsRecords(clusterName string, cachedService *
 	// the state of the service when we last successfully sync'd it's DNS records.
 	// So this time around we only need to patch that (add new records, remove deleted records, and update changed records.
 	//
+	if s.dns == nil {
+		return nil


I think we should return an error here, as the record sets are not sync'd?

dns equals nil means the nds provider initialization failed, right? Even I did not setup dnsprovider I can still run the service controller now. If we want to throw error, we should throw it in init phase. The reason I added this line is that with the change, I can still test the service controller without dns provider

I think that your code should not call ensureDnsRecords if the DNS provider is not initialized. And if it does, ensureDnsRecords should return an error (as it originally did), not nil like this. But again, I can fix this in a followup PR if you prefer.

ghost · 2016-06-04T00:18:54Z

I've reviewed the DNS-related changes. Jesse, let me know what you think.
@nikhiljindal could you review the API versioning and config changes, as you have more recent context on that. I remain somewhat uncomfortable with the explicitly versioned API's (because all of this code will presumably need to be changed when we want to use later API version), but I don't have enough context to know whether there's a better way.

nikhiljindal · 2016-06-05T02:16:06Z

federation/pkg/federation-controller/util/cluster_util.go

+
+// This is to inject a different kubeconfigGetter in tests.
+// We dont use the standard one which calls NewInCluster in tests to avoid having to setup service accounts and mount files with secret tokens.
+var KubeconfigGetterForCluster = func(c *federation_v1alpha1.Cluster) clientcmd.KubeconfigGetter {


You will get KubeconfigGetterForSecret as well when you rebase

nikhiljindal · 2016-06-05T02:19:35Z

Thanks for sending this change @mfanjie
Looks good.

Regarding point 2. It is fine to call KubeconfigGetterForCluster for unsecured connection. It can just return clientcmd.Load(data) if c.Spec.SecretRef == nil. No need to read POD_NAMESPACE env var or create a client in that case.

nikhiljindal · 2016-06-05T02:22:13Z

federation/pkg/federation-controller/util/cluster_util.go

+	if serverAddress != "" {
+		if c.Spec.SecretRef == nil {
+			glog.Infof("didnt find secretRef for cluster %s. Trying insecure access", c.Name)
+			clusterConfig, err = clientcmd.BuildConfigFromFlags(serverAddress, "")


This is fine. But using BuildConfigFromKubeconfigGetter is fine as well. We can update BuildConfigFromKubeconfigGetter to not create the client and read POD_NAMESPACE if c.Spec.SecretRef is nil.

I am keeping this, as

we need serverAddress to build the cluster configure, we we put the logic into BuildConfigFromKubeconfigGetter, we need either pass serverAddress into it, or we need to parse the serverAddress inside BuildConfigFromKubeconfigGetter again.

the clientcmd.load need data which is bytes array representing the how client config, but we only have serverAddress. if we use clientcmd.load(), we need extra logic to build the byte array. From my view, we can simply use clientcmd.BuildConfigFromFlags(serverAddress, "")

Sorry I didnt understand, BuildConfigFromKubeconfigGetter already takes a serverAddress.
We can always call clientcmd.BuildConfigFromKubeconfigGetter(serverAddress, kubeconfigGetter) here.
And ensure that BuildConfigFromKubeconfigGetter works fine even if c.Spec.SecretRef is nil.

What you have is fine as well.

…ller can reuse

mfanjie · 2016-06-06T05:44:32Z

federation/cmd/federation-controller-manager/app/controllermanager.go

@@ -77,7 +77,7 @@ func Run(s *options.CMServer) error {
 		glog.Errorf("unable to register configz: %s", err)
 	}
 	// Create the config to talk to federation-apiserver.
-	kubeconfigGetter := clustercontroller.KubeconfigGetterForSecret(FederationAPIServerSecretName)
+	kubeconfigGetter := util.KubeconfigGetterForSecret(FederationAPIServerSecretName)


@nikhiljindal is this a prerequisite for starting controller manager? do we need always create secret for kubeconfig of federation controller plane? I keep this as is but want to let you know , originally, we can simply run ./federation-controller-manager --v=5 to start controller manager without any preconfiguration, but now I have to create secret for kubeconfig.

Yes federation-controller-manager needs the kubeconfig of federation-apiserver to be able to talk to it.

We can update this code to try insecure access if secret does not exist, which will ensure that your local setup will work without secret. For clusters on GCE/GKE or any other cloud provider, we will need the secret.

mfanjie · 2016-06-06T07:26:11Z

@nikhiljindal @quinton-hoole I modified the code based on your comments and the most reasonable way from my view point. please review again, thanks.

nikhiljindal · 2016-06-06T17:19:34Z

The controller changes LGTM. I will let @quinton-hoole have a look at the DNS changes

ghost · 2016-06-06T23:35:04Z

I'm still not happy with the DNS changes, as detailed in comments. But I can fix that in a followup PR. So lets get this merged to unblock Jesse.

mfanjie · 2016-06-07T01:21:01Z

@quinton-hoole ok, thanks

mfanjie · 2016-06-07T07:26:54Z

e2e failed, but the logs did not show anything related to this PR.

yujuhong · 2016-06-07T20:03:01Z

@k8s-bot node e2e test this issue: #26431

k8s-github-robot · 2016-06-07T23:54:01Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-06-08T00:53:24Z

GCE e2e build/test passed for commit 6dde087.

k8s-github-robot · 2016-06-08T01:49:51Z

Automatic merge from submit-queue

googlebot added the cla: yes label Jun 2, 2016

k8s-github-robot assigned ghost Jun 2, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jun 2, 2016

mfanjie force-pushed the extract_func_for_build_cluster_config branch from 6517394 to 5bc1d55 Compare June 3, 2016 05:23

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 3, 2016

mfanjie changed the title ~~extract public function for build cluster config~~ change clientset of service controller to versioned Jun 3, 2016

mfanjie force-pushed the extract_func_for_build_cluster_config branch from 5bc1d55 to fd193bb Compare June 3, 2016 07:40

mfanjie mentioned this pull request Jun 3, 2016

Make clientset of federation controllers identical #26608

Closed

ghost added this to the v1.3 milestone Jun 4, 2016

ghost reviewed Jun 4, 2016
View reviewed changes

ghost assigned nikhiljindal and unassigned ghost Jun 4, 2016

ghost added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 4, 2016

mfanjie mentioned this pull request Jun 4, 2016

Ubernetes e2e flakes #26762

Closed

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

nikhiljindal reviewed Jun 5, 2016
View reviewed changes

extract public function for build cluster config so that other contro…

c2dbce6

…ller can reuse

mfanjie force-pushed the extract_func_for_build_cluster_config branch from fd193bb to 7b3f0d8 Compare June 6, 2016 05:39

mfanjie reviewed Jun 6, 2016
View reviewed changes

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2016

change clientset of service controller to versioned one

6dde087

mfanjie force-pushed the extract_func_for_build_cluster_config branch from 7b3f0d8 to 6dde087 Compare June 6, 2016 09:04

ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2016

ghost mentioned this pull request Jun 6, 2016

Address outstanding DNS review comments in #26694 #26921

Closed

k8s-github-robot merged commit e5da972 into kubernetes:master Jun 8, 2016

irfanurrehman mentioned this pull request Oct 30, 2017

Address outstanding DNS review comments in #26694 kubernetes-retired/federation#48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change clientset of service controller to versioned #26694

change clientset of service controller to versioned #26694

mfanjie commented Jun 2, 2016

k8s-bot commented Jun 2, 2016

k8s-bot commented Jun 2, 2016

k8s-bot commented Jun 2, 2016

mfanjie commented Jun 3, 2016 •

edited

Loading

ghost commented Jun 4, 2016

ghost commented Jun 4, 2016

ghost commented Jun 4, 2016

ghost Jun 4, 2016

mfanjie Jun 4, 2016

ghost Jun 6, 2016

ghost commented Jun 4, 2016

nikhiljindal Jun 5, 2016

nikhiljindal commented Jun 5, 2016

nikhiljindal Jun 5, 2016

mfanjie Jun 6, 2016

nikhiljindal Jun 6, 2016

mfanjie Jun 6, 2016 •

edited

Loading

nikhiljindal Jun 6, 2016

mfanjie commented Jun 6, 2016 •

edited

Loading

nikhiljindal commented Jun 6, 2016

ghost commented Jun 6, 2016

mfanjie commented Jun 7, 2016

mfanjie commented Jun 7, 2016

yujuhong commented Jun 7, 2016

k8s-github-robot commented Jun 7, 2016

k8s-bot commented Jun 8, 2016

k8s-github-robot commented Jun 8, 2016

change clientset of service controller to versioned #26694

change clientset of service controller to versioned #26694

Conversation

mfanjie commented Jun 2, 2016

k8s-bot commented Jun 2, 2016

k8s-bot commented Jun 2, 2016

k8s-bot commented Jun 2, 2016

mfanjie commented Jun 3, 2016 • edited Loading

ghost commented Jun 4, 2016

ghost commented Jun 4, 2016

ghost commented Jun 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 4, 2016

Choose a reason for hiding this comment

nikhiljindal commented Jun 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfanjie Jun 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfanjie commented Jun 6, 2016 • edited Loading

nikhiljindal commented Jun 6, 2016

ghost commented Jun 6, 2016

mfanjie commented Jun 7, 2016

mfanjie commented Jun 7, 2016

yujuhong commented Jun 7, 2016

k8s-github-robot commented Jun 7, 2016

k8s-bot commented Jun 8, 2016

k8s-github-robot commented Jun 8, 2016

mfanjie commented Jun 3, 2016 •

edited

Loading

mfanjie Jun 6, 2016 •

edited

Loading

mfanjie commented Jun 6, 2016 •

edited

Loading