Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read the federation controller manager kubeconfig from a filesystem path #30601

Merged

Conversation

madhusudancs
Copy link
Contributor

@madhusudancs madhusudancs commented Aug 15, 2016

This decoupling from the Kubernetes API allows admins to run federation control plane components wherever they like, even outside Kubernetes. This also makes the federation controller manager read its config from one single place in a uniform and/or consistent way, instead of spreading the config around command line flags and secrets.

Federation controller manager can consume the federation API server kubeconfig from a file configured via --kubeconfig flag.

If you are upgrading your Cluster Federation components from v1.4.x, please update your `federation-apiserver` and `federation-controller-manager` manifests to the new version:

cc @kubernetes/sig-cluster-federation


This change is Reviewable

@madhusudancs madhusudancs added area/cluster-federation release-note-none Denotes a PR that doesn't merit a release note. labels Aug 15, 2016
@madhusudancs madhusudancs added this to the v1.4 milestone Aug 15, 2016
@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 15, 2016
@madhusudancs madhusudancs force-pushed the fed-cm-kubeconfig-from-flags branch from f14b706 to d121530 Compare August 15, 2016 05:53
@@ -17,16 +17,23 @@ spec:
- name: ssl-certs
hostPath:
path: /etc/ssl/certs
- name: kubeconfig
secret:
secretName: federation-apiserver-kubeconfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this secret does not exist?
Can you verify that it works fine with DeprecatedKubeconfigSecretName.

Its bad if the pod fails to come up if the secret with that name does not exist.
If the pod comes up, just that the value of kubeconfig is empty, then its fine (we will fallback to DeprecatedKubeconfigSecretName) in code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I am not sure if we will fallback to DeprecatedKubeconfigSecretName if value of kubeconfig flag is empty. Will BuildConfigFromFlags generate an error if kubeconfig is empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the secret doesn't exist the pod fails to come up. That's not the problem we are trying to solve with the deprecated secret name. We have the deprecated secret name to give an upgrade path to the existing users, so that federation still works if they just update their existing deployment manifest with the new image tag.

They are using this manifest if they are deploying federation from scratch and in that case they are expected to have this secret. It is like a pre-requisite for the new deployments.

I made one minor change though, please take a look.

@nikhiljindal
Copy link
Contributor

The code changes look good.
I am not sure if the fallback to DeprecatedKubeconfigSecretName will still work.
Feel free to apply the LGTM label once you have verified that.
Thanks!

@ghost ghost added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Aug 18, 2016
@madhusudancs madhusudancs force-pushed the fed-cm-kubeconfig-from-flags branch from d121530 to d7ddd3e Compare August 24, 2016 20:48
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 24, 2016
@nikhiljindal
Copy link
Contributor

Thanks for the code changes.
This still breaks someone who is running federation code from HEAD and upgrades by updating the image without using the new deployment yaml file. It will however work fine for someone upgrading from 1.3.x.

How about adding a release note noting this? It doesnt have to be release-note-action-required (since it will work fine for someone upgrading from 1.3, without them having to do anything).

@madhusudancs
Copy link
Contributor Author

This still breaks someone who is running federation code from HEAD and upgrades by updating the image without using the new deployment yaml file.

Indeed. There is no compatibility promise going from HEAD to HEAD, within the same release.

How about adding a release note noting this? It doesnt have to be release-note-action-required (since it will work fine for someone upgrading from 1.3, without them having to do anything).

I added a release note superseding the release note that was in PR #28938 before.

Also, the release note is not about HEAD to HEAD changes and there shouldn't be notes about those changes in the release note. Think about it as a note for end users who upgrade from one release to another. If you are still not convinced we could ask @erictune who was the previous release czar or @pwittrock who is the current release czar.

@pwittrock
Copy link
Member

@mml @madhusudancs @nikhiljindal Saw I was mentioned on this. I am missing some context. Is this a bug fix for an issue in federation?

@madhusudancs
Copy link
Contributor Author

madhusudancs commented Aug 25, 2016

@pwittrock sorry, I brought you in here without giving you any context. This is not exactly a bug fix, but you could call it evolution (for a lack of better term) to make federation-controller-manager deployments more flexible in the future.

Here is some context. This PR makes some changes which is arguably backwards incompatible for users going from v1.3.x to v1.4.x. This change is not in an API or anything, just the way the binary flags are used/config is read. But federation is still in beta, so we have an upgrade path and a release note for that. Also, to facilitate smooth upgrades and rollback, we are keeping the previous mechanism as a fallback. So far, so good.

However, this is going to break for folks who are already on 1.4.x HEAD because we had made a related change in the earlier part of the 1.4 cycle and this change is not compatible with that. So the contention now is around writing a release note for this HEAD-to-HEAD breaking change. This is where we need you to resolve the conflict.

P.S. I am of the opinion that there shouldn't be a release note for the HEAD-to-HEAD changes within the same release because that's going to confuse users.

