[Federation] Use service accounts instead of the user's credentials when accessing joined clusters' API servers. #42042

perotinus · 2017-02-24T08:38:54Z

Release notes:

Modifies kubefed to create and the federation controller manager to use credentials associated with a service account rather than the user's credentials.

k8s-ci-robot · 2017-02-24T08:38:55Z

Hi @perotinus. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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-reviewable · 2017-02-24T08:39:01Z

This change is

perotinus · 2017-02-24T08:39:08Z

cc @madhusudancs, @nikhiljindal

madhusudancs · 2017-02-24T08:56:05Z

@irfanurrehman can you review this? I will also try to do a pass.

irfanurrehman · 2017-02-24T09:35:23Z

@madhusudancs, yes, certainly!

irfanurrehman · 2017-02-24T19:32:46Z

Review status: 0 of 6 files reviewed at latest revision, 26 unresolved discussions.

federation/cluster/federation-up.sh, line 98 at r1 (raw file):

        --host-cluster-context="${HOST_CLUSTER_CONTEXT}" \
        --context="${FEDERATION_NAME}" \
        --secret-name="${context//_/-}" -v=2    # Replace "_" by "-"

Any specific reason for this change?
I think the intention of --v=2 is different (for example intended for additional information from the tool or the libs the tool is using)
When federation-up.sh prints such info, it might surprise users (for example, an info I get from kubefed unjoin at this verbosity is details about all flags passed to kubefed join, which the federation-up.sh user is unaware of).
Other point of view might be why do this only for join, why not for init, or unjoin.. ?

federation/pkg/federation-controller/util/cluster_util.go, line 69 at r1 (raw file):

			clusterConfig, err = clientcmd.BuildConfigFromFlags(serverAddress, "")
		} else {
			kubeconfigGetter := KubeconfigGetterForCluster(c)

you can as well pass the secret to this function, which u already have here.
It will avoid the getSecret() call twice.

federation/pkg/federation-controller/util/cluster_util.go, line 72 at r1 (raw file):

			if err != nil {
				return nil, err
			}

you return nil, nil in case SecretRef.Name == nil from GetSecret() , which unnecessarily traces the rest of the code.

	if secretName == "" {
		return nil, nil
	}

You can as well format an error in return from GetSecret() or handle the scenario here itself, with this check not needed inside GetSecret()

federation/pkg/federation-controller/util/cluster_util.go, line 78 at r1 (raw file):

			// serialized kubeconfig.
			if _, ok := secret.Data["token"]; ok {
				clusterConfig, err = clientcmd.BuildConfigFromFlags(serverAddress, "")

Assigning to clusterConfig without verifying error, clusterConfig could be null?

federation/pkg/federation-controller/util/cluster_util.go, line 97 at r1 (raw file):

// This is to inject a different kubeconfigGetter in tests.
// We don't 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_v1beta1.Cluster) clientcmd.KubeconfigGetter {

U can as well pass the secret object here, rather then the cluster object, because u already have it earlier.
Or even secret.Data, because that is what you consume in the getter().
Also, there wont be any need of nested functions GetterForCluster() and then GetterForSecret(), one function would suffice.

federation/pkg/federation-controller/util/cluster_util.go, line 100 at r1 (raw file):

	return func() (*clientcmdapi.Config, error) {
		secretRefName := ""
		if c.Spec.SecretRef != nil {

you sort of have already done this check earlier (in BuildClusterConfig at line 65), and then call KubeconfigGetterForCluster(), so probably not needed here again.

federation/pkg/federation-controller/util/cluster_util.go, line 109 at r1 (raw file):

}

// GetterForSecret is used to get a secret from the cluster. It is used to inject a different implementation in tests.

s/GetterForSecret/GetSecret
is this statement true or a copy paste mistake ->> "It is used to inject a different implementation in tests" ?

federation/pkg/federation-controller/util/cluster_util.go, line 110 at r1 (raw file):

// GetterForSecret is used to get a secret from the cluster. It is used to inject a different implementation in tests.
func GetSecret(secretName string) (*api.Secret, error) {

You need not export the function if used only here.

federation/pkg/federation-controller/util/cluster_util.go, line 148 at r1 (raw file):

// KubeconfigGetterForSecret gets the kubeconfig from the given secret.
func KubeconfigGetterForSecret(secretName string) clientcmd.KubeconfigGetter {

As in the comment on line 97, you probably don't need the nested function.

federation/pkg/kubefed/join.go, line 148 at r1 (raw file):

	}

	// TODO: This needs to check if the API server for the joining cluster supports RBAC.

I think this might need implementation now, rather then a TODO, otherwise registering older version clusters would fail, which works now and we do not want to break that.

federation/pkg/kubefed/join.go, line 150 at r1 (raw file):

	// TODO: This needs to check if the API server for the joining cluster supports RBAC.
	// If the cluster does not, this should fall back to the old secret creation strategy.
	glog.V(2).Info("Creating federation system namespace")

The info message ideally should be complete because we create somethings in target cluster and somethings in base cluster.
Creating federation system namespace in the joining cluster



---

*[federation/pkg/kubefed/join.go, line 151 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlJD3DpKYH0yyXPjAX:-KdlJD3DpKYH0yyXPjAY:b-gvyatx) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L151)):*
> ```Go
> 	// If the cluster does not, this should fall back to the old secret creation strategy.
> 	glog.V(2).Info("Creating federation system namespace")
> 	err = createFederationSystemNamespace(joiningClusterClient, j.commonOptions.FederationSystemNamespace)
> ```

You need to use the dryRun flag and ensure not to create object in the target cluster if dryRun is true.

---

*[federation/pkg/kubefed/join.go, line 153 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlHUocgAZ69HHTH4S7:-KdlHUod_Qbuwo6e9m_r:bcpobnc) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L153)):*
> ```Go
> 	err = createFederationSystemNamespace(joiningClusterClient, j.commonOptions.FederationSystemNamespace)
> 	if err != nil {
> 		glog.V(2).Info("Error creating federation system namespace: %v", err)
> ```

