Improve autodetect logic for kubeconfig path #4
Conversation
Signed-off-by: Andrei Kvapil <[email protected]>
WalkthroughThe changes refactor and centralize the logic for resolving the Kubernetes configuration file path and determining the default namespace. A new helper function is introduced for kubeconfig path resolution, and both configuration and namespace retrieval functions are updated to use this helper and improved fallback mechanisms for robustness. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Kubeconfig
participant InCluster
User->>App: Start
App->>App: resolvedKubeconfigPath()
alt kubeconfig path found
App->>Kubeconfig: Load config/namespace from kubeconfig
Kubeconfig-->>App: Return config/namespace
else no kubeconfig path
App->>InCluster: Use in-cluster config/namespace
InCluster-->>App: Return config/namespace
end
App-->>User: Proceed with resolved config and namespace
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
main.go (1)
640-659: Silent namespace fallback may mask user errors
defaultNamespace()now swallows any error returned bycc.Namespace()and silently falls back to"default".
If the kubeconfig is syntactically wrong or points to an invalid context the user receives no hint and actions could unexpectedly target thedefaultnamespace (potentially destructive).Consider:
- Logging the error (or including it in the returned error) when the kubeconfig exists but fails to parse.
- Returning
("", false, err)instead of forcing"default"so callers can surface a meaningful message.
| if path := resolvedKubeconfigPath(); path != "" { | ||
| return clientcmd.BuildConfigFromFlags("", path) | ||
| } | ||
| return clientcmd.BuildConfigFromFlags("", cfg) | ||
| return rest.InClusterConfig() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
KUBECONFIG list-support is broken
resolvedKubeconfigPath() forwards the literal content of $KUBECONFIG to clientcmd.BuildConfigFromFlags.
When the variable contains a path-list (colon/semicolon separated) – which is the documented behaviour – ExplicitPath is set to that multi-path string and client-go treats it as one filename, leading to stat /foo:bar: no such file or directory.
- if path := resolvedKubeconfigPath(); path != "" {
- return clientcmd.BuildConfigFromFlags("", path)
+ if path := resolvedKubeconfigPath(); path != "" {
+ // `resolvedKubeconfigPath()` guarantees a *single* existing file.
+ return clientcmd.BuildConfigFromFlags("", path)
}Please ensure the helper splits the list and returns the first existing file (see suggestion below).
Without this fix users with a standard multi-path KUBECONFIG will be unable to run any command outside the cluster.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In main.go around lines 134 to 138, the function resolvedKubeconfigPath()
incorrectly passes the entire KUBECONFIG environment variable as a single path
string to clientcmd.BuildConfigFromFlags, which breaks support for multiple
paths separated by colon or semicolon. To fix this, modify
resolvedKubeconfigPath() to split the KUBECONFIG string by the appropriate path
list separator, iterate over the resulting paths, and return the first path that
exists as a valid file. This ensures clientcmd.BuildConfigFromFlags receives a
single valid kubeconfig file path.
| func resolvedKubeconfigPath() string { | ||
| if kubeCfgPath != "" { | ||
| return kubeCfgPath | ||
| } | ||
| if env := os.Getenv("KUBECONFIG"); env != "" { | ||
| return env | ||
| } | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| cfg := filepath.Join(home, ".kube", "config") | ||
| if _, err := os.Stat(cfg); err == nil { | ||
| return cfg | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle multi-path $KUBECONFIG + tighten file existence check
Nice centralisation! Two small improvements will make it rock-solid:
- Split
$KUBECONFIGwithfilepath.SplitList()and return the first existing file. - Guard the explicit
--kubeconfigpath withos.Statas well, providing an early, clear error.
Proposed implementation:
func resolvedKubeconfigPath() string {
- if kubeCfgPath != "" {
- return kubeCfgPath
+ if kubeCfgPath != "" {
+ if _, err := os.Stat(kubeCfgPath); err == nil {
+ return kubeCfgPath
+ }
}
- if env := os.Getenv("KUBECONFIG"); env != "" {
- return env
+ if env := os.Getenv("KUBECONFIG"); env != "" {
+ for _, p := range filepath.SplitList(env) {
+ if p == "" {
+ continue
+ }
+ if _, err := os.Stat(p); err == nil {
+ return p
+ }
+ }
}
home, err := os.UserHomeDir()
if err != nil {
return ""
}
- cfg := filepath.Join(home, ".kube", "config")
- if _, err := os.Stat(cfg); err == nil {
- return cfg
+ cfg := filepath.Join(home, ".kube", "config")
+ if _, err := os.Stat(cfg); err == nil {
+ return cfg
}
return ""
}This preserves the current precedence while eliminating the multi-path and non-existent file pitfalls.
📝 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.
| func resolvedKubeconfigPath() string { | |
| if kubeCfgPath != "" { | |
| return kubeCfgPath | |
| } | |
| if env := os.Getenv("KUBECONFIG"); env != "" { | |
| return env | |
| } | |
| home, err := os.UserHomeDir() | |
| if err != nil { | |
| return "" | |
| } | |
| cfg := filepath.Join(home, ".kube", "config") | |
| if _, err := os.Stat(cfg); err == nil { | |
| return cfg | |
| } | |
| return "" | |
| } | |
| func resolvedKubeconfigPath() string { | |
| if kubeCfgPath != "" { | |
| if _, err := os.Stat(kubeCfgPath); err == nil { | |
| return kubeCfgPath | |
| } | |
| } | |
| if env := os.Getenv("KUBECONFIG"); env != "" { | |
| for _, p := range filepath.SplitList(env) { | |
| if p == "" { | |
| continue | |
| } | |
| if _, err := os.Stat(p); err == nil { | |
| return p | |
| } | |
| } | |
| } | |
| home, err := os.UserHomeDir() | |
| if err != nil { | |
| return "" | |
| } | |
| cfg := filepath.Join(home, ".kube", "config") | |
| if _, err := os.Stat(cfg); err == nil { | |
| return cfg | |
| } | |
| return "" | |
| } |
🤖 Prompt for AI Agents
In main.go around lines 1278 to 1294, the resolvedKubeconfigPath function should
be improved to handle multi-path KUBECONFIG environment variables and verify
file existence more robustly. Update the function to split the KUBECONFIG
environment variable using filepath.SplitList(), then iterate over the paths and
return the first one that exists. Also, add an os.Stat check for the explicit
kubeCfgPath variable to ensure it points to an existing file before returning
it, returning an empty string or error if it does not exist. This will ensure
the function returns a valid kubeconfig path respecting the current precedence
rules.
|
closed in favor #5 |
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit