[Federation][join-01] Implement `kubefed join` command. #35493

madhusudancs · 2016-10-25T04:35:00Z

Supersedes PR #35155.

Please review only the last commit here. This is based on PR #35492 which will be reviewed independently.

I will add a release note separately for this entire feature, so please don't worry too much about the release note here in the PR.

Design Doc: PR #34484

cc @kubernetes/sig-cluster-federation @quinton-hoole @mwielgus

This change is

nikhiljindal · 2016-10-27T04:22:35Z

federation/pkg/kubefed/join.go

+	join_long = templates.LongDesc(`
+		Join a cluster to a federation.
+
+        Current context is assumed to be a federation endpoint.


"federation endpoint" is not obvious in what it means. How about "is assumed to be for federation control plane" or "is assumed to be for federation apiserver"?

nikhiljindal · 2016-10-27T04:23:32Z

federation/pkg/kubefed/join.go

+// `PathOptions()`) and a mechanism to talk to the federation host
+// cluster.
+type JoinFederationConfig interface {
+	// PathOptions provides filesystem based kubeconfig access.


kubeconfig access to what?

It is kubeconfig-access. In other words access to kubeconfig. I thought about "filesystem based access to kubeconfig", but that doesn't sound right in this context, so wrote it this way.

nikhiljindal · 2016-10-27T04:24:17Z

federation/pkg/kubefed/join.go

+	HostFactory(host, kubeconfigPath string) cmdutil.Factory
+}
+
+// joinFederationConfig implements JoinFederationConfig interface.


Add var _ JoinFederationConfig = &joinFederationConfig{} to verify and document that

nikhiljindal · 2016-10-27T04:25:23Z

federation/pkg/kubefed/join.go

+	}
+}
+
+func (r *joinFederationConfig) PathOptions() *clientcmd.PathOptions {


nit: why use r for joinFederationConfig? :)

Changed from r to j.

You might have already guessed the reason behind r. This receiver struct did not start its life with this name. It started as registerFederationConfig :-)

nikhiljindal · 2016-10-27T04:29:00Z

federation/pkg/kubefed/join.go

+	po.LoadingRules.ExplicitPath = kubeconfig
+	clientConfig, err := po.GetStartingConfig()
+	if err != nil {
+		return err


Its generally recommended to add some context while returning error message. Like
return fmt.Errorf("error %s in getting starting config for path options: %v", po, err)

I would love to do that where the error type isn't important. But it is in this case. Adding that context coerces the error type to some unknown internal type of fmt (it is actually whatever returned by golang/pkg/errors.New()). That's a type precision loss and that means cmdutil.CheckErr(err) in the caller has no idea about the type of the error and can't do the right thing. So I think we should just return the received error without coercing it to a string.

nikhiljindal · 2016-10-27T04:45:34Z

federation/pkg/kubefed/join.go

+
+	// Build the secret object with the minified and flattened
+	// kubeconfig content.
+	secret := &api.Secret{


Use versioned clientset and create versioned secret object here.

cmdutil.Factory doesn't support external types yet, so it is not possible. I started writing code that way, but then had to switch back. I know how hand-tied it feels ;-)

nikhiljindal · 2016-10-27T04:47:26Z

federation/pkg/kubefed/join.go

+	// host:port part of the endpoint URL.
+	u, err := urlParse(cluster.Server)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse cluster endpoint %q for cluster %q", cluster.Server, name)


Also include err in this error message

Thanks for catching that!

Done.

nikhiljindal · 2016-10-27T04:51:02Z

federation/pkg/kubefed/join.go

+	}
+	serverAddress := cluster.Server
+	if u.scheme == "" {
+		// Use "https" as the default scheme if the port isn't "80". It


I think its better to leave this. Users will have unexpected successes/failures by changing the port number. As a user I would have no clue what is happening if I hit this.
PS: By leave this, I mean choose a default. Dont change it as per port number.

If you still decide to keep it, I will urge you to add 8080, since thats the port used by our local cluster up script :)
But still think removing this is best

Ok, that's a fair argument.

Dropping the port number based inference and just plain defaulting to https. Also, removing the complex parsing logic and dramatically simplifying this portion.

Great, thx! :)

nikhiljindal · 2016-10-27T04:54:03Z

federation/pkg/kubefed/join.go

+		hostport = segs[1]
+	}
+
+	remsegs := strings.SplitN(hostport, "/", 2)


whats rem in remsegs? :)

remaining? :)

Yeah. Sorry, remsegs wasn't obvious. Changed it to remainingSegs for clarity.

nikhiljindal · 2016-10-27T04:57:33Z

federation/pkg/kubefed/join.go

+	if len(segs) == 2 {
+		scheme = segs[0]
+		hostport = segs[1]
+	}


You could call net/url.Parse now once you have figured out the scheme?

I thought about that, but it is uglier than this. You need to check if the scheme was already in the string prefix. And only if not you need to stick it in front of the string and then parse it. This is easier/less uglier.

nikhiljindal · 2016-10-27T05:06:38Z

High level comments:

Will be good to add glog.V(2).Info() type comments which can help a lot in debugging. Right now there are none :)
Its generally recommended to add some context while returning an err, which makes it easier to track the error was from which caller.
Re code structure. As per kubectl code convention, this code should be in federation/pkg/kubefed/cmd/join.go. Was choosing a different dir structure intentional?

madhusudancs

@nikhiljindal addressed the comments. PTAL.

madhusudancs · 2016-10-27T05:57:16Z

federation/pkg/kubefed/join.go

+	join_long = templates.LongDesc(`
+		Join a cluster to a federation.
+
+        Current context is assumed to be a federation endpoint.


madhusudancs · 2016-10-27T05:59:34Z

federation/pkg/kubefed/join.go

+// `PathOptions()`) and a mechanism to talk to the federation host
+// cluster.
+type JoinFederationConfig interface {
+	// PathOptions provides filesystem based kubeconfig access.


It is kubeconfig-access. In other words access to kubeconfig. I thought about "filesystem based access to kubeconfig", but that doesn't sound right in this context, so wrote it this way.

madhusudancs · 2016-10-27T06:00:53Z

federation/pkg/kubefed/join.go

+	HostFactory(host, kubeconfigPath string) cmdutil.Factory
+}
+
+// joinFederationConfig implements JoinFederationConfig interface.


madhusudancs · 2016-10-27T06:02:38Z

federation/pkg/kubefed/join.go

+	}
+}
+
+func (r *joinFederationConfig) PathOptions() *clientcmd.PathOptions {


Changed from r to j.

You might have already guessed the reason behind r. This receiver struct did not start its life with this name. It started as registerFederationConfig :-)