Same as previous comment about the info message (and please apply it to all subsequent info messages)

---

*[federation/pkg/kubefed/join.go, line 158 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlJQt6H49yjrArH1-h:-KdlJQt6H49yjrArH1-i:bnbhlri) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L158)):*
> ```Go
> 
> 	glog.V(2).Info("Creating service account")
> 	saName, err := createServiceAccount(joiningClusterClient, j.commonOptions.FederationSystemNamespace, j.commonOptions.Host)
> ```

Same comment for the use of dryRun, and please apply the same to all subsequent functions.

---

*[federation/pkg/kubefed/join.go, line 249 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlKLfS3BGt44rrDvwZ:-KdlKLfS3BGt44rrDvw_:bgiaf89) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L249)):*
> doesn't exist
doesn't already exist

---

*[federation/pkg/kubefed/join.go, line 259 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlJnncbAZfwhcX6PAK:-KdlJnncbAZfwhcX6PAL:bs59dp0) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L259)):*
> Could not create federation-system namespace in client
Could not create federation-system namespace in joining cluster.

---

*[federation/pkg/kubefed/join.go, line 265 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlK9qy64QT0NgN4yww:-KdlK9qy64QT0NgN4ywx:b-xu9nt4) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L265)):*
> ```Go
> }
> 
> // createServiceAccount creates a service account in the cluster associated with clusterClienr with
> ```

s/clusterClienr/clusterClient

---

*[federation/pkg/kubefed/join.go, line 268 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlL_6w3eOxfKw9yjRJ:-KdlL_6w3eOxfKw9yjRK:bqu53jl) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L268)):*
> ```Go
> // credentials that will be used by the host cluster to access its API server.
> func createServiceAccount(clusterClient *internalclientset.Clientset, namespace, hostContext string) (string, error) {
> 	saName := util.ClusterServiceAccountName(hostContext)
> ```

Because the same cluster can join multiple federations (with a possibility that 2 admins might use a same context name for the cluster), I think we should contemplate getting a more unique name for the service account.

---

*[federation/pkg/kubefed/join.go, line 277 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlM7434_SFtXuz1vvN:-KdlM744qi73c2WBU3AU:b-bawas6) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L277)):*
> ```Go
> 
> 	// Check for an existing service account with that name. Delete it if it exists.
> 	err := clusterClient.Core().ServiceAccounts(namespace).Delete(saName, &metav1.DeleteOptions{})
> ```

In line with the previous comment, I think a TODO explaining what is needed should be added here.
Options are:
1 - we generate and use an unique id (but we will need to remember it in subsequent calls, especially in unjoin, and if join is tried again)
2 - append this federation name, rather then "federation-controller-manager" that u have used to give some amount of uniqueness.
We will anyways need to brainstorm more on it.. :-)
As of now, probably only todo, suffices (or may be implement 2)

