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

OpenId token not refreshed if the config is in-memory #5880

Closed
ttbadr opened this issue Apr 11, 2024 · 10 comments · Fixed by #5888
Closed

OpenId token not refreshed if the config is in-memory #5880

ttbadr opened this issue Apr 11, 2024 · 10 comments · Fixed by #5888
Assignees
Milestone

Comments

@ttbadr
Copy link
Contributor

ttbadr commented Apr 11, 2024

As title, If we use a in-memory config, the token not refreshed.
I found the method below, it only save the new token when the config is file

@rohanKanojia
Copy link
Member

@ttbadr : Could you please provide more details? How are OpenID parameters being provided to KubernetesClient (like refresh_token, OpenID identity provider URL, etc)?

@ttbadr
Copy link
Contributor Author

ttbadr commented Apr 11, 2024

@rohanKanojia I think the root cause is here
this method persistKubeConfigWithUpdatedAuthInfo will be called when the refresh token request success, and persist the new token to the config.
but you can see that if the config is not a file then return, if the config is a file then save the new token to the kubeconfig.
so if the config is in-memory, the new token will be ignore, the next client request will use the old token and fail

@rohanKanojia
Copy link
Member

@ttbadr : May I know which cluster you're using? It might be difficult for us to reproduce this. Is it possible for you to create a pull request to fix this?

@ttbadr
Copy link
Contributor Author

ttbadr commented Apr 13, 2024

@rohanKanojia I use the k8s, I create a pull request to fix it #5888, can you help to review it. thx

@rohanKanojia
Copy link
Member

rohanKanojia commented Apr 13, 2024

@ttbadr : Thanks a lot! Could you please add a test case to validate your fix? Also, could you please provide more details about your setup? I'm wondering from which source KubernetesClient is fetchin refresh token for performing refresh, it will help us out in doing review.

@ttbadr
Copy link
Contributor Author

ttbadr commented Apr 13, 2024

@rohanKanojia ok, I can add some test cases. Sorry I can't provide the source, It's a internal k8s cluster

@rohanKanojia
Copy link
Member

@ttbadr : I'm not asking for your cluster details. I'm requesting you to elaborate more on your problem. How is Config loaded by KubernetesClient? Is it via some local .kubeconfig file or via ConfigBuilder?

@manusa manusa added this to the 6.13.0 milestone Apr 13, 2024 — with automated-tasks
@ttbadr
Copy link
Contributor Author

ttbadr commented Apr 14, 2024

@rohanKanojia I build the Kubeclient via ConfigBuilder, the code like below:

Config config = new ConfigBuilder().withAutoConfigure(false)
        .withNameSpace(namespace)
        .withMasterUrl(url)
        .withAutoOAuthToken(token)
        .withCaCertData(base64CaData)
        .withAuthProvider(new AuthProviderConfigBuilder()
                .withName(providerName)
                .withConfig(configMap)
                .build()).build();
new KubernetesClientBuilder().withConfig(config).build();

btw, I can't find the ConfigBuilder class in the source, is it genarated by some maven task?

@rohanKanojia
Copy link
Member

@ttbadr: Yes, it's generated using sundrio annotation processor

@manusa manusa modified the milestones: 6.13.0, 6.12.1 Apr 16, 2024
@manusa
Copy link
Member

manusa commented Apr 18, 2024

After working on the refactor of the OpenIDConnectionUtils, I'm noticing that these changes might actually create a regression for #4802 which was fixed by #4951

Please, @Vlatombe, could you review #5888 and verify it won't cause a regression for your use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants