[Federation][kubefed] Add option to expose federation apiserver on nodeport service #40516

shashidharatd · 2017-01-26T13:29:10Z

What this PR does / why we need it:
This PR adds an option to kubefed to expose federation api server over nodeport. This can be useful to deploy federation in non-cloud environments. This PR is target to address #39271

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

Special notes for your reviewer:

Release note:

[Federation] kubefed init learned a new flag, `--api-server-service-type`, that allows service type to be specified for the federation API server.
[Federation] kubefed init also learned a new flag, `--api-server-advertise-address`, that allows specifying advertise address for federation API server in case the service type is NodePort.

@kubernetes/sig-federation-misc @madhusudancs

k8s-reviewable · 2017-01-26T13:29:16Z

This change is

marun

This is helpful, thank you!

Is there an issue that this is targeting? I think it may be problematic to be submitting code without a clear relationship to a documented/discussed use-case or bug since otherwise it may not be obvious to reviewers whether the solution is appropriate.

Is your intent to ensure that kubefed init can complete without error in a non-cloud environment or ensure that it can deploy a working control plane? If the latter, there is a piece missing: dnsaas configuration.

The controller manager depends on cloud auth credentials being available in the container, and that won't be true for non-cloud. Here's what I needed to do to get google dns working after kubefed had deployed the control plane to a local cluster:

create a service account with the "Compute Network User" role as per the google instructions and download the resulting json auth file
create a secret in the federation namespace from the auth file
edit the controller manager deployment template to include:
- a volume for the secret
- a mount for the secret volume
- an env definition for GOOGLE_APPLICATION_CREDENTIALS that pointed to the mounted file
  - setting --dns-provider-config resulted in the controller manager complaining about invalid configuration. It didn't seem to expect json.

A similar procedure would likely be required for route 53 running outside of aws. I think kubefed should ensure that for non-cloud the control manager can be configured properly for the chosen dns provider (likely by accepting a file containing the appropriate credentials and configuring the controller manager deployment accordingly) and/or that dns can be optionally disabled (e.g. --dns-provider=none). This seems orthogonal to the issue of api accessibility, though, so it may make sense to reduce the scope of this PR (e.g. 'Ensure kubefed init can complete successfully on non-cloud') rather than trying to do it all here.

marun · 2017-01-26T16:00:28Z

federation/pkg/kubefed/util/util.go

@@ -150,3 +156,57 @@ func CreateKubeconfigSecret(clientset *client.Clientset, kubeconfig *clientcmdap
 	}
 	return secret, nil
 }
