-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Read the federation controller manager kubeconfig from a filesystem path #30601
Conversation
f14b706
to
d121530
Compare
@@ -17,16 +17,23 @@ spec: | |||
- name: ssl-certs | |||
hostPath: | |||
path: /etc/ssl/certs | |||
- name: kubeconfig | |||
secret: | |||
secretName: federation-apiserver-kubeconfig |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
The code changes look good. |
d121530
to
d7ddd3e
Compare
Thanks for the code changes. 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). |
Indeed. There is no compatibility promise going from HEAD to HEAD, within the same release.
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. |
@mml @madhusudancs @nikhiljindal Saw I was mentioned on this. I am missing some context. Is this a bug fix for an issue in federation? |
@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. |
@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. |
GCE e2e build/test passed for commit d7ddd3e. |
@nikhiljindal just wanted to check. Is there anything you me want to do here (after our offline conversations)? |
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? |
@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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try KubeconfigSecretName as well?
There was a problem hiding this comment.
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.
d7ddd3e
to
14f118c
Compare
@nikhiljindal I have updated the PR. PTAL. I made a change slightly different than the one we discussed offline to avoid overwriting of |
@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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 :)
lgtm with 2 minor comments |
Jenkins Federation GCE e2e failed for commit 14f118c. Full PR test history. The magic incantation to run this job again is |
…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.
…ly when `--kubeconfig` is non-empty.
14f118c
to
efafff4
Compare
@nikhiljindal addressed the comments. Adding the LGTM label. |
Jenkins GCI GKE smoke e2e failed for commit efafff4. Full PR test history. The magic incantation to run this job again is |
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 |
@k8s-bot gci gke e2e test this |
@k8s-bot gce etcd3 e2e test this |
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 |
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 |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
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.
cc @kubernetes/sig-cluster-federation
This change is