madhusudancs · 2016-10-27T06:10:15Z

federation/pkg/kubefed/join.go

+	po.LoadingRules.ExplicitPath = kubeconfig
+	clientConfig, err := po.GetStartingConfig()
+	if err != nil {
+		return err


I would love to do that where the error type isn't important. But it is in this case. Adding that context coerces the error type to some unknown internal type of fmt (it is actually whatever returned by golang/pkg/errors.New()). That's a type precision loss and that means cmdutil.CheckErr(err) in the caller has no idea about the type of the error and can't do the right thing. So I think we should just return the received error without coercing it to a string.

madhusudancs · 2016-10-27T06:13:56Z

federation/pkg/kubefed/join.go

+
+	// Build the secret object with the minified and flattened
+	// kubeconfig content.
+	secret := &api.Secret{


cmdutil.Factory doesn't support external types yet, so it is not possible. I started writing code that way, but then had to switch back. I know how hand-tied it feels ;-)

madhusudancs · 2016-10-27T06:14:45Z

federation/pkg/kubefed/join.go

+	// host:port part of the endpoint URL.
+	u, err := urlParse(cluster.Server)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse cluster endpoint %q for cluster %q", cluster.Server, name)


Thanks for catching that!

Done.

madhusudancs · 2016-10-27T06:17:51Z

federation/pkg/kubefed/join.go

+	if len(segs) == 2 {
+		scheme = segs[0]
+		hostport = segs[1]
+	}


I thought about that, but it is uglier than this. You need to check if the scheme was already in the string prefix. And only if not you need to stick it in front of the string and then parse it. This is easier/less uglier.

madhusudancs · 2016-10-27T06:19:34Z

federation/pkg/kubefed/join.go

+		hostport = segs[1]
+	}
+
+	remsegs := strings.SplitN(hostport, "/", 2)


Yeah. Sorry, remsegs wasn't obvious. Changed it to remainingSegs for clarity.

madhusudancs · 2016-10-27T07:13:46Z

federation/pkg/kubefed/join.go

+	}
+	serverAddress := cluster.Server
+	if u.scheme == "" {
+		// Use "https" as the default scheme if the port isn't "80". It


Ok, that's a fair argument.

Dropping the port number based inference and just plain defaulting to https. Also, removing the complex parsing logic and dramatically simplifying this portion.

madhusudancs · 2016-10-27T07:55:48Z

@nikhiljindal PTAL.

Will be good to add glog.V(2).Info() type comments which can help a lot in debugging. Right now there are none :)