+
+func WaitForLoadBalancerAddress(clientset *client.Clientset, svc *api.Service) ([]string, []string, error) {


(No action required) No tests for either of these functions (and hence no test changes required).

marun · 2017-01-26T16:02:47Z

federation/pkg/kubefed/init/init.go

@@ -62,8 +62,7 @@ const (
 	AdminCN                     = "admin"
 	HostClusterLocalDNSZoneName = "cluster.local."

-	lbAddrRetryInterval = 5 * time.Second
-	podWaitInterval     = 2 * time.Second
+	apiserverHealthCheckInterval = 2 * time.Second


Nit: Consider using apiServer instead of apiserver for consistency.

marun · 2017-01-26T16:08:01Z

federation/pkg/kubefed/init/init.go

@@ -485,6 +507,21 @@ func createAPIServer(clientset *client.Clientset, namespace, name, image, creden
 		return dep, nil
 	}

+	if !cloudEnvironment {


Defining pvc config in the spec and then removing it here if necessary may complicate future maintenance. Consider excluding the pvc configuration from the spec and adding it in this block instead.

Shouldn't pvc be optional rather than simply disabled for non-cloud? I'm pretty sure non-cloud deployments will still require persistent storage.

totally agree with you. I was reluctant to introduce new flags. what you are bringing is more variety of use cases and thanks for the review.

marun · 2017-01-26T16:12:01Z

federation/pkg/kubefed/init/init.go

@@ -386,7 +408,7 @@ func createPVC(clientset *client.Clientset, namespace, svcName, etcdPVCapacity s
 	return clientset.Core().PersistentVolumeClaims(namespace).Create(pvc)
 }

-func createAPIServer(clientset *client.Clientset, namespace, name, image, credentialsName, pvcName, advertiseAddress, storageBackend string, dryRun bool) (*extensions.Deployment, error) {
+func createAPIServer(clientset *client.Clientset, namespace, name, image, credentialsName, pvcName, advertiseAddress, storageBackend string, dryRun bool, cloudEnvironment bool) (*extensions.Deployment, error) {


Modifying existing code without a test change is an anti-pattern. Please add a test case that validates the existing behavior and then another that ensures the new behavior works as expected.

Agree, will add a testcase

marun · 2017-01-26T16:15:42Z

federation/pkg/kubefed/util/util.go

@@ -210,3 +210,26 @@ func WaitForPods(clientset *client.Clientset, pods []string, namespace string) e
 	})
 	return err
 }
+
+func GetFirstNodeIP(clientset *client.Clientset) ([]string, error) {


This function seems like something that might exist elsewhere in the codebase. Have you looked for and not found an existing implementation?

If this function is required, please ensure it is tested. It may be helpful to break the function apart to avoid having to mock the client (i.e. focus on testing a function that accepts a NodeList and returns the first address).

oops, sorry i should have checked. at first it seemed simply straight forward. I should be reusing this function GetPreferredNodeAddress. Thanks for pointing this issue.

shashidharatd · 2017-01-26T17:40:23Z

@marun, Firstly I apologize for my non-standard way of working. I happened to discuss this off-line with @madhusudancs few days back to enable users to deploy federation on non-cloud environments and we felt it is one of important thing to be done.

This PR was targeted towards enabling federation control plane to be brought up in non-cloud environments to experience the features of federation and yes, as you rightly pointed out, we need to configure dns-provider using --dns-provider-config and i have raised another PR #40528

I would create an issue and link these PR to the issue.

marun · 2017-01-26T18:37:21Z

~~This appears to target #39870~~

The correct issue is #39271

rrati · 2017-01-26T18:53:18Z

federation/pkg/kubefed/init/init.go

+		if cloudEnvironment {
+			ips, hostnames, err = util.WaitForLoadBalancerAddress(hostClientset, svc)
+		} else {
+			ips, err = util.GetFirstNodeIP(hostClientset)


Would be preferable to have this ip be configurable and not only auto-magically devined. There are use cases where you might have more than 1 "external" ip and need to provide the specific one for this service.

Shall we achieve this in subsequent PR?

IMO it falls under the umbrella of this change and isn't too much more work on top of what you already have. I don't feel too strongly about doing to in this PR vs a different one, although if multiple flags is to be used (later comment) then is makes sense to all this flag with those.

Maybe move discussion to #39271 as per @madhusudancs?

marun · 2017-01-26T19:15:56Z

I wonder whether specific options would be preferable to a --cloud-env flag to ensure future cloud-specific and non-cloud-specific options can be specified ad hoc.

As per an inline comment, I think enabling PVC could be a separate flag (e.g. --etcd-pvc={true,false}. Similarly, the method to expose the api service could be configurable (e.g. --api-service-type={loadbalancer,nodeport,etc}). Whether a --cloud-env flag was added to default these to sane non-cloud defaults (e.g. --etcd-pvc=false, --api-service-type=nodeport) is more of a ux nicety than a requirement, since the docs for non-cloud could as well just provide an example command that would be likely to work (e.g. kubefed --init [name] --etcd-pvc=false --api-service=type=nodeport).

rrati · 2017-01-26T19:21:37Z

@marun +1 to multiple flags. You can have them default to cloud services, but using multiple flags gives most flexibility and functionality.

shashidharatd · 2017-01-26T19:31:54Z

@marun, @rrati, multiple flags is the best way forward.

marun · 2017-01-26T19:33:10Z

@shashidharatd I'm not authoritative here, I'm just fomenting discussion. I think one or more of the experienced federation contributors should have the final say and I recommend waiting on their input before you proceed.

madhusudancs · 2017-01-26T19:49:14Z

@marun @shashidharatd

Note, although it looks similar, this PR and PR #40528 aren't exactly targeting issue #39870. Issue #39870 is for a deployment mechanism that we now consider deprecated. We still need an issue for this PR and discuss the potential solutions there.

I would also create more specific issues for making kubefed work without load balancer addresses, for making it work without PV dynamic provisioning and optionally create an umbrella issue to track all of them for non-cloud environments. But note that these features are not necessarily specific to non-cloud environments. It might as well apply to cloud environments where the provider doesn't support the features provided by other providers. This is also the reason why having a catch-all --cloud-env is not sufficient here and we need the flags to be specific to the particular feature as discussed in the comments above.

In any case, I agree that we need an issue to discuss all these.

rrati · 2017-01-26T20:22:34Z

@madhusudancs I disagree that #39870 isn't applicable. The issue, whether it is the docs or the new, is that the build/deploy is too cloud dependent and kubefed as it is now doesn't solve that problem, hence this PR. Additionally, the issue with using images from source is the same if kubefed is used or not.

marun · 2017-01-26T20:22:57Z

@madhusudancs I agree that it makes sense to tackle the flags separately. The issue @shashidharatd created is probably as good a starting point as any: #40536

madhusudancs · 2017-01-26T21:30:09Z

@rrati we acknowledge that kubefed doesn't work in non-cloud scenarios and hence this PR.

@marun this PR is a good starting place, but it isn't sufficient. We need a place where we discuss potential solutions and then account for it. PRs are Ok, but I have rarely seen people going back to PRs for archaeology. Issues give a good overview of the problem in question and the potential solutions considered without distractions such as "fix this formatting error" or "move this function to a different package".

marun · 2017-01-26T23:21:42Z

@madhusudancs Thank you for the clarification, I think that is the best approach. I wasn't sure whether the PR could proceed without an issue, but had encouraged @shashidharatd to create issues in both this PR and #40528 anyway. In response to that discussion, he went ahead and created the issue I referred to in my last comment (#40536) and I meant that it would be a good starting point for the umbrella issue.

marun · 2017-01-26T23:35:25Z

@shashidharatd I think it makes sense to target this PR at #39271, which implies that the scope be limited to supporting other methods of exposing the federation control plane. The discussion of how best to address the issue should occur there as per @madhusudancs.

shashidharatd · 2017-01-31T18:41:01Z

as suggested by @marun, this pr is now scoped down to just supporting other methods of exposing the federation control plane via nodeport service.

madhusudancs · 2017-02-03T09:09:44Z

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

federation/pkg/kubefed/init/init.go, line 184 at r1 (raw file):

Previously, shashidharatd (Shashidhara T D) wrote…

Done.

Ah, makes sense now.

federation/pkg/kubefed/init/init.go, line 178 at r2 (raw file):

Previously, shashidharatd (Shashidhara T D) wrote…

Done.

I would just leave this out for this PR. There are already too many things going on here.

federation/pkg/kubefed/init/init.go, line 368 at r4 (raw file):

Previously, shashidharatd (Shashidhara T D) wrote…

Done.

Yeah, leave it the way it is now.

federation/pkg/kubefed/init/init.go, line 790 at r6 (raw file):

func printSuccess(cmdOut io.Writer, ips, hostnames []string, apiserverServiceType v1.ServiceType, svc *api.Service) error {
	svcEndpoints := append(ips, hostnames...)

I am now worried about not printing all the endpoints in the non-NodePort case. Can we please bring that back for non-NodePorts? For NodePorts, if there is more than one IP, please add an ellipsis. Some that looks like 10.45.50.128, ... or something like that.

federation/pkg/kubefed/init/init.go, line 393 at r7 (raw file):

Previously, shashidharatd (Shashidhara T D) wrote…

Done.

Thanks for verifying.

federation/pkg/kubefed/init/init.go, line 269 at r8 (raw file):

	}

	// Pick the first ip/hostname to update the api server endpoint in kubeconfig and also to give information to user

Can you please explain here in the comments that the ips in NodePort case are the IPs of the nodes?

Comments from Reviewable

shashidharatd · 2017-02-03T16:59:09Z

@madhusudancs handled your latest comments and rebased. PTAL

Review status: 1 of 4 files reviewed at latest revision, 12 unresolved discussions.

federation/pkg/kubefed/init/init.go, line 790 at r6 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

I am now worried about not printing all the endpoints in the non-NodePort case. Can we please bring that back for non-NodePorts? For NodePorts, if there is more than one IP, please add an ellipsis. Some that looks like 10.45.50.128, ... or something like that.

Handled. PTAL

federation/pkg/kubefed/init/init.go, line 269 at r8 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

Can you please explain here in the comments that the ips in NodePort case are the IPs of the nodes?

Done.

Comments from Reviewable

madhusudancs · 2017-02-03T18:50:06Z

/lgtm
/approve

Reviewed 1 of 3 files at r9.
Review status: 2 of 4 files reviewed at latest revision, 9 unresolved discussions.

Comments from Reviewable

madhusudancs · 2017-02-03T18:50:45Z

Reviewed 1 of 4 files at r5, 2 of 3 files at r9.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

Comments from Reviewable

madhusudancs · 2017-02-03T18:57:15Z

@shashidharatd please add a release note and I will approve the PR.

ixdy · 2017-02-03T19:22:38Z

/approve
for hack/verify-flags

k8s-github-robot · 2017-02-03T19:26:54Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: ixdy, madhusudancs, shashidharatd

Needs approval from an approver in each of these OWNERS Files:

~~federation/OWNERS~~ [madhusudancs]
~~hack/OWNERS~~ [ixdy]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

shashidharatd · 2017-02-04T00:01:02Z

@madhusudancs Added release notes. PTAL. Thanks

madhusudancs · 2017-02-04T00:14:05Z

@shashidharatd don't think about release note like the way you think about commit logs. These are targeted for users. Make it sound something like this.

"""
[Federation] kubefed init learned a new flag, --api-server-service-type, that allows service type to be specified for the federation API server.
[Federation] kubefed init also learned a new flag, --api-server-advertise-address, that allows specifying advertise address for federation API server in case the NodePort type service.
"""

shashidharatd · 2017-02-04T00:20:32Z

Thanks @madhusudancs for the suggestion. Have updated the release notes. PTAL

madhusudancs · 2017-02-05T19:30:42Z

@shashidharatd thanks!

k8s-ci-robot · 2017-02-05T20:25:57Z

@shashidharatd: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins CRI GCE e2e	`03928df`	link	`@k8s-bot cri e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-02-05T20:48:33Z

Automatic merge from submit-queue

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

k8s-github-robot assigned colhom Jan 26, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jan 26, 2017

shashidharatd force-pushed the kubefed-1 branch from 1759712 to b33f09c Compare January 26, 2017 14:03

marun reviewed Jan 26, 2017

View reviewed changes

shashidharatd changed the title ~~[Federation][kubefed] Support to run kubefed on non-cloud environment~~ [Federation][kubefed] Ensure kubefed init can complete successfully on non-cloud environments Jan 26, 2017

marun mentioned this pull request Jan 26, 2017

[Federation][kubefed] Support configuring dns-provider #40528

Merged

rrati reviewed Jan 26, 2017

View reviewed changes

shashidharatd mentioned this pull request Jan 26, 2017

Federation: Support deploying federation control plane on non-cloud environment #40536

Closed

6 tasks

k8s-github-robot assigned madhusudancs and unassigned colhom Jan 30, 2017

apelisse assigned colhom Jan 31, 2017

k8s-github-robot assigned jbeda Jan 31, 2017

shashidharatd force-pushed the kubefed-1 branch from b33f09c to 845470e Compare January 31, 2017 18:33

shashidharatd changed the title ~~[Federation][kubefed] Ensure kubefed init can complete successfully on non-cloud environments~~ [Federation][kubefed] Add option to expose federation apiserver on nodeport service Jan 31, 2017

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

Add option to expose federation apiserver on nodeport service

03928df

shashidharatd force-pushed the kubefed-1 branch from 61caac4 to 03928df Compare February 3, 2017 16:51

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2017

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

madhusudancs unassigned jbeda and colhom Feb 3, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 3, 2017

madhusudancs added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Feb 3, 2017

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

madhusudancs added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 3, 2017

madhusudancs removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 5, 2017

k8s-github-robot merged commit 419d38a into kubernetes:master Feb 5, 2017

irfanurrehman mentioned this pull request Feb 9, 2017

[Federation] Add override flags options to kubefed init #40917

Merged

madhusudancs mentioned this pull request Mar 13, 2017

[Federation] kubefed init should also work on clusters without a service load balancer implementation #39271

Closed

[Federation][kubefed] Add option to expose federation apiserver on nodeport service #40516

[Federation][kubefed] Add option to expose federation apiserver on nodeport service #40516

Conversation

shashidharatd commented Jan 26, 2017 • edited by madhusudancs

k8s-reviewable commented Jan 26, 2017

marun 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

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

shashidharatd commented Jan 26, 2017

marun commented Jan 26, 2017 • edited

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

marun commented Jan 26, 2017

rrati commented Jan 26, 2017

shashidharatd commented Jan 26, 2017

marun commented Jan 26, 2017

madhusudancs commented Jan 26, 2017

rrati commented Jan 26, 2017

marun commented Jan 26, 2017

madhusudancs commented Jan 26, 2017 • edited

marun commented Jan 26, 2017

marun commented Jan 26, 2017

shashidharatd commented Jan 31, 2017

madhusudancs commented Feb 3, 2017

shashidharatd commented Feb 3, 2017

madhusudancs commented Feb 3, 2017

madhusudancs commented Feb 3, 2017

madhusudancs commented Feb 3, 2017 • edited

ixdy commented Feb 3, 2017

k8s-github-robot commented Feb 3, 2017

shashidharatd commented Feb 4, 2017

madhusudancs commented Feb 4, 2017

shashidharatd commented Feb 4, 2017

madhusudancs commented Feb 5, 2017

k8s-ci-robot commented Feb 5, 2017

k8s-github-robot commented Feb 5, 2017

shashidharatd commented Jan 26, 2017 •

edited by madhusudancs

marun commented Jan 26, 2017 •

edited

madhusudancs commented Jan 26, 2017 •

edited

madhusudancs commented Feb 3, 2017 •

edited