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

kube: load KubeConfig (token) from FS on every reconcile #480

Merged
merged 3 commits into from
May 12, 2022
Merged

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented May 10, 2022

4371610 is a partial cherry-pick of commit ae4f499, including
changes around kube. This to include some of the changes around the
construction of the ConfigFlags RESTClientGetter, as an attempt to
solve token refresh issues.

As icing on the cake, 93bd5ed always retrieves the current token from the FS which fixes #479 and fluxcd/flux2#2074.

@hiddeco hiddeco added bug Something isn't working enhancement New feature or request area/helm Helm related issues and pull requests labels May 10, 2022
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

LGTM

🔝 🔝!

@hiddeco hiddeco added the hold Issues and pull requests put on hold label May 10, 2022
@hiddeco hiddeco marked this pull request as draft May 10, 2022 13:54
@hiddeco hiddeco removed the hold Issues and pull requests put on hold label May 10, 2022
This is a partial cherry-pick of commit ae4f499, including
changes around `kube`. This to include some of the changes around the
construction of the ConfigFlags RESTClientGetter, as an attempt to
solve token refresh issues.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Signed-off-by: Hidde Beydals <hello@hidde.co>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

I think we need to change the title and the description of this PR to be about token refresh. The code looks Ok to me.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco changed the title kube: change construction of RESTClientGetters kube: retrieve token in-cluster KubeConfig from FS May 12, 2022
@hiddeco
Copy link
Member Author

hiddeco commented May 12, 2022

@stefanprodan I changed the PR title to make mention of it.

@stefanprodan stefanprodan changed the title kube: retrieve token in-cluster KubeConfig from FS kube: load the SA token at every reconciliation May 12, 2022
@hiddeco hiddeco changed the title kube: load the SA token at every reconciliation kube: load KubeConfig (token) from FS on every reconcile May 12, 2022
@hiddeco hiddeco merged commit 6254549 into main May 12, 2022
@hiddeco hiddeco deleted the imprv-kube branch May 12, 2022 13:23
@cgchinmay
Copy link

cgchinmay commented May 13, 2022

Hi, could you elaborate on how "always retrieves the current token from the FS" fixes the token refresh issue ?

@hiddeco
Copy link
Member Author

hiddeco commented May 13, 2022

My understanding is that by initializing it from zero every time, it reads the latest runtime values and thereby is able to obtain a correct token.

Note that this part of the code is very Helm specific, and the further controller (and controller-runtime manager) properly update their token automagically.

@michael12312
Copy link

hi @hiddeco

Per the doc from K8s mentioned:
The following Kubernetes client SDKs refresh tokens automatically within the required time frame:
Go v0.15.7 and later

I see helm-controller already use the version of k8s.io/client-go v0.23.6.

Per my understanding the expired token issue should not exists, can you explain why you put this fixing? Why load KubeConfig (token) from FS on every reconcile?

@hiddeco
Copy link
Member Author

hiddeco commented May 16, 2022

As said before in #480 (comment).

This patch is purely required because of how we integrate with Helm, and the controller-runtime client itself has been updating the token as expected. This process does however not update the token in the loaded REST config, from which we used to derive our client for Helm. It should not be taken as a further example on how to solve this for your project, as the context around why and how we require this does likely not apply to you.

@cgchinmay
Copy link

As said before in #480 (comment).

This patch is purely required because of how we integrate with Helm, and the controller-runtime client itself has been updating the token as expected. This process does however not update the token in the loaded REST config, from which we used to derive our client for Helm. It should not be taken as a further example on how to solve this for your project, as the context around why and how we require this does likely not apply to you.

Thanks for the explanation. I had the same thought as @michael12312 to ask the question. Also, I noticed the same issue with Multus even though it uses Go client > v0.15.7. So was trying to understand if there's a common pattern here.

@hiddeco
Copy link
Member Author

hiddeco commented May 16, 2022

Think you need to look in the area of https://github.com/k8snetworkplumbingwg/multus-cni/blob/706de7c2c6a7d86401e1b0650bc050e4bdee984b/pkg/k8sclient/k8sclient.go#L384-L412 in that case. Although on a first 10 second glance, https://github.com/k8snetworkplumbingwg/multus-cni/blob/706de7c2c6a7d86401e1b0650bc050e4bdee984b/pkg/k8sclient/k8sclient.go#L405 should be doing the right thing and pretty much equals to what we are doing here.

@michael12312
Copy link

hi @hiddeco

I saw you commit this change 4 days ago, want to ask: when do you plan to create 1 release for this change? Any tag we can refer to?

@hiddeco
Copy link
Member Author

hiddeco commented Jun 1, 2022

@michael12312 KubeCon and finalizing of source-controller changes got in the way for a bit. #491 is now pending.

hiddeco added a commit that referenced this pull request Jun 7, 2022
Fixing a regression introduced in #480 which would always pick the
namespace of the release. In addition, historically seen the
configuration of the impersonation username while making use of a
KubeConfig has never worked correctly, this has been adressed as well.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests bug Something isn't working enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Helm-controller pod is using stale tokens
5 participants