fall back to kubeconfig loader behavior (exec plugin capable) - #986
fall back to kubeconfig loader behavior (exec plugin capable)#986reaper8055 wants to merge 10 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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 | |
There was a problem hiding this comment.
Please reply to each review comment. Either show how you incorporated the feedback or explain why you're rejecting it.
|
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
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! |
d60452c to
2bf19e2
Compare
|
@reaper8055 regarding error handling are we bubbling up errors like missing the exec binary , failure to exec etc to users |
the kubeConfig |
a085afc to
1ff70d2
Compare
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>
…dential plugins Signed-off-by: reaper8055 <reaper8055@gmail.com>
Signed-off-by: reaper8055 <reaper8055@gmail.com>
📝 WalkthroughWalkthroughKubernetes configuration discovery now preserves client configuration loaders and supports REST client getters. ChangesKubernetes authentication flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant detectKubeConfig
participant RESTClientGetter
participant Helm
participant ExecPlugin
participant KubernetesAPI
Client->>detectKubeConfig: discover REST config and loader
detectKubeConfig-->>Client: return configuration
Client->>RESTClientGetter: create getter and resolve REST config
Client->>Helm: create action configuration with getter
Helm->>KubernetesAPI: request discovery
KubernetesAPI-->>Helm: reject expired credential
Helm->>ExecPlugin: execute credential provider
ExecPlugin-->>Helm: return renewed token
Helm->>KubernetesAPI: retry discovery
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
utils/kubernetes/apply-helm-chart.go (1)
415-417: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe MeshKit error is wrapped twice.
createHelmActionConfigreturnsErrApplyHelmChart(err). The caller at line 269 wraps the same error again withErrApplyHelmChart(err). The result nests one MeshKit error inside another and duplicates the error code. Return the raw error here and letApplyHelmChartwrap it once. This matches the pattern used forsetupChartVersionandgetHelmLocalPathat lines 247-254.♻️ Proposed fix
actionConfig := new(action.Configuration) if err := actionConfig.Init(restClientGetter, cfg.Namespace, string(cfg.HelmDriver), cfg.Logger); err != nil { - return nil, ErrApplyHelmChart(err) + return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/kubernetes/apply-helm-chart.go` around lines 415 - 417, Update createHelmActionConfig to return the raw actionConfig.Init error instead of wrapping it with ErrApplyHelmChart. Let the ApplyHelmChart caller perform the single ErrApplyHelmChart wrapping, matching setupChartVersion and getHelmLocalPath.utils/kubernetes/kubernetes.go (2)
64-71: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe fallback builds a new getter on every call.
For a
Clientconstructed withoutNew, eachgetRESTClientGetter()call allocates a new getter and a new REST config copy. Each getter owns its own memory discovery cache, so callers do not share discovery results. Consider caching the fallback getter on theClient.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/kubernetes/kubernetes.go` around lines 64 - 71, Update Client.getRESTClientGetter to cache the fallback newRESTConfigRESTClientGetter result on c.restClientGetter before returning it, while preserving the existing getter when already initialized. Ensure direct Client constructions reuse the same getter and discovery cache across calls.
59-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
configureRESTConfigoverwrites caller-configured rate limits.
ToRESTConfigcalls this helper on every invocation. A caller that constructsClientdirectly and setsRestConfig.QPSorRestConfig.Burstloses those values. The test atutils/kubernetes/client-config-getter_test.golines 178-192 encodes this:QPS: 1, Burst: 2becomes50, 100.If the override is intentional, keep it. If not, apply the defaults only when the fields are zero.
♻️ Proposed change to apply defaults only when unset
func configureRESTConfig(config *rest.Config) { - config.QPS = float32(50) - config.Burst = int(100) + if config.QPS == 0 { + config.QPS = float32(50) + } + if config.Burst == 0 { + config.Burst = 100 + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/kubernetes/kubernetes.go` around lines 59 - 62, Update configureRESTConfig so it assigns the default QPS and Burst values only when the corresponding rest.Config fields are zero, preserving caller-provided rate limits when ToRESTConfig invokes the helper.utils/kubernetes/client-config-getter_test.go (1)
58-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnchor the
-test.runpattern.
-test.runtakes a regular expression.TestExecCredentialHelperProcessis unanchored, so any future test whose name contains this substring also runs in the helper process. Extra test output on stdout then corrupts theExecCredentialJSON that client-go parses. Anchor the pattern.♻️ Proposed fix
- Args: []string{"-test.run=TestExecCredentialHelperProcess"}, + Args: []string{"-test.run=^TestExecCredentialHelperProcess$"},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/kubernetes/client-config-getter_test.go` around lines 58 - 59, Anchor the -test.run regular expression in the helper command arguments so it matches only TestExecCredentialHelperProcess, preventing similarly named tests from running in the subprocess and emitting extra stdout. Update the Args value in the relevant test setup while preserving the existing helper-process behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utils/kubernetes/apply-helm-chart.go`:
- Line 566: Update the comment for GetEntryWithChartVersion so the second
parameter is described as “the chart version” instead of “the appversion,”
keeping the rest of the documentation unchanged.
- Around line 411-412: Update the environment setup in ApplyHelmChart to avoid
mutating process-global state: only configure HELM_DRIVER_SQL_CONNECTION_STRING
when cfg.HelmDriver is the SQL driver and cfg.SQLConnectionString is non-empty,
and handle the error returned by os.Setenv. Ensure the value is scoped to Helm
and is not inherited by exec credential plugins or concurrent ApplyHelmChart
calls.
In `@utils/kubernetes/client-config-getter.go`:
- Around line 72-89: Update restConfigClientConfig.RawConfig to add an AuthInfo
entry for connectionName that mirrors the REST config authentication fields,
including bearer token/file, client certificate/key data, username/password,
exec provider, and proxy settings. Link the existing context’s AuthInfo field to
connectionName while preserving the current cluster and context configuration.
In `@utils/kubernetes/client.go`:
- Around line 39-48: Update the KUBECONFIG handling in the client configuration
flow to avoid shadowing the named err: use assignment with the existing err
variable when calling ProcessConfig and loadClientConfigFromKubeconfig. On
either failure, do not return immediately; preserve the error and continue to
the default ~/.kube/config fallback, while returning successfully loaded
KUBECONFIG settings unchanged.
---
Nitpick comments:
In `@utils/kubernetes/apply-helm-chart.go`:
- Around line 415-417: Update createHelmActionConfig to return the raw
actionConfig.Init error instead of wrapping it with ErrApplyHelmChart. Let the
ApplyHelmChart caller perform the single ErrApplyHelmChart wrapping, matching
setupChartVersion and getHelmLocalPath.
In `@utils/kubernetes/client-config-getter_test.go`:
- Around line 58-59: Anchor the -test.run regular expression in the helper
command arguments so it matches only TestExecCredentialHelperProcess, preventing
similarly named tests from running in the subprocess and emitting extra stdout.
Update the Args value in the relevant test setup while preserving the existing
helper-process behavior.
In `@utils/kubernetes/kubernetes.go`:
- Around line 64-71: Update Client.getRESTClientGetter to cache the fallback
newRESTConfigRESTClientGetter result on c.restClientGetter before returning it,
while preserving the existing getter when already initialized. Ensure direct
Client constructions reuse the same getter and discovery cache across calls.
- Around line 59-62: Update configureRESTConfig so it assigns the default QPS
and Burst values only when the corresponding rest.Config fields are zero,
preserving caller-provided rate limits when ToRESTConfig invokes the helper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0532983-d8be-4f1c-afcf-1783bab9afa2
📒 Files selected for processing (5)
utils/kubernetes/apply-helm-chart.goutils/kubernetes/client-config-getter.goutils/kubernetes/client-config-getter_test.goutils/kubernetes/client.goutils/kubernetes/kubernetes.go
| // Set the environment variable needed by the Init methods | ||
| _ = os.Setenv("HELM_DRIVER_SQL_CONNECTION_STRING", cfg.SQLConnectionString) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
os.Setenv mutates process-global state and now leaks into exec credential plugins.
Three problems in this line:
- The call is not concurrency safe. Two concurrent
ApplyHelmChartcalls with differentSQLConnectionStringvalues race on the same process environment. - If
cfg.SQLConnectionStringis empty, the call clears any value that was set earlier in the process. - This PR makes Helm spawn exec credential plugins, for example
aws eks get-token. Child processes inherit the process environment, so the SQL connection string, including any embedded password, is now exposed to those plugins.
Set the variable only when cfg.HelmDriver is the SQL driver and cfg.SQLConnectionString is not empty. Also check the returned error.
🔐 Proposed fix
- // Set the environment variable needed by the Init methods
- _ = os.Setenv("HELM_DRIVER_SQL_CONNECTION_STRING", cfg.SQLConnectionString)
+ // Set the environment variable needed by the Init methods.
+ // Only set it when a connection string is supplied, so an empty value does not
+ // clear a previously configured one and does not reach spawned credential plugins.
+ if cfg.SQLConnectionString != "" {
+ if err := os.Setenv("HELM_DRIVER_SQL_CONNECTION_STRING", cfg.SQLConnectionString); err != nil {
+ return nil, err
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/kubernetes/apply-helm-chart.go` around lines 411 - 412, Update the
environment setup in ApplyHelmChart to avoid mutating process-global state: only
configure HELM_DRIVER_SQL_CONNECTION_STRING when cfg.HelmDriver is the SQL
driver and cfg.SQLConnectionString is non-empty, and handle the error returned
by os.Setenv. Ensure the value is scoped to Helm and is not inherited by exec
credential plugins or concurrent ApplyHelmChart calls.
| } | ||
|
|
||
| // GetEntryWithAppVersion takes in the entry name and the appversion and returns the corresponding | ||
| // GetEntryWithChartVersion takes in the entry name and the appversion and returns the corresponding |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The comment still says "appversion".
The function is GetEntryWithChartVersion. The comment describes the second parameter as "the appversion". Update the prose to "the chart version" so it matches the function name.
📝 Proposed fix
-// GetEntryWithChartVersion takes in the entry name and the appversion and returns the corresponding
+// GetEntryWithChartVersion takes in the entry name and the chart version and returns the corresponding📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GetEntryWithChartVersion takes in the entry name and the appversion and returns the corresponding | |
| // GetEntryWithChartVersion takes in the entry name and the chart version and returns the corresponding |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/kubernetes/apply-helm-chart.go` at line 566, Update the comment for
GetEntryWithChartVersion so the second parameter is described as “the chart
version” instead of “the appversion,” keeping the rest of the documentation
unchanged.
| func (c *restConfigClientConfig) RawConfig() (clientcmdapi.Config, error) { | ||
| const connectionName = "meshkit-connection" | ||
|
|
||
| config := clientcmdapi.NewConfig() | ||
| config.Clusters[connectionName] = &clientcmdapi.Cluster{ | ||
| Server: c.restConfig.Host, | ||
| TLSServerName: c.restConfig.ServerName, | ||
| InsecureSkipTLSVerify: c.restConfig.Insecure, | ||
| CertificateAuthority: c.restConfig.CAFile, | ||
| CertificateAuthorityData: c.restConfig.CAData, | ||
| DisableCompression: c.restConfig.DisableCompression, | ||
| } | ||
| config.Contexts[connectionName] = &clientcmdapi.Context{ | ||
| Cluster: connectionName, | ||
| } | ||
| config.CurrentContext = connectionName | ||
| return *config, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
RawConfig drops the authentication data of the REST config.
The synthetic config contains a cluster and a context, but no AuthInfo. It omits BearerToken, BearerTokenFile, client certificate and key data, Username/Password, ExecProvider, and Proxy. Consumers that build clients from RawConfig instead of ClientConfig receive an unauthenticated configuration. Helm reads the namespace and context from this path today, so the impact is limited, but the omission is silent.
Add an AuthInfo that mirrors the REST config credentials, and link the context to it.
🔐 Proposed fix to preserve credentials in RawConfig
config.Contexts[connectionName] = &clientcmdapi.Context{
Cluster: connectionName,
+ AuthInfo: connectionName,
}
+ config.AuthInfos[connectionName] = &clientcmdapi.AuthInfo{
+ Token: c.restConfig.BearerToken,
+ TokenFile: c.restConfig.BearerTokenFile,
+ ClientCertificate: c.restConfig.CertFile,
+ ClientCertificateData: c.restConfig.CertData,
+ ClientKey: c.restConfig.KeyFile,
+ ClientKeyData: c.restConfig.KeyData,
+ Username: c.restConfig.Username,
+ Password: c.restConfig.Password,
+ Exec: c.restConfig.ExecProvider,
+ }
config.CurrentContext = connectionName🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/kubernetes/client-config-getter.go` around lines 72 - 89, Update
restConfigClientConfig.RawConfig to add an AuthInfo entry for connectionName
that mirrors the REST config authentication fields, including bearer token/file,
client certificate/key data, username/password, exec provider, and proxy
settings. Link the existing context’s AuthInfo field to connectionName while
preserving the current cluster and context configuration.
| kubeconfig := os.Getenv("KUBECONFIG") | ||
| if kubeconfig != "" { | ||
| _, cfgFile, err := ProcessConfig(kubeconfig, "") | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| if config, err = clientcmd.RESTConfigFromKubeConfig(cfgFile); err == nil { | ||
| return config, nil | ||
| if config, loader, err = loadClientConfigFromKubeconfig(cfgFile); err == nil { | ||
| return config, loader, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The $KUBECONFIG branch shadows err and aborts the fallback chain.
Two points in this block:
- Line 41 declares a new
errwith:=. Line 45 then assigns that innererr, not the named return. IfloadClientConfigFromKubeconfigfails here, the failure never reaches the caller, and the final error at line 60 reports the default-path failure instead. - If
ProcessConfigfails for a malformed$KUBECONFIG, line 43 returns immediately. The default~/.kube/configpath is then never tried, even though it may be valid.
Assign to the named err and continue to the default path on failure.
♻️ Proposed fix for the $KUBECONFIG branch
kubeconfig := os.Getenv("KUBECONFIG")
if kubeconfig != "" {
- _, cfgFile, err := ProcessConfig(kubeconfig, "")
- if err != nil {
- return nil, nil, err
- }
- if config, loader, err = loadClientConfigFromKubeconfig(cfgFile); err == nil {
- return config, loader, nil
+ var cfgFile []byte
+ if _, cfgFile, err = ProcessConfig(kubeconfig, ""); err == nil {
+ if config, loader, err = loadClientConfigFromKubeconfig(cfgFile); err == nil {
+ return config, loader, nil
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubeconfig := os.Getenv("KUBECONFIG") | |
| if kubeconfig != "" { | |
| _, cfgFile, err := ProcessConfig(kubeconfig, "") | |
| if err != nil { | |
| return nil, err | |
| return nil, nil, err | |
| } | |
| if config, err = clientcmd.RESTConfigFromKubeConfig(cfgFile); err == nil { | |
| return config, nil | |
| if config, loader, err = loadClientConfigFromKubeconfig(cfgFile); err == nil { | |
| return config, loader, nil | |
| } | |
| } | |
| kubeconfig := os.Getenv("KUBECONFIG") | |
| if kubeconfig != "" { | |
| var cfgFile []byte | |
| if _, cfgFile, err = ProcessConfig(kubeconfig, ""); err == nil { | |
| if config, loader, err = loadClientConfigFromKubeconfig(cfgFile); err == nil { | |
| return config, loader, nil | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/kubernetes/client.go` around lines 39 - 48, Update the KUBECONFIG
handling in the client configuration flow to avoid shadowing the named err: use
assignment with the existing err variable when calling ProcessConfig and
loadClientConfigFromKubeconfig. On either failure, do not return immediately;
preserve the error and continue to the default ~/.kube/config fallback, while
returning successfully loaded KUBECONFIG settings unchanged.
6 participants
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
Summary by CodeRabbit
New Features
Bug Fixes