---

*[federation/pkg/kubefed/join.go, line 278 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlNJ52QtVtt5g-NJm8:-KdlNJ52QtVtt5g-NJm9:bfzpvl3) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L278)):*
> IsNotFound
If the service account already existed, wouldn't it also mean that the secret created by it (by whoever created the service account) would also exist
We should delete that too, I guess!
Also, I am not entirely sure why would u want to delete service account, if it already exists, rather then just use if it already exists. 
Is it to invalidate the existing secret data ?

---

*[federation/pkg/kubefed/join.go, line 292 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlOt0hTE9b-BL7PaOM:-KdlOt0hTE9b-BL7PaON:b-7zmlwv) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L292)):*
> ```Go
> 		}
> 
> 		return false, fmt.Errorf("service account still exists in cluster")
> ```

I am not sure, but I guess if u return error then the poll loop would exit with error.
Effectively exiting this loop after 1 try of get().
If you want to continue, u need to 
return false, nil
( but I am not sure about this one, and right now too lazy to verify :) )

---

*[federation/pkg/kubefed/join.go, line 325 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlSx4V2rtqkUBMkDJc:-KdlSx4V2rtqkUBMkDJd:bsl1fwl) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L325)):*
> namespace
Doesnt this mean, that this role binding would allow access to objects only in the namespace pointed by "namespace", even if it actually binds to a cluster role with all privileges.
I am not very sure about this, but the documentation sort of says so -> https://kubernetes.io/docs/admin/authorization/

---

*[federation/pkg/kubefed/join.go, line 357 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlXR7Rz5mTFKdJp5Qp:-KdlXR7Rz5mTFKdJp5Qq:b-ws54l4) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L357)):*
> ```Go
> 		sa, err = clusterClient.Core().ServiceAccounts(namespace).Get(saName, metav1.GetOptions{})
> 		if err != nil {
> 			return false, err
> ```

Similar to the previous comment about pollInfinite()
I think u need to use ->
return false, nil

---

*[federation/pkg/kubefed/join.go, line 380 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlYNOytjgn-qFuwAJ_:-KdlYNOytjgn-qFuwAJa:bebgl1a) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/join.go#L380)):*
> ```Go
> 	v1Secret := api.Secret{
> 		ObjectMeta: metav1.ObjectMeta{
> 			Name: secretName,
> ```

Wouldn't u need namespace here ?

---

*[federation/pkg/kubefed/unjoin.go, line 77 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlYdsikj00EGjq7v23:-KdlYdsikj00EGjq7v24:bidgjna) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/unjoin.go#L77)):*
> ```Go
> 
> // unjoinFederation is the implementation of the `unjoin` command.
> func (u *unjoinFederation) Run(f cmdutil.Factory, cmdOut, cmdErr io.Writer, config util.AdminConfig) error {
> ```

Wouldn't we need to remove whtever objects that we created in join when the cluster is removed from this federation.?

---

*[federation/pkg/kubefed/util/util.go, line 50 at r1](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042#-KdlZQpqZ4ErwyjCE4nj:-KdlZQpqZ4ErwyjCE4nk:b-wqy6fd) ([raw file](https://github.com/kubernetes/kubernetes/blob/e9ab6462d4b5844b617aac6de42bb602895c0b52/federation/pkg/kubefed/util/util.go#L50)):*
> ```Go
> 	// accounts are used by the Federation control plane to access
> 	// the cluster's API server.
> 	clusterServiceAccountNamePrefix = "federation-controller-manager"
> ```

A comment about this name is given at usage. I guess for now its not a hard requirement to have an unique name, but it certainly needs a thought.

---


*Comments from [Reviewable](https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/42042)*
<!-- Sent from Reviewable.io -->

perotinus · 2017-02-24T23:52:00Z

Thanks for the review! I have a few questions, so I've left a few

Given the code freeze coming up, I'm not sure how much of the TODO work I'll be able to get done and tested. I think supporting old clusters is probably pretty important, so I'm going to start on that now.

Review status: 0 of 5 files reviewed at latest revision, 26 unresolved discussions.

federation/pkg/federation-controller/util/cluster_util.go, line 69 at r1 (raw file):