I have added log statements where I think is appropriate.

Its generally recommended to add some context while returning an err, which makes it easier to track the error was from which caller.

Explained inline why this shouldn't be done in this case. But also pasting here.

I would love to do that where the error type isn't important. But it is in this case. Adding that context coerces the error type to some unknown internal type of fmt (it is actually whatever returned by golang/pkg/errors.New()). That's a type precision loss and that means cmdutil.CheckErr(err) in the caller has no idea about the type of the error and can't do the right thing. So I think we should just return the received error without coercing it to a string.

Re code structure. As per kubectl code convention, this code should be in federation/pkg/kubefed/cmd/join.go. Was choosing a different dir structure intentional?

Yeah. I am trying not to blindly copy the structure unless there is a good reason/explanation. kubectl has another level in the hierarchy (.../cmd/...) because there are things outside cmd such as generators etc. We don't have that today. And this is just a package. The actual binary is built from entry point federation/cmd which already has cmd to indicate that this is a command. There is no reason to have it here again. Also, kubectl's package needs to be reorganized anyway. Things are all over the place there.

k8s-ci-robot · 2016-10-27T08:58:57Z

Jenkins GKE smoke e2e failed for commit a542b337d1435f67db7c3ee8a1f24ed62ca5d219. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

nikhiljindal · 2016-10-28T06:08:01Z

Only remaining comment is regarding ensuring that secret name is valid.
All else looks good.

madhusudancs · 2016-10-28T18:27:24Z

@nikhiljindal after much deliberation, I am leaning towards not doing an automatic conversion at all. It might surprise users. And we have to explain how we do this conversion. And that conversion might break in several cases: len(context) > 256 for 2 clusters that when truncated aren't unique, some characters we miss/fail to convert and all the edge cases that come with it.

Instead we should just provide a way to customize these values via flags, so that users have more control over how these names are created if they don't confer to RFC1123 subdomain spec and have a way to reason about it.

I will add open an issue for it and implement it in a follow up PR. What do you think?

k8s-ci-robot · 2016-10-30T02:58:31Z

Jenkins GCE Node e2e failed for commit ca59772. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

nikhiljindal · 2016-11-01T07:21:09Z

Yes Adding those flags in a separate PR is fine.
This LGTM now.
Feel free to add the label once you have got the tests passing.

k8s-ci-robot · 2016-11-01T08:55:19Z

Jenkins verification failed for commit c862ffbf3e169ac4dc1e029ff72a77c853f56de8. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-01T09:20:48Z

Jenkins unit/integration failed for commit 215a629. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

madhusudancs · 2016-11-01T19:08:09Z

@k8s-bot unit test this

madhusudancs · 2016-11-01T19:08:31Z

@k8s-bot verify test this

Also, add unit tests for `kubefed join`.

madhusudancs · 2016-11-01T19:45:59Z

PR #35492 was merged. Adding the LGTM label.

k8s-github-robot · 2016-11-02T16:58:08Z

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

