fall back to kubeconfig loader behavior (exec plugin capable)#986
fall back to kubeconfig loader behavior (exec plugin capable)#986reaper8055 wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Helm chart application logic to support exec-auth kubeconfigs by conditionally bypassing the override of the local kubeconfig. A critical issue was identified where essential configuration parameters, such as the API server address and insecure skip TLS verify flag, are omitted when using this new authentication path, potentially causing Helm to target the wrong cluster.
| if !useKubeconfigAuth { | ||
| // Set KubeConfig to DevNull to prevent read from local kubeconfig | ||
| // to prevent conflicts between "data" and "files" properties (CAFile, CAData and KeyFile, KeyData) | ||
| // ConfigFlags only allows setting CAFile, KeyFile but not CAData, KeyData. | ||
| // When the library reads the original kubeconfig containing cert data / key data AND we specify cert file / key file, these configurations conflict | ||
| devNull := os.DevNull | ||
| kubeConfig.KubeConfig = &devNull | ||
| kubeConfig.APIServer = &c.RestConfig.Host | ||
| kubeConfig.BearerToken = &c.RestConfig.BearerToken | ||
| kubeConfig.Insecure = &c.RestConfig.Insecure | ||
|
|
||
| // Set username and password for basic auth if available | ||
| if c.RestConfig.Username != "" { | ||
| kubeConfig.Username = &c.RestConfig.Username | ||
| } | ||
| if c.RestConfig.Password != "" { | ||
| kubeConfig.Password = &c.RestConfig.Password | ||
| } | ||
|
|
There was a problem hiding this comment.
When useKubeconfigAuth is true, the APIServer and Insecure settings (as well as basic auth credentials) are not applied to the kubeConfig flags. This causes Helm to fall back to the default values found in the local kubeconfig file, which may point to a different cluster than the one intended by the Client's RestConfig. These settings should be applied unconditionally to ensure consistency with the client's configuration even when using the kubeconfig loader for authentication.
| if !useKubeconfigAuth { | |
| // Set KubeConfig to DevNull to prevent read from local kubeconfig | |
| // to prevent conflicts between "data" and "files" properties (CAFile, CAData and KeyFile, KeyData) | |
| // ConfigFlags only allows setting CAFile, KeyFile but not CAData, KeyData. | |
| // When the library reads the original kubeconfig containing cert data / key data AND we specify cert file / key file, these configurations conflict | |
| devNull := os.DevNull | |
| kubeConfig.KubeConfig = &devNull | |
| kubeConfig.APIServer = &c.RestConfig.Host | |
| kubeConfig.BearerToken = &c.RestConfig.BearerToken | |
| kubeConfig.Insecure = &c.RestConfig.Insecure | |
| // Set username and password for basic auth if available | |
| if c.RestConfig.Username != "" { | |
| kubeConfig.Username = &c.RestConfig.Username | |
| } | |
| if c.RestConfig.Password != "" { | |
| kubeConfig.Password = &c.RestConfig.Password | |
| } | |
| kubeConfig.APIServer = &c.RestConfig.Host | |
| kubeConfig.Insecure = &c.RestConfig.Insecure | |
| // Set username and password for basic auth if available | |
| if c.RestConfig.Username != "" { | |
| kubeConfig.Username = &c.RestConfig.Username | |
| } | |
| if c.RestConfig.Password != "" { | |
| kubeConfig.Password = &c.RestConfig.Password | |
| } | |
| if !useKubeconfigAuth { | |
| // Set KubeConfig to DevNull to prevent read from local kubeconfig | |
| // to prevent conflicts between "data" and "files" properties (CAFile, CAData and KeyFile, KeyData) | |
| // ConfigFlags only allows setting CAFile, KeyFile but not CAData, KeyData. | |
| // When the library reads the original kubeconfig containing cert data / key data AND we specify cert file / key file, these configurations conflict | |
| devNull := os.DevNull | |
| kubeConfig.KubeConfig = &devNull | |
| kubeConfig.BearerToken = &c.RestConfig.BearerToken | |
|
When We'll probably want to serializing the in-memory kubeconfig (including the ExecProvider) to a temp file, appending it to tempFiles, and pointing kubeConfig.KubeConfig at that file so cluster/context stay bound to what the user provided. As we work through this, let's be aware of the flows initiated via both Meshery CLI and Server. |
|
Test coverage 👀 |
|
It'd be good to get our follow up issue for GKE, OKE, AKS, opened and referenced now. |
Yes this can happen if the user runs Maybe, we can have a prompt saying:
I'm assuming, we would want to somehow persist this file across start/stop commands? And since f, err := os.CreateTemp("/tmp", "meshery-eks-kubeconfig")OR Initiate the EKS flow using aws sdk which generates the in memory config and use client-go to write this config to a temp file instead of relying on user to run
ACK! The saving of current config depends on the remote provider, which happens in two paths:
|
Hello @leecalcote do you want me to increase the test coverage for this package? The current test coverage looks low: ok github.com/meshery/meshkit/utils/kubernetes 1.188s coverage: 7.9% of statements
github.com/meshery/meshkit/utils/kubernetes/describe coverage: 0.0% of statements
github.com/meshery/meshkit/utils/kubernetes/expose coverage: 0.0% of statements
ok github.com/meshery/meshkit/utils/kubernetes/kompose 1.564s coverage: 75.4% of statementsThis can be done but will require a fair bit of refactoring of logic especially to be able to mock behavior effectively. |
9ccde91 to
bd87acf
Compare
40c37cd to
6ba0cc4
Compare
|
@reaper8055 just wanted to confirm this is ready for review ? |
Yes @lekaf974 this is ready for review |
lekaf974
left a comment
There was a problem hiding this comment.
small comments LGTM otherwise
d07b578 to
0e6a539
Compare
|
We're a couple of months into this one. I have my 🤞 for it. |
Resoling the comments and updating the PR as I write this. Will get this done! |
Signed-off-by: reaper8055 <11490705+reaper8055@users.noreply.github.com> Signed-off-by: reaper8055 <reaper8055@gmail.com>
Signed-off-by: reaper8055 <reaper8055@gmail.com> Signed-off-by: reaper8055 <11490705+reaper8055@users.noreply.github.com> Signed-off-by: reaper8055 <reaper8055@gmail.com>
Signed-off-by: reaper8055 <11490705+reaper8055@users.noreply.github.com> Signed-off-by: reaper8055 <reaper8055@gmail.com>
Signed-off-by: reaper8055 <11490705+reaper8055@users.noreply.github.com> Signed-off-by: reaper8055 <reaper8055@gmail.com>
Signed-off-by: reaper8055 <11490705+reaper8055@users.noreply.github.com> Signed-off-by: reaper8055 <reaper8055@gmail.com>
Signed-off-by: reaper8055 <11490705+reaper8055@users.noreply.github.com> Signed-off-by: reaper8055 <reaper8055@gmail.com>
Signed-off-by: reaper8055 <11490705+reaper8055@users.noreply.github.com> Signed-off-by: reaper8055 <reaper8055@gmail.com>
Signed-off-by: reaper8055 <reaper8055@gmail.com>
|
@reaper8055 regarding error handling are we bubbling up errors like missing the exec binary , failure to exec etc to users |
Description
This PR fixes #985
Notes for Reviewers
Added
useKubeconfigAuth := c.RestConfig.ExecProvider != nil && c.RestConfig.BearerToken == ""tocreateHelmActionConfigc.RestConfig.ExecProvider != nilmeans auth came from an exec plugin (EKS style aws eks get-token, etc).c.RestConfig.BearerToken == ""means there is no static token available in the rest.Config right now.(1.) && (2.) means:
Cluster needs exec-plugin token retrieval and static bearer-token wiring is insufficient.
Therefore, use kubeconfig-based auth resolution path (so client-go can execute plugin), instead of forcing /dev/null and expecting bearer token to already exist.
Signed commits