backend: loading: Fix in-cluster kubeconfig panic and errors#4428
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes in-cluster startup robustness by avoiding panics when in-cluster context loading fails and by reducing noisy kubeconfig “file not found” errors (especially on fresh installs without persisted dynamic kubeconfigs).
Changes:
- Add nil-guard around in-cluster context setup to prevent panic when
GetInClusterContextfails. - Skip loading dynamic kubeconfig persistence file when it doesn’t exist to avoid misleading error logs.
- Ignore empty kubeconfig path entries when loading multiple kubeconfig files; add/adjust tests accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| backend/cmd/headlamp.go | Prevents in-cluster panic and suppresses dynamic kubeconfig load errors when the persistence file is absent. |
| backend/pkg/kubeconfig/kubeconfig.go | Skips empty kubeconfig path entries during multi-file loading; also changes ClusterID generation logic. |
| backend/pkg/kubeconfig/kubeconfig_test.go | Adds regression coverage for empty kubeconfig strings and updates ClusterID expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please try to run the backend test locally? Its failing in the GitHub check. |
dc6fd00 to
e1e1294
Compare
2ee9444 to
5058670
Compare
|
@illume , I ran the all the backend tests thoroughly , It ran correctly sir for the latest changes....and I also took care of the Copilot instruction sir |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // returns True if a file exists. | ||
| func fileExists(filename string) bool { | ||
| info, err := os.Stat(filename) | ||
| if os.IsNotExist(err) { | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
PR description mentions adding a fileExists guard to avoid logging errors when the dynamic kubeconfig persistence file is missing, but this change isn’t present in createHeadlampHandler (it still unconditionally calls LoadAndStoreKubeConfigs for the persistence file). Consider either adding the existence check where the persistence file is loaded, or updating the PR description to match the actual behavior.
There was a problem hiding this comment.
Agreed, this would make it easier for reviewers to understand. Or it could be a bug?
| kubeConfigPaths := splitKubeConfigPath(kubeConfigs) | ||
| for _, kubeConfigPath := range kubeConfigPaths { | ||
| if strings.TrimSpace(kubeConfigPath) == "" { | ||
| continue | ||
| } | ||
|
|
||
| kubeConfigContexts, errs, err := LoadContextsFromFile(kubeConfigPath, source) | ||
| if err != nil { |
There was a problem hiding this comment.
LoadContextsFromMultipleFiles checks TrimSpace(kubeConfigPath) for emptiness, but then passes the untrimmed value to LoadContextsFromFile. If the path list contains entries with leading/trailing whitespace, this will still fail to open the file. Consider trimming once (e.g., assign kubeConfigPath = strings.TrimSpace(kubeConfigPath)) and using the trimmed value for both the emptiness check and the subsequent load.
There was a problem hiding this comment.
This is a good point. Can you please consider?
| t.Run("empty_file_string", func(t *testing.T) { | ||
| kubeConfigFile := "" | ||
| err := kubeconfig.LoadAndStoreKubeConfigs(contextStore, kubeConfigFile, kubeconfig.KubeConfig, nil) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
This subtest shares contextStore state with the earlier valid_file run. That makes the subtests order-dependent and doesn’t verify that the empty-string input leaves the store unchanged. Consider creating a fresh ContextStore per subtest (or clearing it) and asserting expected store contents after the call.
There was a problem hiding this comment.
Agreed. It's not clear to me how this tests the issue being fixed?
Does the test fail without your other changes as it should fail?
illume
left a comment
There was a problem hiding this comment.
Thanks for those changes. I appreciate it.
Please run the backend:format and backend:lint checks locally? There is an error in CI with the backend lint job
I left some other comments with the copilot review. Can you please have a look?
| require.NoError(t, err, "Expected no error for valid file") | ||
| require.Empty(t, contextErrors, "Expected no context errors for valid file") | ||
| require.Equal(t, 2, len(contexts), "Expected 3 contexts from valid file") | ||
| require.Equal(t, 2, len(contexts), "Expected 2 contexts from valid file") |
There was a problem hiding this comment.
Do you know why this is changed? Can you please add an explanation to the commit message?
Take a look at this good explanation of what to include in the commit body message:
https://chris.beams.io/git-commit#why-not-how
| // returns True if a file exists. | ||
| func fileExists(filename string) bool { | ||
| info, err := os.Stat(filename) | ||
| if os.IsNotExist(err) { | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Agreed, this would make it easier for reviewers to understand. Or it could be a bug?
| kubeConfigPaths := splitKubeConfigPath(kubeConfigs) | ||
| for _, kubeConfigPath := range kubeConfigPaths { | ||
| if strings.TrimSpace(kubeConfigPath) == "" { | ||
| continue | ||
| } | ||
|
|
||
| kubeConfigContexts, errs, err := LoadContextsFromFile(kubeConfigPath, source) | ||
| if err != nil { |
There was a problem hiding this comment.
This is a good point. Can you please consider?
| t.Run("empty_file_string", func(t *testing.T) { | ||
| kubeConfigFile := "" | ||
| err := kubeconfig.LoadAndStoreKubeConfigs(contextStore, kubeConfigFile, kubeconfig.KubeConfig, nil) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
Agreed. It's not clear to me how this tests the issue being fixed?
Does the test fail without your other changes as it should fail?
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
0d4832a to
022469e
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dibyanshu-pal-kushwaha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This PR fixes a bug where running Headlamp in-cluster caused a panic and generated misleading "no such file or directory" error logs when no kubeconfig was present.
Related Issue
related #4401
Changes
1.backend/cmd/headlamp.go:
-Added a nil check for the in-cluster context to prevent a panic when context loading fails (e.g. missing environment variables).
-Added a fileExists check before loading the dynamic cluster config file to suppress error logs on fresh installations where this file does not yet exist.
2.backend/pkg/kubeconfig/kubeconfig.go
-Updated LoadContextsFromMultipleFiles to skip empty strings in the kubeconfig path list, preventing errors like open : no such file.
3.backend/pkg/kubeconfig/kubeconfig_test.go :
-added regression for empty kubeconfig string and fix ClusterID MD5 in TestLoadContextsFromFile
Steps to Test
1.Build the backend binary: cd backend && go build -o headlamp ./cmd
2.Simulate an in-cluster environment (locally): Run: HOME=/tmp/fakehome KUBECONFIG="" timeout 5s ./headlamp -in-cluster
Screenshots (if applicable)
Notes for the Reviewer
-I verified this locally by running the binary in a clean environment as described in the testing steps.