k8s-github-robot · 2016-11-02T17:35:55Z

Automatic merge from submit-queue

@quinton-hoole

Automatic merge from submit-queue [Federation][unjoin-00] Implement `kubefed unjoin` command. Please review only the last commit here. This is based on PR #35493 which will be reviewed independently. I will add a release note separately for this entire feature, so please don't worry too much about the release note here in the PR. Design Doc: PR #34484 cc @kubernetes/sig-cluster-federation @quinton-hoole @nikhiljindal

madhusudancs added area/cluster-federation release-note-none Denotes a PR that doesn't merit a release note. labels Oct 25, 2016

madhusudancs added this to the v1.5 milestone Oct 25, 2016

madhusudancs assigned nikhiljindal Oct 25, 2016

googlebot added the cla: yes label Oct 25, 2016

This was referenced Oct 25, 2016

[Federation][unjoin-00] Implement kubefed unjoin command. #35494

Merged

[Federation][(Un)join-02] Implement kubectl join federation command. #35155

Closed

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 25, 2016

madhusudancs force-pushed the federation-kubefed-01 branch from d30a682 to 55d633b Compare October 25, 2016 06:55

nikhiljindal reviewed Oct 27, 2016

View reviewed changes

madhusudancs force-pushed the federation-kubefed-01 branch from 55d633b to 1c5c307 Compare October 27, 2016 07:30

madhusudancs commented Oct 27, 2016

View reviewed changes

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 27, 2016

madhusudancs force-pushed the federation-kubefed-01 branch from 1c5c307 to 3f07063 Compare October 27, 2016 07:55

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 27, 2016

madhusudancs force-pushed the federation-kubefed-01 branch from 7e7283a to a542b33 Compare October 27, 2016 08:21

madhusudancs force-pushed the federation-kubefed-01 branch from a542b33 to e1a2752 Compare October 27, 2016 21:22

madhusudancs force-pushed the federation-kubefed-01 branch from e1a2752 to ca59772 Compare October 30, 2016 02:36

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 30, 2016

madhusudancs force-pushed the federation-kubefed-01 branch from ca59772 to 0ac3e3d Compare November 1, 2016 08:09

madhusudancs force-pushed the federation-kubefed-01 branch from 0ac3e3d to c862ffb Compare November 1, 2016 08:29

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 1, 2016

madhusudancs force-pushed the federation-kubefed-01 branch from c862ffb to 215a629 Compare November 1, 2016 08:58

[Federation][join-01] Implement kubefed join command.

2342f6e

Also, add unit tests for `kubefed join`.

madhusudancs force-pushed the federation-kubefed-01 branch from 215a629 to 2342f6e Compare November 1, 2016 19:45

madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 1, 2016

k8s-github-robot merged commit df8db65 into kubernetes:master Nov 2, 2016

madhusudancs deleted the federation-kubefed-01 branch November 2, 2016 21:58

madhusudancs mentioned this pull request Nov 17, 2016

It should be fast and painless to deploy a Federation of Kubernetes clusters kubernetes/enhancements#97

Closed

22 tasks

[Federation][join-01] Implement kubefed join command. #35493

[Federation][join-01] Implement kubefed join command. #35493

Conversation

madhusudancs commented Oct 25, 2016 • 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

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

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

nikhiljindal commented Oct 27, 2016

madhusudancs 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

madhusudancs commented Oct 27, 2016

k8s-ci-robot commented Oct 27, 2016

nikhiljindal commented Oct 28, 2016

madhusudancs commented Oct 28, 2016

k8s-ci-robot commented Oct 30, 2016

nikhiljindal commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

k8s-ci-robot commented Nov 1, 2016

madhusudancs commented Nov 1, 2016

madhusudancs commented Nov 1, 2016

madhusudancs commented Nov 1, 2016

k8s-github-robot commented Nov 2, 2016

k8s-github-robot commented Nov 2, 2016

[Federation][join-01] Implement `kubefed join` command. #35493

[Federation][join-01] Implement `kubefed join` command. #35493

madhusudancs commented Oct 25, 2016 •

edited

Loading