@pwittrock
Copy link
Member

@madhusudancs What is the relationship between this and feature freeze (8/22). It seems like it should be subject to the feature freeze if I am reading this right.

@madhusudancs madhusudancs deleted the fed-cm-kubeconfig-from-flags branch August 29, 2016 22:18
@madhusudancs madhusudancs restored the fed-cm-kubeconfig-from-flags branch August 29, 2016 22:20
@madhusudancs madhusudancs reopened this Aug 29, 2016
@madhusudancs madhusudancs modified the milestones: pre-1.5-general, v1.4 Aug 29, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 29, 2016

GCE e2e build/test passed for commit d7ddd3e.

@mml mml assigned nikhiljindal and unassigned mml Sep 8, 2016
@madhusudancs
Copy link
Contributor Author

@nikhiljindal just wanted to check. Is there anything you me want to do here (after our offline conversations)?

@nikhiljindal
Copy link
Contributor

I re-read the comments above and IIUC, this is not just a HEAD to HEAD breakage anymore, so we will need to support the upgrade path?
Try reading secret KubeconfigSecretName as well?

@madhusudancs madhusudancs added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Oct 8, 2016
@madhusudancs madhusudancs removed the release-note-none Denotes a PR that doesn't merit a release note. label Oct 8, 2016
@madhusudancs
Copy link
Contributor Author

@nikhiljindal I have updated the release note with release-note-action-required label. Can we get this in now?

// Create the config to talk to federation-apiserver.
kubeconfigGetter := util.KubeconfigGetterForSecret(KubeconfigSecretName)
restClientCfg, err := clientcmd.BuildConfigFromKubeconfigGetter(s.Master, kubeconfigGetter)
restClientCfg, err = clientcmd.BuildConfigFromFlags(s.Master, s.Kubeconfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, this will overwrite the config.
Can remove what we are doing above.

// TODO(madhusudancs): Remove this in 1.5. This is only temporary to give an
// upgrade path in 1.4.
func restClientConfigFromSecret(master string) (*restclient.Config, error) {
kubeconfigGetter := util.KubeconfigGetterForSecret(DeprecatedKubeconfigSecretName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try KubeconfigSecretName as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updated to that name.

@madhusudancs madhusudancs force-pushed the fed-cm-kubeconfig-from-flags branch from d7ddd3e to 14f118c Compare November 2, 2016 22:27
@madhusudancs
Copy link
Contributor Author

@nikhiljindal I have updated the PR. PTAL.

I made a change slightly different than the one we discussed offline to avoid overwriting of restClientCfg. Let me know if this makes sense.

@nikhiljindal
Copy link
Contributor

@k8s-bot federation gce e2e test this

@@ -187,3 +193,14 @@ func StartControllers(s *options.CMServer, restClientCfg *restclient.Config) err

select {}
}

// TODO(madhusudancs): Remove this in 1.5. This is only temporary to give an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comment to say 1.6 and 1.5

kubeconfigGetter := util.KubeconfigGetterForSecret(DeprecatedKubeconfigSecretName)
restClientCfg, err := clientcmd.BuildConfigFromKubeconfigGetter(master, kubeconfigGetter)
if err != nil {
return nil, fmt.Errorf("failed to find the Federation API server kubeconfig, tried the flags and the deprecated secret %s: %v", DeprecatedKubeconfigSecretName, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: mention which flags? So that user knows which flag to look at amongst so many other flags :)

@nikhiljindal
Copy link
Contributor

lgtm with 2 minor comments

@k8s-ci-robot
Copy link
Contributor

Jenkins Federation GCE e2e failed for commit 14f118c. Full PR test history.

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

…ath.

This decoupling from the Kubernetes API allows admins to run federation
control plane components wherever they like, even outside Kubernetes.
…nfig from the secret if --kubeconfig flag is empty.
@madhusudancs madhusudancs force-pushed the fed-cm-kubeconfig-from-flags branch from 14f118c to efafff4 Compare November 4, 2016 18:31
@madhusudancs madhusudancs modified the milestones: v1.5, pre-1.5-general Nov 4, 2016
@madhusudancs
Copy link
Contributor Author

@nikhiljindal addressed the comments. Adding the LGTM label.

@madhusudancs madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016
@k8s-ci-robot
Copy link
Contributor

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

The magic incantation to run this job again is @k8s-bot gci gke e2e 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
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit efafff4. 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.

@madhusudancs
Copy link
Contributor Author

@k8s-bot cvm gke e2e test this

@madhusudancs
Copy link
Contributor Author

@k8s-bot gci gke e2e test this

@madhusudancs
Copy link
Contributor Author

@k8s-bot gce etcd3 e2e test this

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit efafff4. 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.

@madhusudancs
Copy link
Contributor Author

@k8s-bot node e2e test this

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit efafff4. Full PR test history.

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

@madhusudancs
Copy link
Contributor Author

@k8s-bot gce etcd3 e2e test this

@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants